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

Make ERC1967Upgrades a library instead of an abstract contract #4325

Merged
merged 10 commits into from
Jun 15, 2023

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Jun 7, 2023

Fixes #4204
Fixes LIB-880

This PR requires 0.8.20, and forces it in ERC1967Upgrades.sol for the following bugfix:

ABI: Include events in the ABI that are emitted by a contract but defined outside of it.

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

@changeset-bot
Copy link

changeset-bot bot commented Jun 7, 2023

🦋 Changeset detected

Latest commit: 875b37b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Amxx Amxx force-pushed the refactor/ERC1967Upgrade-library branch from b2c680e to 33f0adc Compare June 7, 2023 19:05
@Amxx Amxx requested a review from frangio June 7, 2023 19:14
@frangio
Copy link
Contributor

frangio commented Jun 7, 2023

Sorry for asking this now but can you remind me why we want this to be a library? I don't see a clear benefit at the moment.

@Amxx
Copy link
Collaborator Author

Amxx commented Jun 8, 2023

  • It doesn't need to be a contract, because it does not include any public/external function.
  • It's just tooling. Close example is SafeERC20. Should SafeERC20 be an abstract contracts
  • if it's a contract, all the events it declares (or its parents declare) are included in the ABI. You end up with a uups proxy having BeaconUpgraded in its abi.
  • when we do partial transpilation, we will not want to transpile this one.

@frangio
Copy link
Contributor

frangio commented Jun 9, 2023

  • if it's a contract, all the events it declares (or its parents declare) are included in the ABI. You end up with a uups proxy having BeaconUpgraded in its abi.

Ok, this seems like one of the strongest reasons for me.

My main concern was that the functions implicitly access storage, which is not something I'd usually expect from a library. What do you think about this?

@Amxx
Copy link
Collaborator Author

Amxx commented Jun 9, 2023

I don't see that as an issue.

IMO it's explicit that the ERC1967Upgrade library provides helper to interact with the ERC1967 storage slots.

It's not like the abstract contracts was declaring the storage explicitely either.

@Amxx Amxx requested a review from ernestognw June 14, 2023 14:25
contracts/proxy/ERC1967/ERC1967Upgrade.sol Outdated Show resolved Hide resolved
contracts/proxy/ERC1967/ERC1967Upgrade.sol Outdated Show resolved Hide resolved
contracts/proxy/ERC1967/ERC1967Upgrade.sol Outdated Show resolved Hide resolved
.changeset/grumpy-worms-tease.md Outdated Show resolved Hide resolved
Comment on lines 112 to 116
// Upgrades from old implementations will perform a rollback test. This test requires the new
// implementation to upgrade back to the old, non-ERC1822 compliant, implementation. Removing
// this special case will break upgrade paths from old UUPS implementation to new ones.
if (StorageSlot.getBooleanSlot(_ROLLBACK_SLOT).value) {
_setImplementation(newImplementation);
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we should be able to remove this in 5.0 since we are not guaranteeing upgradeability from 4.x versions.

Additionally, I don't understand why this function is in ERC1967Upgrade. It's not really ERC-1967 and if we rename this to ERC1967Utils this becomes even clearer IMO.

It's not exactly ERC-1822 either because we never submitted an update to extend the ERC with the semantics of proxiableUUID we're using.

I don't see why we can't put this logic directly inside UUPSUpgradeable. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I implemented this change but I think it deserve a dedicated PR
(two small PR are easier to review then one big IMO)

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good.

@frangio frangio changed the title Make ERC1967Upgrades a library (instead of an abstract contract) Make ERC1967Upgrades a library instead of an abstract contract Jun 15, 2023
@Amxx Amxx requested review from frangio and ernestognw June 15, 2023 19:44
Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

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

LGTM 🙌

@@ -29,7 +29,7 @@ npx @openzeppelin/upgrade-safe-transpiler@latest -D \
-x 'contracts/proxy/**/*' \
-x '!contracts/proxy/Clones.sol' \
-x '!contracts/proxy/ERC1967/ERC1967Storage.sol' \
-x '!contracts/proxy/ERC1967/ERC1967Upgrade.sol' \
-x '!contracts/proxy/ERC1967/ERC1967Utils.sol' \
Copy link
Member

Choose a reason for hiding this comment

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

Aren't we ignoring libraries for transpilation already?

If so, let's also create an issue with good-first-issue label

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is part of the partial transpilation proposal

Copy link
Collaborator Author

@Amxx Amxx Jun 15, 2023

Choose a reason for hiding this comment

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

Basically the library are needed by the contracts, so if we don't transpile them we need to get them from somewhere.

The partial transpilation proposal is to not transpile the libraries and the interfaces, and import the vanila version instead. (this creates a dependency from the upgradeable contracts to the vanilla contracts)

@Amxx Amxx enabled auto-merge (squash) June 15, 2023 19:56
@Amxx Amxx disabled auto-merge June 15, 2023 20:00
@Amxx Amxx merged commit ff85c7b into OpenZeppelin:master Jun 15, 2023
@Amxx Amxx deleted the refactor/ERC1967Upgrade-library branch June 15, 2023 20:01
This was referenced Sep 10, 2024
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.

Turn ERC1967Upgrade into a library
3 participants