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

feat!: remove nested option size validation #2322

Merged
merged 3 commits into from
May 17, 2024

Conversation

danielbate
Copy link
Contributor

@danielbate danielbate commented May 17, 2024

Closes #2321

For v0 encoding, an option would contain a WORD_SIZE for the enum case key (Some or None) and the property space for the value (i.e u64 would be another WORD_SIZE. Even if the value was None, we would still receive the expected value property space, meaning we could validate correct for both values. However in v1, 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 the NumberCoder 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.

@danielbate danielbate added the feat Issue is a feature label May 17, 2024
@danielbate danielbate self-assigned this May 17, 2024
@danielbate danielbate marked this pull request as ready for review May 17, 2024 05:58
@fuel-service-user
Copy link
Contributor

fuel-service-user commented May 17, 2024

✨ A PR has been created under the rc-2322 tag on fuels-wallet repo.
FuelLabs/fuels-wallet#1307

Copy link
Contributor

@petertonysmith94 petertonysmith94 left a 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.

Copy link
Contributor

Coverage Report:

Lines Branches Functions Statements
79.82%(+0.03%) 70.56%(+0.09%) 76.9%(+0.05%) 79.91%(+0.03%)
Changed Files:

Coverage values did not change👌.

@danielbate danielbate merged commit 0da455a into master May 17, 2024
19 checks passed
@danielbate danielbate deleted the db/feat/remove-nested-option-size-validation branch May 17, 2024 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Issue is a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StructCoder.decode throwing Invalid struct data size
5 participants