Skip to content

Commit f2c40bd

Browse files
committed
[SDK] Improve test coverage for ERC721 extensions (#4964)
## Problem solved Short description of the bug fixed or feature added <!-- start pr-codex --> --- ## PR-Codex overview This PR focuses on enhancing the `erc721` extension by adding error handling, improving test coverage, and refining contract interactions, particularly regarding token URIs and contract support checks. ### Detailed summary - Updated `getNFT.ts` to check for trimmed `uri`. - Added new imports in test files for contracts and wallets. - Implemented tests for unsupported contract methods in `getOwnedTokenIds.test.ts`. - Added support check tests in `getNFTs.test.ts`. - Created a test for default URI handling in `getNFT.test.ts`. > ✨ Ask PR-Codex anything about this PR by commenting with `/codex {your question}` <!-- end pr-codex -->
1 parent 26e3c57 commit f2c40bd

File tree

4 files changed

+126
-4
lines changed

4 files changed

+126
-4
lines changed

packages/thirdweb/src/extensions/erc721/read/getNFT.test.ts

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,16 @@
11
import { describe, expect, it } from "vitest";
22

3+
import { ANVIL_CHAIN } from "~test/chains.js";
4+
import { TEST_CONTRACT_URI } from "~test/ipfs-uris.js";
5+
import { TEST_CLIENT } from "~test/test-clients.js";
36
import { DOODLES_CONTRACT } from "~test/test-contracts.js";
7+
import { TEST_ACCOUNT_A } from "~test/test-wallets.js";
8+
import { getContract } from "../../../contract/contract.js";
9+
import { deployERC721Contract } from "../../../extensions/prebuilts/deploy-erc721.js";
10+
import { sendAndConfirmTransaction } from "../../../transaction/actions/send-and-confirm-transaction.js";
11+
import { parseNFT } from "../../../utils/nft/parseNft.js";
12+
import { setTokenURI } from "../__generated__/INFTMetadata/write/setTokenURI.js";
13+
import { mintTo } from "../write/mintTo.js";
414
import { getNFT } from "./getNFT.js";
515

616
describe.runIf(process.env.TW_SECRET_KEY)("erc721.getNFT", () => {
@@ -89,4 +99,60 @@ describe.runIf(process.env.TW_SECRET_KEY)("erc721.getNFT", () => {
8999
}
90100
`);
91101
});
102+
103+
it("should return a default value if the URI of the token doesn't exist", async () => {
104+
/**
105+
* To create this test scenario, we first deploy an NFTCollection/Edition contract,
106+
* mint a token, then purposefully change that token's URI to an empty string, using setTokenURI
107+
*/
108+
const contract = getContract({
109+
address: await deployERC721Contract({
110+
chain: ANVIL_CHAIN,
111+
client: TEST_CLIENT,
112+
account: TEST_ACCOUNT_A,
113+
type: "TokenERC721",
114+
params: {
115+
name: "",
116+
contractURI: TEST_CONTRACT_URI,
117+
},
118+
}),
119+
chain: ANVIL_CHAIN,
120+
client: TEST_CLIENT,
121+
});
122+
123+
await sendAndConfirmTransaction({
124+
transaction: mintTo({
125+
contract,
126+
nft: { name: "token 0" },
127+
to: TEST_ACCOUNT_A.address,
128+
}),
129+
account: TEST_ACCOUNT_A,
130+
});
131+
132+
await sendAndConfirmTransaction({
133+
transaction: setTokenURI({
134+
contract,
135+
tokenId: 0n,
136+
// Need to have some spaces because NFTMetadata.sol does not allow to update an empty valud
137+
uri: " ",
138+
}),
139+
account: TEST_ACCOUNT_A,
140+
});
141+
142+
expect(await getNFT({ contract, tokenId: 0n })).toStrictEqual(
143+
parseNFT(
144+
{
145+
id: 0n,
146+
type: "ERC721",
147+
uri: "",
148+
},
149+
{
150+
tokenId: 0n,
151+
tokenUri: "",
152+
type: "ERC721",
153+
owner: null,
154+
},
155+
),
156+
);
157+
});
92158
});

packages/thirdweb/src/extensions/erc721/read/getNFT.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ export async function getNFT(
4848
: null,
4949
]);
5050

51-
if (!uri) {
51+
if (!uri?.trim()) {
5252
return parseNFT(
5353
{
5454
id: options.tokenId,

packages/thirdweb/src/extensions/erc721/read/getNFTs.test.ts

Lines changed: 44 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,17 @@
1+
import { type Abi, toFunctionSelector } from "viem";
12
import { describe, expect, it } from "vitest";
2-
3-
import { DOODLES_CONTRACT } from "~test/test-contracts.js";
3+
import { ANVIL_CHAIN } from "~test/chains.js";
4+
import { TEST_CONTRACT_URI } from "~test/ipfs-uris.js";
5+
import { TEST_CLIENT } from "~test/test-clients.js";
6+
import {
7+
DOODLES_CONTRACT,
8+
UNISWAPV3_FACTORY_CONTRACT,
9+
} from "~test/test-contracts.js";
10+
import { TEST_ACCOUNT_B } from "~test/test-wallets.js";
11+
import { resolveContractAbi } from "../../../contract/actions/resolve-abi.js";
12+
import { getContract } from "../../../contract/contract.js";
13+
import { deployERC721Contract } from "../../../extensions/prebuilts/deploy-erc721.js";
14+
import { isGetNFTSupported } from "./getNFT.js";
415
import { getNFTs } from "./getNFTs.js";
516

617
describe.runIf(process.env.TW_SECRET_KEY)("erc721.getNFTs", () => {
@@ -185,4 +196,35 @@ describe.runIf(process.env.TW_SECRET_KEY)("erc721.getNFTs", () => {
185196
it.todo("works for a contract with `1` indexed NFTs", async () => {
186197
// TODO find a contract that we can use that has "1 indexed" NFTs, then re-enable this test
187198
});
199+
200+
it("isGetNFTsSupported should work", async () => {
201+
const contract = getContract({
202+
address: await deployERC721Contract({
203+
chain: ANVIL_CHAIN,
204+
client: TEST_CLIENT,
205+
type: "TokenERC721",
206+
params: {
207+
name: "",
208+
contractURI: TEST_CONTRACT_URI,
209+
},
210+
account: TEST_ACCOUNT_B,
211+
}),
212+
chain: ANVIL_CHAIN,
213+
client: TEST_CLIENT,
214+
});
215+
216+
const abi = await resolveContractAbi<Abi>(contract);
217+
const selectors = abi
218+
.filter((f) => f.type === "function")
219+
.map((f) => toFunctionSelector(f));
220+
expect(isGetNFTSupported(selectors)).toBe(true);
221+
});
222+
223+
it("should throw error if totalSupply and nextTokenIdToMint are not supported", async () => {
224+
await expect(() =>
225+
getNFTs({ contract: UNISWAPV3_FACTORY_CONTRACT }),
226+
).rejects.toThrowError(
227+
"Contract requires either `nextTokenIdToMint` or `totalSupply` function available to determine the next token ID to mint",
228+
);
229+
});
188230
});

packages/thirdweb/src/extensions/erc721/read/getOwnedTokenIds.test.ts

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
import { describe, expect, it } from "vitest";
2-
import { DOODLES_CONTRACT } from "~test/test-contracts.js";
2+
import {
3+
DOODLES_CONTRACT,
4+
UNISWAPV3_FACTORY_CONTRACT,
5+
} from "~test/test-contracts.js";
6+
import { TEST_ACCOUNT_B } from "~test/test-wallets.js";
37
import { getOwnedTokenIds } from "./getOwnedTokenIds.js";
48

59
describe.runIf(process.env.TW_SECRET_KEY)("erc721.getOwnedTokenIds", () => {
@@ -14,4 +18,14 @@ describe.runIf(process.env.TW_SECRET_KEY)("erc721.getOwnedTokenIds", () => {
1418
// so the data should not change
1519
expect(tokenIds.length).toBe(81);
1620
});
21+
22+
it("should throw if tokenOfOwnerByIndex or tokensOfOwner not supported", async () => {
23+
// We know current Lens contract on Polygon doesn't have this.
24+
const contract = UNISWAPV3_FACTORY_CONTRACT;
25+
await expect(() =>
26+
getOwnedTokenIds({ contract, owner: TEST_ACCOUNT_B.address }),
27+
).rejects.toThrowError(
28+
`The contract at ${contract.address} on chain ${contract.chain.id} does not support the tokenOfOwnerByIndex or tokensOfOwner interface`,
29+
);
30+
});
1731
});

0 commit comments

Comments
 (0)