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

Remove root from MerkleTree #4949

Merged
merged 2 commits into from
Mar 12, 2024

Conversation

ernestognw
Copy link
Member

@ernestognw ernestognw commented Mar 10, 2024

Fixes #4946

After discussing, we considered it rather unnecessary to store the root of the tree along with it, and its core functionality is enough for anyone to build use cases where the root is tracked.

With this change the user may decide to keep track of the root for purposes that may not require on-chain availability (e.g. a whitelist that stores roots every N insertions) or that may require a lookup mechanism (e.g. Bitmaps) for which storing the root is already an opinionated decision.

We can consider adding a default recommendation, such as #4911.

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

Copy link

changeset-bot bot commented Mar 10, 2024

⚠️ No Changeset found

Latest commit: 3af9ca8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@ernestognw ernestognw requested a review from Amxx March 11, 2024 22:59
@Amxx
Copy link
Collaborator

Amxx commented Mar 12, 2024

That works, but thecking the event is a bit counterintuitive IMO, and it doesn't "show" the need for devs to store the root "outside the library"

I modified the mock so that the old test don't need to me modify at all. I think minimizing the diff is preferable.

@ernestognw ernestognw merged commit 6b4ec6c into OpenZeppelin:master Mar 12, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider removing the _root for the MerkleTree structure
2 participants