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

Improve Option::None size validation #2323

Closed
danielbate opened this issue May 17, 2024 · 6 comments
Closed

Improve Option::None size validation #2323

danielbate opened this issue May 17, 2024 · 6 comments
Labels
feat Issue is a feature
Milestone

Comments

@danielbate
Copy link
Contributor

In #2322 we removed the expected size validation for nested options. That should be reintroduced.

@arboleya
Copy link
Member

Is this still a thing?

@arboleya arboleya added the p2 label Jun 12, 2024
@danielbate
Copy link
Contributor Author

danielbate commented Jun 13, 2024

It is yes, as we are not correctly validating option length. Not a massive deal as Option::Some is validated correctly at the child level and None is empty bytes. But with the correct design we can solve for this. However improved length validation in #1795 would also solve this.

@arboleya
Copy link
Member

Two more questions:

  1. Can this be a blocker for mainnet?
  2. Is there any relationship between this issue and this PR?

@danielbate
Copy link
Contributor Author

  1. No it should not. At some level, an option will need to have a native type, at which point it will be validated correctly
  2. No, there is a single line missing coverage in the EnumCoder that this will resolve.

@danielbate
Copy link
Contributor Author

Going to un-assign my self as this could be picked up by anyone and is not on my immediate radar.

@danielbate danielbate removed their assignment Jun 17, 2024
@danielbate
Copy link
Contributor Author

At some level, an option will need to have a native type, at which point it will be validated correctly

For the above reason, I think we can deprioritise this issue. Additionally this code has now been audited in a few ways and it remains sound. There is enough validation in other coders and around the empty option that we can have confidence.

IMO the real value will come from improved coder wide length validation in #1795.

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

No branches or pull requests

2 participants