Skip to content
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

Jump to bottom

Breaking: changed Parity serialization to u8 #401

Merged
merged 2 commits into from Feb 24, 2022
Merged

Breaking: changed Parity serialization to u8 #401

merged 2 commits into from Feb 24, 2022

Conversation

Kixunil
Copy link
Collaborator

@Kixunil Kixunil commented Feb 9, 2022

Serializing the value as u8 is more compact but this is a breaking
change.

Visitor was renamed to avoid hungarian notation and maybe allow other
integers in the future.

For next major version, depends on #400

@Kixunil
Improved error handling in Parity serde impl
662843e 
* Fixes error message to be according to the trait documentation
* Uses `unexpected_value` to provide more information about the error
@Kixunil
Breaking: changed Parity serialization to u8
e6cb588 
Serializing the value as `u8` is more compact but this is a breaking
change.

`Visitor` was renamed to avoid hungarian notation and maybe allow other
integers in the future.
tcharding
tcharding reviewed Feb 22, 2022
View reviewed changes
Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK e6cb588

@Kixunil
Copy link
Collaborator Author

Kixunil commented Feb 22, 2022

@tcharding your ACK got me to think about this again and maybe we should impl deserialize to suggest deserializing u8 but support all integers. That way it'd be backwards-compatible for typed formats. (Untyped binary formats are screwed but that's the case anyway 🤷‍♂️ )

@tcharding
Copy link
Member

My initial thoughts are to not support all integers because there is no real limit to all so what ever subset we support is arbitrary. (I know this argument is a bit contrived.) Is there a use case you can think of where having support for additional integer sizes would be useful?

@Kixunil
Copy link
Collaborator Author

Kixunil commented Feb 23, 2022

We would accept old formats. But really is there any harm accepting 0u64 and such? The only issue I can see is not being strict can make debugging serializers harder. I think it'd be fine at least for i32.

@tcharding
Copy link
Member

I don't know if it matters but it would also mean deserialize -> seralize roundtrip will not produce the same output as input.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Feb 23, 2022

Doesn't seem important to me...

@apoelstra
Copy link
Member

Is this ready for review?

@apoelstra apoelstra mentioned this pull request Feb 23, 2022
Merged
@Kixunil Kixunil marked this pull request as ready for review February 23, 2022 19:49
@Kixunil
Copy link
Collaborator Author

Kixunil commented Feb 23, 2022

Ready

apoelstra
apoelstra approved these changes Feb 24, 2022
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK e6cb588

@apoelstra apoelstra merged commit c7d6cdb into rust-bitcoin:master Feb 24, 2022
@Kixunil Kixunil deleted the parity-serde-as-u8 branch February 24, 2022 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tcharding tcharding tcharding left review comments

@apoelstra apoelstra apoelstra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Kixunil @tcharding @apoelstra