From 1c7b6d4ce070ef32d5743df05854e42c6e232b16 Mon Sep 17 00:00:00 2001 From: klambert-ledger Date: Thu, 2 Dec 2021 16:41:56 +0100 Subject: [PATCH 1/2] Fix NFT merging in incremental sync + tests --- src/bridge/jsHelpers.ts | 13 ++-- src/families/ethereum/nft.test.ts | 90 +++++++++++++++++++++++- src/families/ethereum/synchronisation.ts | 17 +---- src/nft/helpers.ts | 2 +- 4 files changed, 99 insertions(+), 23 deletions(-) diff --git a/src/bridge/jsHelpers.ts b/src/bridge/jsHelpers.ts index 651dd44541..cbbdc2e179 100644 --- a/src/bridge/jsHelpers.ts +++ b/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]; } diff --git a/src/families/ethereum/nft.test.ts b/src/families/ethereum/nft.test.ts index 8ae040af39..dd88f90bfb 100644 --- a/src/families/ethereum/nft.test.ts +++ b/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, diff --git a/src/families/ethereum/synchronisation.ts b/src/families/ethereum/synchronisation.ts index 34f9d8a6a2..9cc417d9d8 100644 --- a/src/families/ethereum/synchronisation.ts +++ b/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 = { @@ -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) { diff --git a/src/nft/helpers.ts b/src/nft/helpers.ts index 07fa1c88d2..9908dcfe4f 100644 --- a/src/nft/helpers.ts +++ b/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), From 5205b9aa2ccabe1d1e0f42c6d9664d5a598a8114 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ga=C3=ABtan=20Renaudeau?= Date: Thu, 2 Dec 2021 18:02:18 +0100 Subject: [PATCH 2/2] snapshot --- .../__snapshots__/all.libcore.ts.snap | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/__tests__/__snapshots__/all.libcore.ts.snap b/src/__tests__/__snapshots__/all.libcore.ts.snap index 7bb97dde33..6e86dd3488 100644 --- a/src/__tests__/__snapshots__/all.libcore.ts.snap +++ b/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, },