diff --git a/.changeset/celocli-governance-approve-flags.md b/.changeset/celocli-governance-approve-flags.md new file mode 100644 index 000000000..d6b4b8e33 --- /dev/null +++ b/.changeset/celocli-governance-approve-flags.md @@ -0,0 +1,11 @@ +--- +'@celo/celocli': minor +--- + +Add new flags to `governance:approve` command for better multisig transaction control: + +- `--submit`: Force submission of approval transaction to multisig without checking for prior confirmations onchain. Use with caution - this bypasses the check for existing submissions. Example: `celocli governance:approve --proposalID 99 --from 0x... --useMultiSig --submit` + +- `--multisigTx`: Specify exact multisig transaction ID to confirm, rather than searching onchain. Useful when you know the transaction ID from offchain sources. Example: `celocli governance:approve --proposalID 99 --from 0x... --useMultiSig --multisigTx 5` + +Both flags depend on `--proposalID` and `--useMultiSig` being provided. diff --git a/.changeset/contractkit-submit-transaction.md b/.changeset/contractkit-submit-transaction.md new file mode 100644 index 000000000..7f29197a5 --- /dev/null +++ b/.changeset/contractkit-submit-transaction.md @@ -0,0 +1,5 @@ +--- +'@celo/contractkit': patch +--- + +Add `submitTransaction` method to `MultiSigWrapper` to submit transactions to multisig without automatic confirmation. This complements the existing `submitOrConfirmTransaction` method by providing more granular control over the submission process. diff --git a/.changeset/open-donkeys-nail.md b/.changeset/open-donkeys-nail.md new file mode 100644 index 000000000..a9c42b152 --- /dev/null +++ b/.changeset/open-donkeys-nail.md @@ -0,0 +1,6 @@ +--- +'@celo/contractkit': patch +'@celo/celocli': patch +--- + +Updates logic for submiting/confirming transaction with multisig to only search thru non executed transactions diff --git a/packages/cli/src/commands/governance/approve.test.ts b/packages/cli/src/commands/governance/approve.test.ts index 4f962ebe7..2e3bb2db9 100644 --- a/packages/cli/src/commands/governance/approve.test.ts +++ b/packages/cli/src/commands/governance/approve.test.ts @@ -1,6 +1,7 @@ import { hexToBuffer, StrongAddress } from '@celo/base' import { CeloProvider } from '@celo/connect/lib/celo-provider' import { newKitFromWeb3 } from '@celo/contractkit' +import { GovernanceWrapper } from '@celo/contractkit/lib/wrappers/Governance' import { DEFAULT_OWNER_ADDRESS, setBalance, @@ -13,13 +14,22 @@ import Safe, { PredictedSafeProps, SafeAccountConfig, } from '@safe-global/protocol-kit' +import BigNumber from 'bignumber.js' +import fetch from 'cross-fetch' import Web3 from 'web3' import { changeMultiSigOwner } from '../../test-utils/chain-setup' -import { stripAnsiCodesAndTxHashes, testLocallyWithWeb3Node } from '../../test-utils/cliUtils' +import { + stripAnsiCodesAndTxHashes, + stripAnsiCodesFromNestedArray, + testLocallyWithWeb3Node, +} from '../../test-utils/cliUtils' import { deployMultiCall } from '../../test-utils/multicall' -import { setupSafeContracts } from '../../test-utils/multisigUtils' +import { createMultisig, setupSafeContracts } from '../../test-utils/multisigUtils' import Approve from './approve' +// Mock fetch for HTTP status tests +jest.mock('cross-fetch') + process.env.NO_SYNCCHECK = 'true' testWithAnvilL2( @@ -88,34 +98,34 @@ testWithAnvilL2( ).rejects.toThrow("Some checks didn't pass!") expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "approved": false, - "councilApproved": false, - "executed": false, - "executionTimeLimit": "0", - } - `) + { + "approved": false, + "councilApproved": false, + "executed": false, + "executionTimeLimit": "0", + } + `) expect( logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) ).toMatchInlineSnapshot(` - [ - [ - "Running Checks:", - ], - [ - " ✔ 0xf750153fc4211e4Ef325A7fD87d8258222e0b510 is security council address ", - ], - [ - " ✘ 0x5409ED021D9299bf6814279A6A1411A7e866A631 is security council multisig signatory ", - ], - [ - " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already approved by security council ", - ], - [ - " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already executed ", - ], - ] - `) + [ + [ + "Running Checks:", + ], + [ + " ✔ 0xf750153fc4211e4Ef325A7fD87d8258222e0b510 is security council address ", + ], + [ + " ✘ 0x5409ED021D9299bf6814279A6A1411A7e866A631 is security council multisig signatory ", + ], + [ + " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already approved by security council ", + ], + [ + " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already executed ", + ], + ] + `) expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) }) @@ -135,34 +145,34 @@ testWithAnvilL2( ).rejects.toThrow("Some checks didn't pass!") expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "approved": false, - "councilApproved": false, - "executed": false, - "executionTimeLimit": "0", - } - `) + { + "approved": false, + "councilApproved": false, + "executed": false, + "executionTimeLimit": "0", + } + `) expect( logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) ).toMatchInlineSnapshot(` - [ - [ - "Running Checks:", - ], - [ - " ✔ 0xf750153fc4211e4Ef325A7fD87d8258222e0b510 is approver address ", - ], - [ - " ✘ 0x5409ED021D9299bf6814279A6A1411A7e866A631 is approver multisig signatory ", - ], - [ - " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already approved ", - ], - [ - " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already executed ", - ], - ] - `) + [ + [ + "Running Checks:", + ], + [ + " ✔ 0xf750153fc4211e4Ef325A7fD87d8258222e0b510 is approver address ", + ], + [ + " ✘ 0x5409ED021D9299bf6814279A6A1411A7e866A631 is approver multisig signatory ", + ], + [ + " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already approved ", + ], + [ + " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already executed ", + ], + ] + `) expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) }) @@ -204,31 +214,31 @@ testWithAnvilL2( ).rejects.toThrow("Some checks didn't pass!") expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "approved": false, - "councilApproved": false, - "executed": false, - "executionTimeLimit": "0", - } - `) + { + "approved": false, + "councilApproved": false, + "executed": false, + "executionTimeLimit": "0", + } + `) expect( logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) ).toMatchInlineSnapshot(` - [ - [ - "Running Checks:", - ], - [ - " ✘ 0xE36Ea790bc9d7AB70C55260C66D52b1eca985f84 is security council address ", - ], - [ - " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already approved by security council ", - ], - [ - " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already executed ", - ], - ] - `) + [ + [ + "Running Checks:", + ], + [ + " ✘ 0xE36Ea790bc9d7AB70C55260C66D52b1eca985f84 is security council address ", + ], + [ + " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already approved by security council ", + ], + [ + " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already executed ", + ], + ] + `) expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) }) @@ -266,31 +276,31 @@ testWithAnvilL2( ).rejects.toThrow("Some checks didn't pass!") expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "approved": false, - "councilApproved": false, - "executed": false, - "executionTimeLimit": "0", - } - `) + { + "approved": false, + "councilApproved": false, + "executed": false, + "executionTimeLimit": "0", + } + `) expect( logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) ).toMatchInlineSnapshot(` - [ - [ - "Running Checks:", - ], - [ - " ✘ 0xE36Ea790bc9d7AB70C55260C66D52b1eca985f84 is approver address ", - ], - [ - " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already approved ", - ], - [ - " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already executed ", - ], - ] - `) + [ + [ + "Running Checks:", + ], + [ + " ✘ 0xE36Ea790bc9d7AB70C55260C66D52b1eca985f84 is approver address ", + ], + [ + " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already approved ", + ], + [ + " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already executed ", + ], + ] + `) expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) }) @@ -330,47 +340,47 @@ testWithAnvilL2( ) expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "approved": false, - "councilApproved": true, - "executed": false, - "executionTimeLimit": "0", - } - `) + { + "approved": false, + "councilApproved": true, + "executed": false, + "executionTimeLimit": "0", + } + `) expect( logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) ).toMatchInlineSnapshot(` - [ - [ - "Running Checks:", - ], - [ - " ✔ 0x6Ecbe1DB9EF729CBe972C83Fb886247691Fb6beb is security council address ", - ], - [ - " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already approved by security council ", - ], - [ - " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already executed ", - ], - [ - "All checks passed", - ], - [ - "SendTransaction: approveTx", - ], - [ - "txHash: 0xtxhash", - ], - [ - "HotfixApproved:", - ], - [ - "hash: 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d - approver: 0x6Ecbe1DB9EF729CBe972C83Fb886247691Fb6beb", - ], - ] - `) + [ + [ + "Running Checks:", + ], + [ + " ✔ 0x6Ecbe1DB9EF729CBe972C83Fb886247691Fb6beb is security council address ", + ], + [ + " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already approved by security council ", + ], + [ + " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already executed ", + ], + [ + "All checks passed", + ], + [ + "SendTransaction: approveTx", + ], + [ + "txHash: 0xtxhash", + ], + [ + "HotfixApproved:", + ], + [ + "hash: 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d + approver: 0x6Ecbe1DB9EF729CBe972C83Fb886247691Fb6beb", + ], + ] + `) expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) }) @@ -406,47 +416,47 @@ testWithAnvilL2( await testLocallyWithWeb3Node(Approve, ['--from', approver, '--hotfix', HOTFIX_HASH], web3) expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "approved": true, - "councilApproved": false, - "executed": false, - "executionTimeLimit": "0", - } - `) + { + "approved": true, + "councilApproved": false, + "executed": false, + "executionTimeLimit": "0", + } + `) expect( logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) ).toMatchInlineSnapshot(` - [ - [ - "Running Checks:", - ], - [ - " ✔ 0x5409ED021D9299bf6814279A6A1411A7e866A631 is approver address ", - ], - [ - " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already approved ", - ], - [ - " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already executed ", - ], - [ - "All checks passed", - ], - [ - "SendTransaction: approveTx", - ], - [ - "txHash: 0xtxhash", - ], - [ - "HotfixApproved:", - ], - [ - "hash: 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d - approver: 0x5409ED021D9299bf6814279A6A1411A7e866A631", - ], - ] - `) + [ + [ + "Running Checks:", + ], + [ + " ✔ 0x5409ED021D9299bf6814279A6A1411A7e866A631 is approver address ", + ], + [ + " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already approved ", + ], + [ + " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already executed ", + ], + [ + "All checks passed", + ], + [ + "SendTransaction: approveTx", + ], + [ + "txHash: 0xtxhash", + ], + [ + "HotfixApproved:", + ], + [ + "hash: 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d + approver: 0x5409ED021D9299bf6814279A6A1411A7e866A631", + ], + ] + `) expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) }) @@ -503,44 +513,44 @@ testWithAnvilL2( ) expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "approved": false, - "councilApproved": true, - "executed": false, - "executionTimeLimit": "0", - } - `) + { + "approved": false, + "councilApproved": true, + "executed": false, + "executionTimeLimit": "0", + } + `) expect( logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) ).toMatchInlineSnapshot(` - [ - [ - "Running Checks:", - ], - [ - " ✔ 0xf750153fc4211e4Ef325A7fD87d8258222e0b510 is security council address ", - ], - [ - " ✔ 0x5409ED021D9299bf6814279A6A1411A7e866A631 is security council multisig signatory ", - ], - [ - " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already approved by security council ", - ], - [ - " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already executed ", - ], - [ - "All checks passed", - ], - [ - "SendTransaction: approveTx", - ], - [ - "txHash: 0xtxhash", - ], - ] - `) + [ + [ + "Running Checks:", + ], + [ + " ✔ 0xf750153fc4211e4Ef325A7fD87d8258222e0b510 is security council address ", + ], + [ + " ✔ 0x5409ED021D9299bf6814279A6A1411A7e866A631 is security council multisig signatory ", + ], + [ + " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already approved by security council ", + ], + [ + " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already executed ", + ], + [ + "All checks passed", + ], + [ + "SendTransaction: approveTx", + ], + [ + "txHash: 0xtxhash", + ], + ] + `) expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) }) @@ -618,13 +628,13 @@ testWithAnvilL2( ]) expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "approved": false, - "councilApproved": false, - "executed": false, - "executionTimeLimit": "0", - } - `) + { + "approved": false, + "councilApproved": false, + "executed": false, + "executionTimeLimit": "0", + } + `) await testLocallyWithWeb3Node( Approve, @@ -656,13 +666,13 @@ testWithAnvilL2( ) expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "approved": false, - "councilApproved": false, - "executed": false, - "executionTimeLimit": "0", - } - `) + { + "approved": false, + "councilApproved": false, + "executed": false, + "executionTimeLimit": "0", + } + `) // Make sure the account has enough balance to pay for the transaction await setBalance( @@ -688,74 +698,74 @@ testWithAnvilL2( ) expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "approved": false, - "councilApproved": true, - "executed": false, - "executionTimeLimit": "0", - } - `) + { + "approved": false, + "councilApproved": true, + "executed": false, + "executionTimeLimit": "0", + } + `) 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:", - ], - [ - " ✔ 0x6Ecbe1DB9EF729CBe972C83Fb886247691Fb6beb is security council safe signatory ", - ], - [ - " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already approved by security council ", - ], - [ - " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already executed ", - ], - [ - "All checks passed", - ], - [ - "Running Checks:", - ], - [ - " ✔ 0x6C666E57A5E8715cFE93f92790f98c4dFf7b69e2 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", - ], - [ - "txHash: 0xtxhash", - ], - ] - `) + [ + [ + "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:", + ], + [ + " ✔ 0x6Ecbe1DB9EF729CBe972C83Fb886247691Fb6beb is security council safe signatory ", + ], + [ + " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already approved by security council ", + ], + [ + " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already executed ", + ], + [ + "All checks passed", + ], + [ + "Running Checks:", + ], + [ + " ✔ 0x6C666E57A5E8715cFE93f92790f98c4dFf7b69e2 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", + ], + [ + "txHash: 0xtxhash", + ], + ] + `) expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) }) @@ -777,43 +787,43 @@ testWithAnvilL2( ) expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "approved": true, - "councilApproved": false, - "executed": false, - "executionTimeLimit": "0", - } - `) + { + "approved": true, + "councilApproved": false, + "executed": false, + "executionTimeLimit": "0", + } + `) expect( logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) ).toMatchInlineSnapshot(` - [ - [ - "Running Checks:", - ], - [ - " ✔ 0xf750153fc4211e4Ef325A7fD87d8258222e0b510 is approver address ", - ], - [ - " ✔ 0x5409ED021D9299bf6814279A6A1411A7e866A631 is approver multisig signatory ", - ], - [ - " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already approved ", - ], - [ - " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already executed ", - ], - [ - "All checks passed", - ], - [ - "SendTransaction: approveTx", - ], - [ - "txHash: 0xtxhash", - ], - ] - `) + [ + [ + "Running Checks:", + ], + [ + " ✔ 0xf750153fc4211e4Ef325A7fD87d8258222e0b510 is approver address ", + ], + [ + " ✔ 0x5409ED021D9299bf6814279A6A1411A7e866A631 is approver multisig signatory ", + ], + [ + " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already approved ", + ], + [ + " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already executed ", + ], + [ + "All checks passed", + ], + [ + "SendTransaction: approveTx", + ], + [ + "txHash: 0xtxhash", + ], + ] + `) expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) }) @@ -866,46 +876,489 @@ testWithAnvilL2( ) expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "approved": false, - "councilApproved": true, - "executed": false, - "executionTimeLimit": "0", - } - `) + { + "approved": false, + "councilApproved": true, + "executed": false, + "executionTimeLimit": "0", + } + `) + expect( + logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) + ).toMatchInlineSnapshot(` + [ + [ + "Running Checks:", + ], + [ + " ✔ 0xf750153fc4211e4Ef325A7fD87d8258222e0b510 is security council address ", + ], + [ + " ✔ 0x5409ED021D9299bf6814279A6A1411A7e866A631 is security council multisig signatory ", + ], + [ + " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already approved by security council ", + ], + [ + " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already executed ", + ], + [ + "All checks passed", + ], + [ + "SendTransaction: approveTx", + ], + [ + "txHash: 0xtxhash", + ], + ] + `) + expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) + }) + }) + + describe('proposal approval', () => { + let proposalId: BigNumber.Value + let governance: GovernanceWrapper + let accounts: StrongAddress[] + + beforeEach(async () => { + accounts = (await web3.eth.getAccounts()) as StrongAddress[] + const kit = newKitFromWeb3(web3) + governance = await kit.contracts.getGovernance() + + // Create and dequeue a proposal + const minDeposit = (await governance.minDeposit()).toString() + await governance + .propose([], 'https://example.com/proposal') + .sendAndWaitForReceipt({ from: accounts[0], value: minDeposit }) + + proposalId = new BigNumber(1) + + // Dequeue the proposal + const dequeueFrequency = (await governance.dequeueFrequency()).toNumber() + const { timeTravel } = await import('@celo/dev-utils/ganache-test') + await timeTravel(dequeueFrequency + 1, web3) + await governance.dequeueProposalsIfReady().sendAndWaitForReceipt({ from: accounts[0] }) + + // Make accounts[0] the multisig owner + await changeMultiSigOwner(kit, accounts[0]) + }) + + it('should approve proposal using multisig when address is approver multisig signatory', async () => { + const writeMock = jest.spyOn(ux.write, 'stdout') + const logMock = jest.spyOn(console, 'log') + + await testLocallyWithWeb3Node( + Approve, + ['--from', accounts[0], '--proposalID', proposalId.toString(), '--useMultiSig'], + web3 + ) + + expect(await governance.isApproved(proposalId)).toBe(true) + expect( + logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) + ).toMatchInlineSnapshot(` + [ + [ + "Running Checks:", + ], + [ + " ✔ 0xf750153fc4211e4Ef325A7fD87d8258222e0b510 is approver address ", + ], + [ + " ✔ 0x5409ED021D9299bf6814279A6A1411A7e866A631 is multisig signatory ", + ], + [ + " ✔ 1 is an existing proposal ", + ], + [ + " ✔ 1 is in stage Referendum or Execution ", + ], + [ + " ✔ 1 not already approved ", + ], + [ + "All checks passed", + ], + [ + "SendTransaction: approveTx", + ], + [ + "txHash: 0xtxhash", + ], + ] + `) + expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) + }) + + it('approve proposal using --submit flag fails if already submitted', async () => { + const logMock = jest.spyOn(console, 'log') + + // Mock the fetch to return that proposal has already been submitted + ;(fetch as jest.Mock).mockResolvedValue({ + ok: true, + json: async () => ({ + proposalId: 1, + count: 1, + approvals: [ + { + approver: accounts[0], + multisigTx: 0, + confirmedAt: Date.now(), + blockNumber: 100, + transactionHash: '0xabcd', + }, + ], + }), + }) + + await expect( + testLocallyWithWeb3Node( + Approve, + [ + '--from', + accounts[0], + '--proposalID', + proposalId.toString(), + '--useMultiSig', + '--submit', + ], + web3 + ) + ).rejects.toThrow("Some checks didn't pass!") + expect( logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) ).toMatchInlineSnapshot(` - [ - [ - "Running Checks:", - ], - [ - " ✔ 0xf750153fc4211e4Ef325A7fD87d8258222e0b510 is security council address ", - ], - [ - " ✔ 0x5409ED021D9299bf6814279A6A1411A7e866A631 is security council multisig signatory ", - ], - [ - " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already approved by security council ", - ], - [ - " ✔ Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already executed ", - ], - [ - "All checks passed", - ], [ - "SendTransaction: approveTx", - ], + [ + "Running Checks:", + ], + [ + " ✔ 0xf750153fc4211e4Ef325A7fD87d8258222e0b510 is approver address ", + ], + [ + " ✔ 0x5409ED021D9299bf6814279A6A1411A7e866A631 is multisig signatory ", + ], + [ + " ✔ 1 is an existing proposal ", + ], + [ + " ✔ 1 is in stage Referendum or Execution ", + ], + [ + " ✔ 1 not already approved ", + ], + [ + " ✘ Proposal has not been submitted to multisig ", + ], + ] + `) + }) + it('approve proposal using --submit flag can succeed', async () => { + const logMock = jest.spyOn(console, 'log') + + // Mock the fetch to return that proposal has already been submitted + ;(fetch as jest.Mock).mockResolvedValue({ + ok: true, + json: async () => ({ + proposalId: proposalId, + count: 0, + approvals: [], + }), + }) + + await expect( + testLocallyWithWeb3Node( + Approve, + [ + '--from', + accounts[0], + '--proposalID', + proposalId.toString(), + '--useMultiSig', + '--submit', + ], + web3 + ) + ).resolves.toBeUndefined() + + expect( + logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) + ).toMatchInlineSnapshot(` + [ + [ + "Running Checks:", + ], + [ + " ✔ 0xf750153fc4211e4Ef325A7fD87d8258222e0b510 is approver address ", + ], + [ + " ✔ 0x5409ED021D9299bf6814279A6A1411A7e866A631 is multisig signatory ", + ], + [ + " ✔ 1 is an existing proposal ", + ], + [ + " ✔ 1 is in stage Referendum or Execution ", + ], + [ + " ✔ 1 not already approved ", + ], + [ + " ✔ Proposal has not been submitted to multisig ", + ], + [ + "All checks passed", + ], + [ + "SendTransaction: approveTx", + ], + [ + "txHash: 0xtxhash", + ], + ] + `) + }) + + it('should confirm existing multisig transaction when --multisigTx is provided', async () => { + const logMock = jest.spyOn(console, 'log') + const kit = newKitFromWeb3(web3) + + // Create a 2-signer multisig so the transaction won't execute immediately + const twoSignerMultisig = await createMultisig(kit, [accounts[0], accounts[1]], 2, 2) + + // Set the new multisig as the governance approver + await withImpersonatedAccount(web3, DEFAULT_OWNER_ADDRESS, async () => { + await ( + await kit.sendTransaction({ + to: governance.address, + from: DEFAULT_OWNER_ADDRESS, + data: `0x3156560e000000000000000000000000${twoSignerMultisig.replace('0x', '').toLowerCase()}`, + }) + ).waitReceipt() + }) + + // Get the new multisig wrapper + const multisig = await governance.getApproverMultisig() + + // First, submit the transaction to multisig from accounts[0] + // This won't execute because it requires 2 confirmations + const approveTx = await governance.approve(proposalId) + await ( + await multisig.submitTransaction(governance.address, approveTx.txo) + ).sendAndWaitForReceipt({ from: accounts[0] }) + + // Verify proposal is not yet approved + expect(await governance.isApproved(proposalId)).toBe(false) + + // Mock the fetch to return the multisigTx + ;(fetch as jest.Mock).mockResolvedValue({ + ok: true, + json: async () => ({ + proposalId: 1, + count: 1, + approvals: [ + { + approver: accounts[0], + multisigTx: 0, + confirmedAt: Date.now(), + blockNumber: 100, + transactionHash: '0xabcd', + }, + ], + }), + }) + + // Now confirm it with the multisigTx from accounts[1] + await expect( + testLocallyWithWeb3Node( + Approve, + [ + '--from', + accounts[1], + '--proposalID', + proposalId.toString(), + '--useMultiSig', + '--multisigTx', + '0', + ], + web3 + ) + ).resolves.toBeUndefined() + + // The proposal should now be approved + expect(await governance.isApproved(proposalId)).toBe(true) + expect( + logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) + ).toMatchInlineSnapshot(` + [ + [ + "Running Checks:", + ], + [ + " ✔ ${twoSignerMultisig} is approver address ", + ], + [ + " ✔ ${accounts[1]} is multisig signatory ", + ], + [ + " ✔ 1 is an existing proposal ", + ], + [ + " ✔ 1 is in stage Referendum or Execution ", + ], + [ + " ✔ 1 not already approved ", + ], + [ + " ✔ multisgTXId provided is valid ", + ], + [ + "All checks passed", + ], + [ + "SendTransaction: approveTx", + ], + [ + "txHash: 0xtxhash", + ], + ] + `) + }) + + it('should fail when invalid --multisigTx is provided', async () => { + const logMock = jest.spyOn(console, 'log') + + // Mock the fetch to return a different multisigTx + ;(fetch as jest.Mock).mockResolvedValue({ + ok: true, + json: async () => ({ + proposalId: 1, + count: 1, + approvals: [ + { + approver: accounts[0], + multisigTx: 5, // Different ID + confirmedAt: Date.now(), + blockNumber: 100, + transactionHash: '0xabcd', + }, + ], + }), + }) + + await expect( + testLocallyWithWeb3Node( + Approve, + [ + '--from', + accounts[0], + '--proposalID', + proposalId.toString(), + '--useMultiSig', + '--multisigTx', + '0', // Invalid ID + ], + web3 + ) + ).rejects.toThrow("Some checks didn't pass!") + + expect( + logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) + ).toMatchInlineSnapshot(` + [ + [ + "Running Checks:", + ], + [ + " ✔ 0xf750153fc4211e4Ef325A7fD87d8258222e0b510 is approver address ", + ], + [ + " ✔ 0x5409ED021D9299bf6814279A6A1411A7e866A631 is multisig signatory ", + ], + [ + " ✔ 1 is an existing proposal ", + ], + [ + " ✔ 1 is in stage Referendum or Execution ", + ], + [ + " ✔ 1 not already approved ", + ], + [ + " ✘ multisgTXId provided is valid ", + ], + ] + `) + }) + + it('should succeed without --submit when proposal was already submitted to multisig', async () => { + const logMock = jest.spyOn(console, 'log') + // Mock the fetch to return that proposal has already been submitted + ;(fetch as jest.Mock).mockResolvedValue({ + ok: true, + json: async () => ({ + proposalId: 1, + count: 1, + approvals: [ + { + approver: accounts[0], + multisigTx: 0, + confirmedAt: Date.now(), + blockNumber: 100, + transactionHash: '0xabcd', + }, + ], + }), + }) + + // Without --submit flag, this should work because the default behavior + // is submitOrConfirmTransaction which will confirm if it exists + await testLocallyWithWeb3Node( + Approve, + ['--from', accounts[0], '--proposalID', proposalId.toString(), '--useMultiSig'], + web3 + ) + + expect(stripAnsiCodesFromNestedArray(logMock.mock.calls)).toMatchInlineSnapshot(` [ - "txHash: 0xtxhash", - ], - ] - `) - expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) + [ + "Running Checks:", + ], + [ + " ✔ 0xf750153fc4211e4Ef325A7fD87d8258222e0b510 is approver address ", + ], + [ + " ✔ 0x5409ED021D9299bf6814279A6A1411A7e866A631 is multisig signatory ", + ], + [ + " ✔ 1 is an existing proposal ", + ], + [ + " ✔ 1 is in stage Referendum or Execution ", + ], + [ + " ✔ 1 not already approved ", + ], + [ + "All checks passed", + ], + [ + "SendTransaction: approveTx", + ], + [ + "txHash: 0xtxhash", + ], + ] + `) + + // Should succeed because submitOrConfirmTransaction finds and confirms the existing transaction + expect(await governance.isApproved(proposalId)).toBe(true) }) }) + afterEach(() => { jest.clearAllMocks() }) diff --git a/packages/cli/src/commands/governance/approve.ts b/packages/cli/src/commands/governance/approve.ts index 1db726913..72173cbe3 100644 --- a/packages/cli/src/commands/governance/approve.ts +++ b/packages/cli/src/commands/governance/approve.ts @@ -4,6 +4,10 @@ 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 fetch from 'cross-fetch' +import debugFactory from 'debug' +import { Hex } from 'viem' + import Web3 from 'web3' import { BaseCommand } from '../../base' import { newCheckBuilder } from '../../utils/checks' @@ -32,6 +36,17 @@ export default class Approve extends BaseCommand { description: 'UUID of proposal to approve', exclusive: ['hotfix'], }), + multisigTx: Flags.string({ + dependsOn: ['proposalID', 'useMultiSig'], + exclusive: ['hotfix', 'useSafe'], + description: + 'Optionally provide the exact multisig transaction ID to confirm. Otherwise will search onchain for transaction which matches the proposal call data.', + }), + submit: Flags.boolean({ + dependsOn: ['proposalID', 'useMultiSig'], + description: + 'Submit the approval transaction to multisig without checking for prior confirmations onchain. (Use with caution!)', + }), from: CustomFlags.address({ required: true, description: "Approver's address" }), useMultiSig: Flags.boolean({ description: 'True means the request will be sent through multisig.', @@ -107,6 +122,26 @@ export default class Approve extends BaseCommand { .proposalExists(id) .proposalInStages(id, ['Referendum', 'Execution']) .addCheck(`${id} not already approved`, async () => !(await governance.isApproved(id))) + .addConditionalCheck( + 'Proposal has not been submitted to multisig', + res.flags.submit, + async () => { + // We would prefer it allow for submissions if there is ambiguity, only fail if we confirm that it has been submitted + const confrimations = await fetchConfirmationsForProposals(id) + return confrimations === null || confrimations.count === 0 + } + ) + .addConditionalCheck('multisgTXId provided is valid', !!res.flags.multisigTx, async () => { + const confirmations = await fetchConfirmationsForProposals(id) + // if none are found the api could be wrong, so we allow it. + if (!confirmations || confirmations.count === 0) { + return true + } + // if we have confirmations, ensure one matches the provided id + return confirmations.approvals.some( + (approval) => approval.multisigTx.toString() === res.flags.multisigTx + ) + }) .runChecks() governanceTx = await governance.approve(id) logEvent = 'ProposalApproved' @@ -137,6 +172,17 @@ export default class Approve extends BaseCommand { governanceTx.txo ) + await displaySendTx('approveTx', tx, {}, logEvent) + } else if (res.flags.multisigTx && useMultiSig) { + const tx = await governanceApproverMultiSig!.confirmTransaction( + parseInt(res.flags.multisigTx) + ) + await displaySendTx('approveTx', tx, {}, logEvent) + } else if (res.flags.submit && useMultiSig) { + const tx = await governanceApproverMultiSig!.submitTransaction( + governance.address, + governanceTx.txo + ) await displaySendTx('approveTx', tx, {}, logEvent) } else { const tx = useMultiSig @@ -216,3 +262,29 @@ const addDefaultChecks = async ( governanceApproverMultiSig!.isOwner(account) ) } +const debugRpcPayload = debugFactory('mento-api') + +async function fetchConfirmationsForProposals(proposalId: string): Promise { + const response = await fetch( + `https://mondo.celo.org/api/governance/${proposalId}/approval-confirmations` + ) + if (response.ok) { + const data = (await response.json()) as MondoAPITx + debugRpcPayload('Fetched confirmations for proposal %s: %O', proposalId, data) + return data + } else { + return null + } +} + +interface MondoAPITx { + proposalId: number + count: number + approvals: Array<{ + approver: StrongAddress + multisigTx: number + confirmedAt: number + blockNumber: number + transactionHash: Hex + }> +} diff --git a/packages/sdk/contractkit/src/wrappers/MultiSig.ts b/packages/sdk/contractkit/src/wrappers/MultiSig.ts index 964163bcf..3b83744d7 100644 --- a/packages/sdk/contractkit/src/wrappers/MultiSig.ts +++ b/packages/sdk/contractkit/src/wrappers/MultiSig.ts @@ -39,8 +39,11 @@ export class MultiSigWrapper extends BaseWrapper { async submitOrConfirmTransaction(destination: string, txObject: CeloTxObject, value = '0') { const data = stringToSolidityBytes(txObject.encodeABI()) const transactionCount = await this.contract.methods.getTransactionCount(true, true).call() - let transactionId - for (transactionId = Number(transactionCount) - 1; transactionId >= 0; transactionId--) { + const transactionIds = await this.contract.methods + .getTransactionIds(0, transactionCount, true, false) + .call() + + for (const transactionId of transactionIds) { const transaction = await this.contract.methods.transactions(transactionId).call() if ( transaction.data === data && @@ -66,6 +69,13 @@ export class MultiSigWrapper extends BaseWrapper { this.contract.methods.confirmTransaction(transactionId) ) } + async submitTransaction(destination: string, txObject: CeloTxObject, value = '0') { + const data = stringToSolidityBytes(txObject.encodeABI()) + return toTransactionObject( + this.connection, + this.contract.methods.submitTransaction(destination, value, data) + ) + } isOwner: (owner: Address) => Promise = proxyCall(this.contract.methods.isOwner) getOwners = proxyCall(this.contract.methods.getOwners)