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.

Sign up for GitHub

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

Jump to bottom

(groups): upgradeableRenounceableProxy for group policies #78

Merged
merged 16 commits into from
Nov 18, 2024
Merged

(groups): upgradeableRenounceableProxy for group policies #78

merged 16 commits into from
Nov 18, 2024

Conversation

benjaminbollen
Copy link
Member

@benjaminbollen benjaminbollen commented Oct 14, 2024 •
edited
Loading

  • built on open zeppelin ERC1967
  • the deployer of the proxy is set as the admin
  • only admin can call upgradeToAndCall(address newImplementation, bytes data) -- for our use, leave this data empty @vanshika-srivastava
  • because we want policies to be reliable, the proxy can be "renounced", meaning that the implementation can no longer be upgraded (admin is set to address(0x01) )

@cducrest
Copy link

Looks good to me

@benjaminbollen benjaminbollen mentioned this pull request Oct 30, 2024
Closed
3 tasks
@roleengineer roleengineer self-requested a review November 6, 2024 16:13
roleengineer
roleengineer reviewed Nov 8, 2024
View reviewed changes
Copy link
Contributor

@roleengineer roleengineer left a comment

Choose a reason for hiding this comment

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

Upgradeable renounceable proxy implementation in general LGTM. I suggest to reduce external functions inside the proxy to avoid selector clashes.

src/groups/UpgradeableRenounceableProxy.sol Show resolved Hide resolved
src/groups/UpgradeableRenounceableProxy.sol Outdated Show resolved Hide resolved
roleengineer
roleengineer previously approved these changes Nov 8, 2024
View reviewed changes
Copy link
Contributor

@roleengineer roleengineer left a comment

Choose a reason for hiding this comment

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

Approved in advance. LGTM.

benjaminbollen and others added 2 commits November 8, 2024 16:24
@benjaminbollen @roleengineer
Update src/groups/UpgradeableRenounceableProxy.sol
ad5f87c 
Co-authored-by: Yevgeniy <35062472+roleengineer@users.noreply.github.com>
@benjaminbollen @roleengineer
Update src/groups/UpgradeableRenounceableProxy.sol
be1657c 
Co-authored-by: Yevgeniy <35062472+roleengineer@users.noreply.github.com>
@benjaminbollen
Copy link
Member Author

committed suggestions to branch; will continue with unit tests on this branch to test

@benjaminbollen benjaminbollen changed the base branch from develop to beta November 14, 2024 18:12
@benjaminbollen benjaminbollen mentioned this pull request Nov 18, 2024
Closed
roleengineer
roleengineer approved these changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@roleengineer roleengineer left a comment

Choose a reason for hiding this comment

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

In general LGTM, can be merged.
As a next step, I would recommend testing the accessibility of the implementation functions by the admin. A simple mock policy implementation with an admin whitelist function should work.

test/groups/upgradeableProxy/adminOperationsUpgradeableRenounceableProxy.t.sol Outdated Show resolved Hide resolved
benjaminbollen and others added 2 commits November 18, 2024 17:00
@benjaminbollen @roleengineer
Update test/groups/upgradeableProxy/adminOperationsUpgradeableRenounc…
e63bc6c 
…eableProxy.t.sol

Co-authored-by: Yevgeniy <35062472+roleengineer@users.noreply.github.com>
@benjaminbollen
(test/groups): forge fmt
723ea36
@benjaminbollen benjaminbollen merged commit 91cb016 into beta Nov 18, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roleengineer roleengineer roleengineer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Circles Protocol - Hardening contracts for Stable
Status: 💪Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@benjaminbollen @cducrest @roleengineer