Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfix: πŸ› Spam filtering errors unhandled #8404

Merged
merged 1 commit into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/silly-seals-argue.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"ledger-live-desktop": minor
"live-mobile": minor
"@ledgerhq/live-nft-react": minor
---

Fix undefined join on Spam filter
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,12 @@ export function useSyncNFTsWithAccounts() {
const [, setCurrentIndex] = useState(0);

const { refetch } = useCheckNftAccount({
addresses: groupToFetch.join(","),
addresses: groupToFetch?.join(",") || "",
nftsOwned,
chains: SUPPORTED_NFT_CURRENCIES,
threshold,
action: hideSpamCollection,
enabled,
enabled: enabled && groupToFetch.length > 0,
});

// Refetch with new last group when addressGroups length changes
Expand All @@ -91,7 +91,7 @@ export function useSyncNFTsWithAccounts() {
const interval = setInterval(() => {
setCurrentIndex(prevIndex => {
const nextIndex = (prevIndex + 1) % addressGroups.length;
setGroupToFetch(addressGroups[nextIndex]);
setGroupToFetch(addressGroups[nextIndex] || []);
return nextIndex;
});
}, TIMER);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ export default function SpamScore(props: HookResult) {
return (error as Error).message;
};

const getScore = (data?: SimpleHashResponse) => data?.nfts[0].collection.spam_score;
const getScore = (data?: SimpleHashResponse) => data?.nfts[0]?.collection.spam_score || 100;

const text = checkSpamScore.isError
? getErrorText(checkSpamScore.error)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,12 @@ export function useSyncNFTsWithAccounts() {
const [, setCurrentIndex] = useState(0);

const { refetch } = useCheckNftAccount({
addresses: groupToFetch.join(","),
addresses: groupToFetch?.join(",") || "",
nftsOwned,
chains: SUPPORTED_NFT_CURRENCIES,
threshold,
action: hideSpamCollection,
enabled,
enabled: enabled && groupToFetch.length > 0,
});

// Refetch with new last group when addressGroups length changes
Expand All @@ -91,7 +91,7 @@ export function useSyncNFTsWithAccounts() {
const interval = setInterval(() => {
setCurrentIndex(prevIndex => {
const nextIndex = (prevIndex + 1) % addressGroups.length;
setGroupToFetch(addressGroups[nextIndex]);
setGroupToFetch(addressGroups[nextIndex] || []);
return nextIndex;
});
}, TIMER);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { waitFor, renderHook } from "@testing-library/react";
import { SimpleHashResponse } from "@ledgerhq/live-nft/api/types";
import { notifyManager } from "@tanstack/react-query";

import { wrapper, generateNftsOwned } from "../../tools/helperTests";
import { wrapper, generateNftsOwned, generateNft } from "../../tools/helperTests";
import { useCheckNftAccount } from "../useCheckNftAccount";

jest.setTimeout(30000);
Expand Down Expand Up @@ -79,4 +79,57 @@ describe("useCheckNftAccount", () => {
expect(callCount).toBe(nftsOwned.length / pagedBy);
expect(result.current.nfts.length).toEqual(nftsOwned.length);
});

it("should call action", async () => {
const addresses = "0x34";
const chains = ["ethereum"];

const actionMockMulti = jest.fn();

const { result: resultBis } = renderHook(
() =>
useCheckNftAccount({
addresses,
nftsOwned: [
generateNft("0x1221", "1"),
generateNft("0x2321", "7"),
generateNft("0x27B21", "3"),
],
chains,
threshold: 80,
action: actionMockMulti,
}),
{
wrapper,
},
);

await waitFor(() => !resultBis.current.hasNextPage);

expect(actionMockMulti).toHaveBeenCalledTimes(3);
});
it("should not call action", async () => {
const addresses = "0x34";
const chains = ["ethereum"];

const actionMockUnique = jest.fn();

const { result } = renderHook(
() =>
useCheckNftAccount({
addresses,
nftsOwned,
chains,
threshold: 80,
action: actionMockUnique,
}),
{
wrapper,
},
);

await waitFor(() => !result.current.hasNextPage);

expect(actionMockUnique).not.toHaveBeenCalled();
});
});
15 changes: 8 additions & 7 deletions libs/live-nft-react/src/hooks/useCheckNftAccount.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export function useCheckNftAccount({

const processingNFTs = queryResult.data?.pages.flatMap(page => page.nfts);

if (!queryResult.hasNextPage && processingNFTs) {
if (!queryResult.hasNextPage && processingNFTs?.length) {
for (const nft of processingNFTs) {
const hash = hashProtoNFT(nft.contract_address, nft.token_id, nft.chain);
const existing = nftsWithProperties.get(hash);
Expand All @@ -76,14 +76,15 @@ export function useCheckNftAccount({

if (action) {
const spams = nftsOwned.filter(nft => !nfts.some(ownedNft => ownedNft.id === nft.id));

const collections = nftsByCollections(spams);

Object.entries(collections).map(([contract, nfts]: [string, ProtoNFT[]]) => {
const { accountId } = decodeNftId(nfts[0].id);
const collection = `${accountId}|${contract}`;
action(collection);
});
if (spams.length > 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a unit test to cover this new use case, please?

Object.entries(collections).map(([contract, nfts]: [string, ProtoNFT[]]) => {
const { accountId } = decodeNftId(nfts[0].id);
const collection = `${accountId}|${contract}`;
action(collection);
});
}
}
}
return { ...queryResult, nfts };
Expand Down
20 changes: 10 additions & 10 deletions libs/live-nft-react/src/tools/helperTests.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,25 +17,25 @@ export type FakeNFTRaw = {
contract: string;
standard: "ERC721";
currencyId: string;
metadata: undefined;
};

export const generateNftsOwned = () => {
const nfts: FakeNFTRaw[] = [];

NFTs.forEach(nft => {
for (let i = 1; i <= 20; i++) {
nfts.push({
id: encodeNftId("foo", nft.collection.contract, String(i), "ethereum"),
tokenId: String(i),
amount: new BigNumber(0),
contract: nft.collection.contract,
standard: "ERC721" as const,
currencyId: "ethereum",
metadata: undefined,
});
nfts.push(generateNft(nft.collection.contract, String(i)));
}
});

return nfts;
};

export const generateNft = (contract: string, tokenId: string): FakeNFTRaw => ({
id: encodeNftId("foo", contract, tokenId, "ethereum"),
themooneer marked this conversation as resolved.
Show resolved Hide resolved
tokenId: tokenId,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just tokenId is sufficient object-shorthand

amount: new BigNumber(0),
contract: contract,
standard: "ERC721" as const,
currencyId: "ethereum",
});
Loading