OpenZeppelin / openzeppelin-contracts

OpenZeppelin Contracts is a library for secure smart contract development.
https://openzeppelin.com/contracts
MIT License
24.44k stars 11.68k forks source link

Fix invalid memory-safe annotation #5058

Closed Amxx closed 1 month ago

Amxx commented 1 month ago

See https://github.com/OpenZeppelin/openzeppelin-contracts/pull/5052#discussion_r1616302783

changeset-bot[bot] commented 1 month ago

⚠️ No Changeset found

Latest commit: 9df7bbc05bddcd51d35b8bf3a69269e1f87942d6

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

frangio commented 1 month ago

Removing the annotation is not good because it globally disables optimizations in any contract that includes this code. It's better to make the code memory safe as in https://github.com/OpenZeppelin/openzeppelin-contracts/pull/5057

ernestognw commented 1 month ago

Agree with @frangio it's better to make it memory-safe, but I was wondering what'd be the problem with it given it's reverting.

We do the same in Proxy.sol

frangio commented 1 month ago

In Proxy.sol it would be interesting to test the performance of making the block memory-safe by using the free memory pointer instead of writing to memory pointer 0. Although I think the optimization consequences only apply in IR mode.