-
Notifications
You must be signed in to change notification settings - Fork 8
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
[WIP] Feat/add safe scripts #21
base: main
Are you sure you want to change the base?
Conversation
…ail-recovery into feat/add-safe-scripts
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.
Thanks for raising this, a few additional comments:
- We have two scripts that are similar:
DeploySafeAccount.s.sol
andDeployAccount.s.sol
. We should consolidate this logic. Which one is the one that works? If only one works, we keep that and remove the other one. If both work, we should choose one - not currently opinionated on which. WithDeployAccount.s.sol
, that was one I wrote and the module installation is done separately inInstallModuleSafe7579Account.s.sol
- Can we add a comment to each deployment script outlining what the script does? Some of them are quite unwieldy and are difficult to understand
@@ -1,3 +1,5 @@ | |||
BASE_SEPOLIA_RPC_URL= | |||
PRIVATE_KEY= | |||
BASE_SCAN_API_KEY= | |||
SAFE_ACCOUNT_SALT=0x0000000000000000000000000000000000000000000000000000000000000004 # You need to update some of random salt to make it different | |||
SAFE_7579_ADOPTER= # You can get this via running the script/DeploySafeRecovery.s.sol |
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.
SAFE_7579_ADOPTER= # You can get this via running the script/DeploySafeRecovery.s.sol | |
SAFE_7579_ADAPTER= # You can get this via running the script/DeploySafeRecovery.s.sol |
@@ -28,6 +28,7 @@ | |||
"@matterlabs": "github:matter-labs/era-contracts", | |||
"@openzeppelin/contracts-upgradeable": "v5.0.1", | |||
"@rhinestone/modulekit": "github:rhinestonewtf/modulekit#test/safe-latest", | |||
"@safe-global/safe-contracts": "1.4.1-build.0", |
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.
Why did the safe contracts get added here? We can actually pull them in without doing this
See remappings
See example
@@ -68,7 +68,7 @@ library GuardianUtils { | |||
revert IncorrectNumberOfWeights(); | |||
} | |||
|
|||
if (threshold == 0) { | |||
if (threshold == 0 && guardianCount > 0) { |
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.
Why did this get added? Can this be removed?
validators[0] = ModuleInit({ module: validatorAddr, initData: abi.encode(accountOwner) }); | ||
|
||
ModuleInit[] memory executors = new ModuleInit[](0); | ||
// managerAddr = vm.envOr("RECOVERY_MANAGER", address(0)); |
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.
Can we remove all the commented out code in all of the deployment scripts?
No description provided.