-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added serde feature #413
Added serde feature #413
Conversation
f54f6c7
to
ad61992
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good too. I just have the same comment as in another PR, whether we want to introduce so many custom error strings or just pass the underlying error through where applicable.
de3dffc
to
a8a3c66
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general. I would suggest using the same seed for randomness in the test cases.
Additionally, I think adding another test that checks the equality of a serialized struct against a serde_json string of an event is a good idea to make sure we have a test case that can catch if the attribute names of the struct ever change.
Example:
#[test]
fn event_fields() {
let serde_json_string = "{\"source\":\"0000000000000000000000000000000000000000000000000000000000000000\",\"topic\":\"\",\"data\":\"\"}";
let event = Event {
source: ContractId::from_bytes([0; CONTRACT_ID_BYTES]),
topic: String::new(),
data: Vec::new(),
};
let ser = serde_json::to_string(&event).unwrap();
assert_eq!(serde_json_string, ser);
}
a8a3c66
to
ba58656
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks very good, just test fun rename seems important to me
for practical test usability
ba58656
to
11b4c6a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
11b4c6a
to
1d3a99d
Compare
For dusk-network/rusk/issues/2773 and #414