-
Notifications
You must be signed in to change notification settings - Fork 42
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: Configure Fast Withdrawals using Safe #177
base: main
Are you sure you want to change the base?
feat: Configure Fast Withdrawals using Safe #177
Conversation
94615df
to
4cd0a06
Compare
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 @InoMurko !
I've left some initial comments about the general structure, let me know what you think 🙏
@@ -35,5 +35,10 @@ | |||
"**/@wagmi/cli/viem/ws": "8.17.1", | |||
"**/@ethersproject/providers/ws": "7.5.10", | |||
"**/elliptic": "6.5.7" | |||
}, | |||
"dependencies": { |
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.
protocol-kit
is already in src/package.json
. The other two should go inside the examples folder, since they are only used there.
|
||
|
||
|
||
## Multisig ownership |
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.
Since this flow is quite different than the current one, I think it makes sense to create a new folder in examples
. The current folder can be called setup-fast-withdrawal-eoa
, and the new one can be called setup-fast-withdrawal-multisig
.
What do you think?
## Multisig ownership | ||
Beware: At least one of the signers needs to be an EOA account so that it can propose transactions through this script. | ||
|
||
1. Build this example: rm -rf dist/ && tsc --outDir dist && ls dist |
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 step is already provided in the script yarn dev
Beware: At least one of the signers needs to be an EOA account so that it can propose transactions through this script. | ||
|
||
1. Build this example: rm -rf dist/ && tsc --outDir dist && ls dist | ||
2. OWNER_1_ADDRESS_PRIVATE_KEY= PARENT_CHAIN_ID= SAFE_ADDRESS= FC_VALIDATORS='["0x1234567890123456789012345678901234567890"]' node ./dist/1-create_multisig.js |
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.
env variables should be referenced in the .env.example files. That way users can also choose to add them there instead of referencing them in the command line.
Maybe add notes to the ones that are used in specific steps.
Rollups tend to have ownership transferred to a Safe multi-signature account. These 4 scripts propose transactions to the Safe if there's at least one EOA account in the Safe ownership.
I had to change two types in
src/createSafePrepareTransactionRequest.ts
andsrc/setAnyTrustFastConfirmerPrepareTransactionRequest.ts
because they were too restrictive on the type of the account passed in.The first one is
creating the Safe
transaction. The owner here is the Safe that owns the Rollup. Hence there's no Private Key. The second one is setting thenew safe
as the Fast Confirmer, this is also sent from the Rollup Safe.