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: prevent usage of enums without a default variant in storage #6422

Closed
enitrat opened this issue Sep 25, 2024 · 2 comments · Fixed by #6459
Closed

feat: prevent usage of enums without a default variant in storage #6422

enitrat opened this issue Sep 25, 2024 · 2 comments · Fixed by #6459
Labels
enhancement New feature or request

Comments

@enitrat
Copy link
Contributor

enitrat commented Sep 25, 2024

I think enums without a default variant should throw an error at compile time if they try to derive starknet::Store - otherwise, attempting a storage read on an uninitialized slot will cause a runtime panic.

See description in cairo-book/cairo-book#990

@enitrat enitrat added the enhancement New feature or request label Sep 25, 2024
@orizi
Copy link
Collaborator

orizi commented Oct 6, 2024

But why is that an issue specifically? it is the decision of the contract writer to know they only are reading valid values.

adding a diagnostic now may even be more confusing - even possibly marking any of the existing keys as #[default] causing a storage layout change.

(not saying no - just want to make sure the cure is better than the disease)

@enitrat
Copy link
Contributor Author

enitrat commented Oct 6, 2024

it is the decision of the contract writer to know they only are reading valid values.

I consider it an issue because I don't think reading a value from storage should default to a panic if that value is not set. By ensuring a default variant exists, you are leveraging the compiler so that:

  • You catch eventual errors at compile time by forcing a variant to act as the default one - if required, the contract creator can just add a None variant that fulfills this role.
  • You force the definition of a type associated to an empty storage
  • You can "clear" an enum in storage by resetting it to the default variant
  • Contract creator might not now about this default panic scenario (as reported in the issue) - I actually did not even know about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants