[SDK] Add pagination to getOwned for ERC721 & 1155

[kien-ngo]

Problem solved

Add optional pagination for getOwned.

Changes made

• Public API changes: list the public API changes made if any
• Internal API changes: explain the internal logic changes

How to test

• Automated tests: link to unit test file
• Manual tests: step by step instructions on how to test

[changeset-bot]

🦋 Changeset detected

Latest commit: 9d1bb6e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages

Name	Type
@thirdweb-dev/sdk	Patch
thirdweb	Patch
@thirdweb-dev/react-core	Patch
@thirdweb-dev/react-native	Patch
@thirdweb-dev/react	Patch
@thirdweb-dev/unity-js-bridge	Patch
@thirdweb-dev/wallets	Patch
@thirdweb-dev/auth	Patch
@thirdweb-dev/react-native-compat	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[codecov]

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Files	Coverage Δ
packages/sdk/src/evm/common/gas-price.ts	47.54% <100.00%> (+1.77%)	⬆️
packages/sdk/src/evm/common/role.ts	100.00% <100.00%> (ø)
packages/sdk/src/evm/constants/contract.ts	100.00% <100.00%> (ø)
packages/sdk/src/evm/constants/erc721-features.ts	100.00% <100.00%> (ø)
packages/sdk/src/evm/constants/urls.ts	59.68% <100.00%> (ø)
...contracts/prebuilt-implementations/edition-drop.ts	90.00% <100.00%> (ø)
.../evm/contracts/prebuilt-implementations/edition.ts	86.84% <100.00%> (ø)
packages/sdk/src/evm/core/classes/erc-1155.ts	75.97% <ø> (ø)
...kages/sdk/src/evm/core/classes/erc-721-standard.ts	91.42% <100.00%> (ø)
packages/sdk/src/evm/core/classes/factory.ts	52.12% <ø> (ø)
... and 33 more

... and 4 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

[kien-ngo]

@joaquim-verges Thanks again for taking your time to review this. This code of mine:

const start = queryParams?.start || 0;
const count = queryParams?.count || DEFAULT_QUERY_ALL_COUNT;
const startIndex = start * count;
tokenIds = tokenIds.slice(startIndex, startIndex + count);

is based on how I think the pagination for getOwned should work as demonstrated in the image:

which is a bit different to how it works in getAll. Do you think there's anything wrong with this approach?

p/s: I have set up the unit tests here:
ERC1155:

js/packages/sdk/test/evm/edition.test.ts

Lines 293 to 333 in c7476e8

	it("should respect pagination for getOwned", async () => {
	const nfts = [] as { metadata: { name: string }; supply: number }[];
	for (let i = 0; i < 10; i++) {
	nfts.push({
	metadata: { name: `Test${i}` },
	supply: 10,
	});
	}
	await bundleContract.mintBatch(nfts);
	const total = await bundleContract.getTotalCount();
	expect(total.toNumber()).to.eq(10);
	const page1 = await bundleContract.getOwned(adminWallet.address, {
	count: 2,
	start: 0,
	});
	expect(page1).to.be.an("array").length(2);
	const page3 = await bundleContract.getOwned(adminWallet.address, {
	count: 3,
	start: 2,
	});
	expect(page3).to.be.an("array").length(3);
	expect(page3[0].metadata.id).to.eq("6");
	expect(page3[1].metadata.id).to.eq("7");
	});
	it("getOwned pagination should return all records when queryParams.count is greater than the total supply", async () => {
	const nfts = [] as { metadata: { name: string }; supply: number }[];
	for (let i = 0; i < 10; i++) {
	nfts.push({
	metadata: { name: `Test${i}` },
	supply: 10,
	});
	}
	await bundleContract.mintBatch(nfts);
	const items = await bundleContract.getOwned(undefined, {
	count: 1000,
	start: 0,
	});
	expect(items).to.be.an("array").length(nfts.length);
	});
	});

ERC721:

js/packages/sdk/test/evm/nft.test.ts

Lines 291 to 319 in c7476e8

	it("should respect pagination for getOwned", async () => {
	const _tokenIds: number[] = Array.from({ length: 11 }, (_, index) => index); // [0, 1, ... 10]
	const metadata = _tokenIds.map((num) => ({ name: `Test${num}` }));
	await nftContract.mintBatch(metadata);
	const nftPage1 = await nftContract.getOwned(undefined, {
	count: 2,
	start: 0,
	});
	expect(nftPage1).to.be.an("array").length(2);
	const nftPage2 = await nftContract.getOwned(undefined, {
	count: 3,
	start: 2,
	});
	expect(nftPage2).to.be.an("array").length(3);
	expect(nftPage2[0].metadata.id).to.eq("6");
	expect(nftPage2[1].metadata.id).to.eq("7");
	});
	it("getOwned pagination should return all records when queryParams.count is greater than the total supply", async () => {
	const _tokenIds: number[] = Array.from({ length: 11 }, (_, index) => index); // [0, 1, ... 10]
	const metadata = _tokenIds.map((num) => ({ name: `Test${num}` }));
	await nftContract.mintBatch(metadata);
	const nfts = await nftContract.getOwned(undefined, {
	count: 1000,
	start: 0,
	});
	expect(nfts).to.be.an("array").length(_tokenIds.length);
	});
	});

[joaquim-verges]

@kien-ngo you're still considering start as the page number instead of the actual start tokenId.

This is a different behavior from the existing erc721.getAll(), and will the dashboard pagination and any customer that uses pagination today to implement 2 different kinds of paginations.

please keep the current behavior and keep it consistent between getAll and getOwned.

[kien-ngo]

Ah I see. Okay so the start should be one of the "owned" tokenIds to make it consistent with getAll. Will update my code today ^

[joaquim-verges]

@kien-ngo to be clear - start doesn't need to be the actual tokenId number, but rather the index of that token in the array of owned tokenIds.

for getAll it just happens to be that index == tokenId, but not the case for getOwned.

In general the way to think about it is:

let's say i owned 20 NFTs total, calling

getOwned("0x...", { start: 8, count: 3 })

should give me my 8th, 9th and 10th owned NFT (regardless of their tokenID)

lmk if that makes sense

[kien-ngo]

Yes ser it does. Thanks again. I will update the code and the tests

[kien-ngo]

@joaquim-verges Hi there, I updated the code & added unit tests. Please have a look when you have time thank you.

[joaquim-verges]

Looks good! Thanks @kien-ngo