Skip to content

Use oz-upgrades-unsafe-allow-reachable in ERC1967Upgrade#3971

Merged
frangio merged 8 commits intomasterfrom
Amxx-patch-1
Jan 19, 2023
Merged

Use oz-upgrades-unsafe-allow-reachable in ERC1967Upgrade#3971
frangio merged 8 commits intomasterfrom
Amxx-patch-1

Conversation

@Amxx
Copy link
Copy Markdown
Collaborator

@Amxx Amxx commented Jan 18, 2023

This is needed to work with the latest version of the upgrade plugin.

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@Amxx Amxx requested a review from frangio January 18, 2023 21:10
@frangio
Copy link
Copy Markdown
Contributor

frangio commented Jan 18, 2023

This is needed to work with the latest version of the upgrade plugin.

A correction, this isn't needed, but it's an improvement to this library because the unsafe-allow is used more granularly.

I think this PR is doing the opposite of what we want. These functions by themselves are not safe. I was thinking it should be _upgradeToAndCallUUPS that should have unsafe-allow-reachable delegatecall, but the proxiableUUID check is not what makes it safe. What makes it safe is the onlyProxy modifier in UUPSUpgradeable, so the annotation should be placed in UUPSUpgradeable.upgradeTo[AndCall].

@frangio
Copy link
Copy Markdown
Contributor

frangio commented Jan 18, 2023

Looks good! I would actually update the changelog, it's important to mention that this requires a recent version of Upgrades Plugins (@openzeppelin/[email protected]).

@Amxx Amxx mentioned this pull request Jan 18, 2023
Closed
3 tasks
@Amxx Amxx marked this pull request as ready for review January 19, 2023 13:55
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Jan 19, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 654ef38

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 Minor

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

@frangio frangio enabled auto-merge (squash) January 19, 2023 21:53
@frangio frangio merged commit c404862 into master Jan 19, 2023
@frangio frangio deleted the Amxx-patch-1 branch January 19, 2023 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@frangio frangio frangio approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Amxx @frangio