LL-8508 - Fix NFT merging in incremental sync + tests (#1552)

* Fix NFT merging in incremental sync + tests

* snapshot

Co-authored-by: Gaëtan Renaudeau <[email protected]>

src/__tests__/__snapshots__/all.libcore.ts.snap

@@ -15177,7 +15177,7 @@ Array [
     "starred": false,
     "subAccounts": Array [],
     "swapHistory": Array [],
-    "syncHash": "[\\"ethereum/erc20/ampleforth\\",\\"ethereum/erc20/steth\\"]_6026",
+    "syncHash": "[\\"ethereum/erc20/ampleforth\\",\\"ethereum/erc20/steth\\"]_false_6026",
     "unitMagnitude": 18,
     "used": true,
   },
@@ -15218,7 +15218,7 @@ Array [
     "starred": false,
     "subAccounts": Array [],
     "swapHistory": Array [],
-    "syncHash": "[\\"ethereum/erc20/ampleforth\\",\\"ethereum/erc20/steth\\"]_6026",
+    "syncHash": "[\\"ethereum/erc20/ampleforth\\",\\"ethereum/erc20/steth\\"]_false_6026",
     "unitMagnitude": 18,
     "used": true,
   },
@@ -15698,7 +15698,7 @@ Array [
     "starred": false,
     "subAccounts": Array [],
     "swapHistory": Array [],
-    "syncHash": "[\\"ethereum/erc20/ampleforth\\",\\"ethereum/erc20/steth\\"]_6026",
+    "syncHash": "[\\"ethereum/erc20/ampleforth\\",\\"ethereum/erc20/steth\\"]_false_6026",
     "unitMagnitude": 18,
     "used": true,
   },
@@ -16033,7 +16033,7 @@ Array [
     "starred": false,
     "subAccounts": Array [],
     "swapHistory": Array [],
-    "syncHash": "[\\"ethereum/erc20/ampleforth\\",\\"ethereum/erc20/steth\\"]_6026",
+    "syncHash": "[\\"ethereum/erc20/ampleforth\\",\\"ethereum/erc20/steth\\"]_false_6026",
     "unitMagnitude": 18,
     "used": true,
   },
@@ -16130,7 +16130,7 @@ Array [
     "starred": false,
     "subAccounts": Array [],
     "swapHistory": Array [],
-    "syncHash": "[\\"ethereum/erc20/ampleforth\\",\\"ethereum/erc20/steth\\"]_6026",
+    "syncHash": "[\\"ethereum/erc20/ampleforth\\",\\"ethereum/erc20/steth\\"]_false_6026",
     "unitMagnitude": 18,
     "used": true,
   },
@@ -16255,7 +16255,7 @@ Array [
     "starred": false,
     "subAccounts": Array [],
     "swapHistory": Array [],
-    "syncHash": "[\\"ethereum/erc20/ampleforth\\",\\"ethereum/erc20/steth\\"]_6026",
+    "syncHash": "[\\"ethereum/erc20/ampleforth\\",\\"ethereum/erc20/steth\\"]_false_6026",
     "unitMagnitude": 18,
     "used": true,
   },
@@ -16352,7 +16352,7 @@ Array [
     "starred": false,
     "subAccounts": Array [],
     "swapHistory": Array [],
-    "syncHash": "[\\"ethereum/erc20/ampleforth\\",\\"ethereum/erc20/steth\\"]_6026",
+    "syncHash": "[\\"ethereum/erc20/ampleforth\\",\\"ethereum/erc20/steth\\"]_false_6026",
     "unitMagnitude": 18,
     "used": true,
   },
@@ -48285,7 +48285,7 @@ Array [
     "starred": false,
     "subAccounts": Array [],
     "swapHistory": Array [],
-    "syncHash": "[\\"ethereum/erc20/ampleforth\\",\\"ethereum/erc20/steth\\"]_0",
+    "syncHash": "[\\"ethereum/erc20/ampleforth\\",\\"ethereum/erc20/steth\\"]_false_0",
     "unitMagnitude": 18,
     "used": true,
   },
@@ -48312,7 +48312,7 @@ Array [
     "starred": false,
     "subAccounts": Array [],
     "swapHistory": Array [],
-    "syncHash": "[\\"ethereum/erc20/ampleforth\\",\\"ethereum/erc20/steth\\"]_0",
+    "syncHash": "[\\"ethereum/erc20/ampleforth\\",\\"ethereum/erc20/steth\\"]_false_0",
     "unitMagnitude": 18,
     "used": true,
   },
@@ -48339,7 +48339,7 @@ Array [
     "starred": false,
     "subAccounts": Array [],
     "swapHistory": Array [],
-    "syncHash": "[\\"ethereum/erc20/ampleforth\\",\\"ethereum/erc20/steth\\"]_0",
+    "syncHash": "[\\"ethereum/erc20/ampleforth\\",\\"ethereum/erc20/steth\\"]_false_0",
     "unitMagnitude": 18,
     "used": true,
   },

src/bridge/jsHelpers.ts

@@ -134,15 +134,16 @@ export const mergeNfts = (
   const nfts = oldNfts?.slice() ?? [];
   for (let i = 0; i < nfts.length; i++) {
     const nft = nfts[i];
+
     // The NFTs are the same, do don't anything
-    if (!newNftsPerId[nft.id] || isEqual(nft, newNftsPerId[nft.id])) {
-      // NFT already in, deleting it from the newNfts to keep only the un-added ones at the end
-      delete newNftsPerId[nft.id];
-      continue;
+    if (!newNftsPerId[nft.id]) {
+      nfts.splice(i, 1);
+      i--;
+    } else if (!isEqual(nft, newNftsPerId[nft.id])) {
+      // Use the new NFT instead (as a copy cause we're deleting the reference just after)
+      nfts[i] = Object.assign({}, newNftsPerId[nft.id]);
     }
 
-    // Use the new NFT instead (as a copy cause we're deleting the reference just after)
-    nfts[i] = Object.assign({}, newNftsPerId[nft.id]);
     // Delete it from the newNfts to keep only the un-added ones at the end
     delete newNftsPerId[nft.id];
   }

src/families/ethereum/nft.test.ts

@@ -1,11 +1,14 @@
 import "../../__tests__/test-helpers/setup";
+import BigNumber from "bignumber.js";
 import { reduce } from "rxjs/operators";
-import { fromAccountRaw, toAccountRaw } from "../../account";
-import type { Account, AccountRaw } from "../../types";
+import { fromAccountRaw, toAccountRaw, toNFTRaw } from "../../account";
+import type { Account, AccountRaw, NFT } from "../../types";
 import { getAccountBridge } from "../../bridge";
 import { makeBridgeCacheSystem } from "../../bridge/cache";
 import { patchAccount } from "../../reconciliation";
 import { setEnv } from "../../env";
+import { mergeNfts } from "../../bridge/jsHelpers";
+import { encodeNftId } from "../../nft";
 
 jest.setTimeout(120000);
 
@@ -94,6 +97,78 @@ const cache = makeBridgeCacheSystem({
   },
 });
 
+describe("nft merging", () => {
+  const makeNFT = (tokenId: string, contract: string, amount: number): NFT => ({
+    id: encodeNftId("test", contract, tokenId),
+    tokenId,
+    amount: new BigNumber(amount),
+    collection: {
+      contract,
+      standard: "erc721",
+    },
+  });
+  const oldNfts = [
+    makeNFT("1", "contract1", 10),
+    makeNFT("2", "contract1", 1),
+    makeNFT("3", "contract2", 6),
+  ];
+
+  test("should remove first NFT and return new array with same refs", () => {
+    const nfts = [makeNFT("2", "contract1", 1), makeNFT("3", "contract2", 6)];
+    const newNfts = mergeNfts(oldNfts, nfts);
+
+    expect(newNfts.map(toNFTRaw)).toEqual(nfts.map(toNFTRaw));
+    expect(oldNfts[1]).toBe(newNfts[0]);
+    expect(oldNfts[2]).toBe(newNfts[1]);
+  });
+
+  test("should remove any NFT and return new array with same refs", () => {
+    const nfts = [makeNFT("1", "contract1", 10), makeNFT("3", "contract2", 6)];
+    const newNfts = mergeNfts(oldNfts, nfts);
+
+    expect(newNfts.map(toNFTRaw)).toEqual(nfts.map(toNFTRaw));
+    expect(oldNfts[0]).toBe(newNfts[0]);
+    expect(oldNfts[2]).toBe(newNfts[1]);
+  });
+
+  test("should change NFT amount and return new array with new ref", () => {
+    const nfts = [
+      makeNFT("1", "contract1", 10),
+      makeNFT("2", "contract1", 5),
+      makeNFT("3", "contract2", 6),
+    ];
+    const addToNft1 = mergeNfts(oldNfts, nfts);
+
+    expect(addToNft1.map(toNFTRaw)).toEqual(nfts.map(toNFTRaw));
+    expect(oldNfts[0]).toBe(addToNft1[0]);
+    expect(oldNfts[1]).not.toBe(addToNft1[1]);
+    expect(oldNfts[2]).toBe(addToNft1[2]);
+  });
+
+  test("should add NFT and return new array with new ref", () => {
+    const nfts = [
+      makeNFT("1", "contract1", 10),
+      makeNFT("2", "contract1", 1),
+      makeNFT("3", "contract2", 6),
+      makeNFT("4", "contract2", 4),
+    ];
+    const addToNft1 = mergeNfts(oldNfts, nfts);
+
+    expect(addToNft1.map(toNFTRaw)).toEqual(
+      [
+        makeNFT("4", "contract2", 4),
+        makeNFT("1", "contract1", 10),
+        makeNFT("2", "contract1", 1),
+        makeNFT("3", "contract2", 6),
+      ].map(toNFTRaw)
+    );
+    expect(oldNfts[0]).toBe(addToNft1[1]);
+    expect(oldNfts[1]).toBe(addToNft1[2]);
+    expect(oldNfts[2]).toBe(addToNft1[3]);
+    expect(addToNft1[0]).toBe(nfts[3]);
+  });
+});
+
 describe("gaspard NFT on ethereum", () => {
   let account = fromAccountRaw(gaspardAccount);
 
@@ -122,6 +197,17 @@ describe("gaspard NFT on ethereum", () => {
     expect(resync.nfts).toEqual(account.nfts);
   });
 
+  test("remove half NFTs will restore them with half operations", async () => {
+    const halfOps = Math.ceil(account.operations.length / 2);
+    const copy = {
+      ...account,
+      operations: account.operations.slice(halfOps),
+      nfts: account.nfts?.slice(Math.ceil((account.nfts?.length || 0) / 2)),
+    };
+    const resync = await sync(copy);
+    expect(resync.nfts).toEqual(account.nfts);
+  });
+
   test("patchAccount restore new NFTs correctly", async () => {
     const copy = {
       ...account,

src/families/ethereum/synchronisation.ts

@@ -15,7 +15,7 @@ import {
 } from "../../account";
 import { listTokensForCryptoCurrency } from "../../currencies";
 import { encodeAccountId } from "../../account";
-import type { Operation, TokenAccount, Account, NFT } from "../../types";
+import type { Operation, TokenAccount, Account } from "../../types";
 import { API, apiForCurrency, Tx } from "../../api/Ethereum";
 import { digestTokenAccounts, prepareTokenAccounts } from "./modules";
 import { findTokenByAddressInCurrency } from "@ledgerhq/cryptoassets";
@@ -82,10 +82,8 @@ export const getAccountShape: GetAccountShape = async (
   let newOps = flatMap(txs, txToOps({ address, id: accountId, currency }));
   // extracting out the sub operations by token account
   const perTokenAccountIdOperations = {};
-  // extracting and concat all nft operations for an account
-  const flatNftOps: Operation[] = [];
   newOps.forEach((op) => {
-    const { subOperations, nftOperations } = op;
+    const { subOperations } = op;
 
     if (subOperations?.length) {
       subOperations.forEach((sop) => {
@@ -96,10 +94,6 @@ export const getAccountShape: GetAccountShape = async (
         perTokenAccountIdOperations[sop.accountId].push(sop);
       });
     }
-
-    if (nftOperations?.length) {
-      nftOperations.forEach((nop) => flatNftOps.push(nop));
-    }
   });
   const subAccountsExisting = {};
   initialAccount?.subAccounts?.forEach((a) => {
@@ -173,7 +167,7 @@ export const getAccountShape: GetAccountShape = async (
   const operations = mergeOps(initialStableOperations, newOps);
 
   const nfts = isNFTActive(currency)
-    ? mergeNfts(initialAccount?.nfts, await getNfts(flatNftOps))
+    ? mergeNfts(initialAccount?.nfts, nftsFromOperations(operations))
     : undefined;
 
   const accountShape: Partial<Account> = {
@@ -621,11 +615,6 @@ async function loadERC20Balances(tokenAccounts, address, api) {
     .filter(Boolean);
 }
 
-function getNfts(nftOperations: Operation[]): NFT[] {
-  const nfts: NFT[] = nftsFromOperations(nftOperations);
-  return nfts.filter((nft) => nft.amount.gt(0));
-}
-
 const SAFE_REORG_THRESHOLD = 80;
 
 function stableOperations(a) {

src/nft/helpers.ts

@@ -24,7 +24,7 @@ export const nftsFromOperations = (ops: Operation[]): NFT[] => {
       const nftKey = contract + nftOp.tokenId!;
       const { tokenId, standard, id } = nftOp;
 
-      const nft = (acc[nftKey] ?? {
+      const nft = (acc[nftKey] || {
         id,
         tokenId: tokenId!,
         amount: new BigNumber(0),