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

Add memory safe assembly annotations #3384

Conversation

nathan-lapinski
Copy link
Contributor

@nathan-lapinski nathan-lapinski commented Apr 30, 2022

Part of #3336

Adds memory safe annotations for blocks of inline assembly in the following files:

  • Address.sol
  • Base64.sol
  • Clones.sol
  • EnumerableSet.sol
  • ERC721.sol
  • ERC2771Context.sol
  • MinimalForwarder.sol
  • StorageSlot.sol
  • SafeMathMock.sol

As discussed here, I am happy to add more files into this PR before merging, but I wanted to open this now to gather feedback.

This is also my first contribution to this repository, so please let me know if I have made any errors.🙏

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@nathan-lapinski nathan-lapinski marked this pull request as draft April 30, 2022 20:16
@nathan-lapinski nathan-lapinski marked this pull request as ready for review April 30, 2022 21:09
@frangio
Copy link
Contributor

frangio commented May 6, 2022

Thank you @nathan-lapinski!

A few comments:

  • PR titles should be written like commit messages, e.g. "Add memory safe annotation", and should not include the issue number
  • These annotations aren't really needed in the mock, because it's just for testing purposes I think we should remove it to keep the files as clean as possible.

@nathan-lapinski nathan-lapinski changed the title Chore/memory safe assembly annotations #3336 Add memory safe assembly annotations May 7, 2022
@nathan-lapinski
Copy link
Contributor Author

nathan-lapinski commented May 7, 2022

Sounds good. Thank you for the review, @frangio !

I will add a few more files to this PR before requesting another review.

frangio
frangio previously approved these changes May 11, 2022
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good. Do you want to add the remaining files from #3336 (comment) ?

@nathan-lapinski
Copy link
Contributor Author

Thanks for the review @frangio ! Yes, I’ll go ahead and add the remaining files to this PR.

@nathan-lapinski
Copy link
Contributor Author

nathan-lapinski commented May 19, 2022

This covers all of the inline assembly usage except for contracts/proxy/Proxy.sol. This usage seems a little more complex, and I can't really tell yet if it's memory safe or not.

If it looks safe, I'd be happy to add it to this PR, or at least add a comment mentioning why it wasn't marked as memory-safe.

@nathan-lapinski nathan-lapinski requested a review from frangio May 19, 2022 13:20
Copy link
Contributor

@frangio frangio 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! The Proxy assembly block shouldn't make a difference.

@frangio frangio merged commit 65b4572 into OpenZeppelin:master May 23, 2022
@nathan-lapinski nathan-lapinski deleted the chore/memory-safe-assembly-annotations-#3336 branch May 24, 2022 00:24
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