-
Notifications
You must be signed in to change notification settings - Fork 9
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
Use static arrays in FerveoPublicKey
serialization
#136
Conversation
Codecov Report
@@ Coverage Diff @@
## development #136 +/- ##
===============================================
+ Coverage 78.66% 78.82% +0.15%
===============================================
Files 24 24
Lines 4880 4912 +32
===============================================
+ Hits 3839 3872 +33
+ Misses 1041 1040 -1
|
def test_public_key_serialization(): | ||
pk = make_pk() | ||
serialized = bytes(pk) | ||
assert len(serialized) == FerveoPublicKey.serialized_size() |
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.
Is it possible to also use from_bytes(...)
in the test and compare the deserialized object to the original? So not just the serialization, but the deserialization as well.
Same for L59 as well.
def make_pk(): | ||
return Keypair.random().public_key() | ||
|
||
|
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.
Can the commented-out TODOs in this test file now be uncommented due to the updated bindings?
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.
I've tried to update some of these tests but I didn't implement __richcmp__
for all types involved, so I'm going to cut some corners here. Ideally, we would have all class methods implemented, but that's a bit of work and not really worth the effort at this point IMHO.
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.
🎸
Type of PR:
Required reviews:
What this does:
FerveoPublicKey
from 104 bytes to 96 bytesIssues fixed/closed:
Why it's needed:
Notes for reviewers:
DkgPublicKey
serialization: Fix incompatibleDkgPublicKey
byte serialization #128