fix(governance): proxy-repoint log + show proposal id in governance:propose

.changeset/fix-propose-proxy-log-and-proposalid.md

@@ -0,0 +1,44 @@
+---
+'@celo/governance': patch
+'@celo/actions': patch
+'@celo/dev-utils': patch
+'@celo/explorer': patch
+'@celo/celocli': patch
+---
+
+Fix several `governance`/`celocli` command output & safety issues:
+- `governance:propose` logged `undefined is a proxy, repointing to ...` for
+  core-contract proxy repoints (logged `tx.address` which is undefined when the
+  tx is keyed by `contract`); now logs the real proxy id.
+- `governance:propose` now surfaces the new proposal id (`ProposalQueued`), and
+  the `--useMultiSig` path surfaces the multisig transaction id (`Submission` on
+  submit, `Confirmation` on a later signer) plus the proposal id when the submit
+  reaches threshold and executes in the same receipt.
+- `@celo/actions` `getGroupsWithPendingVotes` now filters on pending votes `> 0`
+  (was `>= 0`, which returned every group); fixes `election:activate` selecting
+  groups with no pending votes.
+- `governance:execute` now checks the proposal is approved before sending, so it
+  fails the precondition cleanly instead of reverting with "Proposal not approved".
+- `governance:upvote`/`revokeupvote`/`votePartially` and `multisig:approve` now
+  decode and print their on-chain events (proposal id / transaction id).
+- `governance:propose` can now build a core-contract call whose method is added
+  by an earlier upgrade tx in the same proposal: when the method is absent from
+  the bundled ABI, it is resolved from the implementation a prior tx repoints the
+  proxy to (verified metadata), with a raw `function: "name(uint256)"` signature
+  fallback.
+- `governance:propose` now simulates the proposal by default against a
+  self-contained local fork (bundled `@foundry-rs/anvil`) of the connected node,
+  applying the transactions in order so a transaction that depends on an earlier
+  one (e.g. a method added by a prior upgrade tx) simulates correctly. Use
+  `--simulate <rpcUrl>` to target an external fork, or `--no-simulate` to fall
+  back to the previous independent per-transaction `eth_call` checks.
+- `lockedcelo:withdraw` (and `releasecelo:locked-gold` withdraw) no longer spin
+  in an infinite loop when no pending withdrawal is available, and re-fetch
+  between withdrawals to avoid stale indices.
+- `@celo/dev-utils` anvil test harness now resolves the foundry-installed
+  `anvil` (snapshot-compatible) instead of a package-manager `anvil` bin shim,
+  so packages that bundle a newer anvil don't break the devchain state load.
+- `@celo/explorer` `fetchMetadata` now uses the Sourcify v2 API (the v1 repo API
+  has been sunset / returns 503), so contract ABI resolution (used by
+  `governance:propose` to build calls to verified contracts, including
+  implementations added by an in-proposal upgrade) works again.

packages/actions/src/contracts/election.test.ts

@@ -0,0 +1,43 @@
+import type { Address } from 'viem'
+import { describe, expect, it, vi } from 'vitest'
+import { getGroupsWithPendingVotes } from './election.js'
+
+// resolveAddress hits the on-chain registry; stub it so the unit test only
+// exercises the pending-votes filtering logic.
+vi.mock('./registry.js', () => ({
+  resolveAddress: vi.fn(async () => '0x0000000000000000000000000000000000000001'),
+}))
+
+const ACCOUNT = '0x00000000000000000000000000000000000000aa' as Address
+const GROUP_A = '0x000000000000000000000000000000000000000a' as Address
+const GROUP_B = '0x000000000000000000000000000000000000000b' as Address
+const GROUP_C = '0x000000000000000000000000000000000000000c' as Address
+
+function clients(groups: Address[], pendingVotes: bigint[]) {
+  return {
+    public: {
+      readContract: vi.fn(async () => groups),
+      multicall: vi.fn(async () => pendingVotes),
+    },
+  } as any
+}
+
+describe('getGroupsWithPendingVotes', () => {
+  it('returns only groups whose pending votes are strictly greater than zero', async () => {
+    // Regression: the filter used `>= 0`, which kept every group (including
+    // those with 0 pending votes). It must be `> 0`.
+    const result = await getGroupsWithPendingVotes(
+      clients([GROUP_A, GROUP_B, GROUP_C], [BigInt(0), BigInt(5), BigInt(0)]),
+      ACCOUNT
+    )
+    expect(result).toEqual([GROUP_B])
+  })
+
+  it('returns an empty array when every group has zero pending votes', async () => {
+    const result = await getGroupsWithPendingVotes(
+      clients([GROUP_A, GROUP_B], [BigInt(0), BigInt(0)]),
+      ACCOUNT
+    )
+    expect(result).toEqual([])
+  })
+})

packages/actions/src/contracts/election.ts

@@ -1,5 +1,5 @@
 import { electionABI } from '@celo/abis'
-import { Address, getContract, GetContractReturnType, Hex, isAddressEqual } from 'viem'
+import { Address, GetContractReturnType, getContract, Hex, isAddressEqual } from 'viem'
 import { Clients } from '../client.js'
 import { resolveAddress } from './registry.js'
 
@@ -47,7 +47,7 @@ export async function getGroupsWithPendingVotes(
         }) as const
     ),
   })
-  const groupsWithPendingVotes = groups.filter((_, i) => pendingVotes[i] >= 0)
+  const groupsWithPendingVotes = groups.filter((_, i) => pendingVotes[i] > BigInt(0))
   return groupsWithPendingVotes
 }
 

packages/cli/package.json

@@ -54,6 +54,7 @@
     "@celo/wallet-hsm-azure": "^8.0.4",
     "@celo/wallet-ledger": "^8.0.4",
     "@celo/wallet-local": "^8.0.4",
+    "@foundry-rs/anvil": "1.7.1",
     "@ledgerhq/hw-transport-node-hid": "^6.28.5",
     "@oclif/core": "^3.27.0",
     "@oclif/plugin-autocomplete": "^3.2.0",
@@ -65,6 +66,7 @@
     "@safe-global/protocol-kit": "^5.0.4",
     "@safe-global/types-kit": "^1.0.0",
     "@types/command-exists": "^1.2.3",
+    "@viem/anvil": "^0.0.9",
     "bignumber.js": "9.0.0",
     "chalk": "^2.4.2",
     "command-exists": "^1.2.9",

packages/cli/src/commands/account/authorize.test.ts

@@ -256,10 +256,7 @@ testWithAnvilL2('account:authorize cmd', (provider) => {
 
         provider
       )
-    ).rejects.toMatchInlineSnapshot(`
-      [Error: Nonexistent flags: --blsKey, --blsPop
-      See more help with --help]
-    `)
+    ).rejects.toMatchInlineSnapshot(`[Error: BLS keys are not supported anymore]`)
 
     expect(stripAnsiCodesFromNestedArray(logMock.mock.calls)).toMatchInlineSnapshot(`[]`)
   })

packages/cli/src/commands/account/authorize.ts

@@ -29,13 +29,18 @@ export default class Authorize extends BaseCommand {
       default: false,
       hidden: true,
     }),
+    // Declared (hidden, deprecated) only so passing them yields the clear
+    // "BLS keys are not supported anymore" error below instead of oclif's
+    // generic unknown-flag rejection.
+    blsKey: Flags.string({ hidden: true, deprecated: true }),
+    blsPop: Flags.string({ hidden: true, deprecated: true }),
   }
 
   static args = {}
 
   static examples = [
     'authorize --from 0x5409ED021D9299bf6814279A6A1411A7e866A631 --role vote --signer 0x6ecbe1db9ef729cbe972c83fb886247691fb6beb --signature 0x1b9fca4bbb5bfb1dbe69ef1cddbd9b4202dcb6b134c5170611e1e36ecfa468d7b46c85328d504934fce6c2a1571603a50ae224d2b32685e84d4d1a1eebad8452eb',
-    'authorize --from 0x5409ED021D9299bf6814279A6A1411A7e866A631 --role validator --signer 0x6ecbe1db9ef729cbe972c83fb886247691fb6beb --signature 0x1b9fca4bbb5bfb1dbe69ef1cddbd9b4202dcb6b134c5170611e1e36ecfa468d7b46c85328d504934fce6c2a1571603a50ae224d2b32685e84d4d1a1eebad8452eb --blsKey 0x4fa3f67fc913878b068d1fa1cdddc54913d3bf988dbe5a36a20fa888f20d4894c408a6773f3d7bde11154f2a3076b700d345a42fd25a0e5e83f4db5586ac7979ac2053cd95d8f2efd3e959571ceccaa743e02cf4be3f5d7aaddb0b06fc9aff00 --blsPop 0xcdb77255037eb68897cd487fdd85388cbda448f617f874449d4b11588b0b7ad8ddc20d9bb450b513bb35664ea3923900',
+    'authorize --from 0x5409ED021D9299bf6814279A6A1411A7e866A631 --role validator --signer 0x6ecbe1db9ef729cbe972c83fb886247691fb6beb --signature 0x1b9fca4bbb5bfb1dbe69ef1cddbd9b4202dcb6b134c5170611e1e36ecfa468d7b46c85328d504934fce6c2a1571603a50ae224d2b32685e84d4d1a1eebad8452eb',
   ]
 
   async run() {

packages/cli/src/commands/account/deauthorize.test.ts

@@ -47,6 +47,15 @@ testWithAnvilL2('account:deauthorize cmd', (provider) => {
 
     expect(stripAnsiCodesFromNestedArray(logMock.mock.calls)).toMatchInlineSnapshot(`
       [
+        [
+          "Running Checks:",
+        ],
+        [
+          "   ✔  0x5409ED021D9299bf6814279A6A1411A7e866A631 is a registered Account ",
+        ],
+        [
+          "All checks passed",
+        ],
         [
           "SendTransaction: deauthorizeTx",
         ],

packages/cli/src/commands/account/deauthorize.ts

@@ -1,5 +1,6 @@
 import { Flags } from '@oclif/core'
 import { BaseCommand } from '../../base'
+import { newCheckBuilder } from '../../utils/checks'
 import { displayViemTx } from '../../utils/cli'
 import { CustomFlags } from '../../utils/command'
 
@@ -36,6 +37,8 @@ export default class Deauthorize extends BaseCommand {
       return
     }
 
+    await newCheckBuilder(this).isAccount(res.flags.from).runChecks()
+
     const attestationSigner = await accounts.getAttestationSigner(res.flags.from)
 
     if (res.flags.signer !== attestationSigner) {

packages/cli/src/commands/account/delete-payment-delegation.ts

@@ -1,4 +1,5 @@
 import { BaseCommand } from '../../base'
+import { newCheckBuilder } from '../../utils/checks'
 import { displayViemTx } from '../../utils/cli'
 import { CustomFlags } from '../../utils/command'
 
@@ -24,6 +25,8 @@ export default class DeletePaymentDelegation extends BaseCommand {
     kit.defaultAccount = res.flags.account
     const accounts = await kit.contracts.getAccounts()
 
+    await newCheckBuilder(this).isAccount(res.flags.account).runChecks()
+
     await displayViemTx('deletePaymentDelegation', accounts.deletePaymentDelegation(), publicClient)
 
     console.log('Deleted payment delegation.')

packages/cli/src/commands/account/set-payment-delegation.ts

@@ -1,6 +1,7 @@
 import { valueToFixidityString } from '@celo/contractkit/lib/wrappers/BaseWrapper'
 import { Flags } from '@oclif/core'
 import { BaseCommand } from '../../base'
+import { newCheckBuilder } from '../../utils/checks'
 import { displayViemTx } from '../../utils/cli'
 import { CustomFlags } from '../../utils/command'
 
@@ -28,6 +29,8 @@ export default class SetPaymentDelegation extends BaseCommand {
     kit.defaultAccount = res.flags.account
     const accounts = await kit.contracts.getAccounts()
 
+    await newCheckBuilder(this).isAccount(res.flags.account).runChecks()
+
     await displayViemTx(
       'setPaymentDelegation',
       accounts.setPaymentDelegation(

packages/cli/src/commands/account/set-wallet.ts

@@ -39,17 +39,19 @@ export default class SetWallet extends BaseCommand {
     await newCheckBuilder(this).isAccount(res.flags.account).runChecks()
 
     if (res.flags.signature !== undefined) {
+      let signature: ReturnType<typeof accounts.parseSignatureOfAddress>
       try {
-        accounts.parseSignatureOfAddress(res.flags.account, res.flags.signer, res.flags.signature)
-      } catch (error) {
-        console.error('Error: Failed to parse signature')
+        signature = accounts.parseSignatureOfAddress(
+          res.flags.account,
+          res.flags.signer,
+          res.flags.signature
+        )
+      } catch (_error) {
+        return this.error('Failed to parse signature')
       }
       await displayViemTx(
         'setWalletAddress',
-        accounts.setWalletAddress(
-          res.flags.wallet,
-          accounts.parseSignatureOfAddress(res.flags.account, res.flags.signer, res.flags.signature)
-        ),
+        accounts.setWalletAddress(res.flags.wallet, signature),
         publicClient
       )
     } else {

packages/cli/src/commands/governance/execute.test.ts

@@ -1,3 +1,4 @@
+import path from 'node:path'
 import { AbiItem, PROXY_ADMIN_ADDRESS } from '@celo/connect'
 import { newKitFromProvider } from '@celo/contractkit'
 import { Proposal } from '@celo/contractkit/lib/wrappers/Governance'
@@ -9,10 +10,9 @@ import {
 } from '@celo/dev-utils/anvil-test'
 import { timeTravel } from '@celo/dev-utils/ganache-test'
 import fs from 'fs'
-import path from 'node:path'
+import { decodeFunctionResult, encodeFunctionData, parseEther } from 'viem'
 import { stripAnsiCodesAndTxHashes, testLocallyWithNode } from '../../test-utils/cliUtils'
 import Execute from './execute'
-import { decodeFunctionResult, encodeFunctionData, parseEther } from 'viem'
 
 process.env.NO_SYNCCHECK = 'true'
 
@@ -196,6 +196,9 @@ testWithAnvilL2('governance:execute cmd', (provider) => {
         [
           "   ✔  1 is in stage Execution ",
         ],
+        [
+          "   ✔  Proposal 1 is approved ",
+        ],
         [
           "   ✔  Proposal 1 is passing corresponding constitutional quorum ",
         ],
@@ -217,4 +220,54 @@ testWithAnvilL2('governance:execute cmd', (provider) => {
       ]
     `)
   })
+
+  it('fails the approved check for an unapproved proposal in the Execution stage', async () => {
+    const kit = newKitFromProvider(provider)
+    const governanceWrapper = await kit.contracts.getGovernance()
+    const [, proposer, voter] = await kit.connection.getAccounts()
+    const minDeposit = (await governanceWrapper.minDeposit()).toFixed()
+    const lockedGold = await kit.contracts.getLockedGold()
+    const majorityOfVotes = (await lockedGold.getTotalLockedGold()).multipliedBy(0.6)
+    const dequeueFrequency = (await governanceWrapper.dequeueFrequency()).toNumber()
+    const proposalId = 1
+
+    await setCode(provider, PROXY_ADMIN_ADDRESS, TEST_TRANSACTIONS_BYTECODE)
+
+    const proposeHash = await governanceWrapper.propose(PROPOSAL_TRANSACTIONS, 'URL', {
+      from: proposer,
+      value: minDeposit,
+    })
+    await kit.connection.viemClient.waitForTransactionReceipt({
+      hash: proposeHash as `0x${string}`,
+    })
+
+    const accountWrapper = await kit.contracts.getAccounts()
+    const createHash = await accountWrapper.createAccount({ from: voter })
+    await kit.connection.viemClient.waitForTransactionReceipt({ hash: createHash as `0x${string}` })
+    const lockHash = await lockedGold.lock({ from: voter, value: majorityOfVotes.toFixed() })
+    await kit.connection.viemClient.waitForTransactionReceipt({ hash: lockHash as `0x${string}` })
+
+    await timeTravel(dequeueFrequency + 1, provider)
+    const dequeueHash = await governanceWrapper.dequeueProposalsIfReady({ from: proposer })
+    await kit.connection.viemClient.waitForTransactionReceipt({
+      hash: dequeueHash as `0x${string}`,
+    })
+
+    // NB: intentionally NOT approving the proposal, then vote + advance to Execution
+    const lockHash2 = await lockedGold.lock({ from: voter, value: minDeposit })
+    await kit.connection.viemClient.waitForTransactionReceipt({ hash: lockHash2 as `0x${string}` })
+    const voteHash = await governanceWrapper.vote(proposalId, 'Yes', { from: voter })
+    await kit.connection.viemClient.waitForTransactionReceipt({ hash: voteHash as `0x${string}` })
+    await timeTravel((await governanceWrapper.stageDurations()).Referendum.toNumber() + 1, provider)
+
+    // The command must fail the on-chain approval precondition check rather than
+    // sending a tx that reverts with "Proposal not approved".
+    await expect(
+      testLocallyWithNode(
+        Execute,
+        ['--proposalID', proposalId.toString(), '--from', proposer],
+        provider
+      )
+    ).rejects.toThrow(/checks didn't pass/i)
+  })
 })

packages/cli/src/commands/governance/execute.ts

@@ -27,6 +27,7 @@ export default class Execute extends BaseCommand {
     await newCheckBuilder(this, account)
       .proposalExists(id)
       .proposalInStage(id, 'Execution')
+      .proposalIsApproved(id)
       .proposalIsPassing(id)
       .runChecks()
 

packages/cli/src/commands/governance/propose.test.ts

@@ -193,6 +193,8 @@ testWithAnvilL2(
         const proposalBefore = await governance.getProposal(1)
         expect(proposalBefore).toEqual([])
 
+        const logMock = jest.spyOn(console, 'log')
+
         await testLocallyWithNode(
           Propose,
           [
@@ -208,6 +210,13 @@ testWithAnvilL2(
           client
         )
 
+        // The command must surface the newly created proposal id (decoded from
+        // the ProposalQueued event), not just the tx hash.
+        const loggedText = stripAnsiCodesFromNestedArray(logMock.mock.calls).flat().join('\n')
+        logMock.mockRestore()
+        expect(loggedText).toContain('ProposalQueued:')
+        expect(loggedText).toContain('proposalId: 1')
+
         const proposal = await governance.getProposal(1)
         expect(proposal.length).toEqual(transactions.length)
         expect(proposal[0].to).toEqual(goldToken.address)
@@ -251,6 +260,8 @@ testWithAnvilL2(
         const proposalBefore = await governance.getProposal(1)
         expect(proposalBefore).toEqual([])
 
+        const logMock = jest.spyOn(console, 'log')
+
         await testLocallyWithNode(
           Propose,
           [
@@ -269,6 +280,16 @@ testWithAnvilL2(
           client
         )
 
+        // With a single-signer multisig the submit reaches threshold and the
+        // underlying governance.propose executes in the same receipt, so the
+        // command must surface BOTH the multisig submission id and the new
+        // proposal id (not just the tx hash).
+        const loggedText = stripAnsiCodesFromNestedArray(logMock.mock.calls).flat().join('\n')
+        logMock.mockRestore()
+        expect(loggedText).toContain('Submission:')
+        expect(loggedText).toContain('ProposalQueued:')
+        expect(loggedText).toContain('proposalId: 1')
+
         const proposal = await governance.getProposal(1)
         expect(proposal.length).toEqual(transactions.length)
         expect(proposal[0].to).toEqual(goldToken.address)