-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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!: remove nested option size validation #2322
feat!: remove nested option size validation #2322
Conversation
✨ A PR has been created under the |
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.
Looks good! 🍏
Testing variables names could do with some work (e.g. someValue
and someInput
). Would prefer an expectedValue
for readability, however, not a showstopper.
Coverage Report:
Changed Files:Coverage values did not change👌. |
Closes #2321
For
v0
encoding, an option would contain aWORD_SIZE
for the enum case key (Some
orNone
) and the property space for the value (i.eu64
would be anotherWORD_SIZE
. Even if the value wasNone
, we would still receive the expected value property space, meaning we could validate correct for both values. However inv1
,None
will now only return the case bytes, and no value bytes. Meaning we can no longer validate the expected byte length before checking the case value.We are still validating at the option value level (i.e.
Option<u8>
will be validated in theNumberCoder
so we should not be too concerned about underflow/overflow here, but there are definitely improvements that could be made, that I'll investigate here. But for now, this PR will remove the invalid check for options.Breaking Changes:
Any parent type that contains a nested
option
will no longer throw due to size validation.