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

Use immutable beacon address in BeaconProxy #4435

Merged
merged 11 commits into from
Jul 8, 2023

Conversation

ericglau
Copy link
Member

@ericglau ericglau commented Jul 7, 2023

Fixes #2538

Uses an immutable variable to store the beacon's address in BeaconProxy, in addition to the beacon storage slot. This avoids an extra SLOAD when retrieving the beacon address before each delegate call.

The immutable variable is used in _implementation() to read the beacon address before each delegate call.
The beacon slot is still written during construction, so that the beacon address can be read externally according to ERC-1967. This beacon slot is required because the immutable variable must be private to avoid generating a getter function that could clash with a function from the implementation.

Rationale:

  • Although there is an upgradeBeaconToAndCall internal function in ERC1967Utils which sets the beacon slot, that function is not exposed in BeaconProxy but is only called on construction. The BeaconProxy itself does not provide a way to modify the beacon address after the BeaconProxy is deployed.
  • Therefore, we should not need to continue reading the beacon slot before every delegate call during the proxy's fallback function. We can use an immutable variable instead.

PR Checklist

  • Tests (not applicable as this only changes the proxy's internal implementation)
  • Documentation (updated comments)
  • Changeset entry (run npx changeset add)

@changeset-bot
Copy link

changeset-bot bot commented Jul 7, 2023

🦋 Changeset detected

Latest commit: 6e2594e

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 Patch

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 previously approved these changes Jul 8, 2023
ericglau and others added 2 commits July 8, 2023 15:35
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

I've updated docs to describe the potential issue with upgradeToAndCall.

@Amxx Amxx merged commit 5229b75 into OpenZeppelin:master Jul 8, 2023
@gitpoap-bot
Copy link

gitpoap-bot bot commented Jul 8, 2023

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2023 OpenZeppelin Contracts Contributor:

GitPOAP: 2023 OpenZeppelin Contracts Contributor GitPOAP Badge

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

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.

Use immutable variable to store beacon address in BeaconProxy
3 participants