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

feat: de/serialize based on human readability #532

Merged
merged 4 commits into from
Jul 16, 2024

Conversation

kariy
Copy link
Contributor

@kariy kariy commented Dec 21, 2023

This PR improves the serde impls used to mainly de/serialize the FieldElement type. The de/serialization implementations now take into consideration whether Serialize/Deserialize implementations should expect to de/serialize in human-readable form or not.

This possibly(?) introduces a breaking change due to this PR #527.

@xJonathanLEI
Copy link
Owner

This PR is all good by itself. I wanted to merge it, but it this is an unfortunately timing to introduce a breaking change as we're migrating to the official Felt type. Asking users to deal with a breaking change only to immediately break them again via the migration doesn't sound like a good idea. That's the reason for the long delay.

@kariy
Copy link
Contributor Author

kariy commented Feb 26, 2024

This PR is all good by itself. I wanted to merge it, but it this is an unfortunately timing to introduce a breaking change as we're migrating to the official Felt type. Asking users to deal with a breaking change only to immediately break them again via the migration doesn't sound like a good idea. That's the reason for the long delay.

Understandable! It's not urgent for now so I don't mind the delay 👍.

@xJonathanLEI xJonathanLEI changed the title refactor: de/serialize based on human readability feat: de/serialize based on human readability Jul 16, 2024
@xJonathanLEI
Copy link
Owner

Rebased and added a new bound check for deserializing as the new Felt type accepts any integer input, which makes sense as they can always just take the modulo, but it makes little sense to me for deserialization purposes - you'd never serialize the value into an out-of-range value in the first place.

@xJonathanLEI
Copy link
Owner

I feel like we need more tests as new functionality has been added with this one.

@xJonathanLEI
Copy link
Owner

Hmm an issue with this PR is that it leaves UfeHexOption unchanged. So it gets a bit weird.

@xJonathanLEI
Copy link
Owner

Just pushed a commit adding binary serde to UfeHexOption and UfePendingBlockHash too. When the value is None it encodes an empty slice.

@xJonathanLEI xJonathanLEI merged commit b8a0003 into xJonathanLEI:master Jul 16, 2024
26 checks passed
@kariy
Copy link
Contributor Author

kariy commented Jul 16, 2024

thanks, appreciate the help!!

@kariy kariy deleted the refactor-serde branch July 16, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants