-
Notifications
You must be signed in to change notification settings - Fork 204
Get ready for ownership changeover #401
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
base: main
Are you sure you want to change the base?
Conversation
🟡 Heimdall Review Status
|
…th ownership management
❌ Upgrade Structure Validation FailedThe validation check found issues with your upgrade folder structure. Please ensure:
Please check the CI logs above for specific error details and fix the issues before merging. 📖 See the validation documentation for more details. |
Not required for this PR. |
❌ Upgrade Structure Validation FailedThe validation check found issues with your upgrade folder structure. Please ensure:
Please check the CI logs above for specific error details and fix the issues before merging. 📖 See the validation documentation for more details. |
❌ Upgrade Structure Validation FailedThe validation check found issues with your upgrade folder structure. Please ensure:
Please check the CI logs above for specific error details and fix the issues before merging. 📖 See the validation documentation for more details. |
❌ Upgrade Structure Validation FailedThe validation check found issues with your upgrade folder structure. Please ensure:
Please check the CI logs above for specific error details and fix the issues before merging. 📖 See the validation documentation for more details. |
❌ Upgrade Structure Validation FailedThe validation check found issues with your upgrade folder structure. Please ensure:
Please check the CI logs above for specific error details and fix the issues before merging. 📖 See the validation documentation for more details. |
❌ Upgrade Structure Validation FailedThe validation check found issues with your upgrade folder structure. Please ensure:
Please check the CI logs above for specific error details and fix the issues before merging. 📖 See the validation documentation for more details. To disable the validation check please add a validation.yml file to your upgrade folder with 'disabled': true |
require(OWNERS_TO_REMOVE.length > 0); | ||
address prevOwner = SENTINEL_OWNERS; | ||
IGnosisSafe ownerSafe = IGnosisSafe(payable(OWNER_SAFE)); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check for owner changes here or is that pedantic?
require(keccak256(abi.encode(ownerSafe.getOwners())) == keccak256(abi.encode(EXISTING_OWNERS)), "Owner list changed");
require(!ownerSafe.isOwner(OWNERS_TO_ADD[index]), "New owner already owner"); | ||
// Prevent duplicates | ||
require(!expectedOwner[OWNERS_TO_ADD[index]], "Duplicate owner detected"); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check for whether the owner is the owner safe itself?
require(OWNERS_TO_ADD[i] != OWNER_SAFE, "Owner cannot be self");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a bad idea for next time!
address prevOwner = SENTINEL_OWNERS; | ||
IGnosisSafe ownerSafe = IGnosisSafe(payable(OWNER_SAFE)); | ||
|
||
for (uint256 i = OWNERS_TO_ADD.length; i > 0; i--) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why we're iterating backwards instead of forwards (i++) like with all the other for loops? Is this just a style thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because owners are stored in a linked list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally there's a lot of boilerplate here that doesn't seem necessary from my naive eyes. Feel like we could make our own separate repo for basenames multisig if we wanted and just optimize for our workflows so each new script diff is minimized to just the new thing we are doing for clarity. Would also let us share the same internals over time and make writing new scripts less risky inherently.
uint256 public immutable THRESHOLD; | ||
|
||
constructor() { | ||
OWNER_SAFE = vm.envAddress("OWNER_SAFE"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need to be env var? Would be easier to verify current owners are trying to remove if we had address in script
} | ||
} | ||
|
||
function _buildCalls() internal view override returns (IMulticall3.Call3Value[] memory) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
confused that I'm not seeing this used at all? Same for _postCheck
? I'd normally expect the script to be something like:
function run() public {
_buildCalls();
_postCheck();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
look under the hood at the multisig script infra
OWNERS_TO_REMOVE = abi.decode(jsonData.parseRaw(".OwnersToRemove"), (address[])); | ||
} | ||
|
||
function setUp() public { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would appreciate some more commenting to make it easier to skim and orient on the intent of each section to make it easier to verify we're doing what we think we want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For instance, I cannot tell easily why we are doing all these loops vs just simply:
calls = _buildCalls();
execute(calls);
postCheck();
Basenames ecosystem multisig is changing ownership. 6 previous signers are being swapped for 7 new signers.