Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
623 changes: 296 additions & 327 deletions packages/cli/src/commands/governance/approve.test.ts

Large diffs are not rendered by default.

111 changes: 62 additions & 49 deletions packages/cli/src/commands/governance/approve.ts
Original file line number Diff line number Diff line change
@@ -1,23 +1,17 @@
import { StrongAddress } from '@celo/base'
import { CeloTransactionObject } from '@celo/connect'
import { type Provider } from '@celo/connect'
import { GovernanceWrapper } from '@celo/contractkit/lib/wrappers/Governance'
import { MultiSigWrapper } from '@celo/contractkit/lib/wrappers/MultiSig'
import { toBuffer } from '@ethereumjs/util'
import { hexToBytes } from 'viem'
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'
import { displaySendTx, failWith } from '../../utils/cli'
import { displayViemTx, failWith } from '../../utils/cli'
import { CustomFlags } from '../../utils/command'
import {
createSafeFromWeb3,
performSafeTransaction,
safeTransactionMetadataFromCeloTransactionObject,
} from '../../utils/safe'
import { createSafe, performSafeTransaction, safeTransactionMetadata } from '../../utils/safe'

enum HotfixApprovalType {
APPROVER = 'approver',
Expand Down Expand Up @@ -80,6 +74,7 @@ export default class Approve extends BaseCommand {
async run() {
const checkBuilder = newCheckBuilder(this)
const kit = await this.getKit()
const publicClient = await this.getPublicClient()
const res = await this.parse(Approve)
const account = res.flags.from
const useMultiSig = res.flags.useMultiSig
Expand All @@ -98,7 +93,7 @@ export default class Approve extends BaseCommand {
const approver = useMultiSig ? governanceApproverMultiSig!.address : account

await addDefaultChecks(
await this.getWeb3(),
(await this.getKit()).connection.currentProvider,
checkBuilder,
governance,
!!hotfix,
Expand All @@ -111,11 +106,11 @@ export default class Approve extends BaseCommand {
governanceApproverMultiSig
)

let governanceTx: CeloTransactionObject<any>
let logEvent: string
let encodedGovernanceData: `0x${string}` | undefined
if (id) {
if (await governance.isQueued(id)) {
await governance.dequeueProposalsIfReady().sendAndWaitForReceipt()
const dequeueHash = await governance.dequeueProposalsIfReady()
await publicClient.waitForTransactionReceipt({ hash: dequeueHash })
}

await checkBuilder
Expand All @@ -126,78 +121,96 @@ export default class Approve extends BaseCommand {
'Proposal has not been submitted to multisig',
res.flags.submit,
async () => {
// We would prefer it allow for submissions if there is ambiguity, only fail if we confirm that it has been submitted
const confrimations = await fetchConfirmationsForProposals(id)
return confrimations === null || confrimations.count === 0
}
)
.addConditionalCheck('multisgTXId provided is valid', !!res.flags.multisigTx, async () => {
const confirmations = await fetchConfirmationsForProposals(id)
// if none are found the api could be wrong, so we allow it.
if (!confirmations || confirmations.count === 0) {
return true
}
// if we have confirmations, ensure one matches the provided id
return confirmations.approvals.some(
(approval) => approval.multisigTx.toString() === res.flags.multisigTx
)
})
.runChecks()
governanceTx = await governance.approve(id)
logEvent = 'ProposalApproved'

if (useMultiSig || useSafe) {
const dequeue = await governance.getDequeue()
const proposalIndex = dequeue.findIndex((d) => d.eq(id))
encodedGovernanceData = governance.encodeFunctionData('approve', [
id,
proposalIndex.toString(),
Comment on lines +141 to +144
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject missing dequeue index before encoding approve calldata

When --proposalID is used with --useMultiSig/--useSafe, the code now computes proposalIndex via findIndex but never checks for -1. If the proposal is no longer in getDequeue() (e.g., state changed between checks and encoding), this builds calldata with an invalid index instead of failing fast, so the command can submit an unusable multisig/safe transaction. The previous path (governance.approve(id)) guarded this by throwing when the dequeue index was missing.

Useful? React with 👍 / 👎.

])
}
} else if (hotfix) {
await checkBuilder.runChecks()

// TODO dedup toBuffer
governanceTx = governance.approveHotfix(toBuffer(hotfix) as Buffer)
logEvent = 'HotfixApproved'
if (useMultiSig || useSafe) {
encodedGovernanceData = governance.encodeFunctionData('approveHotfix', [hotfix])
}
} else {
failWith('Proposal ID or hotfix must be provided')
}

if (approvalType === 'securityCouncil' && useSafe) {
await performSafeTransaction(
await this.getWeb3(),
await governance.getSecurityCouncil(),
(await this.getKit()).connection.currentProvider,
(await governance.getSecurityCouncil()) as StrongAddress,
account,
await safeTransactionMetadataFromCeloTransactionObject(governanceTx, governance.address)
safeTransactionMetadata(encodedGovernanceData!, governance.address)
)
} else if (
approvalType === 'securityCouncil' &&
useMultiSig &&
governanceSecurityCouncilMultiSig
) {
const tx = await governanceSecurityCouncilMultiSig.submitOrConfirmTransaction(
governance.address,
governanceTx.txo
await displayViemTx(
'approveTx',
governanceSecurityCouncilMultiSig.submitOrConfirmTransaction(
governance.address,
encodedGovernanceData!
),
publicClient
)

await displaySendTx<string | void | boolean>('approveTx', tx, {}, logEvent)
} else if (res.flags.multisigTx && useMultiSig) {
const tx = await governanceApproverMultiSig!.confirmTransaction(
parseInt(res.flags.multisigTx)
await displayViemTx(
'approveTx',
governanceApproverMultiSig!.confirmTransaction(parseInt(res.flags.multisigTx)),
publicClient
)
await displaySendTx<string | void | boolean>('approveTx', tx, {}, logEvent)
} else if (res.flags.submit && useMultiSig) {
const tx = await governanceApproverMultiSig!.submitTransaction(
governance.address,
governanceTx.txo
await displayViemTx(
'approveTx',
governanceApproverMultiSig!.submitTransaction(governance.address, encodedGovernanceData!),
publicClient
)
} else if (useMultiSig) {
await displayViemTx(
'approveTx',
governanceApproverMultiSig!.submitOrConfirmTransaction(
governance.address,
encodedGovernanceData!
),
publicClient
)
await displaySendTx<string | void | boolean>('approveTx', tx, {}, logEvent)
} else {
const tx = useMultiSig
? await governanceApproverMultiSig!.submitOrConfirmTransaction(
governance.address,
governanceTx.txo
)
: governanceTx
await displaySendTx<string | void | boolean>('approveTx', tx, {}, logEvent)
if (id) {
await displayViemTx('approveTx', governance.approve(id), publicClient)
} else {
await displayViemTx(
'approveTx',
governance.approveHotfix(Buffer.from(hexToBytes(hotfix! as `0x${string}`))),
publicClient
)
}
}
}
}

const addDefaultChecks = async (
web3: Web3,
provider: Provider,
checkBuilder: ReturnType<typeof newCheckBuilder>,
governance: GovernanceWrapper,
isHotfix: boolean,
Expand All @@ -210,7 +223,7 @@ const addDefaultChecks = async (
governanceApproverMultiSig: MultiSigWrapper | undefined
) => {
if (isHotfix) {
const hotfixBuf = toBuffer(hotfix) as Buffer
const hotfixBuf = Buffer.from(hexToBytes(hotfix as `0x${string}`))

if (approvalType === HotfixApprovalType.APPROVER || approvalType === undefined) {
if (useMultiSig) {
Expand Down Expand Up @@ -238,10 +251,10 @@ const addDefaultChecks = async (
})
} else if (useSafe) {
checkBuilder.addCheck(`${account} is security council safe signatory`, async () => {
const protocolKit = await createSafeFromWeb3(
web3,
const protocolKit = await createSafe(
provider,
account,
await governance.getSecurityCouncil()
(await governance.getSecurityCouncil()) as StrongAddress
)

return await protocolKit.isOwner(account)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@ import CeloTokenABI from '@celo/abis/GoldToken.json'
import { testWithAnvilL2 } from '@celo/dev-utils/anvil-test'
import { readJSON, removeSync } from 'fs-extra'
import inquirer from 'inquirer'
import Web3 from 'web3'
import { testLocallyWithWeb3Node } from '../../test-utils/cliUtils'
import { testLocallyWithNode } from '../../test-utils/cliUtils'
import BuildProposal from './build-proposal'

process.env.NO_SYNCCHECK = 'true'
Expand All @@ -13,7 +12,7 @@ jest.mock('inquirer')

const TX_PATH_FOR_TEST = './test-tx.json'

testWithAnvilL2('governance:build-proposal cmd', (web3: Web3) => {
testWithAnvilL2('governance:build-proposal cmd', (provider) => {
describe('building proposal to transfer funds from governance', () => {
beforeEach(async () => {
const promptSpy = jest
Expand All @@ -37,7 +36,7 @@ testWithAnvilL2('governance:build-proposal cmd', (web3: Web3) => {
promptSpy.mockResolvedValueOnce({ 'Celo Contract': '✔ done' })
})
it('generates the json', async () => {
await testLocallyWithWeb3Node(BuildProposal, ['--output', TX_PATH_FOR_TEST], web3)
await testLocallyWithNode(BuildProposal, ['--output', TX_PATH_FOR_TEST], provider)
const result = await readJSON(TX_PATH_FOR_TEST)
expect(result).toMatchInlineSnapshot(`
[
Expand Down
65 changes: 40 additions & 25 deletions packages/cli/src/commands/governance/dequeue.test.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,15 @@
import { newKitFromWeb3 } from '@celo/contractkit'
import { newKitFromProvider } from '@celo/contractkit'
import { testWithAnvilL2 } from '@celo/dev-utils/anvil-test'
import { timeTravel } from '@celo/dev-utils/ganache-test'
import Web3 from 'web3'
import { testLocallyWithWeb3Node } from '../../test-utils/cliUtils'
import { testLocallyWithNode } from '../../test-utils/cliUtils'
import Dequeue from './dequeue'

process.env.NO_SYNCCHECK = 'true'

testWithAnvilL2('governance:dequeue cmd', (web3: Web3) => {
testWithAnvilL2('governance:dequeue cmd', (provider) => {
it('does not dequeue anything if no proposals are ready', async () => {
const kit = newKitFromWeb3(web3)
const [account] = await web3.eth.getAccounts()
const kit = newKitFromProvider(provider)
const [account] = await kit.connection.getAccounts()
const governanceWrapper = await kit.contracts.getGovernance()
const minDeposit = (await governanceWrapper.minDeposit()).toFixed()

Expand All @@ -21,12 +20,16 @@ testWithAnvilL2('governance:dequeue cmd', (web3: Web3) => {
expect(initialDequeue).toEqual([])

// Create first proposal
await governanceWrapper
.propose([], 'URL')
.sendAndWaitForReceipt({ from: account, value: minDeposit })
const proposeHash = await governanceWrapper.propose([], 'URL', {
from: account,
value: minDeposit,
})
await kit.connection.viemClient.waitForTransactionReceipt({
hash: proposeHash as `0x${string}`,
})

// Run dequeue operation
await testLocallyWithWeb3Node(Dequeue, ['--from', account], web3)
await testLocallyWithNode(Dequeue, ['--from', account], provider)

// After first dequeue, we should have either proposal dequeued or still in queue
const afterFirstDequeue = await governanceWrapper.getDequeue()
Expand All @@ -35,12 +38,16 @@ testWithAnvilL2('governance:dequeue cmd', (web3: Web3) => {
expect(totalProposals).toBe(1) // Should have exactly 1 proposal in system

// Create second proposal
await governanceWrapper
.propose([], 'URL2')
.sendAndWaitForReceipt({ from: account, value: minDeposit })
const proposeHash2 = await governanceWrapper.propose([], 'URL2', {
from: account,
value: minDeposit,
})
await kit.connection.viemClient.waitForTransactionReceipt({
hash: proposeHash2 as `0x${string}`,
})

// Run dequeue again
await testLocallyWithWeb3Node(Dequeue, ['--from', account], web3)
await testLocallyWithNode(Dequeue, ['--from', account], provider)

// After second dequeue, we should have 2 total proposals in the system
const finalDequeue = await governanceWrapper.getDequeue()
Expand All @@ -50,8 +57,8 @@ testWithAnvilL2('governance:dequeue cmd', (web3: Web3) => {
})

it('dequeues proposals after time has passed', async () => {
const kit = newKitFromWeb3(web3)
const [account] = await web3.eth.getAccounts()
const kit = newKitFromProvider(provider)
const [account] = await kit.connection.getAccounts()
const governanceWrapper = await kit.contracts.getGovernance()
const minDeposit = (await governanceWrapper.minDeposit()).toFixed()
const dequeueFrequency = (await governanceWrapper.dequeueFrequency()).toNumber()
Expand All @@ -61,28 +68,36 @@ testWithAnvilL2('governance:dequeue cmd', (web3: Web3) => {
expect(await governanceWrapper.getDequeue()).toEqual([])

// Create first proposal
await governanceWrapper
.propose([], 'URL')
.sendAndWaitForReceipt({ from: account, value: minDeposit })
const proposeHash = await governanceWrapper.propose([], 'URL', {
from: account,
value: minDeposit,
})
await kit.connection.viemClient.waitForTransactionReceipt({
hash: proposeHash as `0x${string}`,
})

// Run dequeue immediately (should not dequeue due to timing)
await testLocallyWithWeb3Node(Dequeue, ['--from', account], web3)
await testLocallyWithNode(Dequeue, ['--from', account], provider)

// Should have 1 proposal total in the system
const afterFirstDequeue = await governanceWrapper.getDequeue()
const afterFirstQueue = await governanceWrapper.getQueue()
expect(afterFirstDequeue.length + afterFirstQueue.length).toBe(1)

// Create second proposal
await governanceWrapper
.propose([], 'URL2')
.sendAndWaitForReceipt({ from: account, value: minDeposit })
const proposeHash2 = await governanceWrapper.propose([], 'URL2', {
from: account,
value: minDeposit,
})
await kit.connection.viemClient.waitForTransactionReceipt({
hash: proposeHash2 as `0x${string}`,
})

// Advance time to allow dequeuing
await timeTravel(dequeueFrequency + 1, web3)
await timeTravel(dequeueFrequency + 1, provider)

// Now dequeue should work
await testLocallyWithWeb3Node(Dequeue, ['--from', account], web3)
await testLocallyWithNode(Dequeue, ['--from', account], provider)

// Should have 2 proposals total, and some should be dequeued
const finalDequeue = await governanceWrapper.getDequeue()
Expand Down
5 changes: 3 additions & 2 deletions packages/cli/src/commands/governance/dequeue.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { BaseCommand } from '../../base'
import { displaySendTx } from '../../utils/cli'
import { displayViemTx } from '../../utils/cli'
import { CustomFlags } from '../../utils/command'

export default class Dequeue extends BaseCommand {
Expand All @@ -14,11 +14,12 @@ export default class Dequeue extends BaseCommand {

async run() {
const kit = await this.getKit()
const publicClient = await this.getPublicClient()
const res = await this.parse(Dequeue)
const account = res.flags.from
kit.defaultAccount = account
const governance = await kit.contracts.getGovernance()

await displaySendTx('dequeue', governance.dequeueProposalsIfReady(), {}, 'ProposalsDequeued')
await displayViemTx('dequeue', governance.dequeueProposalsIfReady(), publicClient)
}
}
Loading
Loading