[BUGFIX] Issues on swap (#4741)

* Fix swap related bugs: change abandonSeed, add serializers for gasOptions & prevent re-rendering on EvmFeesStrategy * add loading state to useGasOptions * Optionally prefill evm gas options in EvmFeesStrategy * Update Swap form to set token account Update Swap form to set the fake recipient address (abandonSeed address) during the first account selection (automated after the mount) in order to prevent errors during gas estimation leading to a gasLimit set at 0 * misc: fix tests indentation * add functions to serialize and deserialize GasOptions * update fixtures to test GasOptions serializeation and deserialization * add changesets --------- Co-authored-by: lambertkevin <[email protected]>
LedgerHQ · Sep 19, 2023 · a134f28 · a134f28 · vercel · Sep 19, 2023
1 parent c135c88
commit a134f28
Show file tree

Hide file tree

Showing 17 changed files with 307 additions and 94 deletions.
diff --git a/.changeset/grumpy-flowers-enjoy.md b/.changeset/grumpy-flowers-enjoy.md
@@ -0,0 +1,6 @@
+---
+"ledger-live-desktop": minor
+"live-mobile": minor
+---
+
+Fix swap issue when estimating and editing fees
diff --git a/.changeset/little-papayas-nail.md b/.changeset/little-papayas-nail.md
@@ -0,0 +1,5 @@
+---
+"@ledgerhq/live-common": minor
+---
+
+Add loading state to useGasOptions hook to be used by UI while gasOptions are being fetched
diff --git a/.changeset/nine-cooks-drop.md b/.changeset/nine-cooks-drop.md
@@ -0,0 +1,5 @@
+---
+"@ledgerhq/coin-evm": minor
+---
+
+Add gasOption serialisation and deserialisation to evm transactions
diff --git a/.changeset/pretty-panthers-approve.md b/.changeset/pretty-panthers-approve.md
@@ -0,0 +1,5 @@
+---
+"@ledgerhq/cryptoassets": patch
+---
+
+Don't use `0` address for evm currencies to avoid gas estimation error in swap when interacting with smart contracts (swapping from token accounts)
diff --git a/apps/ledger-live-desktop/src/renderer/screens/exchange/Swap2/Form/index.tsx b/apps/ledger-live-desktop/src/renderer/screens/exchange/Swap2/Form/index.tsx
@@ -125,6 +125,18 @@ const SwapForm = () => {
     timeoutErrorMessage: t("swap2.form.timeout.message"),
   });
 
+  // @TODO: Try to check if we can directly have the right state from `useSwapTransaction`
+  // Used to set the fake transaction recipient
+  // As of today, we need to call setFromAccount to trigger an updateTransaction
+  // in order to set the correct recipient address (abandonSeed address)
+  // cf. https://github.com/LedgerHQ/ledger-live/blob/c135c887b313ecc9f4a3b3a421ced0e3a081dc37/libs/ledger-live-common/src/exchange/swap/hooks/useFromState.ts#L50-L57
+  useEffect(() => {
+    if (swapTransaction.account) {
+      swapTransaction.setFromAccount(swapTransaction.account);
+    }
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, []);
+
   const exchangeRatesState = swapTransaction.swap?.rates;
   const swapError = swapTransaction.fromAmountError || exchangeRatesState?.error;
   const swapWarning = swapTransaction.fromAmountWarning;

diff --git a/apps/ledger-live-mobile/src/components/SendRowsFee.tsx b/apps/ledger-live-mobile/src/components/SendRowsFee.tsx
@@ -19,6 +19,7 @@ type Props = {
   status?: TransactionStatus;
   setTransaction: (..._: Array<Transaction>) => void;
   disabledStrategies?: Array<string>;
+  shouldPrefillEvmGasOptions?: boolean;
 } & CompositeScreenProps<
   | StackNavigatorProps<SendFundsNavigatorStackParamList, ScreenName.SendSummary>
   | StackNavigatorProps<SignTransactionNavigatorParamList, ScreenName.SignTransactionSummary>

diff --git a/apps/ledger-live-mobile/src/families/evm/EvmFeesStrategy.tsx b/apps/ledger-live-mobile/src/families/evm/EvmFeesStrategy.tsx
@@ -1,16 +1,17 @@
+import { getEstimatedFees } from "@ledgerhq/coin-evm/lib/logic";
+import type { Transaction } from "@ledgerhq/coin-evm/types/index";
 import { getMainAccount } from "@ledgerhq/live-common/account/helpers";
 import { getAccountBridge } from "@ledgerhq/live-common/bridge/index";
-import type { Transaction } from "@ledgerhq/coin-evm/types/index";
-import React, { useCallback, useEffect, useState } from "react";
-import SelectFeesStrategy from "./SelectFeesStrategy";
-import { ScreenName } from "../../const";
-import { EvmNetworkFeeInfo } from "./EvmNetworkFeesInfo";
-import { SendRowsFeeProps as Props } from "./types";
 import { useGasOptions } from "@ledgerhq/live-common/families/evm/react";
 import { log } from "@ledgerhq/logs";
+import { InfiniteLoader } from "@ledgerhq/native-ui";
 import { AccountBridge } from "@ledgerhq/types-live";
 import BigNumber from "bignumber.js";
-import { getEstimatedFees } from "@ledgerhq/coin-evm/lib/logic";
+import React, { useCallback, useEffect, useState } from "react";
+import { ScreenName } from "../../const";
+import { EvmNetworkFeeInfo } from "./EvmNetworkFeesInfo";
+import SelectFeesStrategy from "./SelectFeesStrategy";
+import { SendRowsFeeProps as Props } from "./types";
 
 const getCustomStrategy = (transaction: Transaction): BigNumber | null => {
   if (transaction.feesStrategy === "custom") {
@@ -27,12 +28,13 @@ export default function EvmFeesStrategy({
   setTransaction,
   navigation,
   route,
+  shouldPrefillEvmGasOptions = true,
   ...props
 }: Props<Transaction>) {
   const account = getMainAccount(accountProp, parentAccount);
   const bridge: AccountBridge<Transaction> = getAccountBridge(account);
 
-  const [gasOptions, error] = useGasOptions({
+  const [gasOptions, error, loading] = useGasOptions({
     currency: account.currency,
     transaction,
     interval: account.currency.blockAvgTime ? account.currency.blockAvgTime * 1000 : undefined,
@@ -43,12 +45,14 @@ export default function EvmFeesStrategy({
   }
 
   useEffect(() => {
-    const updatedTransaction = bridge.updateTransaction(transaction, {
-      ...transaction,
-      gasOptions,
-    });
-    setTransaction(updatedTransaction);
-  }, [bridge, setTransaction, gasOptions, transaction]);
+    if (shouldPrefillEvmGasOptions) {
+      const updatedTransaction = bridge.updateTransaction(transaction, {
+        ...transaction,
+        gasOptions,
+      });
+      setTransaction(updatedTransaction);
+    }
+  }, [bridge, setTransaction, gasOptions, transaction, shouldPrefillEvmGasOptions]);
 
   const [customStrategy, setCustomStrategy] = useState(getCustomStrategy(transaction));
 
@@ -62,14 +66,14 @@ export default function EvmFeesStrategy({
 
   const onFeesSelected = useCallback(
     ({ feesStrategy }) => {
-      const bridge = getAccountBridge(account, parentAccount);
       setTransaction(
         bridge.updateTransaction(transaction, {
           feesStrategy,
+          gasOptions,
         }),
       );
     },
-    [setTransaction, account, parentAccount, transaction],
+    [setTransaction, bridge, transaction, gasOptions],
   );
 
   const openCustomFees = useCallback(() => {
@@ -84,6 +88,10 @@ export default function EvmFeesStrategy({
     });
   }, [navigation, route.params, account.id, parentAccount, transaction, setTransaction]);
 
+  if (loading) {
+    return <InfiniteLoader size={32} />;
+  }
+
   /**
    * If no gasOptions available, this means this currency does not have a
    * gasTracker. Hence, we do not display the fee fields.

diff --git a/apps/ledger-live-mobile/src/families/evm/types.ts b/apps/ledger-live-mobile/src/families/evm/types.ts
@@ -14,6 +14,7 @@ export type SendRowsFeeProps<T extends Transaction = Transaction> = {
   account: AccountLike;
   parentAccount?: Account | null;
   setTransaction: Result<T>["setTransaction"];
+  shouldPrefillEvmGasOptions?: boolean;
 } & CompositeScreenProps<
   | StackNavigatorProps<SendFundsNavigatorStackParamList, ScreenName.SendSummary>
   | StackNavigatorProps<SignTransactionNavigatorParamList, ScreenName.SignTransactionSummary>

diff --git a/apps/ledger-live-mobile/src/screens/Swap/Form/index.tsx b/apps/ledger-live-mobile/src/screens/Swap/Form/index.tsx
@@ -128,6 +128,18 @@ export function SwapForm({
     timeoutErrorMessage: t("errors.SwapTimeoutError.title"),
   });
 
+  // @TODO: Try to check if we can directly have the right state from `useSwapTransaction`
+  // Used to set the right state (recipient address, data, etc...) when comming from a token account
+  // As of today, we need to call setFromAccount to trigger an updateTransaction in order to set the correct
+  // recipient address (token contract address) and related data to compute the fees
+  // cf. https://github.com/LedgerHQ/ledger-live/blob/c135c887b313ecc9f4a3b3a421ced0e3a081dc37/libs/ledger-live-common/src/exchange/swap/hooks/useFromState.ts#L50-L57
+  useEffect(() => {
+    if (swapTransaction.account) {
+      swapTransaction.setFromAccount(swapTransaction.account);
+    }
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, []);
+
   const exchangeRatesState = swapTransaction.swap?.rates;
   const { partnersList, exchangeRateList } = useMemo(() => {
     const partnerAndExchangeRateDefault = {

diff --git a/apps/ledger-live-mobile/src/screens/Swap/SubScreens/SelectFees.tsx b/apps/ledger-live-mobile/src/screens/Swap/SubScreens/SelectFees.tsx
@@ -41,6 +41,16 @@ export function SelectFees({ navigation, route }: SelectFeesParamList) {
       <NavigationScrollView contentContainerStyle={styles.scrollView}>
         {account && transaction ? (
           <SendRowsFee
+            /**
+             * This is needed to avoid a React side effect caused by the
+             * onSetTransaction callback above. The callback is called
+             * when a tx is updated, which causes a navigation back to the
+             * SwapForm screen. This causes the SendRowsFee screen to unmount.
+             * If we prefill the gas options, the tx will get updated when the
+             * SendRowsFee screen mounts, which causes the callback to be called
+             * cf. apps/ledger-live-mobile/src/families/evm/EvmFeesStrategy.tsx
+             */
+            shouldPrefillEvmGasOptions={false}
             setTransaction={onSetTransaction}
             account={account}
             parentAccount={parentAccount}

diff --git a/libs/coin-evm/src/__tests__/fixtures/transaction.fixtures.ts b/libs/coin-evm/src/__tests__/fixtures/transaction.fixtures.ts
@@ -45,6 +45,26 @@ export const rawEip1559Tx: EvmTransactionEIP1559Raw = Object.freeze(
     maxFeePerGas: "10000",
     maxPriorityFeePerGas: "10000",
     additionalFees: "420",
+    gasOptions: {
+      slow: {
+        maxFeePerGas: "10000",
+        maxPriorityFeePerGas: "15000",
+        gasPrice: null,
+        nextBaseFee: "16000",
+      },
+      medium: {
+        maxFeePerGas: "20000",
+        maxPriorityFeePerGas: "25000",
+        gasPrice: null,
+        nextBaseFee: "16000",
+      },
+      fast: {
+        maxFeePerGas: "30000",
+        maxPriorityFeePerGas: "35000",
+        gasPrice: null,
+        nextBaseFee: "16000",
+      },
+    },
     type: 2,
   }),
 );
@@ -71,6 +91,26 @@ export const eip1559Tx: EvmTransactionEIP1559 = Object.freeze(
     maxFeePerGas: new BigNumber(10000),
     maxPriorityFeePerGas: new BigNumber(10000),
     additionalFees: new BigNumber(420),
+    gasOptions: {
+      slow: {
+        maxFeePerGas: new BigNumber(10000),
+        maxPriorityFeePerGas: new BigNumber(15000),
+        gasPrice: null,
+        nextBaseFee: new BigNumber(16000),
+      },
+      medium: {
+        maxFeePerGas: new BigNumber(20000),
+        maxPriorityFeePerGas: new BigNumber(25000),
+        gasPrice: null,
+        nextBaseFee: new BigNumber(16000),
+      },
+      fast: {
+        maxFeePerGas: new BigNumber(30000),
+        maxPriorityFeePerGas: new BigNumber(35000),
+        gasPrice: null,
+        nextBaseFee: new BigNumber(16000),
+      },
+    },
     type: 2,
   }),
 );
@@ -95,6 +135,26 @@ export const rawLegacyTx: EvmTransactionLegacyRaw = Object.freeze({
   data: testData,
   gasPrice: "10000",
   additionalFees: "420",
+  gasOptions: {
+    slow: {
+      maxFeePerGas: null,
+      maxPriorityFeePerGas: null,
+      gasPrice: "10000",
+      nextBaseFee: null,
+    },
+    medium: {
+      maxFeePerGas: null,
+      maxPriorityFeePerGas: null,
+      gasPrice: "20000",
+      nextBaseFee: null,
+    },
+    fast: {
+      maxFeePerGas: null,
+      maxPriorityFeePerGas: null,
+      gasPrice: "30000",
+      nextBaseFee: null,
+    },
+  },
   type: 0,
 });
 
@@ -119,6 +179,26 @@ export const legacyTx: EvmTransactionLegacy = Object.freeze(
     data: Buffer.from(testData, "hex"),
     gasPrice: new BigNumber(10000),
     additionalFees: new BigNumber(420),
+    gasOptions: {
+      slow: {
+        maxFeePerGas: null,
+        maxPriorityFeePerGas: null,
+        gasPrice: new BigNumber(10000),
+        nextBaseFee: null,
+      },
+      medium: {
+        maxFeePerGas: null,
+        maxPriorityFeePerGas: null,
+        gasPrice: new BigNumber(20000),
+        nextBaseFee: null,
+      },
+      fast: {
+        maxFeePerGas: null,
+        maxPriorityFeePerGas: null,
+        gasPrice: new BigNumber(30000),
+        nextBaseFee: null,
+      },
+    },
     type: 0,
   }),
 );

diff --git a/libs/coin-evm/src/__tests__/unit/transaction.unit.test.ts b/libs/coin-evm/src/__tests__/unit/transaction.unit.test.ts
@@ -61,47 +61,46 @@ describe("EVM Family", () => {
           expect(fromTransactionRaw(rawNftEip1559Tx)).toEqual(nftEip1559tx);
         });
       });
-    });
-
-    describe("with customGasLimit", () => {
-      it("should deserialize a raw EIP1559 transaction into a ledger live transaction", () => {
-        expect(fromTransactionRaw({ ...rawEip1559Tx, customGasLimit: "22000" })).toEqual({
-          ...eip1559Tx,
-          customGasLimit: new BigNumber(22000),
+      describe("with customGasLimit", () => {
+        it("should deserialize a raw EIP1559 transaction into a ledger live transaction", () => {
+          expect(fromTransactionRaw({ ...rawEip1559Tx, customGasLimit: "22000" })).toEqual({
+            ...eip1559Tx,
+            customGasLimit: new BigNumber(22000),
+          });
         });
-      });
 
-      it("should deserialize a raw legacy transaction into a ledger live transaction", () => {
-        expect(fromTransactionRaw({ ...rawLegacyTx, customGasLimit: "22000" })).toEqual({
-          ...legacyTx,
-          customGasLimit: new BigNumber(22000),
+        it("should deserialize a raw legacy transaction into a ledger live transaction", () => {
+          expect(fromTransactionRaw({ ...rawLegacyTx, customGasLimit: "22000" })).toEqual({
+            ...legacyTx,
+            customGasLimit: new BigNumber(22000),
+          });
         });
-      });
 
-      it("should deserialize a raw legacy transaction without type into a ledger live transaction", () => {
-        expect(
-          fromTransactionRaw({
-            ...rawLegacyTx,
-            type: undefined,
-            customGasLimit: "22000",
-          }),
-        ).toEqual({
-          ...legacyTx,
-          customGasLimit: new BigNumber(22000),
+        it("should deserialize a raw legacy transaction without type into a ledger live transaction", () => {
+          expect(
+            fromTransactionRaw({
+              ...rawLegacyTx,
+              type: undefined,
+              customGasLimit: "22000",
+            }),
+          ).toEqual({
+            ...legacyTx,
+            customGasLimit: new BigNumber(22000),
+          });
         });
-      });
 
-      it("should deserialize an nft legacy transaction into a ledger live transaction", () => {
-        expect(fromTransactionRaw({ ...nftRawLegacyTx, customGasLimit: "22000" })).toEqual({
-          ...nftLegacyTx,
-          customGasLimit: new BigNumber(22000),
+        it("should deserialize an nft legacy transaction into a ledger live transaction", () => {
+          expect(fromTransactionRaw({ ...nftRawLegacyTx, customGasLimit: "22000" })).toEqual({
+            ...nftLegacyTx,
+            customGasLimit: new BigNumber(22000),
+          });
         });
-      });
 
-      it("should deserialize an nft EIP1559 transaction into a ledger live transaction", () => {
-        expect(fromTransactionRaw({ ...rawNftEip1559Tx, customGasLimit: "22000" })).toEqual({
-          ...nftEip1559tx,
-          customGasLimit: new BigNumber(22000),
+        it("should deserialize an nft EIP1559 transaction into a ledger live transaction", () => {
+          expect(fromTransactionRaw({ ...rawNftEip1559Tx, customGasLimit: "22000" })).toEqual({
+            ...nftEip1559tx,
+            customGasLimit: new BigNumber(22000),
+          });
         });
       });
     });