fix: abstract getNextNonce function

0xmetropolis · May 9, 2022 · ae8cd09 · ae8cd09
1 parent 721ed2d
commit ae8cd09
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 82 deletions.
diff --git a/src/lib/services/create-safe-transaction.ts b/src/lib/services/create-safe-transaction.ts
@@ -1,14 +1,26 @@
-// This module has helper functions for the transaction service
 import { ethers } from 'ethers';
 import {
   getSafeInfo,
-  getSafeTransactionsBySafe,
   getGasEstimation,
   getSafeTxHash,
   submitSafeTransactionToService,
+  getSafeTransactionsBySafe,
   approveSafeTransaction,
 } from './transaction-service';
 
+/**
+ * Gets the nonce of the next transaction for a safe
+ * @param safeAddress - safe address
+ * @returns Nonce of next transaction
+ */
+export async function getNextNonce(safeAddress: string): Promise<number> {
+  const txs = await getSafeTransactionsBySafe(safeAddress, { limit: 1 });
+  // If the length is 0, that means this is the first transaction, so nonce is 0.
+  // otherwise, txs[0] is the latest nonce, including queued transactions.
+  const nonce = txs.length === 0 ? 0 : txs[0].nonce + 1;
+  return nonce;
+}
+
 /**
  * Creates safe transaction
  * @param input.safe - Address of Gnosis safe
@@ -29,18 +41,12 @@ export async function createSafeTransaction(
   },
   signer: ethers.Signer,
 ) {
-  const [{ threshold }, safeTransaction, safeTxGas] = await Promise.all([
+  const [{ threshold }, nonce, safeTxGas] = await Promise.all([
     getSafeInfo(input.safe),
-    getSafeTransactionsBySafe(input.safe, { limit: 1 }),
+    getNextNonce(input.safe),
     getGasEstimation(input),
   ]);
 
-  // If safeTransaction is an empty array, then this is the first tx on the safe.
-  const isFirst = safeTransaction.length === 0;
-
-  // If it's the first one, set it to 0. Otherwise we need to increment to avoid collision.
-  const nonce = isFirst ? 0 : safeTransaction[0].nonce + 1;
-
   const data = {
     safe: input.safe,
     to: input.to,

diff --git a/test/token.test.ts b/test/token.test.ts
@@ -6,7 +6,7 @@ import * as sdk from '../src';
 import * as utils from '../src/lib/utils';
 import * as fetchers from '../src/fetchers';
 import * as txService from '../src/lib/services/transaction-service';
-import * as createSafeTx from '../src/lib/services/create-safe-transaction';
+import * as createSafe from '../src/lib/services/create-safe-transaction';
 import {
   artNautPod,
   orcaCorePod,
@@ -135,7 +135,7 @@ describe('member actions', () => {
     jest.spyOn(utils, 'getContract').mockReturnValueOnce({
       address: memberTokenAddress,
     });
-    const createSafeTx = jest.spyOn(createSafeTx, 'createSafeTransaction').mockReturnValueOnce({});
+    const createSafeTx = jest.spyOn(createSafe, 'createSafeTransaction').mockReturnValueOnce({});
     const mockSigner = {
       getAddress: jest.fn().mockResolvedValueOnce(userAddress2),
     };
@@ -211,7 +211,7 @@ describe('proposeMintMemberFromPod', () => {
       // This should be a member of admin pod.
       getAddress: jest.fn().mockResolvedValueOnce(orcanautPod.members[0]),
     };
-    const createSafeTx = jest.spyOn(createSafeTx, 'createSafeTransaction').mockReturnValueOnce({});
+    const createSafeTx = jest.spyOn(createSafe, 'createSafeTransaction').mockReturnValueOnce({});
 
     const adminPod = await sdk.getPod(orcanautAddress);
     // subPod is member of adminPod
@@ -256,7 +256,7 @@ describe('proposeMintMemberFromPod', () => {
       // This should be a member of admin pod.
       getAddress: jest.fn().mockResolvedValueOnce(orcanautPod.members[0]),
     };
-    const createSafeTx = jest.spyOn(createSafeTx, 'createSafeTransaction').mockReturnValueOnce({});
+    const createSafeTx = jest.spyOn(createSafe, 'createSafeTransaction').mockReturnValueOnce({});
 
     // subPod is member of adminPod
     const subPod = await sdk.getPod('art-naut.pod.xyz');
@@ -280,7 +280,7 @@ describe('proposeMintMemberFromPod', () => {
       // This should be a member of admin pod.
       getAddress: jest.fn().mockResolvedValueOnce(orcanautPod.members[0]),
     };
-    const createSafeTx = jest.spyOn(createSafeTx, 'createSafeTransaction').mockReturnValueOnce({});
+    const createSafeTx = jest.spyOn(createSafe, 'createSafeTransaction').mockReturnValueOnce({});
 
     const parentPod = await sdk.getPod(orcanautAddress);
     // subPod is member of adminPod
@@ -390,7 +390,7 @@ describe('proposeBurnMemberFromPod', () => {
       // This should be a member of admin pod.
       getAddress: jest.fn().mockResolvedValueOnce(orcanautPod.members[0]),
     };
-    const createSafeTx = jest.spyOn(createSafeTx, 'createSafeTransaction').mockReturnValueOnce({});
+    const createSafeTx = jest.spyOn(createSafe, 'createSafeTransaction').mockReturnValueOnce({});
 
     const adminPod = await sdk.getPod(orcanautAddress);
     // subPod is member of adminPod
@@ -432,7 +432,7 @@ describe('proposeBurnMemberFromPod', () => {
       // This should be a member of admin pod.
       getAddress: jest.fn().mockResolvedValueOnce(orcanautPod.members[0]),
     };
-    const createSafeTx = jest.spyOn(createSafeTx, 'createSafeTransaction').mockReturnValueOnce({});
+    const createSafeTx = jest.spyOn(createSafe, 'createSafeTransaction').mockReturnValueOnce({});
 
     // subPod is member of adminPod
     const subPod = await sdk.getPod('art-naut.pod.xyz');
@@ -456,7 +456,7 @@ describe('proposeBurnMemberFromPod', () => {
       // This should be a member of admin pod.
       getAddress: jest.fn().mockResolvedValueOnce(orcanautPod.members[0]),
     };
-    const createSafeTx = jest.spyOn(createSafeTx, 'createSafeTransaction').mockReturnValueOnce({});
+    const createSafeTx = jest.spyOn(createSafe, 'createSafeTransaction').mockReturnValueOnce({});
 
     const parentPod = await sdk.getPod(orcanautAddress);
     // subPod is member of adminPod
@@ -566,7 +566,7 @@ describe('proposeTransferMembershipFromSubPod', () => {
       // This should be a member of admin pod.
       getAddress: jest.fn().mockResolvedValueOnce(artNautPod.members[0]),
     };
-    const createSafeTx = jest.spyOn(createSafeTx, 'createSafeTransaction').mockReturnValueOnce({});
+    const createSafeTx = jest.spyOn(createSafe, 'createSafeTransaction').mockReturnValueOnce({});
 
     const adminPod = await sdk.getPod(orcanautAddress);
     // subPod is member of adminPod
@@ -677,7 +677,7 @@ describe('proposeTransferAdminFromAdminPod', () => {
       // This should be a member of admin pod.
       getAddress: jest.fn().mockResolvedValueOnce(artNautPod.members[0]),
     };
-    const createSafeTx = jest.spyOn(createSafeTx, 'createSafeTransaction').mockReturnValueOnce({});
+    const createSafeTx = jest.spyOn(createSafe, 'createSafeTransaction').mockReturnValueOnce({});
 
     const adminPod = await sdk.getPod(orcanautAddress);
     // subPod is member of adminPod
@@ -764,7 +764,7 @@ describe('proposeAddAdmin', () => {
     jest.spyOn(utils, 'getContract').mockReturnValueOnce({
       address: memberTokenAddress,
     });
-    const createSafeTx = jest.spyOn(createSafeTx, 'createSafeTransaction').mockReturnValueOnce({});
+    const createSafeTx = jest.spyOn(createSafe, 'createSafeTransaction').mockReturnValueOnce({});
     const mockSigner = {
       getAddress: jest.fn().mockResolvedValueOnce(userAddress2),
     };
@@ -787,7 +787,7 @@ describe('proposeAddAdmin', () => {
     jest.spyOn(utils, 'getContract').mockReturnValueOnce({
       address: memberTokenAddress,
     });
-    const createSafeTx = jest.spyOn(createSafeTx, 'createSafeTransaction').mockReturnValueOnce({});
+    const createSafeTx = jest.spyOn(createSafe, 'createSafeTransaction').mockReturnValueOnce({});
     const mockSigner = {
       getAddress: jest.fn().mockResolvedValueOnce(userAddress2),
     };
@@ -834,7 +834,7 @@ describe('Pod migration', () => {
       address: '0x242e1E6cF6C30d36988D8019d0fE2e187325CCEd',
     });
     jest.spyOn(utils, 'getPreviousModule').mockResolvedValueOnce('0x242e1E6cF6C30d36988D8019d0fE2e187325CCEd');
-    const createSafeTx = jest.spyOn(createSafeTx, 'createSafeTransaction').mockReturnValueOnce({});
+    const createSafeTx = jest.spyOn(createSafe, 'createSafeTransaction').mockReturnValueOnce({});
 
     const pod = await sdk.getPod(orcanautAddress);
 

diff --git a/test/transaction-service.test.ts b/test/transaction-service.test.ts
@@ -2,7 +2,7 @@ import { erc20TransferTransaction } from './fixtures';
 import * as etherscan from '../src/lib/services/etherscan';
 import * as txService from '../src/lib/services/transaction-service';
 import { populateDataDecoded } from '../src/lib/services/transaction-service';
-import { createSafeTransaction } from '../src/lib/services/create-safe-transaction';
+import { getNextNonce } from '../src/lib/services/create-safe-transaction';
 import { userAddress } from '../test/fixtures';
 
 test('populateDataDecoded should be able to decode an erc20 transfer function', async () => {
@@ -34,42 +34,16 @@ test('populateDataDecoded should be able to decode an erc20 transfer function',
   expect(dataDecodedPopulated).toMatchObject(dataDecoded);
 });
 
-describe('createSafeTransaction', () => {
-  test('createSafeTransaction sets nonce to 0 for a safes first tx', async () => {
-    jest.spyOn(txService, 'getSafeInfo').mockImplementation(() => [{ threshold: 1 }]);
+describe('getNextNonce', () => {
+  test('getNextNonce sets nonce to 0 for a safes first tx', async () => {
     jest.spyOn(txService, 'getSafeTransactionsBySafe').mockResolvedValueOnce([]);
-    jest.spyOn(txService, 'getGasEstimation').mockResolvedValueOnce(5);
-    jest.spyOn(txService, 'submitSafeTransactionToService').mockImplementation();
-    jest.spyOn(txService, 'approveSafeTransaction').mockImplementation();
-    const getHash = jest.spyOn(txService, 'getSafeTxHash').mockResolvedValueOnce('0x1234');
 
-    const mockSigner = jest.fn();
+    const nonce = await getNextNonce(userAddress);
 
-    await createSafeTransaction({
-      safe: 'safe',
-      to: 'to',
-      value: '0',
-      data: '0x',
-      sender: userAddress,
-    }, mockSigner);
-
-    expect(getHash).toHaveBeenCalledWith({
-      "baseGas": 0,
-      "confirmationsRequired": undefined,
-      "data": "0x",
-      "gasPrice": "0",
-      "nonce": 0,
-      "operation": 0,
-      "safe": "safe",
-      "safeTxGas": 5,
-      "sender": "0x4B4C43F66ec007D1dBE28f03dAC975AAB5fbb888",
-      "to": "to",
-      "value": "0",
-    });
+    expect(nonce).toBe(0);
   });
 
-  test('createSafeTransaction increments nonce if transactions already exist', async () => {
-    jest.spyOn(txService, 'getSafeInfo').mockImplementation(() => [{ threshold: 1 }]);
+  test('getNextNonce increments nonce if transactions already exist', async () => {
     jest.spyOn(txService, 'getSafeTransactionsBySafe').mockResolvedValueOnce([
         {
           "safe": "0xdab0d648a2a771e6952916A822dddf738b535f5A",
@@ -135,34 +109,9 @@ describe('createSafeTransaction', () => {
         }
       ]
     );
-    jest.spyOn(txService, 'getGasEstimation').mockResolvedValueOnce(5);
-    jest.spyOn(txService, 'submitSafeTransactionToService').mockImplementation();
-    jest.spyOn(txService, 'approveSafeTransaction').mockImplementation();
-    const getHash = jest.spyOn(txService, 'getSafeTxHash').mockResolvedValueOnce('0x1234');
-
-
-    const mockSigner = jest.fn();
 
-    await createSafeTransaction({
-      safe: 'safe',
-      to: 'to',
-      value: '0',
-      data: '0x',
-      sender: userAddress,
-    }, mockSigner);
+    const nonce = await getNextNonce(userAddress);
 
-    expect(getHash).toHaveBeenCalledWith({
-      "baseGas": 0,
-      "confirmationsRequired": undefined,
-      "data": "0x",
-      "gasPrice": "0",
-      "nonce": 3,
-      "operation": 0,
-      "safe": "safe",
-      "safeTxGas": 5,
-      "sender": "0x4B4C43F66ec007D1dBE28f03dAC975AAB5fbb888",
-      "to": "to",
-      "value": "0",
-    });
+    expect(nonce).toBe(3);
   });
 });