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

Removed unnecessary EMPTY_STORAGE_MAP_ROOT const #916

Merged
merged 6 commits into from
Oct 14, 2024

Conversation

polydez
Copy link
Contributor

@polydez polydez commented Oct 11, 2024

In this PR we remove unnecessary EMPTY_STORAGE_MAP_ROOT constant in favor of using EmptySubtreeRoots::entry() method from miden-crypto.

@polydez polydez changed the title Remove unnecessary EMPTY_STORAGE_MAP_ROOT const Removed unnecessary EMPTY_STORAGE_MAP_ROOT const Oct 11, 2024
@polydez polydez marked this pull request as ready for review October 11, 2024 13:35
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Overall, the changes look fine, but I'm not sure I understand the motivation behind the change. It seems to me that having a single constant is a bit better than using EmptySubtreeRoots::entry(StorageMap::STORAGE_MAP_TREE_DEPTH, 0) in a couple of places.

Ideally, we'd set this constant as:

pub const EMPTY_STORAGE_MAP_ROOT: Word = EmptySubtreeRoots::entry(StorageMap::STORAGE_MAP_TREE_DEPTH, 0);

But I'm not sure if that's possible.

@polydez polydez force-pushed the polydez-remove-unnecessary-const branch from ae9f924 to e1140b2 Compare October 14, 2024 13:15
@polydez
Copy link
Contributor Author

polydez commented Oct 14, 2024

@bobbinth

But I'm not sure if that's possible.

Yes, it's possible, please take a look!

Copy link
Contributor

@bobbinth bobbinth 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! Thank you!

@bobbinth bobbinth merged commit ae04006 into next Oct 14, 2024
8 checks passed
@bobbinth bobbinth deleted the polydez-remove-unnecessary-const branch October 14, 2024 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants