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: support for borsh @ 1.5 #416

Merged
merged 12 commits into from
Dec 24, 2024
Merged

feat: support for borsh @ 1.5 #416

merged 12 commits into from
Dec 24, 2024

Conversation

rtrombone
Copy link
Contributor

Motivation

Avoid having to implement a new type to handle BorshSerialize and BorshDeserialize for ruint types.

Solution

This PR implements BorshSerialize and BorshDeserialize for Uint and Bits with basic tests.

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog

@rtrombone rtrombone marked this pull request as ready for review December 17, 2024 23:23
@rtrombone rtrombone requested a review from prestwich as a code owner December 17, 2024 23:23
Copy link
Collaborator

@prestwich prestwich left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me. I am not an expert in borsh, so while I've tried to review the logic I am not 100% sure it's correct. Can you leave a 1-2 paragraph comment here about relevant particulars of borsh serialization in case I need to maintain this code in the future :)

@rtrombone
Copy link
Contributor Author

rtrombone commented Dec 19, 2024

This seems reasonable to me. I am not an expert in borsh, so while I've tried to review the logic I am not 100% sure it's correct. Can you leave a 1-2 paragraph comment here about relevant particulars of borsh serialization in case I need to maintain this code in the future :)

Of course! Borsh serializes integers in little-endian format. For Uint deserialization, I think iterating forward over the limbs and serializing each limb as little-endian (via BorshDeserialize::deserialize_reader) does the trick. Do you mind double-checking me?

For example, the test value uses limbs [1, 2, 3, 4], which should be:

// Human "readable": 25108406941546723056364004793593481054836439088298861789185
1 // 0..8
+ (2 << 64) // 8..16
+ (3 << 128) // 16..24
+ (4 << 192) // 24..32

I also just added lines in the test that call to_le_bytes() to sanity check.

Let me know if I misunderstood how the limbs are put together and I can fix it. Thank you.

For reference: https://borsh.io/ (see Integers section of Specialization).

@prestwich
Copy link
Collaborator

thank you. does borsh serialize fixed-sized integers as fixed-length bitstrings? i.e. is a u64 always exactly 8 bytes?

My concern with the current implementation is that non-standard sized Uint would currently be serialized at sizes other than their bit size rounded to the nearest byte. E.g. a 7 byte integer like Uint<56, 1> would be serialized as 8 bytes. Is this reasonable for Borsh, or should we use try to encode the minimal number of bytes?

@rtrombone
Copy link
Contributor Author

thank you. does borsh serialize fixed-sized integers as fixed-length bitstrings? i.e. is a u64 always exactly 8 bytes?

My concern with the current implementation is that non-standard sized Uint would currently be serialized at sizes other than their bit size rounded to the nearest byte. E.g. a 7 byte integer like Uint<56, 1> would be serialized as 8 bytes. Is this reasonable for Borsh, or should we use try to encode the minimal number of bytes?

You're right. This was an oversight on my part and tried to be clever with iterating over the limbs.

Maybe I should just do to_le_bytes and serialize that. I'll need to look into how to_le_bytes works under the hood for Uint to make sure this is kosher.

previous implementation was bugged
@rtrombone
Copy link
Contributor Author

I couldn't find a better way than to use a macro to implement BITS to BYTES relationship. Maybe at some point in the future when we can use Self::BYTES this can be generalized.

@prestwich
Copy link
Collaborator

thanks, let me spend some time on this early next week and i'll see if i can figure out an elegant solution :)

@prestwich prestwich mentioned this pull request Dec 23, 2024
3 tasks
@prestwich
Copy link
Collaborator

Would you mind checking out my branch here? If you think the implementation is correct, I can push it onto this branch and get this merged soon :)

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