diff --git a/.changeset/multisig-read-batching.md b/.changeset/multisig-read-batching.md new file mode 100644 index 000000000..fd239ce4c --- /dev/null +++ b/.changeset/multisig-read-batching.md @@ -0,0 +1,13 @@ +--- +'@celo/contractkit': patch +--- + +Reduce the RPC load of MultiSigWrapper reads. + +- `getConfirmations` uses the contract's `getConfirmations` view instead of one + `confirmations` read per owner. +- `getTransactionDataByContent` scans transactions newest-first in small batches and + stops at the first match, instead of fetching the multisig's entire history in one + burst (268 parallel requests on the mainnet approver multisig, enough to get + rate-limited). When several transactions share the same content, the most recent + one is now returned. diff --git a/.changeset/remaining-approvals.md b/.changeset/remaining-approvals.md new file mode 100644 index 000000000..09d3714a3 --- /dev/null +++ b/.changeset/remaining-approvals.md @@ -0,0 +1,15 @@ +--- +'@celo/contractkit': minor +'@celo/celocli': minor +--- + +Show how many approvers still need to approve. + +- `governance:show --proposalID` reports the confirmations the approver multisig is still + missing, both in the `approvals` map (`required` / `remaining`) and as a note. +- `governance:show --hotfix` gained an `approvals` section with the approver and security + council progress towards their respective thresholds. +- `multisig:show` reports `confirmations`, `confirmationsRequired` and + `confirmationsRemaining` per displayed transaction, using the internal threshold for + transactions the multisig sends to itself. +- `GovernanceWrapper.getApprovalStatus` returns the new `required` and `remaining` fields. diff --git a/packages/cli/src/commands/governance/show.test.ts b/packages/cli/src/commands/governance/show.test.ts index cc36b01f4..a09264856 100644 --- a/packages/cli/src/commands/governance/show.test.ts +++ b/packages/cli/src/commands/governance/show.test.ts @@ -1,11 +1,18 @@ +import path from 'node:path' +import { StrongAddress } from '@celo/base' import { newKitFromProvider } from '@celo/contractkit' import { unixSecondsTimestampToDateString } from '@celo/contractkit/lib/wrappers/BaseWrapper' import { Proposal } from '@celo/contractkit/lib/wrappers/Governance' import { testWithAnvilL2 } from '@celo/dev-utils/anvil-test' import { timeTravel } from '@celo/dev-utils/ganache-test' import fs from 'fs' -import path from 'node:path' -import { stripAnsiCodesAndTxHashes, testLocallyWithNode } from '../../test-utils/cliUtils' +import { changeMultiSigOwner } from '../../test-utils/chain-setup' +import { + stripAnsiCodesAndTxHashes, + stripAnsiCodesFromNestedArray, + testLocallyWithNode, +} from '../../test-utils/cliUtils' +import Approve from './approve' import Show from './show' process.env.NO_SYNCCHECK = 'true' @@ -95,6 +102,8 @@ testWithAnvilL2('governance:show cmd', (provider) => { completion: 0 / 1 confirmations: + remaining: 1 + required: 1 approved: false metadata: deposit: 100000000000000000000 (~1.000e+20) @@ -118,6 +127,9 @@ testWithAnvilL2('governance:show cmd', (provider) => { No: 0 Yes: 100000000000000000000 (~1.000e+20)", ], + [ + "Note: 1 more approver confirmation(s) needed to approve this proposal (0/1)", + ], [ "Note: required is the minimal amount of yes + abstain votes needed to pass the proposal", ], @@ -131,4 +143,33 @@ testWithAnvilL2('governance:show cmd', (provider) => { ] `) }) + it('shows how many approvals a hotfix still needs', async () => { + const HOTFIX_HASH = '0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d' + const kit = newKitFromProvider(provider) + const [approver] = (await kit.connection.getAccounts()) as StrongAddress[] + // make an account we can send from a signatory of the approver multisig + await changeMultiSigOwner(kit, approver) + + const logMock = jest.spyOn(console, 'log') + logMock.mockClear() + await testLocallyWithNode(Show, ['--hotfix', HOTFIX_HASH], provider) + + const beforeApproval = stripAnsiCodesFromNestedArray(logMock.mock.calls).flat().join('\n') + expect(beforeApproval).toContain( + 'Note: 1 more approver confirmation(s) needed to approve this hotfix (0/1)' + ) + + await testLocallyWithNode( + Approve, + ['--hotfix', HOTFIX_HASH, '--from', approver, '--useMultiSig'], + provider + ) + + logMock.mockClear() + await testLocallyWithNode(Show, ['--hotfix', HOTFIX_HASH], provider) + + const afterApproval = stripAnsiCodesFromNestedArray(logMock.mock.calls).flat().join('\n') + expect(afterApproval).toContain('Note: the approver has approved this hotfix') + expect(afterApproval).toContain('this hotfix still needs the approval of the security council') + }) }) diff --git a/packages/cli/src/commands/governance/show.ts b/packages/cli/src/commands/governance/show.ts index 92fcaf321..6d886e59b 100644 --- a/packages/cli/src/commands/governance/show.ts +++ b/packages/cli/src/commands/governance/show.ts @@ -1,17 +1,43 @@ import { ProposalBuilder, proposalToJSON } from '@celo/governance' -import { hexToBytes } from 'viem' import { Flags } from '@oclif/core' import chalk from 'chalk' import { writeFileSync } from 'fs' +import { hexToBytes } from 'viem' import { BaseCommand } from '../../base' import { newCheckBuilder } from '../../utils/checks' import { printValueMap, printValueMapRecursive } from '../../utils/cli' import { ViewCommmandFlags } from '../../utils/flags' import { + ApprovalProgress, addExistingProposalIDToBuilder, addExistingProposalJSONFileToBuilder, + getHotfixApprovalProgress, } from '../../utils/governance' +function printApprovalNote(label: string, progress: ApprovalProgress) { + if (progress.approved) { + console.log(`Note: the ${label} has approved this hotfix`) + return + } + + switch (progress.kind) { + case 'multisig': + console.log( + progress.remaining > 0 + ? `Note: ${progress.remaining} more ${label} confirmation(s) needed to approve this hotfix (${progress.confirmations.length}/${progress.required})` + : `Note: the ${label} multisig has enough confirmations, the approval is pending execution` + ) + return + case 'safe': + console.log( + `Note: this hotfix still needs the approval of the ${label} SAFE ${progress.address} (${progress.required} signature(s) required, collected offchain)` + ) + return + default: + console.log(`Note: this hotfix still needs the approval of the ${label} ${progress.address}`) + } +} + export default class Show extends BaseCommand { static aliases = [ 'governance:show', @@ -139,6 +165,15 @@ export default class Show extends BaseCommand { schedule, }) + if (record.approvals && !record.approved) { + const { remaining, required, confirmations } = record.approvals + console.log( + remaining > 0 + ? `Note: ${remaining} more approver confirmation(s) needed to approve this proposal (${confirmations.length}/${required})` + : 'Note: the approver multisig has enough confirmations, the approval is pending execution' + ) + } + if (Object.keys(requirements).length !== 0) { console.log( 'Note: required is the minimal amount of yes + abstain votes needed to pass the proposal' @@ -151,6 +186,18 @@ export default class Show extends BaseCommand { const hotfixBuf = Buffer.from(hexToBytes(hotfix as `0x${string}`)) const record = await governance.getHotfixRecord(hotfixBuf) printValueMap(record) + + if (!record.executed) { + const { approver, securityCouncil } = await getHotfixApprovalProgress( + await this.getPublicClient(), + governance, + hotfix, + record + ) + printValueMapRecursive({ approvals: { approver, securityCouncil } }) + printApprovalNote('approver', approver) + printApprovalNote('security council', securityCouncil) + } } else if (account) { const accounts = await kit.contracts.getAccounts() printValueMapRecursive(await governance.getVoter(await accounts.signerToAccount(account))) diff --git a/packages/cli/src/commands/multisig/show.test.ts b/packages/cli/src/commands/multisig/show.test.ts index 6c932988f..3e3647786 100644 --- a/packages/cli/src/commands/multisig/show.test.ts +++ b/packages/cli/src/commands/multisig/show.test.ts @@ -1,8 +1,11 @@ +import { multiSigABI } from '@celo/abis' import { StrongAddress } from '@celo/base' import { ContractKit, newKitFromProvider } from '@celo/contractkit' import { testWithAnvilL2 } from '@celo/dev-utils/anvil-test' +import { encodeFunctionData } from 'viem' import { stripAnsiCodesFromNestedArray, testLocallyWithNode } from '../../test-utils/cliUtils' import { createMultisig } from '../../test-utils/multisigUtils' +import ApproveMultiSig from './approve' import ProposeMultiSig from './propose' import ShowMultiSig from './show' @@ -113,6 +116,10 @@ testWithAnvilL2('multisig:show integration tests', (provider) => { 1: 1000000000000000000 (~1e+18) 2: 0x 3: false + confirmations: + 0: 0x5409ED021D9299bf6814279A6A1411A7e866A631 + confirmationsRemaining: 1 + confirmationsRequired: 2 data: null", ], ] @@ -120,6 +127,48 @@ testWithAnvilL2('multisig:show integration tests', (provider) => { expect(result).toBeUndefined() }) + it('shows how many confirmations a transaction still needs', async () => { + // a dedicated multisig keeps the transaction indexes independent of the other tests. + // 3 signatures are required in general, but only 2 for transactions the multisig sends + // to itself, which is what this transaction does + const isolatedMultisig = await createMultisig(kit, [owner1, owner2, owner3], 3, 2) + const addOwner = encodeFunctionData({ + abi: multiSigABI, + functionName: 'addOwner', + args: [accounts[5]], + }) + const txId = '0' + + await testLocallyWithNode( + ProposeMultiSig, + [isolatedMultisig, '--from', owner1, '--to', isolatedMultisig, '--data', addOwner], + provider + ) + + const logMock = jest.spyOn(console, 'log') + logMock.mockClear() + await testLocallyWithNode(ShowMultiSig, [isolatedMultisig, '--tx', txId], provider) + + // proposing confirms on behalf of the proposer, so 1 of the 2 internally required is missing + const beforeApproval = stripAnsiCodesFromNestedArray(logMock.mock.calls).flat().join('\n') + expect(beforeApproval).toContain('confirmationsRequired: 2') + expect(beforeApproval).toContain('confirmationsRemaining: 1') + + await testLocallyWithNode( + ApproveMultiSig, + ['--from', owner2, '--for', isolatedMultisig, '--tx', txId], + provider + ) + + logMock.mockClear() + await testLocallyWithNode(ShowMultiSig, [isolatedMultisig, '--tx', txId], provider) + + const afterApproval = stripAnsiCodesFromNestedArray(logMock.mock.calls).flat().join('\n') + expect(afterApproval).toContain('confirmationsRemaining: 0') + expect(afterApproval).toContain(owner1) + expect(afterApproval).toContain(owner2) + }) + it('shows raw transaction data', async () => { const logMock = jest.spyOn(console, 'log') diff --git a/packages/cli/src/commands/multisig/show.ts b/packages/cli/src/commands/multisig/show.ts index 1759932d8..f6df99464 100644 --- a/packages/cli/src/commands/multisig/show.ts +++ b/packages/cli/src/commands/multisig/show.ts @@ -2,11 +2,12 @@ import { getMultiSigContract } from '@celo/actions/contracts/multisig' import { CeloContract } from '@celo/contractkit' import { newBlockExplorer } from '@celo/explorer/lib/block-explorer' import { Flags } from '@oclif/core' -import { Address } from 'viem' +import { Address, zeroAddress } from 'viem' import { BaseCommand } from '../../base' import { printValueMapRecursive } from '../../utils/cli' import { CustomArgs } from '../../utils/command' import { ViewCommmandFlags } from '../../utils/flags' +import { getConfirmationProgress } from '../../utils/multisig-utils' export default class ShowMultiSig extends BaseCommand { static description = 'Shows information about multi-sig contract' @@ -44,30 +45,58 @@ export default class ShowMultiSig extends BaseCommand { const multisig = await getMultiSigContract(clients, multisigAddress) const txCount = await multisig.read.getTransactionCount([true, true]) + const [required, internalRequired] = await Promise.all([ + multisig.read.required(), + multisig.read.internalRequired(), + ]) const explorer = await newBlockExplorer(await this.getKit()) await explorer.updateContractDetailsMapping(CeloContract.MultiSig, multisigAddress) - const process = async (txdata: Awaited>) => { - if (raw) return txdata - return { ...txdata, data: await explorer.tryParseTxInput(txdata[0], txdata[2]) } + const confirmationStatus = async (txId: bigint, destination: Address, executed: boolean) => { + if (destination === zeroAddress) { + // transaction does not exist, there is nothing to confirm + return {} + } + const progress = await getConfirmationProgress( + multisig.read, + txId, + { destination, executed }, + { required, internalRequired }, + multisigAddress + ) + // prefixed keys: a bare `required` next to the multisig-wide + // 'Required confirmations' lines would be ambiguous + return { + confirmations: progress.confirmations, + confirmationsRequired: progress.required, + confirmationsRemaining: progress.remaining, + } + } + const process = async ( + txId: bigint, + txdata: Awaited> + ) => { + const [destination, , input, executed] = txdata + const withConfirmations = { + ...txdata, + ...(await confirmationStatus(txId, destination, executed)), + } + if (raw) return withConfirmations + return { ...withConfirmations, data: await explorer.tryParseTxInput(destination, input) } } const txinfo = tx !== undefined - ? await process(await multisig.read.transactions([BigInt(tx)])) + ? await process(BigInt(tx), await multisig.read.transactions([BigInt(tx)])) : all ? await Promise.all( - ( - await Promise.all( - ( - await multisig.read.getTransactionIds([BigInt(0), txCount, true, true]) - ).map((tx) => multisig.read.transactions([tx])) - ) - ).map(process) + (await multisig.read.getTransactionIds([BigInt(0), txCount, true, true])).map( + async (txId) => process(txId, await multisig.read.transactions([txId])) + ) ) : txCount const info = { Owners: await multisig.read.getOwners(), - 'Required confirmations': await multisig.read.required(), - 'Required confirmations (internal)': await multisig.read.internalRequired(), + 'Required confirmations': required, + 'Required confirmations (internal)': internalRequired, Transactions: txinfo, } printValueMapRecursive(info) diff --git a/packages/cli/src/utils/governance.ts b/packages/cli/src/utils/governance.ts index 071498953..f357a83bd 100644 --- a/packages/cli/src/utils/governance.ts +++ b/packages/cli/src/utils/governance.ts @@ -1,11 +1,17 @@ +import { type PublicCeloClient } from '@celo/actions' import { type StrongAddress } from '@celo/base' import { ContractKit } from '@celo/contractkit' -import { ProposalTransaction } from '@celo/contractkit/lib/wrappers/Governance' -import { ProposalBuilder, proposalToJSON, ProposalTransactionJSON } from '@celo/governance' +import { + GovernanceWrapper, + HotfixRecord, + ProposalTransaction, +} from '@celo/contractkit/lib/wrappers/Governance' +import { MultiSigWrapper } from '@celo/contractkit/lib/wrappers/MultiSig' +import { ProposalBuilder, ProposalTransactionJSON, proposalToJSON } from '@celo/governance' import chalk from 'chalk' -import { waitForTransactionReceipt } from 'viem/actions' import { readJsonSync } from 'fs-extra' -import { createWalletClient, http, type Hex } from 'viem' +import { createWalletClient, type Hex, http } from 'viem' +import { waitForTransactionReceipt } from 'viem/actions' import createCeloPublicClient from '../packages-to-be/public-client' export async function checkProposal( @@ -133,6 +139,152 @@ async function tryProposal( return ok } +/** + * What kind of account has to give an approval: + * - `multisig`: a Celo MultiSig, whose confirmations are visible onchain + * - `safe`: a Gnosis Safe, which collects signatures offchain + * - `eoa`: a plain externally owned account + * - `contract`: some other contract we cannot introspect + */ +export type ApproverKind = 'multisig' | 'safe' | 'eoa' | 'contract' + +export interface ApprovalProgress { + /** Address which has to approve */ + address: string + kind: ApproverKind + approved: boolean + confirmations: string[] + /** Confirmations needed before the approval goes through */ + required: number + /** How many signatories still need to confirm */ + remaining: number +} + +const SAFE_GET_THRESHOLD_ABI = [ + { + inputs: [], + name: 'getThreshold', + outputs: [{ name: '', internalType: 'uint256', type: 'uint256' }], + stateMutability: 'view', + type: 'function', + }, +] as const + +/** + * Confirmation progress of a multisig transaction identified by its content. + * Pass `requiredConfirmations` when the threshold was already fetched to save the read. + */ +export async function getMultiSigApprovalProgress( + multiSig: MultiSigWrapper, + destination: string, + encodedData: string, + requiredConfirmations?: number +): Promise> { + const [transaction, required] = await Promise.all([ + multiSig.getTransactionDataByContent(destination, encodedData), + requiredConfirmations !== undefined + ? requiredConfirmations + : multiSig.getRequired().then((r) => r.toNumber()), + ]) + const confirmations = transaction ? transaction.confirmations : [] + + return { + confirmations, + required, + remaining: Math.max(0, required - confirmations.length), + } +} + +/** + * Approval progress of a hotfix for both approval paths (approver and security council). + * Either address may be a Celo MultiSig, a Gnosis Safe, or a plain EOA. + */ +export async function getHotfixApprovalProgress( + publicClient: PublicCeloClient, + governance: GovernanceWrapper, + hotfixHash: string, + record: HotfixRecord +): Promise<{ approver: ApprovalProgress; securityCouncil: ApprovalProgress }> { + const encodedData = governance.encodeFunctionData('approveHotfix', [hotfixHash]) + const [approverMultiSig, securityCouncilMultiSig] = await Promise.all([ + governance.getApproverMultisig(), + governance.getSecurityCouncilMultisig(), + ]) + + const progressFor = async ( + multiSig: MultiSigWrapper, + approved: boolean + ): Promise => { + const address = multiSig.address + + const code = await publicClient.getCode({ address }) + if (!code || code === '0x') { + return { + address, + kind: 'eoa', + approved, + confirmations: approved ? [address] : [], + required: 1, + remaining: approved ? 0 : 1, + } + } + + const required = await multiSig + .getRequired() + .then((r) => r.toNumber()) + .catch(() => undefined) + if (required !== undefined) { + // the executed approval transaction stays in the multisig's history, + // so the confirmation list resolves even after the hotfix was approved + const progress = await getMultiSigApprovalProgress( + multiSig, + governance.address, + encodedData, + required + ) + return { + address, + kind: 'multisig', + approved, + ...progress, + remaining: approved ? 0 : progress.remaining, + } + } + + const threshold = await publicClient + .readContract({ address, abi: SAFE_GET_THRESHOLD_ABI, functionName: 'getThreshold' }) + .then((t) => Number(t)) + .catch(() => undefined) + if (threshold !== undefined) { + // a Safe collects its signatures offchain, so partial progress is not visible here + return { + address, + kind: 'safe', + approved, + confirmations: [], + required: threshold, + remaining: approved ? 0 : threshold, + } + } + + return { + address, + kind: 'contract', + approved, + confirmations: [], + required: 1, + remaining: approved ? 0 : 1, + } + } + + const [approver, securityCouncil] = await Promise.all([ + progressFor(approverMultiSig, record.approved), + progressFor(securityCouncilMultiSig, record.councilApproved), + ]) + + return { approver, securityCouncil } +} + export async function addExistingProposalIDToBuilder( kit: ContractKit, builder: ProposalBuilder, diff --git a/packages/cli/src/utils/multisig-utils.test.ts b/packages/cli/src/utils/multisig-utils.test.ts new file mode 100644 index 000000000..24306cc7a --- /dev/null +++ b/packages/cli/src/utils/multisig-utils.test.ts @@ -0,0 +1,78 @@ +import { Address, Hex, zeroAddress } from 'viem' +import { getConfirmationProgress } from './multisig-utils' + +// deliberately synthetic addresses, they never touch a chain +const MULTISIG = '0x00000000000000000000000000000000000000A1' as Address +const OWNER_1 = '0x0000000000000000000000000000000000000001' as Hex +const OWNER_2 = '0x0000000000000000000000000000000000000002' as Hex +const THRESHOLDS = { required: 3n, internalRequired: 2n } + +const readMultisig = (confirmations: readonly Hex[]) => ({ + getConfirmations: jest.fn().mockResolvedValue(confirmations), +}) + +describe('getConfirmationProgress', () => { + it('counts the confirmations still missing against the regular threshold', async () => { + expect( + await getConfirmationProgress( + readMultisig([OWNER_1]), + 0n, + { destination: zeroAddress, executed: false }, + THRESHOLDS, + MULTISIG + ) + ).toEqual({ + confirmations: [OWNER_1], + required: 3n, + remaining: 2n, + }) + }) + + it('uses the internal threshold for transactions the multisig sends to itself', async () => { + expect( + await getConfirmationProgress( + readMultisig([OWNER_1]), + 0n, + { destination: MULTISIG.toLowerCase() as Address, executed: false }, + THRESHOLDS, + MULTISIG + ) + ).toEqual({ + confirmations: [OWNER_1], + required: 2n, + remaining: 1n, + }) + }) + + it('reports nothing remaining once the transaction executed', async () => { + expect( + await getConfirmationProgress( + readMultisig([OWNER_1, OWNER_2]), + 0n, + { destination: zeroAddress, executed: true }, + THRESHOLDS, + MULTISIG + ) + ).toEqual({ + confirmations: [OWNER_1, OWNER_2], + required: 3n, + remaining: 0n, + }) + }) + + it('never reports a negative remainder when the threshold was lowered', async () => { + expect( + await getConfirmationProgress( + readMultisig([OWNER_1, OWNER_2]), + 0n, + { destination: zeroAddress, executed: false }, + { required: 1n, internalRequired: 1n }, + MULTISIG + ) + ).toEqual({ + confirmations: [OWNER_1, OWNER_2], + required: 1n, + remaining: 0n, + }) + }) +}) diff --git a/packages/cli/src/utils/multisig-utils.ts b/packages/cli/src/utils/multisig-utils.ts index 82c6e86ba..ccb881336 100644 --- a/packages/cli/src/utils/multisig-utils.ts +++ b/packages/cli/src/utils/multisig-utils.ts @@ -1,10 +1,43 @@ -import { Hex } from 'viem' +import { Address, Hex } from 'viem' type ConfirmationGetters = { getConfirmations: (x: [bigint]) => Promise required: () => Promise } +export interface ConfirmationProgress { + confirmations: readonly Hex[] + /** Confirmations needed before the transaction executes */ + required: bigint + /** How many owners still need to confirm */ + remaining: bigint +} + +/** + * How many owners have confirmed a multisig transaction and how many still need to. + * + * MultiSig.isConfirmed compares against the internal threshold for transactions the + * multisig sends to itself, and against the regular one for everything else. + */ +export async function getConfirmationProgress( + readMultisig: Pick, + txIndex: bigint, + transaction: { destination: Address; executed: boolean }, + thresholds: { required: bigint; internalRequired: bigint }, + multisigAddress: Address +): Promise { + const isInternal = transaction.destination.toLowerCase() === multisigAddress.toLowerCase() + const required = isInternal ? thresholds.internalRequired : thresholds.required + const confirmations = await readMultisig.getConfirmations([txIndex]) + const missing = required - BigInt(confirmations.length) + + return { + confirmations, + required, + remaining: transaction.executed || missing < 0n ? 0n : missing, + } +} + export async function viewConfirmationStatus( readMultisig: ConfirmationGetters, txIndex: bigint, diff --git a/packages/sdk/contractkit/src/wrappers/Governance.test.ts b/packages/sdk/contractkit/src/wrappers/Governance.test.ts index cc1e5aa31..771cf47ea 100644 --- a/packages/sdk/contractkit/src/wrappers/Governance.test.ts +++ b/packages/sdk/contractkit/src/wrappers/Governance.test.ts @@ -211,6 +211,27 @@ testWithAnvilL2('Governance Wrapper', (provider) => { expect(approved).toBeTruthy() }) + it('#getApprovalStatus reports the confirmations still needed', async () => { + await proposeFn(accounts[0]) + await timeTravel(dequeueFrequency, provider) + const dequeueHash = await governance.dequeueProposalsIfReady() + await kit.connection.viemClient.waitForTransactionReceipt({ hash: dequeueHash }) + + const required = (await governanceApproverMultiSig.getRequired()).toNumber() + const before = await governance.getApprovalStatus(proposalID) + expect(before.confirmations).toEqual([]) + expect(before.required).toEqual(required) + expect(before.remaining).toEqual(required) + + await approveFn() + + const after = await governance.getApprovalStatus(proposalID) + expect(after.confirmations).toHaveLength(required) + expect(after.required).toEqual(required) + expect(after.remaining).toEqual(0) + expect(await governance.isApproved(proposalID)).toBe(true) + }) + it('#vote', async () => { await proposeFn(accounts[0]) await timeTravel(dequeueFrequency, provider) diff --git a/packages/sdk/contractkit/src/wrappers/Governance.ts b/packages/sdk/contractkit/src/wrappers/Governance.ts index 8923b447e..64886c2a0 100644 --- a/packages/sdk/contractkit/src/wrappers/Governance.ts +++ b/packages/sdk/contractkit/src/wrappers/Governance.ts @@ -88,10 +88,15 @@ export const proposalToParams = (proposal: Proposal, descriptionURL: string): Pr ] } -interface ApprovalStatus { +export interface ApprovalStatus { + /** Confirmations collected out of the total number of approver multisig owners */ completion: string confirmations: string[] approvers: string[] + /** Confirmations the approver multisig needs before the approval executes */ + required: number + /** How many approvers still need to confirm before the approval executes */ + remaining: number } export interface ProposalRecord { @@ -524,17 +529,21 @@ export class GovernanceWrapper extends BaseWrapperForGoverning, + multisig.getRequired(), ]) const confirmations = multisigTxs ? multisigTxs.confirmations : [] + const requiredConfirmations = required.toNumber() return { completion: `${confirmations.length} / ${approvers.length}`, confirmations, approvers, + required: requiredConfirmations, + remaining: Math.max(0, requiredConfirmations - confirmations.length), } } diff --git a/packages/sdk/contractkit/src/wrappers/MultiSig.test.ts b/packages/sdk/contractkit/src/wrappers/MultiSig.test.ts new file mode 100644 index 000000000..0514af294 --- /dev/null +++ b/packages/sdk/contractkit/src/wrappers/MultiSig.test.ts @@ -0,0 +1,195 @@ +import { multiSigABI } from '@celo/abis' +import { MultiSigWrapper } from './MultiSig' + +// synthetic fixtures, never touch a chain +const DESTINATION = '0x00000000000000000000000000000000000000d1' +const OTHER_DESTINATION = '0x00000000000000000000000000000000000000d2' +const WANTED_DATA = '0x0000c0de' +const OTHER_DATA = '0x0000beef' +const OWNERS = [ + '0x0000000000000000000000000000000000000001', + '0x0000000000000000000000000000000000000002', +] + +interface FakeTransaction { + destination: string + value: bigint + data: string + executed: boolean +} + +const otherTransaction = (): FakeTransaction => ({ + destination: OTHER_DESTINATION, + value: BigInt(0), + data: OTHER_DATA, + executed: false, +}) + +const wantedTransaction = (): FakeTransaction => ({ + destination: DESTINATION, + value: BigInt(0), + data: WANTED_DATA, + executed: false, +}) + +function fakeWrapper(transactions: FakeTransaction[]) { + const reads = { transactions: 0 } + const contract = { + address: '0x00000000000000000000000000000000000000a1', + abi: multiSigABI, + read: { + getTransactionCount: jest.fn().mockResolvedValue(BigInt(transactions.length)), + transactionCount: jest.fn().mockResolvedValue(BigInt(transactions.length)), + transactions: jest.fn().mockImplementation(async ([id]: [bigint]) => { + reads.transactions++ + const tx = transactions[Number(id)] + return [tx.destination, tx.value, tx.data, tx.executed] + }), + getConfirmations: jest.fn().mockResolvedValue(OWNERS), + getTransactionIds: jest.fn().mockImplementation(async ([from, to]: [bigint, bigint]) => { + const ids = [] + for (let i = Number(from); i < Number(to); i++) ids.push(BigInt(i)) + return ids + }), + getOwners: jest.fn().mockResolvedValue(OWNERS), + isOwner: jest.fn().mockResolvedValue(true), + required: jest.fn().mockResolvedValue(BigInt(2)), + internalRequired: jest.fn().mockResolvedValue(BigInt(1)), + }, + write: { + confirmTransaction: jest + .fn() + .mockResolvedValue('0x00000000000000000000000000000000000000000000000000000000000000c1'), + submitTransaction: jest + .fn() + .mockResolvedValue('0x00000000000000000000000000000000000000000000000000000000000000c2'), + replaceOwner: jest + .fn() + .mockResolvedValue('0x00000000000000000000000000000000000000000000000000000000000000c3'), + }, + } + const connection = { viemClient: {} } + const wrapper = new MultiSigWrapper(connection as any, contract as any) + return { wrapper, contract, reads } +} + +describe('MultiSigWrapper.getTransactionDataByContent', () => { + it('finds a recent transaction within the first batch', async () => { + const transactions = [...Array.from({ length: 30 }, otherTransaction), wantedTransaction()] + const { wrapper, reads } = fakeWrapper(transactions) + + const result = await wrapper.getTransactionDataByContent(DESTINATION, WANTED_DATA) + + expect(result?.index).toEqual(30) + expect(result?.confirmations).toEqual(OWNERS) + // newest-first batching finds it without reading the whole history + expect(reads.transactions).toBeLessThan(transactions.length) + }) + + it('keeps scanning older batches until it finds the transaction', async () => { + const transactions = Array.from({ length: 60 }, otherTransaction) + transactions[3] = wantedTransaction() + const { wrapper, reads } = fakeWrapper(transactions) + + const result = await wrapper.getTransactionDataByContent(DESTINATION, WANTED_DATA) + + expect(result?.index).toEqual(3) + expect(reads.transactions).toEqual(60) + }) + + it('returns the most recent transaction when several share the same content', async () => { + const transactions = Array.from({ length: 40 }, otherTransaction) + transactions[10] = wantedTransaction() + transactions[35] = wantedTransaction() + const { wrapper } = fakeWrapper(transactions) + + const result = await wrapper.getTransactionDataByContent(DESTINATION, WANTED_DATA) + + expect(result?.index).toEqual(35) + }) + + it('returns undefined after scanning everything without a match', async () => { + const transactions = Array.from({ length: 26 }, otherTransaction) + const { wrapper, reads } = fakeWrapper(transactions) + + const result = await wrapper.getTransactionDataByContent(DESTINATION, WANTED_DATA) + + expect(result).toBeUndefined() + expect(reads.transactions).toEqual(26) + }) + + it('matches on value in addition to destination and data', async () => { + const transactions = [{ ...wantedTransaction(), value: BigInt(7) }] + const { wrapper } = fakeWrapper(transactions) + + expect(await wrapper.getTransactionDataByContent(DESTINATION, WANTED_DATA)).toBeUndefined() + expect((await wrapper.getTransactionDataByContent(DESTINATION, WANTED_DATA, 7))?.index).toEqual( + 0 + ) + }) +}) + +describe('MultiSigWrapper.getConfirmations', () => { + it('returns the confirming owners from a single contract read', async () => { + const { wrapper } = fakeWrapper([wantedTransaction()]) + + expect(await wrapper.getConfirmations(0)).toEqual(OWNERS) + }) +}) + +describe('MultiSigWrapper.submitOrConfirmTransaction', () => { + it('confirms the matching pending transaction instead of submitting a new one', async () => { + const { wrapper, contract } = fakeWrapper([otherTransaction(), wantedTransaction()]) + + await wrapper.submitOrConfirmTransaction(DESTINATION, WANTED_DATA) + + expect(contract.write.confirmTransaction).toHaveBeenCalledWith([BigInt(1)], undefined) + expect(contract.write.submitTransaction).not.toHaveBeenCalled() + }) + + it('submits a new transaction when no pending one matches', async () => { + const { wrapper, contract } = fakeWrapper([otherTransaction()]) + + await wrapper.submitOrConfirmTransaction(DESTINATION, WANTED_DATA) + + expect(contract.write.submitTransaction).toHaveBeenCalled() + expect(contract.write.confirmTransaction).not.toHaveBeenCalled() + }) +}) + +describe('MultiSigWrapper reads', () => { + it('reads transactions with and without confirmations', async () => { + const { wrapper } = fakeWrapper([wantedTransaction()]) + + const withConfirmations = await wrapper.getTransaction(0) + expect(withConfirmations.confirmations).toEqual(OWNERS) + expect(withConfirmations.destination).toEqual(DESTINATION) + expect(withConfirmations.value.toFixed()).toEqual('0') + + expect(await wrapper.getTransactions()).toHaveLength(1) + }) + + it('exposes owners, thresholds and counts', async () => { + const { wrapper } = fakeWrapper([wantedTransaction(), otherTransaction()]) + + expect(await wrapper.getOwners()).toEqual(OWNERS) + expect(await wrapper.isOwner(OWNERS[0] as any)).toEqual(true) + expect((await wrapper.getRequired()).toNumber()).toEqual(2) + expect((await wrapper.getInternalRequired()).toNumber()).toEqual(1) + expect(await wrapper.totalTransactionCount()).toEqual(2) + expect(await wrapper.getTransactionCount(true, true)).toEqual(2) + }) + + it('wraps the write helpers', async () => { + const { wrapper, contract } = fakeWrapper([wantedTransaction()]) + + await wrapper.confirmTransaction(0) + expect(contract.write.confirmTransaction).toHaveBeenCalledWith([BigInt(0)], undefined) + + await wrapper.submitTransaction(DESTINATION, WANTED_DATA) + expect(contract.write.submitTransaction).toHaveBeenCalled() + + await wrapper.replaceOwner(OWNERS[0] as any, OWNERS[1] as any) + expect(contract.write.replaceOwner).toHaveBeenCalled() + }) +}) diff --git a/packages/sdk/contractkit/src/wrappers/MultiSig.ts b/packages/sdk/contractkit/src/wrappers/MultiSig.ts index 393844637..7f03bb00b 100644 --- a/packages/sdk/contractkit/src/wrappers/MultiSig.ts +++ b/packages/sdk/contractkit/src/wrappers/MultiSig.ts @@ -3,13 +3,17 @@ import { Address, CeloTx } from '@celo/connect' import BigNumber from 'bignumber.js' import { BaseWrapper, + stringToSolidityBytes, toViemAddress, toViemBigInt, - stringToSolidityBytes, valueToBigNumber, valueToInt, } from './BaseWrapper' +// small enough to stay under typical RPC rate limits, large enough to +// find a recent transaction in one round trip +const TRANSACTION_SCAN_BATCH_SIZE = 25 + export interface TransactionData { destination: string value: BigNumber @@ -149,24 +153,27 @@ export class MultiSigWrapper extends BaseWrapper { ) { const data = stringToSolidityBytes(encodedData) const transactionCount = await this.getTransactionCount(true, true) - const transactionsOrEmpties = await Promise.all( - new Array(transactionCount).fill(0).map(async (_, index) => { - const tx = await this.getTransaction(index, false) - if (tx.data === data && tx.destination === destination && tx.value.isEqualTo(value)) { - return { index, ...tx } + // scan newest-first in small batches: the wanted transaction is almost always + // recent, and fetching the multisig's entire history at once trips RPC rate limits. + // When several transactions share the same content, the most recent one wins. + for (let end = transactionCount; end > 0; end -= TRANSACTION_SCAN_BATCH_SIZE) { + const start = Math.max(0, end - TRANSACTION_SCAN_BATCH_SIZE) + const indices = Array.from({ length: end - start }, (_, i) => end - 1 - i) + const transactions = await Promise.all( + indices.map(async (index) => ({ index, ...(await this.getTransaction(index, false)) })) + ) + const wantedTransaction = transactions.find( + (tx) => tx.data === data && tx.destination === destination && tx.value.isEqualTo(value) + ) + if (wantedTransaction) { + const confirmations = await this.getConfirmations(wantedTransaction.index) + return { + ...wantedTransaction, + confirmations, } - return null - }) - ) - const wantedTransaction = transactionsOrEmpties.find((tx) => tx !== null) - if (!wantedTransaction) { - return - } - const confirmations = await this.getConfirmations(wantedTransaction.index) - return { - ...wantedTransaction, - confirmations, + } } + return } async getTransaction(i: number): Promise async getTransaction( @@ -198,28 +205,13 @@ export class MultiSigWrapper extends BaseWrapper { } } - private _getConfirmation = async (txId: number, owner: string) => { - return this.contract.read.confirmations([toViemBigInt(txId), toViemAddress(owner)]) - } - /* * Returns array of signer addresses which have confirmed a transaction * when given the index of that transaction. */ async getConfirmations(txId: number) { - const owners = await this.getOwners() - const confirmationsOrEmpties = await Promise.all( - owners.map(async (owner: string) => { - const confirmation = await this._getConfirmation(txId, owner) - if (confirmation) { - return owner - } else { - return null - } - }) - ) - const confirmations = confirmationsOrEmpties.filter((c) => c !== null) as string[] - return confirmations + const res = await this.contract.read.getConfirmations([toViemBigInt(txId)]) + return [...res] as string[] } async getTransactions(): Promise {