Refactor addSafeOperation function params to remove internal coupli…

…ng with Safe4337 module The function expects a `UserOperation` object now, instead of a `SafeOperation` object.
safe-global · May 14, 2024 · 56315ef · 56315ef
1 parent 46fd0c4
commit 56315ef
Show file tree

Hide file tree

Showing 4 changed files with 108 additions and 102 deletions.
diff --git a/packages/api-kit/src/SafeApiKit.ts b/packages/api-kit/src/SafeApiKit.ts
@@ -785,13 +785,14 @@ class SafeApiKit {
    * @throws "Invalid Safe address {safeAddress}"
    * @throws "Module address must not be empty"
    * @throws "Invalid module address {moduleAddress}"
-   * @throws "SafeOperation is not signed by the given signer {signerAddress}"
+   * @throws "Signature must not be empty"
    */
   async addSafeOperation({
+    entryPoint,
     moduleAddress: moduleAddressProp,
+    options,
     safeAddress: safeAddressProp,
-    safeOperation,
-    signer
+    userOperation
   }: AddSafeOperationProps): Promise<void> {
     let safeAddress: string, moduleAddress: string
 
@@ -814,32 +815,29 @@ class SafeApiKit {
       throw new Error(`Invalid module address ${moduleAddressProp}`)
     }
 
-    const signerAddress = await signer.getAddress()
-    const signature = safeOperation.getSignature(signerAddress)
-
-    if (!signature) {
-      throw new Error(`SafeOperation is not signed by the given signer ${signerAddress}`)
+    if (isEmptyData(userOperation.signature)) {
+      throw new Error('Signature must not be empty')
     }
 
-    const { data } = safeOperation
-
     return sendRequest({
       url: `${this.#txServiceBaseUrl}/v1/safes/${safeAddress}/safe-operations/`,
       method: HttpMethod.Post,
       body: {
-        nonce: Number(data.nonce),
-        initCode: isEmptyData(data.initCode) ? null : data.initCode,
-        callData: data.callData,
-        callDataGasLimit: data.callGasLimit.toString(),
-        verificationGasLimit: data.verificationGasLimit.toString(),
-        preVerificationGas: data.preVerificationGas.toString(),
-        maxFeePerGas: data.maxFeePerGas.toString(),
-        maxPriorityFeePerGas: data.maxPriorityFeePerGas.toString(),
-        paymasterAndData: isEmptyData(data.paymasterAndData) ? null : data.paymasterAndData,
-        entryPoint: data.entryPoint,
-        validAfter: !data.validAfter ? null : data.validAfter,
-        validUntil: !data.validUntil ? null : data.validUntil,
-        signature: signature.data,
+        nonce: Number(userOperation.nonce),
+        initCode: isEmptyData(userOperation.initCode) ? null : userOperation.initCode,
+        callData: userOperation.callData,
+        callDataGasLimit: userOperation.callGasLimit.toString(),
+        verificationGasLimit: userOperation.verificationGasLimit.toString(),
+        preVerificationGas: userOperation.preVerificationGas.toString(),
+        maxFeePerGas: userOperation.maxFeePerGas.toString(),
+        maxPriorityFeePerGas: userOperation.maxPriorityFeePerGas.toString(),
+        paymasterAndData: isEmptyData(userOperation.paymasterAndData)
+          ? null
+          : userOperation.paymasterAndData,
+        entryPoint,
+        validAfter: !options?.validAfter ? null : options?.validAfter,
+        validUntil: !options?.validUntil ? null : options?.validUntil,
+        signature: userOperation.signature,
         moduleAddress
       }
     })

diff --git a/packages/api-kit/src/types/safeTransactionServiceTypes.ts b/packages/api-kit/src/types/safeTransactionServiceTypes.ts
@@ -1,8 +1,8 @@
 import { Signer, TypedDataDomain, TypedDataField } from 'ethers'
 import {
   SafeMultisigTransactionResponse,
-  SafeOperation,
-  SafeTransactionData
+  SafeTransactionData,
+  UserOperation
 } from '@safe-global/safe-core-sdk-types'
 
 export type SafeServiceInfoResponse = {
@@ -342,12 +342,19 @@ export type GetSafeOperationListResponse = {
 }
 
 export type AddSafeOperationProps = {
+  /** Address of the EntryPoint contract */
+  entryPoint: string
   /** Address of the Safe4337Module contract */
   moduleAddress: string
   /** Address of the Safe to add a SafeOperation for */
   safeAddress: string
-  /** Signed SafeOperation object to add */
-  safeOperation: SafeOperation
-  /** Signer instance */
-  signer: Signer
+  /** UserOperation object to add */
+  userOperation: UserOperation
+  /** Options object */
+  options?: {
+    /** The UserOperation will remain valid until this block's timestamp */
+    validUntil?: number
+    /** The UserOperation will be valid after this block's timestamp */
+    validAfter?: number
+  }
 }
diff --git a/packages/api-kit/tests/e2e/addSafeOperation.test.ts b/packages/api-kit/tests/e2e/addSafeOperation.test.ts
@@ -3,7 +3,7 @@ import chaiAsPromised from 'chai-as-promised'
 import { Wallet, ethers } from 'ethers'
 import sinon from 'sinon'
 import sinonChai from 'sinon-chai'
-import { EthAdapter } from '@safe-global/safe-core-sdk-types'
+import { EthAdapter, SafeOperation } from '@safe-global/safe-core-sdk-types'
 import SafeApiKit from '@safe-global/api-kit'
 import { Safe4337Pack } from '@safe-global/relay-kit'
 import { generateTransferCallData } from '@safe-global/relay-kit/src/packs/safe-4337/testing-utils/helpers'
@@ -49,6 +49,13 @@ describe('addSafeOperation', () => {
     .returns(
       Promise.resolve({ fast: { maxFeePerGas: '0x3b9aca00', maxPriorityFeePerGas: '0x3b9aca00' } })
     )
+  providerStub.withArgs(RPC_4337_CALLS.ESTIMATE_USER_OPERATION_GAS, sinon.match.any).returns(
+    Promise.resolve({
+      preVerificationGas: BigInt(Date.now()),
+      callGasLimit: BigInt(Date.now()),
+      verificationGasLimit: BigInt(Date.now())
+    })
+  )
 
   providerStub.callThrough()
 
@@ -80,18 +87,32 @@ describe('addSafeOperation', () => {
     })?.networkAddresses[chainId] as string
   })
 
+  const getAddSafeOperationProps = async (safeOperation: SafeOperation) => {
+    const userOperation = safeOperation.toUserOperation()
+    userOperation.signature = safeOperation.encodedSignatures()
+    return {
+      entryPoint: safeOperation.data.entryPoint,
+      moduleAddress,
+      safeAddress: SAFE_ADDRESS,
+      userOperation,
+      options: {
+        validAfter: safeOperation.data.validAfter,
+        validUntil: safeOperation.data.validUntil
+      }
+    }
+  }
+
   describe('should fail', () => {
     it('if safeAddress is empty', async () => {
       const safeOperation = await safe4337Pack.createTransaction({ transactions: [transferUSDC] })
       const signedSafeOperation = await safe4337Pack.signSafeOperation(safeOperation)
+      const addSafeOperationProps = await getAddSafeOperationProps(signedSafeOperation)
 
       await chai
         .expect(
           safeApiKit.addSafeOperation({
-            moduleAddress,
-            safeAddress: '',
-            safeOperation: signedSafeOperation,
-            signer
+            ...addSafeOperationProps,
+            safeAddress: ''
           })
         )
         .to.be.rejectedWith('Safe address must not be empty')
@@ -100,14 +121,13 @@ describe('addSafeOperation', () => {
     it('if safeAddress is invalid', async () => {
       const safeOperation = await safe4337Pack.createTransaction({ transactions: [transferUSDC] })
       const signedSafeOperation = await safe4337Pack.signSafeOperation(safeOperation)
+      const addSafeOperationProps = await getAddSafeOperationProps(signedSafeOperation)
 
       await chai
         .expect(
           safeApiKit.addSafeOperation({
-            moduleAddress,
-            safeAddress: '0x123',
-            safeOperation: signedSafeOperation,
-            signer
+            ...addSafeOperationProps,
+            safeAddress: '0x123'
           })
         )
         .to.be.rejectedWith('Invalid Safe address 0x123')
@@ -116,14 +136,13 @@ describe('addSafeOperation', () => {
     it('if moduleAddress is empty', async () => {
       const safeOperation = await safe4337Pack.createTransaction({ transactions: [transferUSDC] })
       const signedSafeOperation = await safe4337Pack.signSafeOperation(safeOperation)
+      const addSafeOperationProps = await getAddSafeOperationProps(signedSafeOperation)
 
       await chai
         .expect(
           safeApiKit.addSafeOperation({
-            moduleAddress: '',
-            safeAddress: SAFE_ADDRESS,
-            safeOperation: signedSafeOperation,
-            signer
+            ...addSafeOperationProps,
+            moduleAddress: ''
           })
         )
         .to.be.rejectedWith('Module address must not be empty')
@@ -132,62 +151,40 @@ describe('addSafeOperation', () => {
     it('if moduleAddress is invalid', async () => {
       const safeOperation = await safe4337Pack.createTransaction({ transactions: [transferUSDC] })
       const signedSafeOperation = await safe4337Pack.signSafeOperation(safeOperation)
+      const addSafeOperationProps = await getAddSafeOperationProps(signedSafeOperation)
 
       await chai
         .expect(
           safeApiKit.addSafeOperation({
-            moduleAddress: '0x234',
-            safeAddress: SAFE_ADDRESS,
-            safeOperation: signedSafeOperation,
-            signer
+            ...addSafeOperationProps,
+            moduleAddress: '0x234'
           })
         )
         .to.be.rejectedWith('Invalid module address 0x234')
     })
 
     it('if the SafeOperation is not signed', async () => {
       const safeOperation = await safe4337Pack.createTransaction({ transactions: [transferUSDC] })
+      const addSafeOperationProps = await getAddSafeOperationProps(safeOperation)
 
       await chai
-        .expect(
-          safeApiKit.addSafeOperation({
-            moduleAddress,
-            safeAddress: SAFE_ADDRESS,
-            safeOperation,
-            signer
-          })
-        )
-        .to.be.rejectedWith(
-          'SafeOperation is not signed by the given signer 0x56e2C102c664De6DfD7315d12c0178b61D16F171'
-        )
+        .expect(safeApiKit.addSafeOperation(addSafeOperationProps))
+        .to.be.rejectedWith('Signature must not be empty')
     })
   })
 
   it('should add a new SafeOperation', async () => {
-    providerStub.withArgs(RPC_4337_CALLS.ESTIMATE_USER_OPERATION_GAS, sinon.match.any).returns(
-      Promise.resolve({
-        preVerificationGas: BigInt(Date.now()),
-        callGasLimit: BigInt(Date.now()),
-        verificationGasLimit: BigInt(Date.now())
-      })
-    )
-
     const safeOperation = await safe4337Pack.createTransaction({ transactions: [transferUSDC] })
     const signedSafeOperation = await safe4337Pack.signSafeOperation(safeOperation)
+    const addSafeOperationProps = await getAddSafeOperationProps(signedSafeOperation)
 
+    // Get the number of SafeOperations before adding a new one
     const safeOperationsBefore = await safeApiKit.getSafeOperationsByAddress({
       safeAddress: SAFE_ADDRESS
     })
     const initialNumSafeOperations = safeOperationsBefore.results.length
 
-    await chai.expect(
-      safeApiKit.addSafeOperation({
-        moduleAddress,
-        safeAddress: SAFE_ADDRESS,
-        safeOperation: signedSafeOperation,
-        signer
-      })
-    ).to.be.fulfilled
+    await chai.expect(safeApiKit.addSafeOperation(addSafeOperationProps)).to.be.fulfilled
 
     const safeOperationsAfter = await safeApiKit.getSafeOperationsByAddress({
       safeAddress: SAFE_ADDRESS

diff --git a/packages/api-kit/tests/endpoint/index.test.ts b/packages/api-kit/tests/endpoint/index.test.ts
@@ -7,7 +7,7 @@ import SafeApiKit, {
 } from '@safe-global/api-kit/index'
 import * as httpRequests from '@safe-global/api-kit/utils/httpRequests'
 import Safe from '@safe-global/protocol-kit'
-import { EthAdapter, SafeOperation } from '@safe-global/safe-core-sdk-types'
+import { EthAdapter, UserOperation } from '@safe-global/safe-core-sdk-types'
 import chai from 'chai'
 import chaiAsPromised from 'chai-as-promised'
 import sinon from 'sinon'
@@ -669,50 +669,54 @@ describe('Endpoint tests', () => {
     })
 
     it('addSafeOperation', async () => {
-      const signature = '0xsignature'
       const moduleAddress = '0xa581c4A4DB7175302464fF3C06380BC3270b4037'
 
-      const safeOperation = {
-        data: {
-          nonce: 42,
-          initCode: '0xfbc38024f74946d9ec31e0c8658dd65e335c6e57c14575250787ec5fb270c08a',
-          callData:
-            '0x7bb374280000000000000000000000001c7d4b196cb0c7b01d743fbc6116a902379c72380000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000008000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000044a9059cbb00000000000000000000000060c4ab82d06fd7dfe9517e17736c2dcc77443ef000000000000000000000000000000000000000000000000000000000000186a000000000000000000000000000000000000000000000000000000000',
-          callGasLimit: '150799',
-          verificationGasLimit: '200691',
-          preVerificationGas: '50943',
-          maxFeePerGas: '1949282597',
-          maxPriorityFeePerGas: '1380000000',
-          paymasterAndData: '0xdff7fa1077bce740a6a212b3995990682c0ba66d',
-          entryPoint: '0x5FF137D4b0FDCD49DcA30c7CF57E578a026d2789',
-          validAfter: '2024-01-01T00:00:00Z',
-          validUntil: '2024-04-12T00:00:00Z'
-        },
-        getSignature: () => ({ data: signature })
-      } as unknown as SafeOperation
+      const userOperation: UserOperation = {
+        sender: safeAddress,
+        nonce: '42',
+        initCode: '0xfbc38024f74946d9ec31e0c8658dd65e335c6e57c14575250787ec5fb270c08a',
+        callData:
+          '0x7bb374280000000000000000000000001c7d4b196cb0c7b01d743fbc6116a902379c72380000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000008000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000044a9059cbb00000000000000000000000060c4ab82d06fd7dfe9517e17736c2dcc77443ef000000000000000000000000000000000000000000000000000000000000186a000000000000000000000000000000000000000000000000000000000',
+        callGasLimit: 150799n,
+        verificationGasLimit: 200691n,
+        preVerificationGas: 50943n,
+        maxFeePerGas: 1949282597n,
+        maxPriorityFeePerGas: 1380000000n,
+        paymasterAndData: '0xdff7fa1077bce740a6a212b3995990682c0ba66d',
+        signature: '0xsignature'
+      }
+
+      const entryPoint = '0x5FF137D4b0FDCD49DcA30c7CF57E578a026d2789'
+      const options = { validAfter: 123, validUntil: 234 }
 
       await chai
         .expect(
           safeApiKit.addSafeOperation({
+            entryPoint,
             moduleAddress,
+            options,
             safeAddress,
-            safeOperation,
-            signer
+            userOperation
           })
         )
         .to.be.eventually.deep.equals({ data: { success: true } })
 
-      // We need to rename the "callGasLimit" prop here because the api endpoint expects it as "callDataGasLimit"
-      const { callGasLimit: callDataGasLimit, ...safeOperationWithoutCallGasLimit } =
-        safeOperation.data
-
       chai.expect(fetchData).to.have.been.calledWith({
         url: `${txServiceBaseUrl}/v1/safes/${safeAddress}/safe-operations/`,
         method: 'post',
         body: {
-          ...safeOperationWithoutCallGasLimit,
-          callDataGasLimit,
-          signature,
+          nonce: Number(userOperation.nonce),
+          initCode: userOperation.initCode,
+          callData: userOperation.callData,
+          callDataGasLimit: userOperation.callGasLimit.toString(),
+          verificationGasLimit: userOperation.verificationGasLimit.toString(),
+          preVerificationGas: userOperation.preVerificationGas.toString(),
+          maxFeePerGas: userOperation.maxFeePerGas.toString(),
+          maxPriorityFeePerGas: userOperation.maxPriorityFeePerGas.toString(),
+          paymasterAndData: userOperation.paymasterAndData,
+          entryPoint,
+          ...options,
+          signature: userOperation.signature,
           moduleAddress
         }
       })