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

OpenZeppelin Upgrade (ETH-1253) #248

Merged
merged 7 commits into from
Dec 4, 2023
Merged

OpenZeppelin Upgrade (ETH-1253) #248

merged 7 commits into from
Dec 4, 2023

Conversation

iamsahu
Copy link
Contributor

@iamsahu iamsahu commented Nov 30, 2023

Description

The repository currently uses v4.7.0 of OpenZeppelin's contract. Currently the OZ library is at v5.0.0.
There have been multiple updates, bug fixes & enhancements that have been shipped till now. So, it is optimal to upgrade the library to incorporate the changes and ensure safety.

Here are the contracts that are currently being used in the protocol & the updates that have been performed on them:

  • contracts/proxy/transparent/TransparentUpgradeableProxy.sol
    • TransparentUpgradeableProxy: Fix transparency in case of selector clash with non-decodable calldata or payable mutability.
    • Following functions were removed and are handled through fallback
      • Admin
      • Implementation
      • changeAdmin
      • upgradeTo
      • upgradeToAndCall
  • contracts/proxy/ERC1967/ERC1967Proxy.sol
  • contracts/proxy/ERC1967/ERC1967Upgrade.sol
    • ERC1967Upgrade: removed contract-wide oz-upgrades-unsafe-allow delegatecall annotation, replaced by granular annotation in UUPSUpgradeable.
  • contracts/proxy/Proxy.sol
  • contracts/security/ReentrancyGuard.sol
    • ReentrancyGuard: Reduce code size impact of the modifier by using internal functions.
  • contracts/token/ERC20/extensions/ERC20VotesUpgradeable.sol
  • contracts/proxy/utils/Initializable.sol
    • Initializable: optimize _disableInitializers by using != instead of <.
    • Initializable: add internal functions _getInitializedVersion and _isInitializing
  • contracts/token/ERC20/IERC20Upgradeable.sol
  • contracts/governance/utils/IVotesUpgradeable.sol
  • contracts/token/ERC20/IERC20.sol
  • contracts/utils/cryptography/ECDSAUpgradeable.sol
  • contracts/utils/math/MathUpgradeable.sol
  • contracts/governance/utils/IVotesUpgradeable.sol
  • contracts/utils/math/SafeCastUpgradeable.sol

We have decided to upgrade from v4.7.0 to v4.9.3. We choose not to upgrade to v5.0.0 as it is released recently (Oct 5) and might have undiscovered bugs.

Notice

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you assigned this PR to yourself?
  • Have you added at least 1 reviewer?
  • Have you updated the official documentation?
  • Have you added sufficient documentation in your code?
  • Have you added relevant tests to the official test suite?

Pull Request Type

  • 💫 New Feature (Breaking Change)
  • 💫 New Feature (Non-breaking Change)
  • 🛠️ Bug fix (Non-breaking Change: Fixes an issue)
  • 🕹️ Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)

Breaking changes (if applicable)

The following functions are not available as external function on TUPProxy. However, they can be called by passing the function call in data:

  • Admin
  • Implementation
  • changeAdmin
  • upgradeTo
  • upgradeToAndCall

Testing

  • Have you tested this code with the official test suite?
  • Have you tested this code manually?

Additional comments

We had to change the inheritance order for the following files to satisfy the C3 linearisation criteria:

  • ITLC.1.sol
  • ERC20VestableVotesUpgradeable.1.sol

Copy link

codecov bot commented Nov 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@9a9920a). Click here to learn what that means.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #248   +/-   ##
=======================================
  Coverage        ?   95.15%           
=======================================
  Files           ?       72           
  Lines           ?     1694           
  Branches        ?      229           
=======================================
  Hits            ?     1612           
  Misses          ?       27           
  Partials        ?       55           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@iamsahu iamsahu self-assigned this Nov 30, 2023
@iamsahu iamsahu requested a review from mischat November 30, 2023 15:49
@iamsahu iamsahu marked this pull request as ready for review November 30, 2023 15:49
@iamsahu iamsahu requested a review from cebidhem as a code owner November 30, 2023 15:49
@iamsahu iamsahu changed the title OpenZeppelin Upgrade OpenZeppelin Upgrade (ETH-1253) Nov 30, 2023
Copy link
Member

@mischat mischat left a comment

Choose a reason for hiding this comment

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

These look good, and the submodules seem to be pointing to the correct commits in the two repos which we updated: Thanks!

-fd81a96f01cc42ef1c9a5399364968d0e07e9e90 lib/openzeppelin-contracts
-3d4c0d5741b131c231e558d7a6213392ab3672a5 lib/openzeppelin-contracts-upgradeable

:shipit:

@iamsahu iamsahu merged commit 0b15fbf into main Dec 4, 2023
18 checks passed
@iamsahu iamsahu deleted the feat/oz-upgrade branch December 4, 2023 10:31
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