From 6c09d4b96b18d7ca2b0140bc71fe9e622dda51be Mon Sep 17 00:00:00 2001 From: Aaron DeRuvo Date: Tue, 2 Dec 2025 10:25:48 +0100 Subject: [PATCH 01/10] docs(changeset): Updates logic for submiting/confirming transaction with multisig to only search thru non executed transactions --- .changeset/open-donkeys-nail.md | 6 ++++++ packages/sdk/contractkit/src/wrappers/MultiSig.ts | 5 +++-- 2 files changed, 9 insertions(+), 2 deletions(-) create mode 100644 .changeset/open-donkeys-nail.md 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/sdk/contractkit/src/wrappers/MultiSig.ts b/packages/sdk/contractkit/src/wrappers/MultiSig.ts index 964163bcf..d84ef424f 100644 --- a/packages/sdk/contractkit/src/wrappers/MultiSig.ts +++ b/packages/sdk/contractkit/src/wrappers/MultiSig.ts @@ -39,8 +39,9 @@ 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 (let transactionId of transactionIds) { const transaction = await this.contract.methods.transactions(transactionId).call() if ( transaction.data === data && From f0f8e148fd2cb2e459f8a41d7c0df7dc53a01719 Mon Sep 17 00:00:00 2001 From: Aaron DeRuvo Date: Tue, 2 Dec 2025 10:27:02 +0100 Subject: [PATCH 02/10] there are too many executed transactions to search thru all of them --- packages/sdk/contractkit/src/wrappers/MultiSig.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/sdk/contractkit/src/wrappers/MultiSig.ts b/packages/sdk/contractkit/src/wrappers/MultiSig.ts index d84ef424f..ffeda5027 100644 --- a/packages/sdk/contractkit/src/wrappers/MultiSig.ts +++ b/packages/sdk/contractkit/src/wrappers/MultiSig.ts @@ -39,7 +39,9 @@ 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() - const transactionIds = await this.contract.methods.getTransactionIds(0, transactionCount, true, false).call() + const transactionIds = await this.contract.methods + .getTransactionIds(0, transactionCount, true, false) + .call() for (let transactionId of transactionIds) { const transaction = await this.contract.methods.transactions(transactionId).call() From d43a1bb5053e4b4c14309020d6028962b9d157a7 Mon Sep 17 00:00:00 2001 From: Aaron DeRuvo Date: Thu, 4 Dec 2025 09:09:54 +0100 Subject: [PATCH 03/10] mint --- packages/sdk/contractkit/src/wrappers/MultiSig.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/sdk/contractkit/src/wrappers/MultiSig.ts b/packages/sdk/contractkit/src/wrappers/MultiSig.ts index ffeda5027..7d91ccabe 100644 --- a/packages/sdk/contractkit/src/wrappers/MultiSig.ts +++ b/packages/sdk/contractkit/src/wrappers/MultiSig.ts @@ -43,7 +43,7 @@ export class MultiSigWrapper extends BaseWrapper { .getTransactionIds(0, transactionCount, true, false) .call() - for (let transactionId of transactionIds) { + for (const transactionId of transactionIds) { const transaction = await this.contract.methods.transactions(transactionId).call() if ( transaction.data === data && From 397b7760b4f516d981a9000f56a3f1428235f438 Mon Sep 17 00:00:00 2001 From: Aaron DeRuvo Date: Fri, 12 Dec 2025 15:13:34 +0100 Subject: [PATCH 04/10] Add --submit and --multisigTXId flags to governance:approve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds two new flags to the governance:approve command for better control over multisig transactions: - --submit: Force submission without checking for existing confirmations - --multisigTXId: Specify exact multisig transaction ID to confirm Also adds submitTransaction method to MultiSigWrapper to support the new functionality. Includes comprehensive tests for the new features. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../celocli-governance-approve-flags.md | 11 + .changeset/contractkit-submit-transaction.md | 5 + .../src/commands/governance/approve.test.ts | 1199 +++++++++++------ .../cli/src/commands/governance/approve.ts | 71 +- .../sdk/contractkit/src/wrappers/MultiSig.ts | 7 + 5 files changed, 909 insertions(+), 384 deletions(-) create mode 100644 .changeset/celocli-governance-approve-flags.md create mode 100644 .changeset/contractkit-submit-transaction.md diff --git a/.changeset/celocli-governance-approve-flags.md b/.changeset/celocli-governance-approve-flags.md new file mode 100644 index 000000000..57d24fa83 --- /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` + +- `--multisigTXId`: 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 --multisigTXId 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/packages/cli/src/commands/governance/approve.test.ts b/packages/cli/src/commands/governance/approve.test.ts index 4f962ebe7..55ac18ebc 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,33 @@ testWithAnvilL2( ).rejects.toThrow("Some checks didn't pass!") expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "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 ", - ], - ] - `) + { + "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 ", + ], + ] + `) expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) }) @@ -135,34 +144,33 @@ testWithAnvilL2( ).rejects.toThrow("Some checks didn't pass!") expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "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 ", - ], - ] - `) + { + "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 ", + ], + ] + `) expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) }) @@ -204,31 +212,30 @@ testWithAnvilL2( ).rejects.toThrow("Some checks didn't pass!") expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "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 ", - ], - ] - `) + { + "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 ", + ], + ] + `) expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) }) @@ -266,31 +273,30 @@ testWithAnvilL2( ).rejects.toThrow("Some checks didn't pass!") expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "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 ", - ], - ] - `) + { + "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 ", + ], + ] + `) expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) }) @@ -330,47 +336,46 @@ testWithAnvilL2( ) expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "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", - ], - ] - `) + { + "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", + ], + ] + `) expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) }) @@ -406,47 +411,46 @@ 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", - } - `) - 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", - ], - ] - `) + { + "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", + ], + ] + `) expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) }) @@ -503,44 +507,43 @@ testWithAnvilL2( ) expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "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", - ], - ] - `) + { + "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(`[]`) }) @@ -618,13 +621,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 +659,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 +691,73 @@ testWithAnvilL2( ) expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "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", - ], - ] - `) + { + "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", + ], + ] + `) expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) }) @@ -777,43 +779,42 @@ testWithAnvilL2( ) expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "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", - ], - ] - `) + { + "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", + ], + ] + `) expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) }) @@ -866,46 +867,482 @@ testWithAnvilL2( ) expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "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 ", - ], + { + "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], + multisigTxId: 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(` [ - "All checks passed", - ], + [ + "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 --multisigTXId 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 multisigTxId + ;(fetch as jest.Mock).mockResolvedValue({ + ok: true, + json: async () => ({ + proposalId: 1, + count: 1, + approvals: [ + { + approver: accounts[0], + multisigTxId: 0, + confirmedAt: Date.now(), + blockNumber: 100, + transactionHash: '0xabcd', + }, + ], + }), + }) + + // Now confirm it with the multisigTXId from accounts[1] + await expect(testLocallyWithWeb3Node( + Approve, [ - "SendTransaction: approveTx", + '--from', + accounts[1], + '--proposalID', + proposalId.toString(), + '--useMultiSig', + '--multisigTXId', + '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 --multisigTXId is provided', async () => { + const logMock = jest.spyOn(console, 'log') + + // Mock the fetch to return a different multisigTxId + ;(fetch as jest.Mock).mockResolvedValue({ + ok: true, + json: async () => ({ + proposalId: 1, + count: 1, + approvals: [ + { + approver: accounts[0], + multisigTxId: 5, // Different ID + confirmedAt: Date.now(), + blockNumber: 100, + transactionHash: '0xabcd', + }, + ], + }), + }) + + await expect( + testLocallyWithWeb3Node( + Approve, + [ + '--from', + accounts[0], + '--proposalID', + proposalId.toString(), + '--useMultiSig', + '--multisigTXId', + '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], + multisigTxId: 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..8440279e3 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,15 @@ export default class Approve extends BaseCommand { description: 'UUID of proposal to approve', exclusive: ['hotfix'], }), + multisigTXId: 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.', + }), + 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 +120,19 @@ 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.multisigTXId, async () => { + const confirmations = await fetchConfirmationsForProposals(id) + if (!confirmations || confirmations.count === 0) { + return false + } + return confirmations.approvals.some(approval => approval.multisigTxId.toString() === res.flags.multisigTXId) + + }) .runChecks() governanceTx = await governance.approve(id) logEvent = 'ProposalApproved' @@ -134,15 +160,28 @@ export default class Approve extends BaseCommand { ) { const tx = await governanceSecurityCouncilMultiSig.submitOrConfirmTransaction( governance.address, - governanceTx.txo + governanceTx.txo, ) await displaySendTx('approveTx', tx, {}, logEvent) - } else { + } else if (res.flags.multisigTXId && useMultiSig) { + const tx = await governanceApproverMultiSig!.confirmTransaction( + parseInt(res.flags.multisigTXId) + ) + 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 ? await governanceApproverMultiSig!.submitOrConfirmTransaction( governance.address, - governanceTx.txo + governanceTx.txo, ) : governanceTx await displaySendTx('approveTx', tx, {}, logEvent) @@ -216,3 +255,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 + "multisigTxId": 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 7d91ccabe..3b83744d7 100644 --- a/packages/sdk/contractkit/src/wrappers/MultiSig.ts +++ b/packages/sdk/contractkit/src/wrappers/MultiSig.ts @@ -69,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) From 2699cf6681c7bc08d5b1f503d1bfd6530fa3c217 Mon Sep 17 00:00:00 2001 From: Aaron DeRuvo Date: Tue, 2 Dec 2025 10:25:48 +0100 Subject: [PATCH 05/10] docs(changeset): Updates logic for submiting/confirming transaction with multisig to only search thru non executed transactions --- .changeset/open-donkeys-nail.md | 6 ++++++ packages/sdk/contractkit/src/wrappers/MultiSig.ts | 5 +++-- 2 files changed, 9 insertions(+), 2 deletions(-) create mode 100644 .changeset/open-donkeys-nail.md 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/sdk/contractkit/src/wrappers/MultiSig.ts b/packages/sdk/contractkit/src/wrappers/MultiSig.ts index 964163bcf..d84ef424f 100644 --- a/packages/sdk/contractkit/src/wrappers/MultiSig.ts +++ b/packages/sdk/contractkit/src/wrappers/MultiSig.ts @@ -39,8 +39,9 @@ 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 (let transactionId of transactionIds) { const transaction = await this.contract.methods.transactions(transactionId).call() if ( transaction.data === data && From cdb3e2677cc587f970e5e13c852ab62318850106 Mon Sep 17 00:00:00 2001 From: Aaron DeRuvo Date: Tue, 2 Dec 2025 10:27:02 +0100 Subject: [PATCH 06/10] there are too many executed transactions to search thru all of them --- packages/sdk/contractkit/src/wrappers/MultiSig.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/sdk/contractkit/src/wrappers/MultiSig.ts b/packages/sdk/contractkit/src/wrappers/MultiSig.ts index d84ef424f..ffeda5027 100644 --- a/packages/sdk/contractkit/src/wrappers/MultiSig.ts +++ b/packages/sdk/contractkit/src/wrappers/MultiSig.ts @@ -39,7 +39,9 @@ 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() - const transactionIds = await this.contract.methods.getTransactionIds(0, transactionCount, true, false).call() + const transactionIds = await this.contract.methods + .getTransactionIds(0, transactionCount, true, false) + .call() for (let transactionId of transactionIds) { const transaction = await this.contract.methods.transactions(transactionId).call() From 731da930b47ab4b83dfe417d9e72c6c60be9316c Mon Sep 17 00:00:00 2001 From: Aaron DeRuvo Date: Thu, 4 Dec 2025 09:09:54 +0100 Subject: [PATCH 07/10] mint --- packages/sdk/contractkit/src/wrappers/MultiSig.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/sdk/contractkit/src/wrappers/MultiSig.ts b/packages/sdk/contractkit/src/wrappers/MultiSig.ts index ffeda5027..7d91ccabe 100644 --- a/packages/sdk/contractkit/src/wrappers/MultiSig.ts +++ b/packages/sdk/contractkit/src/wrappers/MultiSig.ts @@ -43,7 +43,7 @@ export class MultiSigWrapper extends BaseWrapper { .getTransactionIds(0, transactionCount, true, false) .call() - for (let transactionId of transactionIds) { + for (const transactionId of transactionIds) { const transaction = await this.contract.methods.transactions(transactionId).call() if ( transaction.data === data && From ce02fe05ca7b0077e4a73a9a59cd5844adc211a2 Mon Sep 17 00:00:00 2001 From: Aaron DeRuvo Date: Fri, 12 Dec 2025 15:13:34 +0100 Subject: [PATCH 08/10] Add --submit and --multisigTXId flags to governance:approve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds two new flags to the governance:approve command for better control over multisig transactions: - --submit: Force submission without checking for existing confirmations - --multisigTXId: Specify exact multisig transaction ID to confirm Also adds submitTransaction method to MultiSigWrapper to support the new functionality. Includes comprehensive tests for the new features. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../celocli-governance-approve-flags.md | 11 + .changeset/contractkit-submit-transaction.md | 5 + .../src/commands/governance/approve.test.ts | 1199 +++++++++++------ .../cli/src/commands/governance/approve.ts | 71 +- .../sdk/contractkit/src/wrappers/MultiSig.ts | 7 + 5 files changed, 909 insertions(+), 384 deletions(-) create mode 100644 .changeset/celocli-governance-approve-flags.md create mode 100644 .changeset/contractkit-submit-transaction.md diff --git a/.changeset/celocli-governance-approve-flags.md b/.changeset/celocli-governance-approve-flags.md new file mode 100644 index 000000000..57d24fa83 --- /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` + +- `--multisigTXId`: 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 --multisigTXId 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/packages/cli/src/commands/governance/approve.test.ts b/packages/cli/src/commands/governance/approve.test.ts index 4f962ebe7..55ac18ebc 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,33 @@ testWithAnvilL2( ).rejects.toThrow("Some checks didn't pass!") expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "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 ", - ], - ] - `) + { + "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 ", + ], + ] + `) expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) }) @@ -135,34 +144,33 @@ testWithAnvilL2( ).rejects.toThrow("Some checks didn't pass!") expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "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 ", - ], - ] - `) + { + "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 ", + ], + ] + `) expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) }) @@ -204,31 +212,30 @@ testWithAnvilL2( ).rejects.toThrow("Some checks didn't pass!") expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "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 ", - ], - ] - `) + { + "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 ", + ], + ] + `) expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) }) @@ -266,31 +273,30 @@ testWithAnvilL2( ).rejects.toThrow("Some checks didn't pass!") expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "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 ", - ], - ] - `) + { + "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 ", + ], + ] + `) expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) }) @@ -330,47 +336,46 @@ testWithAnvilL2( ) expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "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", - ], - ] - `) + { + "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", + ], + ] + `) expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) }) @@ -406,47 +411,46 @@ 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", - } - `) - 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", - ], - ] - `) + { + "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", + ], + ] + `) expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) }) @@ -503,44 +507,43 @@ testWithAnvilL2( ) expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "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", - ], - ] - `) + { + "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(`[]`) }) @@ -618,13 +621,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 +659,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 +691,73 @@ testWithAnvilL2( ) expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "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", - ], - ] - `) + { + "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", + ], + ] + `) expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) }) @@ -777,43 +779,42 @@ testWithAnvilL2( ) expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "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", - ], - ] - `) + { + "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", + ], + ] + `) expect(writeMock.mock.calls).toMatchInlineSnapshot(`[]`) }) @@ -866,46 +867,482 @@ testWithAnvilL2( ) expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(` - { - "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 ", - ], + { + "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], + multisigTxId: 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(` [ - "All checks passed", - ], + [ + "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 --multisigTXId 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 multisigTxId + ;(fetch as jest.Mock).mockResolvedValue({ + ok: true, + json: async () => ({ + proposalId: 1, + count: 1, + approvals: [ + { + approver: accounts[0], + multisigTxId: 0, + confirmedAt: Date.now(), + blockNumber: 100, + transactionHash: '0xabcd', + }, + ], + }), + }) + + // Now confirm it with the multisigTXId from accounts[1] + await expect(testLocallyWithWeb3Node( + Approve, [ - "SendTransaction: approveTx", + '--from', + accounts[1], + '--proposalID', + proposalId.toString(), + '--useMultiSig', + '--multisigTXId', + '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 --multisigTXId is provided', async () => { + const logMock = jest.spyOn(console, 'log') + + // Mock the fetch to return a different multisigTxId + ;(fetch as jest.Mock).mockResolvedValue({ + ok: true, + json: async () => ({ + proposalId: 1, + count: 1, + approvals: [ + { + approver: accounts[0], + multisigTxId: 5, // Different ID + confirmedAt: Date.now(), + blockNumber: 100, + transactionHash: '0xabcd', + }, + ], + }), + }) + + await expect( + testLocallyWithWeb3Node( + Approve, + [ + '--from', + accounts[0], + '--proposalID', + proposalId.toString(), + '--useMultiSig', + '--multisigTXId', + '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], + multisigTxId: 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..8440279e3 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,15 @@ export default class Approve extends BaseCommand { description: 'UUID of proposal to approve', exclusive: ['hotfix'], }), + multisigTXId: 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.', + }), + 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 +120,19 @@ 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.multisigTXId, async () => { + const confirmations = await fetchConfirmationsForProposals(id) + if (!confirmations || confirmations.count === 0) { + return false + } + return confirmations.approvals.some(approval => approval.multisigTxId.toString() === res.flags.multisigTXId) + + }) .runChecks() governanceTx = await governance.approve(id) logEvent = 'ProposalApproved' @@ -134,15 +160,28 @@ export default class Approve extends BaseCommand { ) { const tx = await governanceSecurityCouncilMultiSig.submitOrConfirmTransaction( governance.address, - governanceTx.txo + governanceTx.txo, ) await displaySendTx('approveTx', tx, {}, logEvent) - } else { + } else if (res.flags.multisigTXId && useMultiSig) { + const tx = await governanceApproverMultiSig!.confirmTransaction( + parseInt(res.flags.multisigTXId) + ) + 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 ? await governanceApproverMultiSig!.submitOrConfirmTransaction( governance.address, - governanceTx.txo + governanceTx.txo, ) : governanceTx await displaySendTx('approveTx', tx, {}, logEvent) @@ -216,3 +255,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 + "multisigTxId": 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 7d91ccabe..3b83744d7 100644 --- a/packages/sdk/contractkit/src/wrappers/MultiSig.ts +++ b/packages/sdk/contractkit/src/wrappers/MultiSig.ts @@ -69,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) From 5bdc6e7fe08ba365225daa6a1084fd4f9d7db26d Mon Sep 17 00:00:00 2001 From: Aaron DeRuvo Date: Fri, 12 Dec 2025 15:21:21 +0100 Subject: [PATCH 09/10] dont prevent submissions if mondo is behind --- .../src/commands/governance/approve.test.ts | 98 +++++++++++-------- .../cli/src/commands/governance/approve.ts | 79 ++++++++------- 2 files changed, 102 insertions(+), 75 deletions(-) diff --git a/packages/cli/src/commands/governance/approve.test.ts b/packages/cli/src/commands/governance/approve.test.ts index 55ac18ebc..ab748a1b8 100644 --- a/packages/cli/src/commands/governance/approve.test.ts +++ b/packages/cli/src/commands/governance/approve.test.ts @@ -105,8 +105,9 @@ testWithAnvilL2( "executionTimeLimit": "0", } `) - expect(logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes))) - .toMatchInlineSnapshot(` + expect( + logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) + ).toMatchInlineSnapshot(` [ [ "Running Checks:", @@ -151,8 +152,9 @@ testWithAnvilL2( "executionTimeLimit": "0", } `) - expect(logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes))) - .toMatchInlineSnapshot(` + expect( + logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) + ).toMatchInlineSnapshot(` [ [ "Running Checks:", @@ -219,8 +221,9 @@ testWithAnvilL2( "executionTimeLimit": "0", } `) - expect(logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes))) - .toMatchInlineSnapshot(` + expect( + logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) + ).toMatchInlineSnapshot(` [ [ "Running Checks:", @@ -280,8 +283,9 @@ testWithAnvilL2( "executionTimeLimit": "0", } `) - expect(logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes))) - .toMatchInlineSnapshot(` + expect( + logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) + ).toMatchInlineSnapshot(` [ [ "Running Checks:", @@ -343,8 +347,9 @@ testWithAnvilL2( "executionTimeLimit": "0", } `) - expect(logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes))) - .toMatchInlineSnapshot(` + expect( + logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) + ).toMatchInlineSnapshot(` [ [ "Running Checks:", @@ -418,8 +423,9 @@ testWithAnvilL2( "executionTimeLimit": "0", } `) - expect(logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes))) - .toMatchInlineSnapshot(` + expect( + logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) + ).toMatchInlineSnapshot(` [ [ "Running Checks:", @@ -515,8 +521,9 @@ testWithAnvilL2( } `) - expect(logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes))) - .toMatchInlineSnapshot(` + expect( + logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) + ).toMatchInlineSnapshot(` [ [ "Running Checks:", @@ -699,8 +706,9 @@ testWithAnvilL2( } `) - expect(logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes))) - .toMatchInlineSnapshot(` + expect( + logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) + ).toMatchInlineSnapshot(` [ [ "Running Checks:", @@ -786,8 +794,9 @@ testWithAnvilL2( "executionTimeLimit": "0", } `) - expect(logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes))) - .toMatchInlineSnapshot(` + expect( + logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) + ).toMatchInlineSnapshot(` [ [ "Running Checks:", @@ -874,8 +883,9 @@ testWithAnvilL2( "executionTimeLimit": "0", } `) - expect(logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes))) - .toMatchInlineSnapshot(` + expect( + logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) + ).toMatchInlineSnapshot(` [ [ "Running Checks:", @@ -946,8 +956,9 @@ testWithAnvilL2( ) expect(await governance.isApproved(proposalId)).toBe(true) - expect(logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes))) - .toMatchInlineSnapshot(` + expect( + logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) + ).toMatchInlineSnapshot(` [ [ "Running Checks:", @@ -1017,8 +1028,9 @@ testWithAnvilL2( ) ).rejects.toThrow("Some checks didn't pass!") - expect(logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes))) - .toMatchInlineSnapshot(` + expect( + logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) + ).toMatchInlineSnapshot(` [ [ "Running Checks:", @@ -1072,8 +1084,9 @@ testWithAnvilL2( ) ).resolves.toBeUndefined() - expect(logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes))) - .toMatchInlineSnapshot(` + expect( + logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) + ).toMatchInlineSnapshot(` [ [ "Running Checks:", @@ -1159,19 +1172,21 @@ testWithAnvilL2( }) // Now confirm it with the multisigTXId from accounts[1] - await expect(testLocallyWithWeb3Node( - Approve, - [ - '--from', - accounts[1], - '--proposalID', - proposalId.toString(), - '--useMultiSig', - '--multisigTXId', - '0', - ], - web3 - )).resolves.toBeUndefined() + await expect( + testLocallyWithWeb3Node( + Approve, + [ + '--from', + accounts[1], + '--proposalID', + proposalId.toString(), + '--useMultiSig', + '--multisigTXId', + '0', + ], + web3 + ) + ).resolves.toBeUndefined() // The proposal should now be approved expect(await governance.isApproved(proposalId)).toBe(true) @@ -1250,8 +1265,9 @@ testWithAnvilL2( ) ).rejects.toThrow("Some checks didn't pass!") - expect(logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes))) - .toMatchInlineSnapshot(` + expect( + logMock.mock.calls.map((args) => args.map(stripAnsiCodesAndTxHashes)) + ).toMatchInlineSnapshot(` [ [ "Running Checks:", diff --git a/packages/cli/src/commands/governance/approve.ts b/packages/cli/src/commands/governance/approve.ts index 8440279e3..ec48890d6 100644 --- a/packages/cli/src/commands/governance/approve.ts +++ b/packages/cli/src/commands/governance/approve.ts @@ -39,11 +39,13 @@ export default class Approve extends BaseCommand { multisigTXId: 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.', + description: + 'Optionally provide the exact multisig transaction id to confirm. otherwise will search onchain for transaction which matches the proposal.', }), submit: Flags.boolean({ dependsOn: ['proposalID', 'useMultiSig'], - description: 'Submit the approval transaction to multisig without checking for prior confirmations onchain. (Use with caution!)', + 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({ @@ -120,19 +122,30 @@ 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.multisigTXId, async () => { - const confirmations = await fetchConfirmationsForProposals(id) - if (!confirmations || confirmations.count === 0) { - return false + .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 } - return confirmations.approvals.some(approval => approval.multisigTxId.toString() === res.flags.multisigTXId) - - }) + ) + .addConditionalCheck( + 'multisgTXId provided is valid', + !!res.flags.multisigTXId, + 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.multisigTxId.toString() === res.flags.multisigTXId + ) + } + ) .runChecks() governanceTx = await governance.approve(id) logEvent = 'ProposalApproved' @@ -160,7 +173,7 @@ export default class Approve extends BaseCommand { ) { const tx = await governanceSecurityCouncilMultiSig.submitOrConfirmTransaction( governance.address, - governanceTx.txo, + governanceTx.txo ) await displaySendTx('approveTx', tx, {}, logEvent) @@ -171,17 +184,15 @@ export default class Approve extends BaseCommand { await displaySendTx('approveTx', tx, {}, logEvent) } else if (res.flags.submit && useMultiSig) { const tx = await governanceApproverMultiSig!.submitTransaction( - governance.address, - governanceTx.txo, - ) + governance.address, + governanceTx.txo + ) await displaySendTx('approveTx', tx, {}, logEvent) - } - - else { + } else { const tx = useMultiSig ? await governanceApproverMultiSig!.submitOrConfirmTransaction( governance.address, - governanceTx.txo, + governanceTx.txo ) : governanceTx await displaySendTx('approveTx', tx, {}, logEvent) @@ -257,12 +268,12 @@ const addDefaultChecks = async ( } const debugRpcPayload = debugFactory('mento-api') - - async function fetchConfirmationsForProposals(proposalId: string): Promise { - const response = await fetch(`https://mondo.celo.org/api/governance/${proposalId}/approval-confirmations`) + const response = await fetch( + `https://mondo.celo.org/api/governance/${proposalId}/approval-confirmations` + ) if (response.ok) { - const data = await response.json() as MondoAPITx + const data = (await response.json()) as MondoAPITx debugRpcPayload('Fetched confirmations for proposal %s: %O', proposalId, data) return data } else { @@ -271,13 +282,13 @@ async function fetchConfirmationsForProposals(proposalId: string): Promise } From 5df8eeab0911e08a329ad3da68b94075c05969c7 Mon Sep 17 00:00:00 2001 From: Aaron DeRuvo Date: Mon, 15 Dec 2025 16:48:38 +0100 Subject: [PATCH 10/10] Just Tx... its cleaner --- .../celocli-governance-approve-flags.md | 2 +- .../src/commands/governance/approve.test.ts | 22 ++++++------ .../cli/src/commands/governance/approve.ts | 34 ++++++++----------- 3 files changed, 27 insertions(+), 31 deletions(-) diff --git a/.changeset/celocli-governance-approve-flags.md b/.changeset/celocli-governance-approve-flags.md index 57d24fa83..d6b4b8e33 100644 --- a/.changeset/celocli-governance-approve-flags.md +++ b/.changeset/celocli-governance-approve-flags.md @@ -6,6 +6,6 @@ Add new flags to `governance:approve` command for better multisig transaction co - `--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` -- `--multisigTXId`: 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 --multisigTXId 5` +- `--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/packages/cli/src/commands/governance/approve.test.ts b/packages/cli/src/commands/governance/approve.test.ts index ab748a1b8..2e3bb2db9 100644 --- a/packages/cli/src/commands/governance/approve.test.ts +++ b/packages/cli/src/commands/governance/approve.test.ts @@ -1004,7 +1004,7 @@ testWithAnvilL2( approvals: [ { approver: accounts[0], - multisigTxId: 0, + multisigTx: 0, confirmedAt: Date.now(), blockNumber: 100, transactionHash: '0xabcd', @@ -1122,7 +1122,7 @@ testWithAnvilL2( `) }) - it('should confirm existing multisig transaction when --multisigTXId is provided', async () => { + it('should confirm existing multisig transaction when --multisigTx is provided', async () => { const logMock = jest.spyOn(console, 'log') const kit = newKitFromWeb3(web3) @@ -1153,7 +1153,7 @@ testWithAnvilL2( // Verify proposal is not yet approved expect(await governance.isApproved(proposalId)).toBe(false) - // Mock the fetch to return the multisigTxId + // Mock the fetch to return the multisigTx ;(fetch as jest.Mock).mockResolvedValue({ ok: true, json: async () => ({ @@ -1162,7 +1162,7 @@ testWithAnvilL2( approvals: [ { approver: accounts[0], - multisigTxId: 0, + multisigTx: 0, confirmedAt: Date.now(), blockNumber: 100, transactionHash: '0xabcd', @@ -1171,7 +1171,7 @@ testWithAnvilL2( }), }) - // Now confirm it with the multisigTXId from accounts[1] + // Now confirm it with the multisigTx from accounts[1] await expect( testLocallyWithWeb3Node( Approve, @@ -1181,7 +1181,7 @@ testWithAnvilL2( '--proposalID', proposalId.toString(), '--useMultiSig', - '--multisigTXId', + '--multisigTx', '0', ], web3 @@ -1228,10 +1228,10 @@ testWithAnvilL2( `) }) - it('should fail when invalid --multisigTXId is provided', async () => { + it('should fail when invalid --multisigTx is provided', async () => { const logMock = jest.spyOn(console, 'log') - // Mock the fetch to return a different multisigTxId + // Mock the fetch to return a different multisigTx ;(fetch as jest.Mock).mockResolvedValue({ ok: true, json: async () => ({ @@ -1240,7 +1240,7 @@ testWithAnvilL2( approvals: [ { approver: accounts[0], - multisigTxId: 5, // Different ID + multisigTx: 5, // Different ID confirmedAt: Date.now(), blockNumber: 100, transactionHash: '0xabcd', @@ -1258,7 +1258,7 @@ testWithAnvilL2( '--proposalID', proposalId.toString(), '--useMultiSig', - '--multisigTXId', + '--multisigTx', '0', // Invalid ID ], web3 @@ -1305,7 +1305,7 @@ testWithAnvilL2( approvals: [ { approver: accounts[0], - multisigTxId: 0, + multisigTx: 0, confirmedAt: Date.now(), blockNumber: 100, transactionHash: '0xabcd', diff --git a/packages/cli/src/commands/governance/approve.ts b/packages/cli/src/commands/governance/approve.ts index ec48890d6..72173cbe3 100644 --- a/packages/cli/src/commands/governance/approve.ts +++ b/packages/cli/src/commands/governance/approve.ts @@ -36,11 +36,11 @@ export default class Approve extends BaseCommand { description: 'UUID of proposal to approve', exclusive: ['hotfix'], }), - multisigTXId: Flags.string({ + 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.', + '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'], @@ -131,21 +131,17 @@ export default class Approve extends BaseCommand { return confrimations === null || confrimations.count === 0 } ) - .addConditionalCheck( - 'multisgTXId provided is valid', - !!res.flags.multisigTXId, - 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.multisigTxId.toString() === res.flags.multisigTXId - ) + .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' @@ -177,9 +173,9 @@ export default class Approve extends BaseCommand { ) await displaySendTx('approveTx', tx, {}, logEvent) - } else if (res.flags.multisigTXId && useMultiSig) { + } else if (res.flags.multisigTx && useMultiSig) { const tx = await governanceApproverMultiSig!.confirmTransaction( - parseInt(res.flags.multisigTXId) + parseInt(res.flags.multisigTx) ) await displaySendTx('approveTx', tx, {}, logEvent) } else if (res.flags.submit && useMultiSig) { @@ -286,7 +282,7 @@ interface MondoAPITx { count: number approvals: Array<{ approver: StrongAddress - multisigTxId: number + multisigTx: number confirmedAt: number blockNumber: number transactionHash: Hex