Skip to content

Conversation

@elisafalk
Copy link
Contributor

This pull request addresses issue #144 by modifying the Governor contract to utilize the onlyGovernance modifier for the _authorizeUpgrade function when the contract is set as UUPS upgradeable.

Changes Made:

  • Updated the setUpgradeableGovernor function in set-upgradeable.ts to replace the onlyOwner modifier with onlyGovernance for the _authorizeUpgrade function.
  • Ensured that the UUPS upgradeability is correctly implemented in the Governor contract, allowing for governance-based upgrades.

Fixes #144

@elisafalk
Copy link
Contributor Author

@ericglau I was having some conflict in the previous PR so I had to close it. Here is a new PR where the master branch is up to date. Please review and let me know if I need to change anything.

Copy link
Member

@ericglau ericglau left a comment

Choose a reason for hiding this comment

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

…mon logic into helper functions for improved code clarity and maintainability.
@elisafalk
Copy link
Contributor Author

@ericglau Could you kindly check if everything is alright now?

@ericglau ericglau changed the title Refactor upgradeable functionality in governor.ts and set-upgradeable.ts Use onlyGovernance to restrict upgrades for Governor with UUPS May 14, 2025
Copy link
Member

@ericglau ericglau left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. I pushed some refactors and fixed the tests. We'll do some additional reviews before merging.

@ericglau ericglau requested review from a team and CoveMB May 14, 2025 03:03
@elisafalk
Copy link
Contributor Author

@ericglau @CoveMB thanks for the feedback. Please merge this PR if it looks correct.

Copy link
Contributor

@CoveMB CoveMB left a comment

Choose a reason for hiding this comment

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

Thanks for contributing

@elisafalk
Copy link
Contributor Author

@ericglau @CoveMB Hope you are having a good day. Thank you so much for your guidance in resolving this issue — it’s been a great learning experience and has definitely boosted my confidence to take on more issues. If everything looks good, could you kindly proceed with the merge?
Thanks again!

@CoveMB CoveMB merged commit 1ccba07 into OpenZeppelin:master May 21, 2025
14 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators May 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use onlyGovernance for Governor with UUPS

3 participants