-
Notifications
You must be signed in to change notification settings - Fork 311
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
TransactionView: Signature Meta #2408
Conversation
as u16; | ||
// Maximum number of signatures should be represented by a single byte, | ||
// thus the MSB should not be set. | ||
const _: () = assert!(MAX_SIGNATURES_PER_PACKET & 0b1000_0000 == 0); |
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.
Do we need const _: () =
?
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.
It's a compile-time check to make sure that the u16 for a valid packet containing as many signatures as possible will be serialized in a single byte.
We don't strictly need it, but if the packet size increased the code below may no longer work - this check makes sure that we'd become aware it was broken.
lgtm modulo the nit. And maybe we should name SignatureMetaData to be specific? |
} | ||
|
||
let signature_offset = *offset as u16; | ||
offset_array_len::<Signature>(bytes, offset, num_signatures)?; |
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 think that offset_array_len
isn't a clear name. What do you think of advance_offset_array_len
or bump_offset_array_len
?
let mut offset = 0; | ||
let meta = SignatureMeta::try_new(&bytes, &mut offset).unwrap(); | ||
assert_eq!(meta.num_signatures, 12); | ||
assert_eq!(meta.offset, 1); |
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.
Would it be important to test a case when meta.offset
is not 1?
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'll add a test because there's nothing explicitly in this code that requires it to be at 1.
For actual transaction packets, this will never be anything but 1 even though the encoding scheme technically allows for it to be [1,3].
Problem
Summary of Changes
SignatureMeta
which parses the number of signatures and stores the offsetFixes #