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

Feature/update to openzeppelin v5 #209

Closed
wants to merge 6 commits into from

Conversation

malteish
Copy link
Collaborator

@malteish malteish commented Oct 24, 2023

This update came with several changes:

  • _beforeTokenTransfer() is now called _update()
  • all openzeppelin contracts throw errors instead of reverting with a message
  • PaymentSplitter was removed, so the corresponding tests have been disabled
  • updated solidity to 0.8.21 (^0.8.20 is required)

Also, openzeppelin upgrades plugin was added in preparation for the next step.

Copy link
Collaborator

@CJentzsch CJentzsch left a comment

Choose a reason for hiding this comment

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

Looks good.
Let's have a final decision on how to distribute dividends before merging.

@malteish
Copy link
Collaborator Author

malteish commented Oct 27, 2023

I [Christoph] asked Consensys about what they think about our plans to upgrade the contracts to OZ 5.0. Here there comment:
The team’s notes on OZ 5.0 are the following:

  • There are breaking changes in the update which should be carefully assessed
  • The team estimates that from a pure security perspective the update won’t bring many additional benefits.
  • The update won’t affect the audit engagement since we face these changes in multiple projects anyway. If you consider it beneficial and update before the audit, we are fine.

Here are the reasons for an against this update:
Pro:

  • If we want to make the token upgradeable, then the token contracts will be very long-lived. Since one of the breaking changes in v5 is the storage structure of the upgradeable contracts, where will be no way to migrate an OZ v4 based token to an OZ v5 based version. Therefore, in order to support upcoming features, we should build on v5 now.

Con:

@ernestognw
Copy link

ernestognw commented Oct 27, 2023

Hey, just provided more information about how to adapt 5.0 to an ERC20Snapshot in OpenZeppelin/openzeppelin-contracts#4276 (comment). In case it's helpful 🙂

@malteish
Copy link
Collaborator Author

Hey, just provided more information about how to adapt 5.0 to an ERC20Snapshot in OpenZeppelin/openzeppelin-contracts#4276 (comment). In case it's helpful 🙂

Thank you @ernestognw !

@malteish
Copy link
Collaborator Author

@CJentzsch and @malteish decided to stay with OZ v4.9 for the foreseeable future.

The decision was based in part on the Snapshot removal, too. Some of the arguments were:

4.9 with Snapshots

  • 4.9 is well established
  • Snapshot exists
  • only few changes necessary

5.0 with SnapshotVotes

  • newest version for new storage layout → great for longevity
  • newest version -> might come with new vulnerabilities
  • changes necessary to replicate ERC20Snapshots with ERC20Votes
  • new also for auditors
  • +1 week work, more code to audit

@malteish malteish closed this Oct 30, 2023
@malteish malteish deleted the feature/updateToOpenzeppelinV5 branch November 20, 2023 07:49
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.

None yet

3 participants