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

EnumerableSet: Remove Boundary Check in _at #2606

Merged

Conversation

wjmelements
Copy link
Contributor

Changes

This check is redundant because solidity does it for you without your consent, at great gas cost.
If you knew all of the things solidity does to arrays you would never use them.
But I'm not asking you to replace all of your solidity arrays.
Instead, complain to the compiler team, who doesn't trust you.
Reviewer @Amxx

@wjmelements
Copy link
Contributor Author

You're basically paying 1000 gas for a cleaner error message you are never expecting to see.

@wjmelements
Copy link
Contributor Author

Here is the difference, from the tests (which I will take the time to repair if you agree that this is a very expensive error message):

       Wrong kind of exception received
      + expected - actual

      -VM Exception while processing transaction: revert 
      +EnumerableSet: index out of bounds

As you can see, solidity asserts in this case, and that is fine.

@Amxx Amxx enabled auto-merge (squash) April 20, 2021 19:13
@Amxx Amxx merged commit 165e6f1 into OpenZeppelin:master Apr 20, 2021
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.

2 participants