chore: 🤖 address the review of the holdings, NFTs and metadata schema - #360
Open
prashantasdeveloper wants to merge 9 commits into
Open
prashantasdeveloper wants to merge 9 commits into
prashantasdeveloper wants to merge 9 commits into
Conversation
S1 from the review of #352. asset::transfer_asset emits CreatedAssetTransfer after settlement::transfer_funds has returned (base_transfer_asset, v8.0.0), so the movement is either already recorded or has not happened: - same identity: FundsTransferred came first and mapAssetMovement wrote the row; - different identities, executed at once: AssetBalanceUpdated wrote it; - receiver affirmation pending: nothing has moved, and AssetBalanceUpdated writes it when the receiver affirms, in a later block or never. The extra row, keyed by its own event, duplicated the first two and recorded the third as a completed movement that stayed even if the receiver rejected it. Its identities were null and isInternalTransfer false, so identity filters missed it while counts and sums included it. Traced on testnet at blocks 25,888,006 / 056 / 071 / 086 / 147. The event is left unhandled. Its memo is already on the FundsTransferred row and on the instruction's InstructionCreated.
S2 from the review of #352. holderCount was only maintained by applyHoldingDelta, on the fungible path, so every NFT collection reported 0 holders, and its description ("where Holding rows cross zero") did not match what it counts. It counts identities, which every holder has, so the NFT path now does the same where an identity's NftHolder rollup goes between empty and non-empty: on issue, redemption, and a transfer between two identities. A move between one identity's portfolios changes nothing. The description now says it counts identities and that a key with no identity is not counted.
S3 from the review of #352. mintNft wrote `metadata: []` and nothing else ever wrote the field, but SubQuery gives every @jsonField a GIN index unless it is declared indexed: false, so every NFT mint, move and burn maintained an index over an empty array. The field is new in this redesign and has no consumer yet, so it is removed with its NftMetadataEntry type rather than kept empty. Filling it from the chain means a metadata read per token, which a bulk mint of thousands cannot afford; it can come back once that is designed.
S4 from the review of #352. Two index facts from the installed SubQuery (@subql/utils, @subql/node-core 19.0.0): every relation gets an index of its own, and in historical mode addBlockRangeColumnToIndexes turns every index into a (…, _block_range) GiST and drops `unique`, since GiST cannot enforce it. Plan 13 measured GiST maintenance as the main cost of historical mode. - Asset.assetId @index(unique: true) held the same value as `id`, enforced nothing, and added a GiST index to one of the most updated tables. The column stays for the SDK; the index goes. Nothing in the indexer looks an asset up by it. - Nft @compositeIndexes(["asset", "nftId"]) duplicated `id` (assetId/padId(nftId)) for point lookups and the automatic `asset` index for scans. Nothing looks a token up by it. Holding's two composite indexes stay: they serve the consumer's asset-by-identity and portfolio-by-asset queries, not lookups another index already covers. burnedEvent is a relation and so is indexed automatically, which the #352 description said it was not.
S5 from the review of #352. A metadata value's lock status is Unlocked | Locked | LockedUntil(Moment), and detailFrom collapsed LockedUntil to isLocked: true and threw the moment away. The chain emits nothing when that lock ends, so the row stayed locked for good with nothing to say when the value became editable. AssetMetadata gains lockedUntil (UTC, like expiry), set from LockedUntil and cleared by any other status or by MetadataValueDeleted; isLocked's description says to compare it with the current time. The review's other two points on this entity hold without a code change, and are now documented: a global key's name is copied once, but the chain has no rename for global metadata names; and GlobalMetadataSpecUpdated's lookup by the non-unique name index is exact, because register_asset_metadata_global_type refuses a name AssetMetadataGlobalNameToKey already holds (v8.0.0).
S6 from the review of #352. Holding rows are never removed when amount and nftCount reach zero, which is right under historical state, but a query such as holdings(filter: { portfolioId: { equalTo: "did/1" } }) then returns past holdings too. The entity description now says so and gives the filters for current holdings.
S7 from the review of #352. getHolding set identityId only when it created the row, and rows are never removed. An account cannot change identity while it holds an asset (a non-zero balance raises AccountKeyRefCount, and unlinking the key fails with AccountKeyIsBeingUsed), but it can once the balance is back to zero. If it then received the same asset again, the old row came back with the old DID while applyHoldingDelta sent the delta to the new DID's AssetHolder rollup, and SUM(Holding.amount) per (asset, identity) = AssetHolder.amount failed for both identities. identityId is now set from the resolved holder on every touch. Both callers save the row next, so it costs no extra write.
…tity S8 from the review of #352. getAssetAllowance resolved owner and spender with getOrCreateAccount, which creates a row only for a key with an identity, and skips multisig signer keys. asset::approve never checks the spender, so an allowance approved to a key with no identity, and never spent, pointed AssetAllowance.spender at an Account that did not exist, and any query selecting spender { … } failed with "Cannot return null for non-nullable field". Both sides now go through ledgerAccount, which falls back to a bare Account row (address and key type) exactly as the POLYX ledger already does for pallet and multisig addresses. The field stays non-null, so no schema change.
S9 from the review of #352. Plan 03's Asset diff has metadata: [AssetMetadata!]! @derivedFrom(field: "asset"); the PR added holdings and nfts beside it but left this one out. A derived field adds no column and no index, so an asset's metadata can now be read from the asset instead of a separate assetMetadata query.
prashantasdeveloper
marked this pull request as ready for review
September 18, 2026 11:15
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes the nine findings from Francis's review of #352 (holdings, NFTs, movement ledger). It is stacked on #359 (
redesign/10-throughput). #352 is further down the stack, so every finding was first checked against this tip (5b55ab0) before being fixed. All nine were still present; none had been fixed higher up.Base:
redesign/10-throughput. There is one commit per finding. Everything these commits fix was added earlier in this unreleased stack, so they are typedchore(anddocsfor S6), notfix/feat, to keep the release notes from listing a feature and then its fix.CreatedAssetTransferwrites anAssetTransactionthat duplicates the real movement, or records one that hasn't happened yetbase_transfer_assetemits it only aftersettlement::transfer_fundsreturns. The movement is already recorded byFundsTransferredorAssetBalanceUpdated, or, when the receiver's affirmation is pending, it hasn't happened yet. The memo is already on theFundsTransferredrow and on the instruction.holderCountis always 0 for NFT collectionsNftHolderrollup goes between empty and non-empty. The docstring now says the field counts identities.Nft.metadatais never filled in but pays for a GIN indexNftMetadataEntrytype are removed. The field is new in this redesign, so it has no consumer yet.@index(unique: true)onAsset.assetId(the column stays) andNft's["asset","nftId"]composite index.Holding's composite indexes stay.LockedUntilloses its timeAssetMetadata.lockedUntil. The other two points hold without a code change, because the chain never renames a global metadata name and never registers one twice. This is now documented.Holdingrows outlive a zero balance, undocumentedHoldingrow keeps its old identityidentityIdis now restamped on every touch.AssetAllowance.spendercan point at a missingAccountledgerAccount, which falls back to a bareAccountrow. That helper was added to the stack after the review, so the field stays non-null and there is no schema change.Asset.metadataderived field missingConsumer impact
Nft.metadataand theNftMetadataEntrytype. Neither was ever released, so nothing breaks.AssetMetadata.lockedUntilandAsset.metadata.Asset.holderCountis now non-zero for NFT collections.assetTransactionsno longer returns the duplicate or pending rows fromCreatedAssetTransfer.Asset.assetIdindex and theNft (asset, nftId)composite index are gone. Look an asset up byid, which holds the same value.schema.graphqlchanged, so this needs a reindex from genesis.AssetAllowance.spendertype.One correction to #352's description:
burnedEvent(formerlyburnedBlock) is a relation, so SubQuery indexes it automatically. The description said it wasn't indexed.Verification
yarn codegen && yarn typecheck && yarn lint && yarn test:unit(679 tests), plusyarn buildandcheck-handlers.LockedUntilmoment;