feat(nfts): collections - #329
Conversation
…ollections only returns claimable
📦 Bundle size impactComparing
Thresholds — 🟡 ≥10% or ≥5.0 KB · 🟠 ≥20% or ≥15.0 KB (bundled). Percentage only applies once the baseline is ≥ 10 KB. Informational — this check never blocks merge. |
Imod7
left a comment
Imod7
left a comment
There was a problem hiding this comment.
The paging design is right, and resting it on the never-reused id space is the right reason. Three things worth settling before merge, each with the detail on its own line: unchecked ids, at packages/nfts/src/paging.ts line 93 and packages/nfts/src/items.ts line 186; and the unknown ItemSelection variant at packages/nfts/src/collections.ts line 208. One shape recommendation, on the readAllKeys line in packages/nfts/src/items.ts: keep attributes as an option and give it a named-key form.
The design fits the repo. The read signature, the SdkError model, the structural chain contract and the compile-time check in packages/sdk/src/nfts/contract.test.ts all match what @parity/product-sdk-individuality established, and the dependency set is leaner than the sibling's. Two things I expected to be divergences are not, the positional id argument and the get* naming. Two consistency points remain, neither blocking, with the detail on each line: packages/nfts/src/index.ts on a missing fromPapi, and packages/nfts/src/chain.ts on how wide NftsChain is.
|
|
||
| // The record carries the index ceiling, so unlike the collection listing | ||
| // this read pays nothing extra to learn where the space ends. | ||
| const record = query.Scarcity.Collections.getValue(id, at); |
There was a problem hiding this comment.
Please validate id before it becomes a storage key. PAPI's u32 encoder truncates rather than rejecting, so getCollectionItems(chain, NaN) returns collection 0's catalogue labelled id: NaN and 1.5 returns collection 1's, both on the ok channel as Found. Number.isInteger(id) && id >= 0 && id < 2 ** 32, returning err, closes it. fromId in packages/nfts/src/paging.ts line 93 needs the same check, so the predicate belongs there.
| const from = usable(options.fromId) ?? 0; | ||
| return { | ||
| limit: Math.min(Math.max(0, Math.trunc(asked)), MAX_PAGE_LIMIT), | ||
| fromId: Math.max(0, Math.trunc(from)), |
There was a problem hiding this comment.
fromId gets a floor but no ceiling, and the id space is u32. PAPI's encoder truncates, so getCollections(chain, { fromId: 2 ** 32 }) on a two-collection chain returns those two as ids 4294967296 and 4294967297, with nextId: null.
| // Widen by the density seen so far, so a sparse range converges in a | ||
| // couple of reads rather than one read per gap. | ||
| const density = Math.max(kept.length / Math.max(scanned, 1), 1 / SCAN_BUDGET_FACTOR); | ||
| const width = Math.min(Math.ceil(want / density), want * SCAN_BUDGET_FACTOR); |
There was a problem hiding this comment.
This width is capped by the scan budget but not by MAX_PAGE_LIMIT, so one probe can ask for 15,000 keys at limit: 1000 on a sparse space. Since getValues is Promise.all(keys.map(getValue)), that is 15,000 concurrent operations against a documented bound of 1,000.
MAX_PAGE_LIMIT's own doc needs rewording too, since it names one bound where there are three.
| // The one branch: named keys for the page, or the whole collection's item | ||
| // metadata when the open bag was asked for. | ||
| const overrides = options.attributes | ||
| ? await readAllKeys(query, id, at) |
There was a problem hiding this comment.
readAllKeys runs even when the page found no items, so a page past the end still prefix-scans the whole collection to return nothing. readTypedKeys on the next line has that guard and this branch does not, so please skip the metadata read when filled.kept is empty. Separately, attributes is a per-page option with a per-collection cost, so a walk repeats the full scan every page. A getCollectionAttributes(chain, id, { at }) read would let a caller pay it once.
| * } | ||
| * if (page.nextId === null) break; | ||
| * const next = await getCollections(chain, { limit: 100, fromId: page.nextId, at }); | ||
| * if (!next.ok) break; |
There was a problem hiding this comment.
This treats a failed page as the end of the walk, so a caller copying it renders a partial list as complete. PAPI holds a pinned block for one storage read only, so a long at-pinned walk will lose it. Please report the failure rather than break, and say in the at doc that a walk has to re-pin. Four more sites: items.ts line 167, SKILL.md lines 98 and 154, and the changeset line 64.
| ItemSelection, | ||
| ClaimableCollection, | ||
| Collection, | ||
| RawBytes, |
There was a problem hiding this comment.
Four types in NftsChain's signature are not exported: RawCollection, RawItemDef, RawMinter and RawMetadataEntry. Importing them fails, and RawCollection gets the suggestion "Did you mean 'Collection'?", which is a different type and would give a wrong implementation. Adding them here is additive and clears four typedoc warnings.
| * characters. Anything below U+0020 that is not tab/newline/carriage-return | ||
| * disqualifies it, as does a lone U+FFFD. | ||
| */ | ||
| function isText(decoded: string): boolean { |
There was a problem hiding this comment.
The doc above asks whether the string is safe for a UI, but the check covers only C0 controls, U+007F and U+FFFD. C1 controls, bidi overrides and zero-width characters pass, so a name containing U+202E renders as a different name. Metadata is author-supplied, so please either narrow the doc or widen the check.
| * page. That costs an extra record read where holes appear and nothing else: a | ||
| * hole never gets a name or registry lookup. A page is short only at the end of | ||
| * the id space, or when a mostly-deleted range hits | ||
| * {@link SCAN_BUDGET_FACTOR}; `nextId === null` is the only end signal. |
There was a problem hiding this comment.
SCAN_BUDGET_FACTOR is not exported, so this link does not resolve and docs:extract warns. Exporting it beside DEFAULT_PAGE_LIMIT and MAX_PAGE_LIMIT fixes the link and gives callers the second half of the paging bound.
|
|
||
| const { limit, fromId } = pageBounds(options); | ||
|
|
||
| if (limit === 0) { |
There was a problem hiding this comment.
fillByIdWindow already handles limit: 0 and returns nextId: null, so this shortcut and its twin in readPage duplicate the helper. getCollectionItems has none and is correct without one. Dropping both costs one getValues([]), which does no network work.
|
|
||
| // Both concurrently, for exactly the ids being returned — `chain.ts` has | ||
| // what a multi-key read costs. | ||
| const [records, names] = await Promise.all([ |
There was a problem hiding this comment.
readPage returns early when its window found nothing and this read does not, so an empty registry page still calls Collections.getValues([]). The same early return keeps the two in step. Separately, the three option interfaces each redeclare limit, fromId, at and signal; a shared PagedReadOptions would document them once.
2 participants
Part of #318.
Adds
@parity/product-sdk-nfts: catalogue reads of theScarcitypallet on Asset Hub. Every value comes from storage, pinned to one finalized block per call.Pagination
DEFAULT_PAGE_LIMIT(100) and caps atMAX_PAGE_LIMIT(1000) - both exported.limit,fromIdidCeiling,nextIdPagination walks the sequential
u32index space each read enumerates, which is sound because the runtime guarantees the shape: ids and indices come from counters that only move forward, and bothdelete_collectionanddelete_itemdocument that identifiers are never reused.Shared options
Every read takes these.
getCollectionItemsaddsattributes.limitnumber1001000; a larger request is capped by max limit rather than fail, andnextIdstill reports where the page stopped.fromIdnumber0nextId.atFinalizedSnapshotsignalAbortSignalatis what makes a walk coherent. Without it every call pins its own finalized block, right for unrelated questions, wrong for one question asked in pages, since a walk over its own snapshots is not a walk of any single chain state. Pass another result'satstraight back in. It also makes two reads agree: the registry and the full listing, or a listing and a catalogue, at one block.API functions
Each pins its block first through
raw.assetHub.getFinalizedBlock()(unless givenat), then addresses storage at that hash, so all values in one result come from one block. Each returns aResult, per the SDK-wide error model.getCollections(chain, options?)→
Result<CollectionsResult, ProductNftsError>Every collection on chain, ascending by id, a page at a time.
selection: nullmeans the collection exists but accepts no claims.raw.assetHub.getFinalizedBlock()atwas givenquery.Scarcity.NextCollectionId.getValue(at)query.Scarcity.Collections.getValues(window)query.NftClaims.CollectionMinters.getValues(page ids)selectionfor the pagequery.Scarcity.CollectionMetadata.getValues(page ids × "name")getClaimableCollections(chain, options?)→
Result<ClaimableCollectionsResult, ProductNftsError>The subset
getCollectionsfilters: collections registered to accept claims, ascending by id.raw.assetHub.getFinalizedBlock()atwas givenquery.Scarcity.NextCollectionId.getValue(at)query.NftClaims.CollectionMinters.getValues(window)query.Scarcity.Collections.getValues(page ids)query.Scarcity.CollectionMetadata.getValues(page ids × "name")getCollectionItems(chain, id, options?)→
Result<CollectionItemsResult, ProductNftsError>One page of a collection's item catalogue, items ascending by index. Applies no registry filter, so it reads a collection that accepts no claims just as well. A collection nobody created is not an error; it rides the
okchannel asNotFound.idCeilingcounts every item ever defined here (indices are never reused);collection.itemCountcounts the ones still alive. They diverge permanently once anything is deleted.Extra option:
attributesbooleanfalseraw.assetHub.getFinalizedBlock()atwas givenquery.Scarcity.Collections.getValue(id, at)NotFoundtestquery.Scarcity.CollectionMetadata.getEntries(id, at)query.Scarcity.ItemDefs.getValues(window)query.Scarcity.ItemMetadata.getValues(page × 3 named keys)name,image,rarity— whenattributesis offquery.Scarcity.ItemMetadata.getEntries(id, at)attributesis onattributesisRecord<string, string> | null, andnullmeans "not fetched". An empty object would claim the item carries no metadata, which is a different statement. The typed fields (name,image,rarity) are keys this package can name, so a page fetches them for its whole window with one exact-key read; the bag's keys are open by definition, so there is nothing to ask for by name and filling it means scanning the collection. Collection-level defaults are inherited either way.Other changes
packages/nfts/src/chain.ts— the client the reads take, typed structurally by the six storage entries they touch rather than by naming a descriptor, so no genesis hash is pinned to read a catalogue. It asks for exactly the accessors the reads use: making every read paged removed four of them. An app pruning its own descriptors must whitelist all six,Scarcity.NextCollectionIdincluded.packages/nfts/src/paging.ts—DEFAULT_PAGE_LIMIT,MAX_PAGE_LIMIT,pageBounds, andfillByIdWindow, shared by all three reads. They differ only in what makes an index interesting (a collection record, a minter entry, an item definition), so the density widening, scan budget and cursor semantics live in one place.packages/nfts/src/metadata.ts— the decode convention. Metadata is untypedVec<u8>→Vec<u8>in three layers, each overriding the last; a catalogue read merges the first two. Values decode as UTF-8 when the bytes are readable text and0x-hex otherwise. Exact-key reads take a plainUint8Array— PAPI 2.x generates[number, Uint8Array]for thisVec<u8>key.packages/nfts/src/errors.ts—NftsChainEntryErrornames the storage entry a read could not reach, carries it onentry, and keeps PAPI's error as thecause.packages/sdk/*— the@parity/product-sdk/nftssubpath, plussrc/nfts/contract.test.ts: compile-time assertions that a realgetChainAPIclient satisfiesNftsChain, with devnet Asset Hub as a negative control. These run underpnpm typecheck, not vitest.examples/nfts-demo/*— demo app plus Playwright specs. The panel forgetCollectionsrenders the ids withselection: nullnext to the registry's, because the gap between the two lists is the thing worth seeing on live data. It also walks the id space in pages of 2 withatpinned, and the spec asserts two things a type cannot: that names from a small-page walk match a single larger page (parameter bivariance means the contract check would accept a wrong exact-key type, and a wrong key silently returnsnullfor every name), and that the whole walk touched exactly one block.skills/product-sdk-nfts/SKILL.md,CLAUDE.md,.claude-plugin/marketplace.json,README.md,.changeset/config.json— the skill and its registration, the new package's row in the package table, and@parity/product-sdk-nfts-demoadded to the changeset ignore list alongside the other demos. Plus one changeset.Notes
Nothing on chain declares the metadata keys or the value types, so
name,imageandrarityare a convention this package applies, not a schema it enforces; every other key is reachable throughattributes. Unconfirmed with the pallet team.imageRefreports the same bytes ashexand astext(nullwhen unreadable): one deployment stores a 32-byte content digest there, another an ASCII CID, and nothing on chain says which, so the caller picks.transferabilityis not returned. It traces topallet_nfts'CollectionSetting::TransferableItemsand has no source inScarcity, not inItemDefs, not in any metadata key the live chain carries.