-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: add swap owner to allowlist, test #37
Conversation
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.
Looks good to me! Double checked the safe logic / function to confirm this is the appropriate function to approve.
Nice test!
assertEq(ISafe(safe).getOwners()[0], NEW_OWNER); | ||
} | ||
|
||
function _execSafeTx(ISafe safe, address to, uint256 value, bytes memory data, Enum.Operation operation) 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.
A future clean up could be adding this function to a base test contract helper since its in most test files
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 sure!
function removeOwner(address prevOwner, address owner, uint256 _threshold) external; | ||
|
||
function swapOwner(address prevOwner, address oldOwner, address newOwner) external; | ||
|
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.
This doesn't work so nicely with my print-state script, since it knows functions in the config contract but not sure we should make decisions based on that.
I can also update the print functionality to also get all functions from contracts in the src folder
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.
easy enough to change – moved it into the config file
No description provided.