-
Notifications
You must be signed in to change notification settings - Fork 144
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
Update to new empty amt serialization #623
Conversation
@@ -49,7 +50,9 @@ pub mod json { | |||
where | |||
D: Deserializer<'de>, | |||
{ | |||
let s: String = Deserialize::deserialize(deserializer)?; | |||
Ok(VRFProof::new(base64::decode(s).map_err(de::Error::custom)?)) | |||
let s: Cow<'de, str> = Deserialize::deserialize(deserializer)?; |
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.
Cow!!!!!
@@ -51,5 +51,5 @@ where | |||
)); | |||
} | |||
}; | |||
Ok(BigInt::from_bytes_be(sign, &bz)) | |||
Ok(BigInt::from_bytes_be(sign, &bz[1..])) |
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.
What happens if there's only a sign byte? This will panic i think, but what is the expected behaviour? I would assume 0 or something, since empty bytes -> bigint deserializes to 0?
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.
on the 0 case, it is serialized as empty bytes, even if it was a sign byte and nothing else, this would just deserialize as zero. Also the functionality is basically the same as it was before.
return Err(serde::de::Error::custom( | ||
"First byte must be 0 to decode as BigUint", | ||
)); | ||
} | ||
|
||
Ok(BigUint::from_bytes_be(&bz)) | ||
Ok(BigUint::from_bytes_be(&bz[1..])) |
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.
Same thing here
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.
empty bytes get serialized as 0 and this would still not have an issue (but again Lotus and our client would not ever serialize as this case)
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.
I'll leave it up to you to decide if that needs to handled as I'm not sure. Otherwise, it lgtm
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links