Skip to content

fix: batch NFT ownership checks via Multicall3#8281

Open
juanmigdr wants to merge 8 commits intomainfrom
fix/do-not-make-On-calls-fo-refresh
Open

fix: batch NFT ownership checks via Multicall3#8281
juanmigdr wants to merge 8 commits intomainfrom
fix/do-not-make-On-calls-fo-refresh

Conversation

@juanmigdr
Copy link
Copy Markdown
Member

@juanmigdr juanmigdr commented Mar 24, 2026

Explanation

  • Replace individual AssetsContractController messenger calls (getERC721OwnerOf, getERC1155BalanceOf) with a new getNftOwnershipForMultipleNfts utility that batches ERC-721 ownerOf and ERC-1155 balanceOf calls through Multicall3's aggregate3, falling back to individual RPC calls on unsupported chains or when multicall fails.
  • Remove checkAndUpdateSingleNftOwnershipStatus in favor of checkAndUpdateAllNftsOwnershipStatus, which now batches all NFTs in a single pass.
  • Add an optional standard parameter to isNftOwner so callers that already know the token standard skip redundant subcalls.

Breaking Changes

  • checkAndUpdateSingleNftOwnershipStatus removed — use checkAndUpdateAllNftsOwnershipStatus instead.
  • AllowedActions narrowedAssetsContractController:getERC721OwnerOf and AssetsContractController:getERC1155BalanceOf are no longer required by NftController's messenger. Consumers constructing the messenger must remove these from their allowed actions list.

References

https://consensyssoftware.atlassian.net/browse/ASSETS-2959

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've communicated my changes to consumers by updating changelogs for packages I've changed
  • I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

Note

Medium Risk
Changes core NFT ownership verification logic and introduces new Multicall3 batching with fallbacks; incorrect handling could mark NFTs as owned/unowned or fail watch/add flows. Also includes breaking API/messenger surface changes that require consumer updates.

Overview
Batches NFT ownership checks via Multicall3. NftController.isNftOwner and checkAndUpdateAllNftsOwnershipStatus now call new getNftOwnershipForMultipleNfts to batch ownerOf/balanceOf calls into fewer RPC requests, with fallback to individual RPC calls when Multicall3 is unavailable or fails.

Breaking API changes. Removes NftController.checkAndUpdateSingleNftOwnershipStatus and drops AssetsContractController:getERC721OwnerOf/getERC1155BalanceOf from NftController AllowedActions; callers must stop wiring these messenger handlers. Updates tests to mock the new multicall ownership path and adds comprehensive unit coverage for the new multicall ownership helper, including standard-aware short-circuiting and unsupported/unknown-standard behavior.

Reviewed by Cursor Bugbot for commit 0ed9859. Bugbot is set up for automated code reviews on this repo. Configure here.

@juanmigdr juanmigdr requested a review from a team as a code owner March 24, 2026 12:36
return acc;
},
[],
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Undefined standard silently skips NFT ownership checks

Medium Severity

The filter to exclude NFTs with unrecognized standards (hasExplicitNonStandard) incorrectly treats undefined the same as an explicitly unrecognized standard like "UNKNOWN". NftMetadata.standard can be undefined at runtime (e.g., NFTs from the API when kind is falsy, or older persisted state), even though TypeScript types it as string | null. Since undefined !== null is true and normalizeNftStandard(undefined) returns null, these NFTs are silently excluded from ownership checks — they'll never be marked as no-longer-owned. The guard nft.standard !== null needs to also account for undefined (e.g., nft.standard != null using loose equality).

Additional Locations (1)
Fix in Cursor Fix in Web

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 0ed9859. Configure here.

);
results[nftIndex].isOwned = new BN(balance.toString()).gt(new BN(0));
}
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing try-catch around multicall result decoding breaks batch

Low Severity

In getNftOwnershipViaMulticall, the forEach loop decoding multicall return data has no try-catch around decodeFunctionResult. If any single subcall returns success: true with malformed ABI data (e.g., a proxy returning unexpected bytes), the decode throws and the entire batch is lost, forcing a full fallback to individual calls for all NFTs. The individual path (getNftOwnershipIndividually) correctly wraps each decode in try-catch. Adding the same per-result error handling in the multicall path would let valid results be preserved instead of discarding the whole batch.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 0ed9859. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant