Merge pull request #1364 from trilitech/fix-operation-executions

Fix operations execution for unrevealed accounts

src/components/SendFlow/MultisigAccount/SignTransactionFormPage.tsx

@@ -98,7 +98,7 @@ export const SignTransactionFormPage: React.FC<SignPageProps<FormValues>> = prop
 
             <AdvancedSettingsAccordion />
 
-            <FormLabel>Approvers</FormLabel>
+            <FormLabel marginTop="24px">Approvers</FormLabel>
             <Flex flexDirection="column" gap="12px" marginBottom="12px" data-testid="approvers">
               {signers.map(signer => (
                 <AddressTile

src/mocks/helpers.ts

@@ -14,5 +14,5 @@ export const addAccount = (account: Account | Multisig) => {
   store.dispatch(accountsActions.addAccount(account));
 };
 
-export const fakeAddressExists = (revealedKeyPairs: { pkh: RawPkh }[]) => (pkh: RawPkh) =>
+export const fakeIsAccountRevealed = (revealedKeyPairs: { pkh: RawPkh }[]) => (pkh: RawPkh) =>
   Promise.resolve(revealedKeyPairs.map(keyPair => keyPair.pkh).includes(pkh));

src/types/AccountOperations.ts

@@ -21,6 +21,9 @@ export type ImplicitOperations = {
 export type AccountOperations = ProposalOperations | ImplicitOperations;
 export type EstimatedAccountOperations = AccountOperations & {
   estimates: Estimation[];
+  // for some operations taquito automatically adds a reveal operation
+  // and returns it as the first estimate
+  revealEstimate?: Estimation;
 };
 
 export const totalFee = (estimates: Estimation[]): number =>

src/utils/hooks/setAccountDataHooks.test.ts

@@ -12,7 +12,7 @@ import {
   mockSecretKeyAccount,
   mockSocialAccount,
 } from "../../mocks/factories";
-import { addAccount, fakeAddressExists } from "../../mocks/helpers";
+import { addAccount, fakeIsAccountRevealed } from "../../mocks/helpers";
 import { mnemonic1 } from "../../mocks/mockMnemonic";
 import { act, renderHook } from "../../mocks/testUtils";
 import { ImplicitAccount, MnemonicAccount } from "../../types/Account";
@@ -36,7 +36,7 @@ describe("setAccountDataHooks", () => {
   });
 
   describe("mnemonic accounts", () => {
-    const addressExistsMock = jest.spyOn(tezosHelpers, "addressExists");
+    const isAccountRevealedMock = jest.spyOn(tezosHelpers, "isAccountRevealed");
     const getFingerPrintMock = jest.spyOn(tezosHelpers, "getFingerPrint");
     const encryptMock = jest.spyOn(functionsToMock, "encrypt");
     const decryptMock = jest.spyOn(functionsToMock, "decrypt");
@@ -108,8 +108,8 @@ describe("setAccountDataHooks", () => {
           await mnemonicAccount(4, `${LABEL_BASE} 5`),
           await mnemonicAccount(5, `${LABEL_BASE} 6`),
         ];
-        addressExistsMock.mockImplementation(
-          fakeAddressExists(revealedAccounts.map(account => account.address))
+        isAccountRevealedMock.mockImplementation(
+          fakeIsAccountRevealed(revealedAccounts.map(account => account.address))
         );
 
         const {
@@ -148,8 +148,8 @@ describe("setAccountDataHooks", () => {
           await mnemonicAccount(2, `${LABEL_BASE} 5`),
         ];
         // Reveal mnemonic accounts
-        addressExistsMock.mockImplementation(
-          fakeAddressExists(expected.map(account => account.address))
+        isAccountRevealedMock.mockImplementation(
+          fakeIsAccountRevealed(expected.map(account => account.address))
         );
 
         const {

src/utils/mnemonic.test.ts

@@ -8,13 +8,13 @@ import { networksActions } from "./redux/slices/networks";
 import { store } from "./redux/store";
 import * as tezosHelpers from "./tezos/helpers";
 import { mockContractContact, mockImplicitContact, mockSocialAccount } from "../mocks/factories";
-import { addAccount, fakeAddressExists } from "../mocks/helpers";
+import { addAccount, fakeIsAccountRevealed } from "../mocks/helpers";
 import { mnemonic1 } from "../mocks/mockMnemonic";
 import { renderHook } from "../mocks/testUtils";
 import { ImplicitAccount } from "../types/Account";
 import { MAINNET } from "../types/Network";
 
-const addressExistsMock = jest.spyOn(tezosHelpers, "addressExists");
+const isAccountRevealedMock = jest.spyOn(tezosHelpers, "isAccountRevealed");
 
 const testPublicKeys = [
   {
@@ -37,7 +37,7 @@ beforeEach(() => {
 
 describe("restoreRevealedPublicKeyPairs", () => {
   it("restores existing accounts", async () => {
-    addressExistsMock.mockImplementation(fakeAddressExists(testPublicKeys));
+    isAccountRevealedMock.mockImplementation(fakeIsAccountRevealed(testPublicKeys));
 
     const result = await restoreRevealedPublicKeyPairs(
       mnemonic1,
@@ -49,7 +49,7 @@ describe("restoreRevealedPublicKeyPairs", () => {
   });
 
   it("restores first account if none exists", async () => {
-    addressExistsMock.mockImplementation(fakeAddressExists([]));
+    isAccountRevealedMock.mockImplementation(fakeIsAccountRevealed([]));
 
     const result = await restoreRevealedPublicKeyPairs(
       mnemonic1,
@@ -61,7 +61,9 @@ describe("restoreRevealedPublicKeyPairs", () => {
   });
 
   it("stops at first unrevealed account", async () => {
-    addressExistsMock.mockImplementation(fakeAddressExists([testPublicKeys[0], testPublicKeys[2]]));
+    isAccountRevealedMock.mockImplementation(
+      fakeIsAccountRevealed([testPublicKeys[0], testPublicKeys[2]])
+    );
 
     const result = await restoreRevealedPublicKeyPairs(
       mnemonic1,
@@ -109,8 +111,8 @@ describe("useRestoreRevealedMnemonicAccounts", () => {
         derivationPathTemplate: "44'/1729'/?'/0'",
       },
     ];
-    addressExistsMock.mockImplementation(
-      fakeAddressExists(expected.map(account => account.address))
+    isAccountRevealedMock.mockImplementation(
+      fakeIsAccountRevealed(expected.map(account => account.address))
     );
 
     const {
@@ -127,7 +129,7 @@ describe("useRestoreRevealedMnemonicAccounts", () => {
   });
 
   it("restores one account if none were revealed", async () => {
-    addressExistsMock.mockImplementation(fakeAddressExists([]));
+    isAccountRevealedMock.mockImplementation(fakeIsAccountRevealed([]));
 
     const {
       result: { current: restoreRevealedMnemonicsHook },
@@ -148,7 +150,7 @@ describe("useRestoreRevealedMnemonicAccounts", () => {
   });
 
   it("sets unique labels for restored accounts", async () => {
-    addressExistsMock.mockImplementation(fakeAddressExists(testPublicKeys.slice(0, 3)));
+    isAccountRevealedMock.mockImplementation(fakeIsAccountRevealed(testPublicKeys.slice(0, 3)));
     store.dispatch(networksActions.setCurrent(MAINNET));
     store.dispatch(contactsActions.upsert(mockImplicitContact(1, CUSTOM_LABEL)));
     store.dispatch(contactsActions.upsert(mockContractContact(0, "ghostnet", `${CUSTOM_LABEL} 4`)));
@@ -180,7 +182,7 @@ describe("useRestoreRevealedMnemonicAccounts", () => {
   });
 
   it("restores existing accounts with a custom derivation path", async () => {
-    addressExistsMock.mockImplementation(fakeAddressExists(testPublicKeys.slice(0, 2)));
+    isAccountRevealedMock.mockImplementation(fakeIsAccountRevealed(testPublicKeys.slice(0, 2)));
 
     const {
       result: { current: restoreRevealedMnemonicsHook },

src/utils/mnemonic.ts

@@ -3,7 +3,7 @@ import { generateMnemonic } from "bip39";
 import { makeDerivationPath } from "./account/derivationPathUtils";
 import { makeMnemonicAccount } from "./account/makeMnemonicAccount";
 import { useGetNextAvailableAccountLabels } from "./hooks/labelsHooks";
-import { PublicKeyPair, addressExists, derivePublicKeyPair, getFingerPrint } from "./tezos";
+import { PublicKeyPair, derivePublicKeyPair, getFingerPrint, isAccountRevealed } from "./tezos";
 import { MnemonicAccount } from "../types/Account";
 import { Network } from "../types/Network";
 
@@ -44,7 +44,7 @@ export const restoreRevealedPublicKeyPairs = async (
       mnemonic,
       makeDerivationPath(derivationPathTemplate, accountIndex)
     );
-  } while (await addressExists(pubKeyPair.pkh, network));
+  } while (await isAccountRevealed(pubKeyPair.pkh, network));
   return result;
 };
 

src/utils/tezos/estimate.test.ts

@@ -1,13 +1,13 @@
 import { estimate, handleTezError } from "./estimate";
-import { addressExists, makeToolkit } from "./helpers";
+import { isAccountRevealed, makeToolkit } from "./helpers";
 import { executeParams } from "../../mocks/executeParams";
 import { mockImplicitAccount, mockTezOperation } from "../../mocks/factories";
 import { makeAccountOperations } from "../../types/AccountOperations";
 import { GHOSTNET } from "../../types/Network";
 jest.mock("./helpers", () => ({
   ...jest.requireActual("./helpers"),
   makeToolkit: jest.fn(),
-  addressExists: jest.fn(),
+  isAccountRevealed: jest.fn(),
 }));
 
 const accountOperations = makeAccountOperations(mockImplicitAccount(1), mockImplicitAccount(1), [
@@ -17,7 +17,7 @@ const accountOperations = makeAccountOperations(mockImplicitAccount(1), mockImpl
 describe("estimate", () => {
   describe("Error handling", () => {
     it("Catches unrevealed account", async () => {
-      jest.mocked(addressExists).mockResolvedValue(false);
+      jest.mocked(isAccountRevealed).mockResolvedValue(false);
 
       jest.mocked(makeToolkit).mockImplementation(
         () =>
@@ -51,7 +51,7 @@ describe("estimate", () => {
     });
   });
 
-  it("returns fee, storageLimit and gasLimit estimate ", async () => {
+  it("returns estimated operations", async () => {
     const estimateResult = [
       {
         burnFeeMutez: 0,
@@ -84,4 +84,50 @@ describe("estimate", () => {
 
     expect(result).toEqual(processedEstimateResult);
   });
+
+  it("can deal with the reveal estimation", async () => {
+    const estimateResult = [
+      {
+        burnFeeMutez: 0,
+        consumedMilligas: 1,
+        gasLimit: 2,
+        minimalFeeMutez: 3,
+        operationFeeMutez: 4,
+        storageLimit: 5,
+        suggestedFeeMutez: 6,
+        totalCost: 7,
+        usingBaseFeeMutez: 8,
+      },
+      {
+        burnFeeMutez: 0,
+        consumedMilligas: 168681,
+        gasLimit: 169,
+        minimalFeeMutez: 269,
+        operationFeeMutez: 168.9,
+        storageLimit: 0,
+        suggestedFeeMutez: 289,
+        totalCost: 269,
+        usingBaseFeeMutez: 269,
+      },
+    ];
+
+    const processedEstimateResult = {
+      ...accountOperations,
+      estimates: [executeParams({ fee: 289, gasLimit: 169 })],
+      revealEstimate: executeParams({ fee: 7, gasLimit: 2, storageLimit: 5 }),
+    };
+
+    jest.mocked(makeToolkit).mockImplementation(
+      () =>
+        ({
+          estimate: {
+            batch: jest.fn().mockResolvedValue(estimateResult),
+          },
+        }) as any
+    );
+
+    const result = await estimate(accountOperations, GHOSTNET);
+
+    expect(result).toEqual(processedEstimateResult);
+  });
 });

src/utils/tezos/estimate.ts

@@ -1,7 +1,19 @@
-import { addressExists, makeToolkit, operationsToBatchParams } from "./helpers";
+import { Estimate } from "@taquito/taquito";
+
+import { isAccountRevealed, makeToolkit, operationsToBatchParams } from "./helpers";
+import { Estimation } from "./types";
 import { AccountOperations, EstimatedAccountOperations } from "../../types/AccountOperations";
 import { Network } from "../../types/Network";
 
+/**
+ * Estimates (and simulates the execution of) the operations.
+ *
+ * Note: if
+ *
+ * @param operations - operations to be estimated with a particular transaction signer
+ * @param network -
+ * @returns operations with their estimated fees, gas and storage limits
+ */
 export const estimate = async (
   operations: AccountOperations,
   network: Network
@@ -14,20 +26,20 @@ export const estimate = async (
   try {
     const estimates = await tezosToolkit.estimate.batch(operationsToBatchParams(operations));
 
+    let revealEstimate = undefined;
+    // taquito automatically adds a reveal operation (sometimes)
+    // if an account is not revealed yet
+    if (estimates.length > operations.operations.length) {
+      revealEstimate = estimateToEstimation(estimates.shift()!);
+    }
+
     return {
       ...operations,
-      estimates: estimates.map(estimate => ({
-        storageLimit: estimate.storageLimit,
-        gasLimit: estimate.gasLimit,
-        fee: Math.max(estimate.suggestedFeeMutez, estimate.totalCost),
-      })),
+      estimates: estimates.map(estimateToEstimation),
+      revealEstimate,
     };
-    // The way taquito works we need to take the max of suggestedFeeMutez and totalCost
-    // because the suggestedFeeMutez does not include the storage & execution cost
-    // and in these cases the totalCost is the one to go (so, for contract calls)
-    // though totalCost doesn't work well with simple tez transfers and suggestedFeeMutez is more accurate
   } catch (err: any) {
-    const isRevealed = await addressExists(operations.signer.address.pkh, network);
+    const isRevealed = await isAccountRevealed(operations.signer.address.pkh, network);
 
     if (!isRevealed) {
       throw new Error(`Signer address is not revealed on the ${network.name}.`);
@@ -39,6 +51,16 @@ export const estimate = async (
   }
 };
 
+const estimateToEstimation = (estimate: Estimate): Estimation => ({
+  storageLimit: estimate.storageLimit,
+  gasLimit: estimate.gasLimit,
+  // The way taquito works we need to take the max of suggestedFeeMutez and totalCost
+  // because the suggestedFeeMutez does not include the storage & execution cost
+  // and in these cases the totalCost is the one to go (so, for contract calls)
+  // though totalCost doesn't work well with simple tez transfers and suggestedFeeMutez is more accurate
+  fee: Math.max(estimate.suggestedFeeMutez, estimate.totalCost),
+});
+
 // Converts a known L1 error message to a more user-friendly one
 export const handleTezError = (err: Error): string => {
   if (err.message.includes("subtraction_underflow")) {