sablier-labs / v2-periphery

🎛 Peripheral smart contracts for interacting with Sablier V2
https://docs.sablier.com
GNU General Public License v3.0
23 stars 7 forks source link

Upgrade to OpenZeppelin v5 #211

Closed PaulRBerg closed 4 months ago

PaulRBerg commented 9 months ago

Just like https://github.com/sablier-labs/v2-core/issues/702 and https://github.com/sablier-labs/v2-core/pull/703.

This should be implemented on a new 2.2 branch.

andreivladbrg commented 9 months ago

There is an issue here with upgrading to v5. We have installed uniswap's version of permit2:

https://github.com/sablier-labs/v2-periphery/blob/51f3c0f0973eb5d038f21ebeb32709ac433fa7fa/.gitmodules#L9-L12

Which is using an older version of openzeppelin.

They are installing it because they use various utils from them, as:

https://github.com/Uniswap/permit2/blob/cc56ad0f3439c502c246fc5cfcc3db92bb8b7219/test/utils/PermitSignature.sol#L5

This path needs to be updated to: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v5.0/contracts/utils/cryptography/EIP712.sol

Otherwise it would revert:

image

Given how much has taken them to merge the package.json PR, should we make the update to v5 in sablier-labs/permit2 and switch back to it?

PaulRBerg commented 9 months ago

Did I mention that I remappings are a mess?

It is possible to make it work by adding this remapping:

openzeppelin-contracts/=lib/permit2/lib/openzeppelin-contracts/

But it's ugly and I would rather not tether our OZ remapping to Permit2's installation.

So yeah let's fork Permit2 again and install OZ v5 in our fork.

andreivladbrg commented 9 months ago

Comming back here because I just found out that these imports are not even used 😅

PaulRBerg commented 9 months ago

lol

@andreivladbrg you should definitely open a GitHub issue in Uniswap's repo (or even a PR) and let them know why it would be helpful to remove those unused imports

andreivladbrg commented 9 months ago

you should definitely open a GitHub issue in Uniswap's repo (or even a PR) and let them know why it would be helpful to remove those unused imports

Will do in the future. For us, it would still be the better choice to change to sablier-labs/permit2

smol-ninja commented 4 months ago

Resolved in https://github.com/sablier-labs/v2-periphery/commit/77a0982cb137118d2abc8bd5e43eab143e8c7dfa.