-
Notifications
You must be signed in to change notification settings - Fork 54
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
chore(rln-relay-v2): add tests for serde #2421
Conversation
You can find the image built from this PR at
Built from d1fecfa |
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.
LGTM! Thanks!
I added a minor comment. Aside from that, maybe it is worth adding a test example exploring "wrong" edge cases such as incorrect rateCommitment, if possible
Co-authored-by: Ivan FB <128452529+Ivansete-status@users.noreply.github.com>
@@ -69,8 +69,8 @@ when defined(rln_v2): | |||
## the serialization is done as instructed in https://github.com/kilic/rln/blob/7ac74183f8b69b399e3bc96c1ae8ab61c026dc43/src/public.rs#L146 | |||
## [ id_key<32> | id_index<8> | epoch<32> | signal_len<8> | signal<var> ] | |||
let memIndexBytes = toBytes(uint64(memIndex), Endianness.littleEndian) | |||
let userMessageLimitBytes = toBytes(uint64(userMessageLimit), Endianness.littleEndian) | |||
let messageIdBytes = toBytes(uint64(messageId), Endianness.littleEndian) | |||
let userMessageLimitBytes = cast[array[32, byte]](toBytes(uint64(userMessageLimit), Endianness.littleEndian)) |
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.
using cast
is a good way to introduce undefined behavior - it should be avoided in most cases - array[...](toBytes...)
will work iff this is a compatible conversion, otherwise you need to construct the array in some other way.
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.
addressed in 4095279
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.
Thanks!
serde is ❤️
Description
This PR includes tests for the serde changes in rln-v2, including
toLeaf
andproofGen/proofVerify
Changes
proofGen
toLeaf
How to test
nim c --out:build/rln_v2_serde -r -d:chronicles_log_level=INFO --verbosity:0 --hints:off -d:git_version="v0.24.0-rc.0-35-g50308e" -d:release --passL:librln_v0.4.1.a --passL:-lm -d:rln_v2 tests/waku_rln_relay/rln_v2/test_rln_relay_v2_serde.nim
(provided you ranRLN_V2=true make -j16 update
before)OR
RLN_V2=true make -j16 test