From fca1828f75db25511d2da6d503b81c4df7810cfd Mon Sep 17 00:00:00 2001 From: Leszek Stachowski Date: Fri, 8 Nov 2024 13:33:57 +0100 Subject: [PATCH] refactor --- .../commands/governance/approve-l2.test.ts | 48 +++++++++- .../cli/src/commands/governance/approve.ts | 89 ++++++++----------- packages/cli/src/utils/cli.ts | 32 +++++++ packages/cli/src/utils/safe.ts | 69 ++++++++++++++ 4 files changed, 181 insertions(+), 57 deletions(-) create mode 100644 packages/cli/src/utils/safe.ts diff --git a/packages/cli/src/commands/governance/approve-l2.test.ts b/packages/cli/src/commands/governance/approve-l2.test.ts index 666726ab4..db328c804 100644 --- a/packages/cli/src/commands/governance/approve-l2.test.ts +++ b/packages/cli/src/commands/governance/approve-l2.test.ts @@ -633,7 +633,6 @@ testWithAnvilL2('governance:approve cmd', (web3: Web3) => { } `) - // We want to test if integration works for accounts that are not added to the node await testLocallyWithWeb3Node( Approve, [ @@ -648,6 +647,30 @@ testWithAnvilL2('governance:approve cmd', (web3: Web3) => { web3 ) + // Run the same command twice with same arguments to make sure it doesn't have any effect + await testLocallyWithWeb3Node( + Approve, + [ + '--from', + securityCouncilSafeSignatory1, + '--hotfix', + HOTFIX_HASH, + '--useSafe', + '--type', + 'securityCouncil', + ], + web3 + ) + + expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` + { + "approved": false, + "councilApproved": false, + "executed": false, + "executionTimeLimit": "0", + } + `) + // Make sure the account has enough balance to pay for the transaction await setBalance(web3, securityCouncilSafeSignatory2, BigInt(web3.utils.toWei('1', 'ether'))) await testLocallyWithWeb3Node( @@ -660,6 +683,7 @@ testWithAnvilL2('governance:approve cmd', (web3: Web3) => { '--useSafe', '--type', 'securityCouncil', + // We want to test if integration works for accounts that are not added to the node '--privateKey', securityCouncilSafeSignatory2PrivateKey, ], @@ -679,6 +703,24 @@ testWithAnvilL2('governance:approve cmd', (web3: Web3) => { expect(logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes))) .toMatchInlineSnapshot(` [ + [ + "Running Checks:", + ], + [ + " ✔ 0x6Ecbe1DB9EF729CBe972C83Fb886247691Fb6beb is security council safe signatory ", + ], + [ + " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already approved by security council ", + ], + [ + " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already executed ", + ], + [ + "All checks passed", + ], + [ + "txHash: 0xtxhash", + ], [ "Running Checks:", ], @@ -707,13 +749,13 @@ testWithAnvilL2('governance:approve cmd', (web3: Web3) => { " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already executed ", ], [ - "SendTransaction: approveTx", + "All checks passed", ], [ "txHash: 0xtxhash", ], [ - "All checks passed", + "txHash: 0xtxhash", ], ] `) diff --git a/packages/cli/src/commands/governance/approve.ts b/packages/cli/src/commands/governance/approve.ts index 76ca1727d..0a85e9fed 100644 --- a/packages/cli/src/commands/governance/approve.ts +++ b/packages/cli/src/commands/governance/approve.ts @@ -1,15 +1,19 @@ import { StrongAddress } from '@celo/base' import { CeloTransactionObject } from '@celo/connect' -import { CeloProvider } from '@celo/connect/lib/celo-provider' import { GovernanceWrapper } from '@celo/contractkit/lib/wrappers/Governance' import { MultiSigWrapper } from '@celo/contractkit/lib/wrappers/MultiSig' import { toBuffer } from '@ethereumjs/util' import { Flags } from '@oclif/core' -import Safe from '@safe-global/protocol-kit' +import Web3 from 'web3' import { BaseCommand } from '../../base' import { newCheckBuilder } from '../../utils/checks' -import { displaySendTx, failWith } from '../../utils/cli' +import { displaySendSafeTx, displaySendTx, failWith } from '../../utils/cli' import { CustomFlags } from '../../utils/command' +import { + createApproveSafeTransactionIfNotApproved, + createExecuteSafeTransactionIfThresholdMet, + createSafeFromWeb3, +} from '../../utils/safe' enum HotfixApprovalType { APPROVER = 'approver', @@ -33,9 +37,11 @@ export default class Approve extends BaseCommand { from: CustomFlags.address({ required: true, description: "Approver's address" }), useMultiSig: Flags.boolean({ description: 'True means the request will be sent through multisig.', + exclusive: ['useSafe'], }), useSafe: Flags.boolean({ description: 'True means the request will be sent through safe.', + exclusive: ['useMultiSig'], }), hotfix: Flags.string({ exclusive: ['proposalID'], @@ -79,6 +85,7 @@ export default class Approve extends BaseCommand { const approver = useMultiSig ? governanceApproverMultiSig!.address : account await addDefaultChecks( + await this.getWeb3(), checkBuilder, governance, isCel2, @@ -122,59 +129,33 @@ export default class Approve extends BaseCommand { failWith('Proposal ID or hotfix must be provided') } - // TODO find out if we need to use api-kit so that Safe UI can be used if (isCel2 && approvalType === 'securityCouncil' && useSafe) { const web3 = await this.getWeb3() + const safe = await createSafeFromWeb3(web3, account, await governance.getSecurityCouncil()) - if (!(web3.currentProvider instanceof CeloProvider)) { - throw new Error('Unexpected web3 provider') - } - - const protocolKit = await Safe.init({ - provider: web3.currentProvider.toEip1193Provider(), - signer: account, - safeAddress: await governance.getSecurityCouncil(), - }) - - const safeTransaction = await protocolKit.createTransaction({ - transactions: [ - { - to: governance.address, - data: governanceTx.txo.encodeABI(), - value: '0', - }, - ], - }) + const approveTx = await createApproveSafeTransactionIfNotApproved( + safe, + governanceTx, + governance.address, + account + ) - const txHash = await protocolKit.getTransactionHash(safeTransaction) - if (!(await protocolKit.getOwnersWhoApprovedTx(txHash)).includes(account)) { - await protocolKit.approveTransactionHash(txHash) + if (approveTx !== null) { + // @ts-expect-error + await displaySendSafeTx('approveTx', approveTx) } - if ( - (await protocolKit.getOwnersWhoApprovedTx(txHash)).length >= - (await protocolKit.getThreshold()) - ) { - const executeTxResponse = await protocolKit.executeTransaction(safeTransaction) - - if (!executeTxResponse.transactionResponse) { - throw new Error('Transaction failed') - } + const executeTx = await createExecuteSafeTransactionIfThresholdMet( + safe, + governanceTx, + governance.address + ) - if ( - 'wait' in (executeTxResponse.transactionResponse as any) && - typeof (executeTxResponse.transactionResponse as any).wait === 'function' - ) { - // @ts-expect-error - const receipt = await executeTxResponse.transactionResponse?.wait() - } + if (executeTx !== null) { + // @ts-expect-error + await displaySendSafeTx('executeTx', executeTx) } - - // TODO: displaySendTx - return - } - - if ( + } else if ( isCel2 && approvalType === 'securityCouncil' && useMultiSig && @@ -199,6 +180,7 @@ export default class Approve extends BaseCommand { } const addDefaultChecks = async ( + web3: Web3, checkBuilder: ReturnType, governance: GovernanceWrapper, isCel2: boolean, @@ -240,12 +222,11 @@ const addDefaultChecks = async ( }) } else if (useSafe) { checkBuilder.addCheck(`${account} is security council safe signatory`, async () => { - const protocolKit = await Safe.init({ - // @ts-expect-error - provider: governance.connection.web3.currentProvider.existingProvider.host, - // signer, - safeAddress: await governance.getSecurityCouncil(), - }) + const protocolKit = await createSafeFromWeb3( + web3, + account, + await governance.getSecurityCouncil() + ) return await protocolKit.isOwner(account) }) diff --git a/packages/cli/src/utils/cli.ts b/packages/cli/src/utils/cli.ts index 74e712f93..d9ef731e5 100644 --- a/packages/cli/src/utils/cli.ts +++ b/packages/cli/src/utils/cli.ts @@ -7,6 +7,7 @@ import { parseDecodedParams, } from '@celo/connect' import { Errors, ux } from '@oclif/core' +import Safe from '@safe-global/protocol-kit' import BigNumber from 'bignumber.js' import chalk from 'chalk' import { ethers } from 'ethers' @@ -21,6 +22,37 @@ export async function displayWeb3Tx(name: string, txObj: any, tx?: Omit +) { + ux.action.start(`Sending Transaction: ${name}`) + + try { + const txResult = await safeTx + + if (!txResult.transactionResponse) { + throw new Error('Transaction failed') + } + + if ( + 'wait' in (txResult.transactionResponse as any) && + typeof (txResult.transactionResponse as any).wait === 'function' + ) { + // @ts-expect-error example taken from the docs and works fine (covered by tests) + const receipt = await txResult.transactionResponse?.wait() + + printValueMap({ txHash: receipt.transactionHash }) + } + + ux.action.stop() + } catch (e) { + ux.action.stop(`failed: ${(e as Error).message}`) + throw e + } +} + // allows building a tx with ethers but signing and sending with celo Connection // cant use displaySendTx because it expects a CeloTransactionObject which isnt really possible to convert to from ethers export async function displaySendEthersTxViaCK( diff --git a/packages/cli/src/utils/safe.ts b/packages/cli/src/utils/safe.ts new file mode 100644 index 000000000..391f6ae66 --- /dev/null +++ b/packages/cli/src/utils/safe.ts @@ -0,0 +1,69 @@ +import { StrongAddress } from '@celo/base' +import { CeloTransactionObject } from '@celo/connect' +import { CeloProvider } from '@celo/connect/lib/celo-provider' +import Safe from '@safe-global/protocol-kit' + +export const createSafeFromWeb3 = async ( + web3: any, + signer: StrongAddress, + safeAddress: StrongAddress +) => { + if (!(web3.currentProvider instanceof CeloProvider)) { + throw new Error('Unexpected web3 provider') + } + + return await Safe.init({ + provider: web3.currentProvider.toEip1193Provider(), + signer, + safeAddress, + }) +} + +export const createApproveSafeTransactionIfNotApproved = async ( + safe: Safe, + tx: CeloTransactionObject, + toAddress: StrongAddress, + ownerAddress: StrongAddress +): Promise | null> => { + const safeTransaction = await safe.createTransaction({ + transactions: [ + { + to: toAddress, + data: tx.txo.encodeABI(), + value: '0', + }, + ], + }) + + const txHash = await safe.getTransactionHash(safeTransaction) + + if (!(await safe.getOwnersWhoApprovedTx(txHash)).includes(ownerAddress)) { + return safe.approveTransactionHash(txHash) + } + + return null +} + +export const createExecuteSafeTransactionIfThresholdMet = async ( + safe: Safe, + tx: CeloTransactionObject, + toAddress: StrongAddress +): Promise | null> => { + const safeTransaction = await safe.createTransaction({ + transactions: [ + { + to: toAddress, + data: tx.txo.encodeABI(), + value: '0', + }, + ], + }) + + const txHash = await safe.getTransactionHash(safeTransaction) + + if ((await safe.getOwnersWhoApprovedTx(txHash)).length >= (await safe.getThreshold())) { + return safe.executeTransaction(safeTransaction) + } + + return null +}