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

Fixing FieldBytes and AffineGroupBytes serialization to also work with RMP #7

Merged
merged 3 commits into from
Jan 9, 2023

Conversation

gitmalet
Copy link

Hi,

Types that contain fields serialized as FieldBytes or AffineGroupBytes did not properly serialize with serialization crates other than serde_json.
The first commit here adds a few lines to the serialization tests that check serialization with rmp_serde (i.e., a crate where it does not work currently). The errors can be seen there.
The second commit changes the implementations of FieldBytes and AffineGroupBytes by implementing them as a generic version of the expansion of serde_conv!. Serializing types with this new implementation now works under both serde_json and rmp_serde.

Please consider merging this in order to allow serialization with RMP (and possibly other serialization crates).

@cykoder cykoder requested a review from lovesh December 21, 2022 20:32
@lovesh
Copy link
Member

lovesh commented Dec 22, 2022

Hi. The no_std and wasm builds fail due to the use of std here. You can import io from ark_std. Some tests fail but if i comment out the rmp_serde serialization here, they work.

@gitmalet
Copy link
Author

Thanks, I did both changes (and a cargo format run) and squashed the commits so that they (hopefully) make sense again.

@lovesh
Copy link
Member

lovesh commented Dec 25, 2022

Hi. The message pack serialization should be checked in the test_serialization macro in test_utils crate like you initial change. And wasm and no_std builds fail due to reason mentioned before.

@gitmalet
Copy link
Author

gitmalet commented Jan 5, 2023

Sorry for the delay.
Unit tests run through on my local machine now at least.
Also the no-std builds should work as well.

A notable downside of this PR is that the error messages are now generic as well. I am not sure how important this is.

Signed-off-by: lovesh <lovesh.bond@gmail.com>
@lovesh
Copy link
Member

lovesh commented Jan 9, 2023

Thanks @gitmalet . LGTM.

@lovesh lovesh merged commit 860c023 into docknetwork:main Jan 9, 2023
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.

None yet

2 participants