feat(revm): initial revive support#7
Open
hassnian wants to merge 9 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdd a Revive EVM contract indexing subsystem (ABIs, viem client, decoders, handlers), collectionKind tracking and migration, Paseo chain support and environment flags, processor subscriptions for EVM logs, mapping wiring/getter routing changes, tests, a SubSquid manifest, and viem dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant Processor
participant ReviveDispatcher as Revive Dispatcher
participant EVMClient as Revive EVM Client
participant CollectionHandler as Collection Handler
participant MintHandler as Mint Handler
participant TransferHandler as Transfer Handler
participant BurnHandler as Burn Handler
participant Store as Database Store
Processor->>ReviveDispatcher: Revive.ContractEmitted (topic0, data)
ReviveDispatcher->>ReviveDispatcher: inspect topic0 / decode
alt CollectionRegistered
ReviveDispatcher->>CollectionHandler: handleCollectionCreate(ctx)
CollectionHandler->>EVMClient: readRegistryCollection(index, block)
EVMClient-->>CollectionHandler: collection data
CollectionHandler->>EVMClient: readContractUri(addr, block)
EVMClient-->>CollectionHandler: uri
CollectionHandler->>Store: save CollectionEntity
else ERC721 Transfer
ReviveDispatcher->>ReviveDispatcher: classify transfer (mint/burn/transfer)
alt Mint
ReviveDispatcher->>MintHandler: handleTokenMint(ctx, transfer)
MintHandler->>EVMClient: readTokenUri(addr, tokenId, block)
EVMClient-->>MintHandler: token uri
MintHandler->>Store: save NFTEntity + update CollectionEntity
else Burn
ReviveDispatcher->>BurnHandler: handleTokenBurn(ctx, transfer)
BurnHandler->>Store: mark NFT burned + update collection
else Transfer
ReviveDispatcher->>TransferHandler: handleTokenTransfer(ctx, transfer)
TransferHandler->>Store: update NFT owner + recalc distribution
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/processor.ts (1)
83-87:⚠️ Potential issue | 🟡 MinorDuplicate event subscriptions detected.
Lines 83-84 and 86-87 register the same events (
NonFungible.clearCollectionMetadataandNonFungible.setCollectionMetadata) twice. This appears to be a pre-existing issue but may cause events to be processed multiple times.🐛 Proposed fix to remove duplicates
processor.addEvent({ name: [NonFungible.clearCollectionMetadata], call: true, extrinsic: true }) processor.addEvent({ name: [NonFungible.setCollectionMetadata], call: true, extrinsic: true }) processor.addEvent({ name: [NonFungible.setCollectionMaxSupply], call: true, extrinsic: true }) -processor.addEvent({ name: [NonFungible.clearCollectionMetadata], call: true, extrinsic: true }) -processor.addEvent({ name: [NonFungible.setCollectionMetadata], call: true, extrinsic: true }) processor.addEvent({ name: [NonFungible.destroyCollection], call: true, extrinsic: true })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/processor.ts` around lines 83 - 87, The processor is registering duplicate event subscriptions: remove the repeated calls to processor.addEvent for NonFungible.clearCollectionMetadata and NonFungible.setCollectionMetadata so each event is only registered once; update the event registration block that calls processor.addEvent (the calls with NonFungible.clearCollectionMetadata and NonFungible.setCollectionMetadata) to leave a single entry per event and keep other events like NonFungible.setCollectionMaxSupply unchanged.
🧹 Nitpick comments (6)
tests/revive.test.ts (1)
23-28: Consider adding edge case tests for error handling.The helper functions include validation (e.g.,
getReviveLogPayloadthrows on invalid payloads). Consider adding tests for:
- Invalid/malformed event payloads
- Missing or empty topics array
- Non-string contract addresses
This would ensure error paths are exercised and provide regression protection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/revive.test.ts` around lines 23 - 28, Add unit tests in revive.test.ts covering error paths for the helper functions: call getReviveLogPayload with malformed/invalid payloads and assert it throws, supply an event object with missing or empty topics to functions that expect topics and assert they throw or return the expected error, and pass non-string contract addresses to isMintTransfer/isBurnTransfer (or any address-validated helper) and assert they reject/throw; target the functions getReviveLogPayload, isMintTransfer, and isBurnTransfer when writing these tests so error handling and input validation are exercised.snack.yaml (1)
20-25: Potential confusion with STARTING_BLOCK env var.The
STARTING_BLOCKenvironment variable (5650083) appears to map to_NFT_STARTING_BLOCKinsrc/environment.ts, while the actualSTARTING_BLOCKconstant is computed asMath.min(PALLET_STARTING_BLOCK, REVIVE_START_BLOCK)when Revive is enabled. SinceREVIVE_START_BLOCK(5649754) is earlier than the configuredSTARTING_BLOCK(5650083), the processor will actually start from block 5649754.Consider either:
- Renaming the env var to
NFT_STARTING_BLOCKfor clarity, or- Adding a comment explaining the relationship between these values
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@snack.yaml` around lines 20 - 25, The configured STARTING_BLOCK env var (5650083) is mapped to _NFT_STARTING_BLOCK in src/environment.ts but the effective STARTING_BLOCK used when revive is enabled is computed as Math.min(PALLET_STARTING_BLOCK, REVIVE_START_BLOCK), which causes the processor to actually start at REVIVE_START_BLOCK (5649754); update the code or docs to avoid confusion by either renaming the env var to NFT_STARTING_BLOCK (and updating references to _NFT_STARTING_BLOCK) or add a clear comment beside the STARTING_BLOCK/_NFT_STARTING_BLOCK, PALLET_STARTING_BLOCK, and REVIVE_START_BLOCK declarations explaining that the runtime STARTING_BLOCK is Math.min(PALLET_STARTING_BLOCK, REVIVE_START_BLOCK) when REVIVE_ENABLED is true so users know which value wins.src/mappings/revive/index.ts (1)
30-31: Consider adding debug logging for unhandled topics.The silent return on unmatched topics could make debugging difficult if unexpected events are delivered to this handler. A debug log would help identify configuration issues or new event types.
🔧 Optional: Add debug logging
+import { logger } from '@kodadot1/metasquid/logger' + // ... in handleReviveEvent default: + logger.debug(`Unhandled Revive topic: ${topic0Of(context.event.args)}`) return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mappings/revive/index.ts` around lines 30 - 31, The switch's default branch currently does a silent "return" for unmatched topics; update that branch in src/mappings/revive/index.ts to emit a debug log (use the module's existing logger variable, e.g., logger or processLogger, or fallback to console.debug) that records the topic name and a short payload summary so unexpected events are visible during debugging; locate the switch over topic (the default: return case) and replace the silent return with a logger.debug/info call including the topic and relevant metadata (and then return).src/mappings/revive/client.ts (2)
38-57: Minor: Redundant type assertion can be simplified.The type assertion on lines 44-49 is unnecessary since TypeScript already narrows the type in the else branch. Consider simplifying:
♻️ Suggested simplification
function asRegistryCollection(result: RegistryCollectionResult) { if (Array.isArray(result)) { const [collectionAddress, deployer, name, createdAt] = result return { collectionAddress, deployer, name, createdAt } } - const value = result as { - collectionAddress: Address - deployer: Address - name: string - createdAt: bigint - } - return { - collectionAddress: value.collectionAddress, - deployer: value.deployer, - name: value.name, - createdAt: value.createdAt, + collectionAddress: result.collectionAddress, + deployer: result.deployer, + name: result.name, + createdAt: result.createdAt, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mappings/revive/client.ts` around lines 38 - 57, The asRegistryCollection function uses an unnecessary type assertion for the else branch; remove the redundant "as { collectionAddress: Address ... }" cast and directly access properties on result (which TypeScript has already narrowed to RegistryCollectionResult in the non-array branch). Update the else/return path in asRegistryCollection to read result.collectionAddress, result.deployer, result.name, and result.createdAt instead of creating the intermediate asserted value.
78-122: Consider logging RPC errors for debugging.The empty catch blocks silently swallow all errors, which could hide RPC connectivity issues or unexpected failures. Consider adding debug-level logging:
♻️ Suggested improvement
+import { warn } from '../utils/logger' + export async function readCollectionOwner(address: string, blockNumber?: bigint): Promise<string> { try { const owner = (await getClient().readContract({ address: address as Address, abi: erc721Abi, functionName: 'owner', blockNumber, })) as Hex return normalizeEvmAddress(owner) - } catch { + } catch (error) { + // owner() is optional for ERC-721, return empty on failure return '' } }This helps distinguish between "contract doesn't implement this method" vs "RPC is down" scenarios during debugging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mappings/revive/client.ts` around lines 78 - 122, The three functions readCollectionOwner, readContractUri and readTokenUri currently swallow all errors in their catch blocks; update each catch to log the caught error (e.g., catch (err) { logger.debug?.('readCollectionOwner error', { address, tokenId?, err }) } or fallback to console.debug) including function name, address, tokenId when relevant, and the raw error/stack before returning '' so RPC/connectivity vs missing-method cases can be distinguished; use the existing logging utility if available (or console.debug) and include blockNumber in the log when present.src/mappings/revive/createCollection.ts (1)
27-50: Consider handling re-registration of existing collections.Using
getOrCreatefollowed by unconditionally overwriting all fields (includingnftCount,supply,ownerCount,volume) will reset these counters if a collection is re-registered. If re-registration is not expected, consider adding a check like inmint.ts:const existing = await getOptional<CE>(context.store, CE, id) if (existing) { warn(OPERATION, `Collection ${id} already exists`) return }If re-registration is valid, this is fine as-is, but worth documenting the intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mappings/revive/createCollection.ts` around lines 27 - 50, The current use of getOrCreate followed by unconditionally setting every field on final (fields like nftCount, supply, ownerCount, volume) will reset counters on re-registration; update createCollection logic to first check for an existing collection via getOptional<CE>(context.store, CE, id) and, if it exists, log a warning (use the same OPERATION warning pattern as in mint.ts) and return early to avoid overwriting existing counters, or alternatively, if re-registration must update some fields, merge values by preserving existing numeric counters (nftCount, supply, ownerCount, volume) from the existing record instead of overwriting them when assigning to final; locate the logic around getOrCreate and final to implement the chosen approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 33-35: The package.json currently lists "engines.node" as ">=16"
while the dependency "viem" is pinned to "^2.47.6" which requires Node.js >=18;
update package.json to either raise "engines.node" to ">=18" (preferred) or
change the "viem" dependency to a version compatible with Node 16, and update
any README/CI configs accordingly; ensure you modify the "engines.node" field
and/or the "viem" entry in dependencies to keep versions consistent.
In `@src/mappings/index.ts`:
- Around line 228-232: The current dual-bound check using STARTING_BLOCK and
_NFT_STARTING_BLOCK can skip forceAssets on the very first batch; update the
condition in the block that computes firstBlock and lastBlockInBatch to only
check STARTING_BLOCK <= firstBlock (and assetsForced === false) before calling
await forceAssets(ctx), leaving assetsForced logic intact to ensure it runs only
once; keep references to firstBlock, lastBlockInBatch, STARTING_BLOCK,
_NFT_STARTING_BLOCK, forceAssets, and assetsForced so the change is localized
and obvious.
In `@src/mappings/revive/burn.ts`:
- Around line 21-25: The burn handlers are passing entity.currentOwner as the
third (newOwner) argument to calculateCollectionOwnerCountAndDistribution, which
is wrong for burns; update the call in the burn handler
(calculateCollectionOwnerCountAndDistribution(...)) to either omit the third
parameter entirely or shift entity.currentOwner into the fourth parameter named
originalOwner so that newOwner is not set for burn operations; locate and fix
the same issue across the three burn handlers (nfts, uniques, revive) to ensure
calculateCollectionOwnerCountAndDistribution receives no newOwner for burns and
the owner being removed is passed as originalOwner when needed.
In `@src/mappings/revive/helpers.ts`:
- Around line 99-103: The guard in getReviveLogPayload is too weak: it only
checks Array.isArray(payload.topics) but allows an empty array which later
breaks when cast to [Hex, ...Hex[]]; update the validation to ensure
payload.topics is a non-empty array (e.g., check payload.topics.length > 0) and
optionally validate payload.topics[0] is a string/hex before returning the
ReviveLogPayload to prevent downstream decode failures.
- Around line 54-59: The toReviveBaseCall function uses Date.now() as a
nondeterministic fallback for timestamp; replace that with a deterministic
fallback (e.g., new Date(0)) or explicitly throw when context.block.timestamp is
missing. Update the return in toReviveBaseCall to construct timestamp from
context.block.timestamp when present and otherwise use a fixed value (new
Date(0)) or throw an error to avoid nondeterministic persisted values; ensure
you modify the BaseCall creation in toReviveBaseCall accordingly and adjust any
related tests/consumers.
In `@src/mappings/revive/transfer.ts`:
- Around line 20-22: The transfer handler in revive mapping fails to clear stale
listing prices; when updating ownership in the transfer logic (where you set
entity.currentOwner = transfer.to and entity.updatedAt = ...), also set
entity.price = null to mirror the uniques handler behavior and prevent old
prices from persisting; update the transfer function in
src/mappings/revive/transfer.ts to clear entity.price after assigning the new
owner.
- Around line 37-44: The revive transfer handler currently calls createEvent
with entity.currentOwner (the new owner), causing inconsistent semantics; change
the argument passed as the owner to the previous owner (oldOwner) to match
uniques and nfts handlers. Locate the createEvent invocation that uses
Interaction.SEND and toReviveBaseCall and replace the entity.currentOwner
parameter with the oldOwner variable (the sender from the transfer context) so
revive events record the prior owner like the other transfer handlers.
---
Outside diff comments:
In `@src/processor.ts`:
- Around line 83-87: The processor is registering duplicate event subscriptions:
remove the repeated calls to processor.addEvent for
NonFungible.clearCollectionMetadata and NonFungible.setCollectionMetadata so
each event is only registered once; update the event registration block that
calls processor.addEvent (the calls with NonFungible.clearCollectionMetadata and
NonFungible.setCollectionMetadata) to leave a single entry per event and keep
other events like NonFungible.setCollectionMaxSupply unchanged.
---
Nitpick comments:
In `@snack.yaml`:
- Around line 20-25: The configured STARTING_BLOCK env var (5650083) is mapped
to _NFT_STARTING_BLOCK in src/environment.ts but the effective STARTING_BLOCK
used when revive is enabled is computed as Math.min(PALLET_STARTING_BLOCK,
REVIVE_START_BLOCK), which causes the processor to actually start at
REVIVE_START_BLOCK (5649754); update the code or docs to avoid confusion by
either renaming the env var to NFT_STARTING_BLOCK (and updating references to
_NFT_STARTING_BLOCK) or add a clear comment beside the
STARTING_BLOCK/_NFT_STARTING_BLOCK, PALLET_STARTING_BLOCK, and
REVIVE_START_BLOCK declarations explaining that the runtime STARTING_BLOCK is
Math.min(PALLET_STARTING_BLOCK, REVIVE_START_BLOCK) when REVIVE_ENABLED is true
so users know which value wins.
In `@src/mappings/revive/client.ts`:
- Around line 38-57: The asRegistryCollection function uses an unnecessary type
assertion for the else branch; remove the redundant "as { collectionAddress:
Address ... }" cast and directly access properties on result (which TypeScript
has already narrowed to RegistryCollectionResult in the non-array branch).
Update the else/return path in asRegistryCollection to read
result.collectionAddress, result.deployer, result.name, and result.createdAt
instead of creating the intermediate asserted value.
- Around line 78-122: The three functions readCollectionOwner, readContractUri
and readTokenUri currently swallow all errors in their catch blocks; update each
catch to log the caught error (e.g., catch (err) {
logger.debug?.('readCollectionOwner error', { address, tokenId?, err }) } or
fallback to console.debug) including function name, address, tokenId when
relevant, and the raw error/stack before returning '' so RPC/connectivity vs
missing-method cases can be distinguished; use the existing logging utility if
available (or console.debug) and include blockNumber in the log when present.
In `@src/mappings/revive/createCollection.ts`:
- Around line 27-50: The current use of getOrCreate followed by unconditionally
setting every field on final (fields like nftCount, supply, ownerCount, volume)
will reset counters on re-registration; update createCollection logic to first
check for an existing collection via getOptional<CE>(context.store, CE, id) and,
if it exists, log a warning (use the same OPERATION warning pattern as in
mint.ts) and return early to avoid overwriting existing counters, or
alternatively, if re-registration must update some fields, merge values by
preserving existing numeric counters (nftCount, supply, ownerCount, volume) from
the existing record instead of overwriting them when assigning to final; locate
the logic around getOrCreate and final to implement the chosen approach.
In `@src/mappings/revive/index.ts`:
- Around line 30-31: The switch's default branch currently does a silent
"return" for unmatched topics; update that branch in
src/mappings/revive/index.ts to emit a debug log (use the module's existing
logger variable, e.g., logger or processLogger, or fallback to console.debug)
that records the topic name and a short payload summary so unexpected events are
visible during debugging; locate the switch over topic (the default: return
case) and replace the silent return with a logger.debug/info call including the
topic and relevant metadata (and then return).
In `@tests/revive.test.ts`:
- Around line 23-28: Add unit tests in revive.test.ts covering error paths for
the helper functions: call getReviveLogPayload with malformed/invalid payloads
and assert it throws, supply an event object with missing or empty topics to
functions that expect topics and assert they throw or return the expected error,
and pass non-string contract addresses to isMintTransfer/isBurnTransfer (or any
address-validated helper) and assert they reject/throw; target the functions
getReviveLogPayload, isMintTransfer, and isBurnTransfer when writing these tests
so error handling and input validation are exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78e5a21d-0a17-4f7a-9708-84eea9fd8445
⛔ Files ignored due to path filters (4)
package-lock.jsonis excluded by!**/package-lock.jsonsrc/model/generated/_collectionKind.tsis excluded by!**/generated/**src/model/generated/collectionEntity.model.tsis excluded by!**/generated/**src/model/generated/index.tsis excluded by!**/generated/**
📒 Files selected for processing (28)
db/migrations/1774691683926-Data.jspackage.jsonschema.graphqlsnack.yamlsrc/environment.tssrc/mappings/assets/forceToken.tssrc/mappings/assets/getters/index.tssrc/mappings/index.tssrc/mappings/nfts/create.tssrc/mappings/nfts/forceCreate.tssrc/mappings/nfts/getters/index.tssrc/mappings/revive/abi/erc721.tssrc/mappings/revive/abi/registry.tssrc/mappings/revive/burn.tssrc/mappings/revive/client.tssrc/mappings/revive/createCollection.tssrc/mappings/revive/helpers.tssrc/mappings/revive/index.tssrc/mappings/revive/mint.tssrc/mappings/revive/transfer.tssrc/mappings/uniques/create.tssrc/mappings/uniques/forceCreate.tssrc/mappings/uniques/getters/index.tssrc/mappings/utils/helper.tssrc/processable.tssrc/processor.tstests/index.test.tstests/revive.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/reviveHandlers.test.ts (2)
4-22: Unused mock functions defined.
pendingMock,successMock,calculateCollectionOwnerCountAndDistributionMock, andcalculateCollectionFloorMockare hoisted but never asserted against in any test. Consider either:
- Removing unused mocks to reduce noise, or
- Adding tests for happy paths that would exercise these functions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/reviveHandlers.test.ts` around lines 4 - 22, The hoisted mocks pendingMock, successMock, calculateCollectionOwnerCountAndDistributionMock, and calculateCollectionFloorMock are defined but never used; either remove these unused mock variables from the vi.hoisted() block (leaving getOptionalMock, getWithMock, warnMock, createEventMock as needed) or add unit tests that exercise the paths invoking pendingMock, successMock, calculateCollectionOwnerCountAndDistributionMock, and calculateCollectionFloorMock (e.g., add a happy-path test that triggers the code paths which call those mocks) so the mocks are asserted against; update tests/reviveHandlers.test.ts by removing unused mock declarations or by adding corresponding tests that call and assert on pendingMock, successMock, calculateCollectionOwnerCountAndDistributionMock, and calculateCollectionFloorMock.
108-125: Missing test: "skips burns for unknown collections"The burn handler has the same unknown collection guard as the transfer handler (see
burn.tslines 17-22 in context snippet 2), but only the transfer handler has a test for this path. Consider adding symmetrical coverage:➕ Proposed additional test
+ it('skips burns for unknown collections', async () => { + getOptionalMock.mockResolvedValue(undefined) + + await handleTokenBurn(context, { + contractAddress: '0x329aaa5b6bea94e750b2dacba74bf41291e6c2bd', + from: '0x1111111111111111111111111111111111111111', + to: '0x0000000000000000000000000000000000000000', + tokenId: 1n, + }) + + expect(getWithMock).not.toHaveBeenCalled() + expect(warnMock).toHaveBeenCalledWith( + 'BURN', + 'Unknown collection 0x329aaa5b6bea94e750b2dacba74bf41291e6c2bd', + ) + }) + it('skips burns when the nft was never indexed', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/reviveHandlers.test.ts` around lines 108 - 125, Add a parallel test for the "unknown collection" path in the burn handler: mock getOptionalMock to return undefined/null (simulating an unrecognized collection), call handleTokenBurn with a burn event payload (use the same contractAddress/tokenId pattern as other tests), and assert that warnMock is called for the burn unknown-collection case and that createEventMock is not called; reference the existing mocks getOptionalMock, getWithMock, warnMock, createEventMock and the function handleTokenBurn to find where to add the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/reviveHandlers.test.ts`:
- Around line 4-22: The hoisted mocks pendingMock, successMock,
calculateCollectionOwnerCountAndDistributionMock, and
calculateCollectionFloorMock are defined but never used; either remove these
unused mock variables from the vi.hoisted() block (leaving getOptionalMock,
getWithMock, warnMock, createEventMock as needed) or add unit tests that
exercise the paths invoking pendingMock, successMock,
calculateCollectionOwnerCountAndDistributionMock, and
calculateCollectionFloorMock (e.g., add a happy-path test that triggers the code
paths which call those mocks) so the mocks are asserted against; update
tests/reviveHandlers.test.ts by removing unused mock declarations or by adding
corresponding tests that call and assert on pendingMock, successMock,
calculateCollectionOwnerCountAndDistributionMock, and
calculateCollectionFloorMock.
- Around line 108-125: Add a parallel test for the "unknown collection" path in
the burn handler: mock getOptionalMock to return undefined/null (simulating an
unrecognized collection), call handleTokenBurn with a burn event payload (use
the same contractAddress/tokenId pattern as other tests), and assert that
warnMock is called for the burn unknown-collection case and that createEventMock
is not called; reference the existing mocks getOptionalMock, getWithMock,
warnMock, createEventMock and the function handleTokenBurn to find where to add
the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0500119-7b22-4622-95dd-2cf0b508b9ea
📒 Files selected for processing (5)
src/mappings/revive/burn.tssrc/mappings/revive/helpers.tssrc/mappings/revive/transfer.tstests/revive.test.tstests/reviveHandlers.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/revive.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/mappings/revive/burn.ts
- src/mappings/revive/transfer.ts
- src/mappings/revive/helpers.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/mappings/revive/updateCollectionMetadata.ts (1)
9-9: Consider avoidingas anyfor operation constant.The
as anycast suggests the logger's operation type doesn't include'CONTRACT_METADATA'. Rather than bypassing type checking, consider extending the operation type union to include this new value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mappings/revive/updateCollectionMetadata.ts` at line 9, The OPERATION constant uses a unsafe cast ("const OPERATION = 'CONTRACT_METADATA' as any"); instead, update the operation/type definition used by the logger to include 'CONTRACT_METADATA' (e.g., extend the Operation union/enum or add the new literal to the existing type) and then declare OPERATION without "as any" (e.g., const OPERATION: Operation = 'CONTRACT_METADATA'), referencing OPERATION and the logger's operation type/enum so the value is type-safe and no cast is needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mappings/revive/updateCollectionMetadata.ts`:
- Line 34: The assignment to collection.updatedAt in updateCollectionMetadata
uses Date.now() as a fallback when context.block.timestamp is missing, causing
nondeterministic timestamps; change the behavior to match
toReviveBaseCall/helpers.ts by either throwing when context.block.timestamp is
undefined or substituting a deterministic value (e.g., a fixed epoch or derived
deterministic value), and update the collection.updatedAt assignment in
updateCollectionMetadata to rely on context.block.timestamp (or the chosen
deterministic alternative) instead of Date.now().
---
Nitpick comments:
In `@src/mappings/revive/updateCollectionMetadata.ts`:
- Line 9: The OPERATION constant uses a unsafe cast ("const OPERATION =
'CONTRACT_METADATA' as any"); instead, update the operation/type definition used
by the logger to include 'CONTRACT_METADATA' (e.g., extend the Operation
union/enum or add the new literal to the existing type) and then declare
OPERATION without "as any" (e.g., const OPERATION: Operation =
'CONTRACT_METADATA'), referencing OPERATION and the logger's operation type/enum
so the value is type-safe and no cast is needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3419ce69-2c97-432d-9f8d-04b95ca498e4
📒 Files selected for processing (7)
src/mappings/revive/abi/erc721.tssrc/mappings/revive/helpers.tssrc/mappings/revive/index.tssrc/mappings/revive/updateCollectionMetadata.tssrc/processor.tstests/revive.test.tstests/reviveCollectionMetadata.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/mappings/revive/index.ts
- src/processor.ts
- src/mappings/revive/abi/erc721.ts
- tests/revive.test.ts
3 tasks
There was a problem hiding this comment.
Pull request overview
Adds initial Revive (EVM) indexing support (ERC-721) alongside existing Uniques/Nfts indexing, with Paseo-focused environment/config changes and schema updates to classify collection kinds.
Changes:
- Introduce Revive event ingestion + handlers (collection register, mint, transfer, burn, contractURI updates) and an EVM RPC client via
viem. - Add
CollectionKind/collectionTypeto the data model + GraphQL schema and migrate existing rows. - Update environment/processor wiring (Paseo chain option, getter chain selection, Revive toggles) and add Vitest coverage for Revive flows.
Reviewed changes
Copilot reviewed 31 out of 35 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/reviveHandlers.test.ts | Unit tests for Revive transfer/burn early-exit paths and warnings |
| tests/reviveCollectionMetadata.test.ts | Unit tests for contractURI update handling & fallback behavior |
| tests/revive.test.ts | Unit tests for Revive helper decoding/classification utilities |
| tests/index.test.ts | Adds sanitizeText helper test and updates helper imports |
| src/processor.ts | Wires Revive log ingestion into the processor behind REVIVE_ENABLED |
| src/processable.ts | Adds Revive.ContractEmitted enum entry |
| src/model/generated/index.ts | Exports generated CollectionKind enum |
| src/model/generated/collectionEntity.model.ts | Adds collectionType column to CollectionEntity model |
| src/model/generated/_collectionKind.ts | Defines CollectionKind enum (UNIQUES/NFTS/ERC721/ERC1155) |
| src/mappings/utils/helper.ts | Switches SS58 encoding to use SS58_PREFIX from environment |
| src/mappings/uniques/getters/index.ts | Uses GETTER_CHAIN for chain-specific getter resolution |
| src/mappings/uniques/forceCreate.ts | Sets collectionType = UNIQUES on forced Unique collections |
| src/mappings/uniques/create.ts | Sets collectionType = UNIQUES on created Unique collections |
| src/mappings/revive/updateCollectionMetadata.ts | Implements handler for ContractURIUpdated updates and metadata ingestion |
| src/mappings/revive/transfer.ts | Implements Revive ERC-721 transfer handler |
| src/mappings/revive/mint.ts | Implements Revive ERC-721 mint handler |
| src/mappings/revive/index.ts | Dispatches Revive contract-emitted logs by topic0 |
| src/mappings/revive/helpers.ts | Adds log payload parsing, event decoding, and ID/address helpers |
| src/mappings/revive/createCollection.ts | Creates/updates collections on registry CollectionRegistered |
| src/mappings/revive/client.ts | Adds viem public client + contract read helpers |
| src/mappings/revive/burn.ts | Implements Revive ERC-721 burn handler |
| src/mappings/revive/abi/registry.ts | Registry ABI + event topic selector |
| src/mappings/revive/abi/erc721.ts | ERC-721 ABI + event topic selectors |
| src/mappings/nfts/getters/index.ts | Uses GETTER_CHAIN for chain-specific getter resolution |
| src/mappings/nfts/forceCreate.ts | Sets collectionType = NFTS on forced Nfts collections |
| src/mappings/nfts/create.ts | Sets collectionType = NFTS on created Nfts collections |
| src/mappings/index.ts | Adds Revive handler routing + changes one-time asset forcing logic |
| src/mappings/assets/getters/index.ts | Uses GETTER_CHAIN for chain-specific getter resolution |
| src/mappings/assets/forceToken.ts | Updates system asset defaults to Paseo (PAS) |
| src/environment.ts | Adds Paseo chain option, Revive toggles, start block logic, getter chain + SS58 prefix |
| snack.yaml | Adds a subsquid manifest configured for AssetHub Paseo + Revive |
| schema.graphql | Adds collectionType: CollectionKind! and defines CollectionKind enum |
| package.json | Adds viem dependency |
| package-lock.json | Locks viem and transitive deps |
| db/migrations/1774691683926-Data.js | Migrates collection_entity.collection_type + backfills/indexes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/mappings/revive/helpers.ts (1)
69-120: Considerstrict: truefor decodeEventLog (optional).With
strict: false, if a payload'sdatais empty/mismatched for a non-indexed field, the corresponding entry indecoded.argsbecomesundefined, andString(undefined)silently yields the literal"undefined"string (e.g.,normalizeEvmAddress('undefined')would return"undefined"rather than throwing). For well-formed on-chain events this is a non-issue, butstrict: truewould surface malformed payloads early with a clearer error instead of persisting bogus values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mappings/revive/helpers.ts` around lines 69 - 120, The decodeEventLog calls in decodeCollectionRegisteredEvent, decodeTransferEvent, and decodeContractUriUpdatedEvent use strict: false which can let malformed logs produce undefined values (e.g., String(undefined)); change strict to true in each decodeEventLog invocation to force validation and surface malformed payloads early, and if needed wrap the calls in try/catch at the call site or let the thrown error propagate so malformed events are rejected instead of producing bogus values.tests/revive.test.ts (1)
16-120: LGTM — good coverage of helpers.Encoding via viem's
encodeEventTopics/encodeAbiParameterskeeps the tests realistic, and covering the negative path (empty topics) plus address normalization is sufficient for these helpers.Optional: add a case for
isMintTransfer/isBurnTransferwith a mixed-case zero address (e.g.,0x0000...0000with uppercase prefix or padded variants from topics) to exercisenormalizeEvmAddressbehavior inside those predicates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/revive.test.ts` around lines 16 - 120, Add a test that verifies isMintTransfer and isBurnTransfer handle mixed-case/padded zero addresses by normalizing them; update the 'classifies mint and burn transfers' spec to include at least one assertion calling isMintTransfer('0x0000000000000000000000000000000000000000' in mixed-case like "0x0000000000000000000000000000000000000000".toUpperCase() or a padded/mixed-case variant) and the same for isBurnTransfer, ensuring the predicates internally normalize via normalizeEvmAddress and still return true for the zero address and false for non-zero variants (use the existing expect(...) .toBe(...) pattern with isMintTransfer and isBurnTransfer).src/mappings/nfts/setAttribute.ts (1)
38-48: Nit:humanalias is redundant.
const human = valuein the catch just re-aliases the already-sanitizedvaluefrom line 20 — you can drop it and referencevaluedirectly. Non-blocking.Proposed tidy-up
} catch (error) { - const human = value - final.recipient = isAddress(human) ? human : '' + final.recipient = isAddress(value) ? value : '' if (final.recipient === '') { console.log(error) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mappings/nfts/setAttribute.ts` around lines 38 - 48, The catch block in the conditional that checks 'recipient' currently creates a redundant alias const human = value; remove that alias and use the already-sanitized value variable directly when calling isAddress and assigning final.recipient (keep the surrounding try/catch and the addressOf(event.value as string) logic intact, and still console.log(error) when final.recipient becomes an empty string).tests/nftSetAttribute.test.ts (1)
53-86: Consider adding a couple more cases to lock down behavior you’re now relying on.The single test covers the happy NFT path with a non-null, sanitizable value. Worthwhile additions while you’re here:
- Null-value path — assert that when
event.value === null, the existing matching attribute is removed (thefilterbranch at line 51). This is the code path that actually exercisessanitizeText(trait)independently fromvalue.- Collection path — when
isNFT(event)is false,markCollectionRarityDirty/markNftAttributesDirtymust not be called. Addingexpect(...).not.toHaveBeenCalled()guards against regressing this invariant.- Special-trait exclusion — feeding
trait: 'royalty'ortrait: 'recipient'should not push a new entry intoattributes(line 56 guard).These are cheap to add and codify the most subtle branches of the handler.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/nftSetAttribute.test.ts` around lines 53 - 86, Add three new test cases around handleAttributeSet: (1) a "null-value path" test where currentEvent.value === null and an existing attribute is removed—assert nft.attributes no longer contains the trait and sanitizeText(trait) is still applied; (2) a "collection path" test where isNFT(event) returns false and assert markCollectionRarityDirty and markNftAttributesDirty were not called; and (3) a "special-trait exclusion" test that calls handleAttributeSet with trait 'royalty' and/or 'recipient' and assert that no new attribute is pushed into nft.attributes. Ensure each test mocks getMock, store.save, and relevant dirtiness mocks and asserts store.save behavior and attribute array contents accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mappings/nfts/setAttribute.ts`:
- Around line 19-20: The current assignment for value only checks event.value
=== null and lets undefined become the string "undefined"; change the null check
to a nullish check (e.g., event.value == null or use the nullish coalescing
pattern) so both null and undefined yield null, and simplify the branch by
calling sanitizeText(unHex(event.value)) only when event.value is not nullish;
update the value assignment in setAttribute.ts (referencing value, sanitizeText,
and unHex) accordingly.
In `@src/mappings/utils/helper.ts`:
- Around line 5-9: The codec is currently set from SS58_PREFIX which maps
non-kusama chains (including Paseo) to 0, causing addressOf() to encode Paseo
addresses with Polkadot's prefix; update the logic so Paseo uses its real
prefix: either restore codec derivation from the CHAIN string (use CHAIN to
select the SS58 codec) or change SS58_PREFIX computation to explicitly map CHAIN
=== 'paseo' to 42 (e.g. CHAIN === 'kusama' ? 2 : CHAIN === 'paseo' ? 42 : 0),
and ensure codec in helper.ts is assigned from that corrected SS58_PREFIX so
addressOf() produces correct Paseo addresses.
---
Nitpick comments:
In `@src/mappings/nfts/setAttribute.ts`:
- Around line 38-48: The catch block in the conditional that checks 'recipient'
currently creates a redundant alias const human = value; remove that alias and
use the already-sanitized value variable directly when calling isAddress and
assigning final.recipient (keep the surrounding try/catch and the
addressOf(event.value as string) logic intact, and still console.log(error) when
final.recipient becomes an empty string).
In `@src/mappings/revive/helpers.ts`:
- Around line 69-120: The decodeEventLog calls in
decodeCollectionRegisteredEvent, decodeTransferEvent, and
decodeContractUriUpdatedEvent use strict: false which can let malformed logs
produce undefined values (e.g., String(undefined)); change strict to true in
each decodeEventLog invocation to force validation and surface malformed
payloads early, and if needed wrap the calls in try/catch at the call site or
let the thrown error propagate so malformed events are rejected instead of
producing bogus values.
In `@tests/nftSetAttribute.test.ts`:
- Around line 53-86: Add three new test cases around handleAttributeSet: (1) a
"null-value path" test where currentEvent.value === null and an existing
attribute is removed—assert nft.attributes no longer contains the trait and
sanitizeText(trait) is still applied; (2) a "collection path" test where
isNFT(event) returns false and assert markCollectionRarityDirty and
markNftAttributesDirty were not called; and (3) a "special-trait exclusion" test
that calls handleAttributeSet with trait 'royalty' and/or 'recipient' and assert
that no new attribute is pushed into nft.attributes. Ensure each test mocks
getMock, store.save, and relevant dirtiness mocks and asserts store.save
behavior and attribute array contents accordingly.
In `@tests/revive.test.ts`:
- Around line 16-120: Add a test that verifies isMintTransfer and isBurnTransfer
handle mixed-case/padded zero addresses by normalizing them; update the
'classifies mint and burn transfers' spec to include at least one assertion
calling isMintTransfer('0x0000000000000000000000000000000000000000' in
mixed-case like "0x0000000000000000000000000000000000000000".toUpperCase() or a
padded/mixed-case variant) and the same for isBurnTransfer, ensuring the
predicates internally normalize via normalizeEvmAddress and still return true
for the zero address and false for non-zero variants (use the existing
expect(...) .toBe(...) pattern with isMintTransfer and isBurnTransfer).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29116d15-80b7-4a95-afa9-7edd390321c0
📒 Files selected for processing (6)
src/mappings/index.tssrc/mappings/nfts/setAttribute.tssrc/mappings/revive/helpers.tssrc/mappings/utils/helper.tstests/nftSetAttribute.test.tstests/revive.test.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 39 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
18
to
38
| @@ -26,14 +27,14 @@ export async function handleAttributeSet(context: Context): Promise<void> { | |||
| final.attributes = [] | |||
| } | |||
|
|
|||
| if (event.value === null) { | |||
| if (value === null) { | |||
| final.attributes = final.attributes?.filter((attr) => attr.trait !== event.trait) | |||
| } else { | |||
| const attribute = final.attributes?.find((attr) => attr.trait === event.trait) | |||
| if (attribute) { | |||
| attribute.value = unHex(event.value) ?? String(event.value) | |||
| attribute.value = value | |||
| } else { | |||
| const newAttribute = attributeFrom({ trait_type: event.trait, value: unHex(event.value) ?? String(event.value) }) | |||
| const newAttribute = attributeFrom({ trait_type: event.trait, value }) | |||
| final.attributes?.push(newAttribute) | |||
There was a problem hiding this comment.
sanitizeText was introduced to strip NUL characters that Postgres cannot store, and nfts/setAttribute now uses it. uniques/setAttribute still persists event.trait and event.value (via attribute.value = value / attributeFrom) without sanitization, so any Uniques attribute containing \u0000 will still cause a DB write failure. Consider applying sanitizeText to both the trait and decoded value here (similar to the Nfts handler) before comparing/storing.
Comment on lines
+132
to
+140
| processor.addReviveContractEmitted({ | ||
| topic0: [ERC721_TRANSFER_TOPIC], | ||
| range: { from: REVIVE_START_BLOCK }, | ||
| }) | ||
|
|
||
| processor.addReviveContractEmitted({ | ||
| topic0: [CONTRACT_URI_UPDATED_TOPIC], | ||
| range: { from: REVIVE_START_BLOCK }, | ||
| }) |
There was a problem hiding this comment.
The addReviveContractEmitted registrations for ERC721_TRANSFER_TOPIC and CONTRACT_URI_UPDATED_TOPIC do not specify a contract filter, so the processor will ingest all Revive ContractEmitted logs matching those topics, regardless of whether the contract is one you index. Given the handlers currently warn on unknown collections/NFTs, this can create significant ingestion/log volume on chains with unrelated Revive ERC-721 activity. If possible, consider adding a contract allowlist (e.g., env-provided addresses) or reducing log severity for expected “unknown” skips to avoid operational noise.
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
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.
Summary by CodeRabbit
New Features
Updates
Tests
Chores