refactor

packages/cli/src/commands/governance/approve-l2.test.ts

@@ -633,7 +633,6 @@ testWithAnvilL2('governance:approve cmd', (web3: Web3) => {
         }
       `)
 
-      // We want to test if integration works for accounts that are not added to the node
       await testLocallyWithWeb3Node(
         Approve,
         [
@@ -648,6 +647,30 @@ testWithAnvilL2('governance:approve cmd', (web3: Web3) => {
         web3
       )
 
+      // Run the same command twice with same arguments to make sure it doesn't have any effect
+      await testLocallyWithWeb3Node(
+        Approve,
+        [
+          '--from',
+          securityCouncilSafeSignatory1,
+          '--hotfix',
+          HOTFIX_HASH,
+          '--useSafe',
+          '--type',
+          'securityCouncil',
+        ],
+        web3
+      )
+
+      expect(await governance.getHotfixRecord(HOTFIX_BUFFER)).toMatchInlineSnapshot(`
+        {
+          "approved": false,
+          "councilApproved": false,
+          "executed": false,
+          "executionTimeLimit": "0",
+        }
+      `)
+
       // Make sure the account has enough balance to pay for the transaction
       await setBalance(web3, securityCouncilSafeSignatory2, BigInt(web3.utils.toWei('1', 'ether')))
       await testLocallyWithWeb3Node(
@@ -660,6 +683,7 @@ testWithAnvilL2('governance:approve cmd', (web3: Web3) => {
           '--useSafe',
           '--type',
           'securityCouncil',
+          // We want to test if integration works for accounts that are not added to the node
           '--privateKey',
           securityCouncilSafeSignatory2PrivateKey,
         ],
@@ -679,6 +703,24 @@ testWithAnvilL2('governance:approve cmd', (web3: Web3) => {
       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:",
           ],
@@ -707,13 +749,13 @@ testWithAnvilL2('governance:approve cmd', (web3: Web3) => {
             "   ✔  Hotfix 0xbf670baa773b342120e1af45433a465bbd6fa289a5cf72763d63d95e4e22482d is not already executed ",
           ],
           [
-            "SendTransaction: approveTx",
+            "All checks passed",
           ],
           [
             "txHash: 0xtxhash",
           ],
           [
-            "All checks passed",
+            "txHash: 0xtxhash",
           ],
         ]
       `)

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

@@ -1,15 +1,19 @@
 import { StrongAddress } from '@celo/base'
 import { CeloTransactionObject } from '@celo/connect'
-import { CeloProvider } from '@celo/connect/lib/celo-provider'
 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 Safe from '@safe-global/protocol-kit'
+import Web3 from 'web3'
 import { BaseCommand } from '../../base'
 import { newCheckBuilder } from '../../utils/checks'
-import { displaySendTx, failWith } from '../../utils/cli'
+import { displaySendSafeTx, displaySendTx, failWith } from '../../utils/cli'
 import { CustomFlags } from '../../utils/command'
+import {
+  createApproveSafeTransactionIfNotApproved,
+  createExecuteSafeTransactionIfThresholdMet,
+  createSafeFromWeb3,
+} from '../../utils/safe'
 
 enum HotfixApprovalType {
   APPROVER = 'approver',
@@ -33,9 +37,11 @@ export default class Approve extends BaseCommand {
     from: CustomFlags.address({ required: true, description: "Approver's address" }),
     useMultiSig: Flags.boolean({
       description: 'True means the request will be sent through multisig.',
+      exclusive: ['useSafe'],
     }),
     useSafe: Flags.boolean({
       description: 'True means the request will be sent through safe.',
+      exclusive: ['useMultiSig'],
     }),
     hotfix: Flags.string({
       exclusive: ['proposalID'],
@@ -79,6 +85,7 @@ export default class Approve extends BaseCommand {
     const approver = useMultiSig ? governanceApproverMultiSig!.address : account
 
     await addDefaultChecks(
+      await this.getWeb3(),
       checkBuilder,
       governance,
       isCel2,
@@ -122,59 +129,33 @@ export default class Approve extends BaseCommand {
       failWith('Proposal ID or hotfix must be provided')
     }
 
-    // TODO find out if we need to use api-kit so that Safe UI can be used
     if (isCel2 && approvalType === 'securityCouncil' && useSafe) {
       const web3 = await this.getWeb3()
+      const safe = await createSafeFromWeb3(web3, account, await governance.getSecurityCouncil())
 
-      if (!(web3.currentProvider instanceof CeloProvider)) {
-        throw new Error('Unexpected web3 provider')
-      }
-
-      const protocolKit = await Safe.init({
-        provider: web3.currentProvider.toEip1193Provider(),
-        signer: account,
-        safeAddress: await governance.getSecurityCouncil(),
-      })
-
-      const safeTransaction = await protocolKit.createTransaction({
-        transactions: [
-          {
-            to: governance.address,
-            data: governanceTx.txo.encodeABI(),
-            value: '0',
-          },
-        ],
-      })
+      const approveTx = await createApproveSafeTransactionIfNotApproved(
+        safe,
+        governanceTx,
+        governance.address,
+        account
+      )
 
-      const txHash = await protocolKit.getTransactionHash(safeTransaction)
-      if (!(await protocolKit.getOwnersWhoApprovedTx(txHash)).includes(account)) {
-        await protocolKit.approveTransactionHash(txHash)
+      if (approveTx !== null) {
+        // @ts-expect-error
+        await displaySendSafeTx('approveTx', approveTx)
       }
 
-      if (
-        (await protocolKit.getOwnersWhoApprovedTx(txHash)).length >=
-        (await protocolKit.getThreshold())
-      ) {
-        const executeTxResponse = await protocolKit.executeTransaction(safeTransaction)
-
-        if (!executeTxResponse.transactionResponse) {
-          throw new Error('Transaction failed')
-        }
+      const executeTx = await createExecuteSafeTransactionIfThresholdMet(
+        safe,
+        governanceTx,
+        governance.address
+      )
 
-        if (
-          'wait' in (executeTxResponse.transactionResponse as any) &&
-          typeof (executeTxResponse.transactionResponse as any).wait === 'function'
-        ) {
-          // @ts-expect-error
-          const receipt = await executeTxResponse.transactionResponse?.wait()
-        }
+      if (executeTx !== null) {
+        // @ts-expect-error
+        await displaySendSafeTx('executeTx', executeTx)
       }
-
-      // TODO: displaySendTx
-      return
-    }
-
-    if (
+    } else if (
       isCel2 &&
       approvalType === 'securityCouncil' &&
       useMultiSig &&
@@ -199,6 +180,7 @@ export default class Approve extends BaseCommand {
 }
 
 const addDefaultChecks = async (
+  web3: Web3,
   checkBuilder: ReturnType<typeof newCheckBuilder>,
   governance: GovernanceWrapper,
   isCel2: boolean,
@@ -240,12 +222,11 @@ const addDefaultChecks = async (
           })
       } else if (useSafe) {
         checkBuilder.addCheck(`${account} is security council safe signatory`, async () => {
-          const protocolKit = await Safe.init({
-            // @ts-expect-error
-            provider: governance.connection.web3.currentProvider.existingProvider.host,
-            // signer,
-            safeAddress: await governance.getSecurityCouncil(),
-          })
+          const protocolKit = await createSafeFromWeb3(
+            web3,
+            account,
+            await governance.getSecurityCouncil()
+          )
 
           return await protocolKit.isOwner(account)
         })

packages/cli/src/utils/cli.ts

@@ -7,6 +7,7 @@ import {
   parseDecodedParams,
 } from '@celo/connect'
 import { Errors, ux } from '@oclif/core'
+import Safe from '@safe-global/protocol-kit'
 import BigNumber from 'bignumber.js'
 import chalk from 'chalk'
 import { ethers } from 'ethers'
@@ -21,6 +22,37 @@ export async function displayWeb3Tx(name: string, txObj: any, tx?: Omit<CeloTx,
   console.log(result)
   ux.action.stop()
 }
+
+export async function displaySendSafeTx(
+  name: string,
+  safeTx: ReturnType<Safe['approveTransactionHash'] | Safe['executeTransaction']>
+) {
+  ux.action.start(`Sending Transaction: ${name}`)
+
+  try {
+    const txResult = await safeTx
+
+    if (!txResult.transactionResponse) {
+      throw new Error('Transaction failed')
+    }
+
+    if (
+      'wait' in (txResult.transactionResponse as any) &&
+      typeof (txResult.transactionResponse as any).wait === 'function'
+    ) {
+      // @ts-expect-error example taken from the docs and works fine (covered by tests)
+      const receipt = await txResult.transactionResponse?.wait()
+
+      printValueMap({ txHash: receipt.transactionHash })
+    }
+
+    ux.action.stop()
+  } catch (e) {
+    ux.action.stop(`failed: ${(e as Error).message}`)
+    throw e
+  }
+}
+
 // allows building a tx with ethers but signing and sending with celo Connection
 // cant use displaySendTx because it expects a CeloTransactionObject which isnt really possible to convert to from ethers
 export async function displaySendEthersTxViaCK(

packages/cli/src/utils/safe.ts

@@ -0,0 +1,69 @@
+import { StrongAddress } from '@celo/base'
+import { CeloTransactionObject } from '@celo/connect'
+import { CeloProvider } from '@celo/connect/lib/celo-provider'
+import Safe from '@safe-global/protocol-kit'
+
+export const createSafeFromWeb3 = async (
+  web3: any,
+  signer: StrongAddress,
+  safeAddress: StrongAddress
+) => {
+  if (!(web3.currentProvider instanceof CeloProvider)) {
+    throw new Error('Unexpected web3 provider')
+  }
+
+  return await Safe.init({
+    provider: web3.currentProvider.toEip1193Provider(),
+    signer,
+    safeAddress,
+  })
+}
+
+export const createApproveSafeTransactionIfNotApproved = async (
+  safe: Safe,
+  tx: CeloTransactionObject<any>,
+  toAddress: StrongAddress,
+  ownerAddress: StrongAddress
+): Promise<ReturnType<Safe['approveTransactionHash']> | null> => {
+  const safeTransaction = await safe.createTransaction({
+    transactions: [
+      {
+        to: toAddress,
+        data: tx.txo.encodeABI(),
+        value: '0',
+      },
+    ],
+  })
+
+  const txHash = await safe.getTransactionHash(safeTransaction)
+
+  if (!(await safe.getOwnersWhoApprovedTx(txHash)).includes(ownerAddress)) {
+    return safe.approveTransactionHash(txHash)
+  }
+
+  return null
+}
+
+export const createExecuteSafeTransactionIfThresholdMet = async (
+  safe: Safe,
+  tx: CeloTransactionObject<any>,
+  toAddress: StrongAddress
+): Promise<ReturnType<Safe['executeTransaction']> | null> => {
+  const safeTransaction = await safe.createTransaction({
+    transactions: [
+      {
+        to: toAddress,
+        data: tx.txo.encodeABI(),
+        value: '0',
+      },
+    ],
+  })
+
+  const txHash = await safe.getTransactionHash(safeTransaction)
+
+  if ((await safe.getOwnersWhoApprovedTx(txHash)).length >= (await safe.getThreshold())) {
+    return safe.executeTransaction(safeTransaction)
+  }
+
+  return null
+}