feat(nft): wire native NFT apply path (SRC-721) via sentrix-nft#775
Merged
Conversation
Dispatch the merged SRC-721 TokenOp variants (DeployNft, MintNft, TransferNft, BurnNft, ApproveNft, SetApprovalForAll) through the existing 3-layer execution path, following the SRC-20 precedent: - read-only validate_block: lazy working-clone dry-run - Pass-1 dry-run: lazy working-clone dry-run (intra-block deploy→mint) - Pass-2 apply: mutate Blockchain.nft_registry, emit via TokenOpEvent Authorization is always the authenticated sender (tx.from_address); the deterministic collection seed is tx.txid. set_approval_for_all pins owner to the sender so a payload can't grant approvals for another owner. nft_registry is added to the Pass-2 snapshot so a failed block rolls back NFT state atomically. Gated by NFT_TOKENOP_HEIGHT (default u64::MAX). State model: nft_registry follows SRC-20 contracts exactly — serialized in the chain.db blob, NOT committed to the trie/state_root. This is deferred to feat/native-module-state-commitment; the apply path is NOT yet production/testnet-deploy ready. Deploy-time soulbound selection also deferred (DeployNft wire format has no transferability field); apply-path soulbound enforcement is wired + tested. Tests: 16 dispatch tests (auth, soulbound enforcement, strict no-reuse, max_supply counts ever-minted, deterministic replay, typed errors) + 4 E2E tests through add_block (deploy/mint/transfer, pre-fork rejection, cross-node determinism, failed-block rollback). Coverage: add sentrix-nft to the llvm-cov package set.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThis PR wires the native NFT (SRC-721) token operation apply path into block execution: adds nft_registry to the Blockchain state model, implements the core apply_nft_token_op dispatcher that applies supported TokenOp variants to the registry, integrates pre-flight validation into block validation against a cloned registry, extends block snapshots to include NFT state for atomic rollback, and updates the Pass-2 commit logic to apply NFT operations with proper error handling and event emission. Comprehensive tests verify deploy→mint→transfer flows, fork-height gating, rollback isolation, and deterministic behavior. Sequence DiagramsequenceDiagram
participant Validator as BlockValidator
participant NftModule as NFT Module
participant Executor as BlockExecutor
participant Registry as nft_registry
participant EventEmit as EventEmitter
Validator->>NftModule: apply_nft_token_op(working_clone, op)
NftModule-->>Validator: Result<Vec<NftEvent>>
Validator->>Validator: Validation pass
Executor->>Executor: Clone Registry to Snapshot
Executor->>NftModule: apply_nft_token_op(&mut Registry, op)
NftModule->>Registry: Apply SRC-721 state change
NftModule-->>Executor: Result<Vec<NftEvent>>
Executor->>EventEmit: emit(convert(NftEvent))
alt Success
Executor->>Executor: Commit snapshot
else Failure
Executor->>Registry: restore from snapshot
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/sentrix-core/src/block_executor.rs (1)
600-609:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSnapshotting
nft_registryunconditionally makes every block O(total NFT state).
BlockchainSnapshotnow clones the full registry before every Pass-2, even when the block has no NFT ops. Once the registry is large, ordinary SRX-only blocks pay a full extra clone and peak-memory copy, and NFT blocks also pay the Pass-1 working clone on top. That turns block admission latency into a function of total NFT state and risks round-time regressions. Please make the rollback snapshot lazy on first NFT mutation, or journal only the touched collection state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/sentrix-core/src/block_executor.rs` around lines 600 - 609, The current snapshot creation in BlockchainSnapshot captures nft_registry unconditionally (nft_registry.clone()) causing full clones every block; change to a lazy snapshot/journal: remove unconditional cloning in the BlockExecutor snapshot creation and instead record either None or a lightweight marker for nft_registry in BlockchainSnapshot, and implement logic to clone-and-store the original nft_registry only on the first NFT mutation during Pass-1 (or alternatively journal only per-collection mutations). Update the NFT mutation entry points/handlers (the functions that apply NFT ops in block execution) to check the snapshot's nft_registry presence and perform the one-time clone of self.nft_registry into the snapshot (or append collection-level entries to a mutation journal) before applying the mutation so subsequent NFT ops in the same block reuse the saved original; keep all other snapshot fields unchanged. Ensure rollback code uses the saved clone/journal to restore state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/sentrix-core/src/block_executor.rs`:
- Around line 944-949: emit_state_fingerprint (the SENTRIX_STATE_FINGERPRINT
output) currently hashes only AccountDB and total_minted and must be extended to
include nft_registry so divergent NFT state is reflected; update the
emit_state_fingerprint implementation to perform a deterministic, sorted
traversal of self.nft_registry (sort by NFT key/ID) and feed each key and its
canonical token data into the same hasher used for AccountDB/total_minted,
ensuring the traversal and serialization are stable across runs (e.g., iterate
sorted keys, canonicalize fields) and include that hash material before
finalizing and printing the fingerprint.
- Around line 950-958: Currently NFT token-op events are emitted inside pass-2
while iterating events (the block_executor code using
self.event_emitter.emit_token_op with nft_event_to_token_op_event(ev, &tx.txid,
block.index)), which can leak events if later commit steps fail; change the flow
to collect/ stage the converted events into a local Vec (e.g., staged_events)
instead of calling self.event_emitter.emit_token_op inline, and only after the
entire commit path succeeds (after trie updates, state_root verification,
snapshot commit) iterate staged_events and call self.event_emitter.emit_token_op
for each; ensure staged_events is populated where events is currently iterated
and flushed only on the final success path so nft_registry rollbacks do not
leave subscribers with events for a non-committed block.
In `@crates/sentrix-core/src/nft.rs`:
- Around line 95-134: The NFT core currently calls
collection_mut(...).mint/transfer/approve/set_approval_for_all without verifying
that input addresses are valid Sentrix addresses; add checks in
crates/sentrix-core/src/nft.rs before calling those methods: validate `to` in
TokenOp::MintNft, `from`/`to` in TokenOp::TransferNft, `spender` in
TokenOp::ApproveNft, and `operator` (and owner if applicable) in
TokenOp::SetApprovalForAll using an existing helper like
is_valid_sentrix_address or is_spendable_sentrix_address (or add one), and
return an appropriate Sentrix error (mapped via nft_err_to_sentrix or the same
error path) when validation fails, only calling
collection_mut(...).mint/transfer/approve/set_approval_for_all after the checks
pass.
---
Outside diff comments:
In `@crates/sentrix-core/src/block_executor.rs`:
- Around line 600-609: The current snapshot creation in BlockchainSnapshot
captures nft_registry unconditionally (nft_registry.clone()) causing full clones
every block; change to a lazy snapshot/journal: remove unconditional cloning in
the BlockExecutor snapshot creation and instead record either None or a
lightweight marker for nft_registry in BlockchainSnapshot, and implement logic
to clone-and-store the original nft_registry only on the first NFT mutation
during Pass-1 (or alternatively journal only per-collection mutations). Update
the NFT mutation entry points/handlers (the functions that apply NFT ops in
block execution) to check the snapshot's nft_registry presence and perform the
one-time clone of self.nft_registry into the snapshot (or append
collection-level entries to a mutation journal) before applying the mutation so
subsequent NFT ops in the same block reuse the saved original; keep all other
snapshot fields unchanged. Ensure rollback code uses the saved clone/journal to
restore state.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5d66e223-0b33-4722-8e4f-832aeaefc0f9
📒 Files selected for processing (5)
.github/workflows/coverage.ymlcrates/sentrix-core/src/block_executor.rscrates/sentrix-core/src/block_executor/validate.rscrates/sentrix-core/src/blockchain.rscrates/sentrix-core/src/nft.rs
Comment on lines
+944
to
+949
| let events = crate::nft::apply_nft_token_op( | ||
| &mut self.nft_registry, | ||
| &op, | ||
| &tx.from_address, | ||
| &tx.txid, | ||
| )?; |
There was a problem hiding this comment.
SENTRIX_STATE_FINGERPRINT no longer fingerprints full consensus state.
This PR adds nft_registry as consensus state mutated in Pass-2, but emit_state_fingerprint() still hashes only AccountDB and total_minted. Two validators can now diverge only in NFT state and still print identical [STATE-FP] lines, which makes the incident tool blind on the new path. Please extend the fingerprint with a deterministic, sorted traversal of the NFT registry before enabling the fork. As per coding guidelines, SENTRIX_STATE_FINGERPRINT is load-bearing for debugging + chain replay. Removing any of them silently is a regression.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/sentrix-core/src/block_executor.rs` around lines 944 - 949,
emit_state_fingerprint (the SENTRIX_STATE_FINGERPRINT output) currently hashes
only AccountDB and total_minted and must be extended to include nft_registry so
divergent NFT state is reflected; update the emit_state_fingerprint
implementation to perform a deterministic, sorted traversal of self.nft_registry
(sort by NFT key/ID) and feed each key and its canonical token data into the
same hasher used for AccountDB/total_minted, ensuring the traversal and
serialization are stable across runs (e.g., iterate sorted keys, canonicalize
fields) and include that hash material before finalizing and printing the
fingerprint.
Comment on lines
+950
to
+958
| if let Some(emitter) = &self.event_emitter { | ||
| for ev in &events { | ||
| emitter.emit_token_op(&crate::nft::nft_event_to_token_op_event( | ||
| ev, | ||
| &tx.txid, | ||
| block.index, | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
Emit NFT events only after the block survives the full commit path.
These events fire inside Pass-2 before later txs, trie update, and peer state_root verification. If any later step errors, the snapshot rolls back nft_registry but subscribers have already seen deploy/mint/transfer events for a block that never committed. Stage the converted events locally and flush them only on the final success path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/sentrix-core/src/block_executor.rs` around lines 950 - 958, Currently
NFT token-op events are emitted inside pass-2 while iterating events (the
block_executor code using self.event_emitter.emit_token_op with
nft_event_to_token_op_event(ev, &tx.txid, block.index)), which can leak events
if later commit steps fail; change the flow to collect/ stage the converted
events into a local Vec (e.g., staged_events) instead of calling
self.event_emitter.emit_token_op inline, and only after the entire commit path
succeeds (after trie updates, state_root verification, snapshot commit) iterate
staged_events and call self.event_emitter.emit_token_op for each; ensure
staged_events is populated where events is currently iterated and flushed only
on the final success path so nft_registry rollbacks do not leave subscribers
with events for a non-committed block.
Comment on lines
+95
to
+134
| TokenOp::MintNft { | ||
| contract, | ||
| to, | ||
| token_id, | ||
| metadata_uri, | ||
| } => collection_mut(registry, contract)? | ||
| // transferable = None → use the collection default. | ||
| .mint(sender, to, *token_id, metadata_uri, None) | ||
| .map_err(nft_err_to_sentrix)?, | ||
| TokenOp::TransferNft { | ||
| contract, | ||
| from, | ||
| to, | ||
| token_id, | ||
| } => collection_mut(registry, contract)? | ||
| // `from` is untrusted claimed data; the domain checks it against | ||
| // the real owner. Authority is `sender` (caller). | ||
| .transfer(sender, from, to, *token_id) | ||
| .map_err(nft_err_to_sentrix)?, | ||
| TokenOp::BurnNft { contract, token_id } => collection_mut(registry, contract)? | ||
| // Domain clears the token's approval as part of the burn. | ||
| .burn(sender, *token_id) | ||
| .map_err(nft_err_to_sentrix)?, | ||
| TokenOp::ApproveNft { | ||
| contract, | ||
| spender, | ||
| token_id, | ||
| } => collection_mut(registry, contract)? | ||
| .approve(sender, spender, *token_id) | ||
| .map_err(nft_err_to_sentrix)?, | ||
| TokenOp::SetApprovalForAll { | ||
| contract, | ||
| operator, | ||
| approved, | ||
| } => collection_mut(registry, contract)? | ||
| // owner == sender: a caller may only manage operators for tokens | ||
| // it owns. The domain re-enforces caller == owner, so a payload | ||
| // can never set approvals on behalf of a different owner. | ||
| .set_approval_for_all(sender, sender, operator, *approved) | ||
| .map_err(nft_err_to_sentrix)?, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
crate_dir="$(fd -td -i 'sentrix-nft' crates | head -n1)"
echo "Inspecting: ${crate_dir}"
fd -e rs . "${crate_dir}" | xargs rg -n -C3 'is_valid_sentrix_address|is_spendable_sentrix_address|ZERO_ADDRESS|fn mint|fn transfer|fn approve|fn set_approval_for_all|deploy_collection'Repository: sentrix-labs/sentrix
Length of output: 16660
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Check whether sentrix-nft references core address helpers (validation / spendability / zero address).
rg -n "is_valid_sentrix_address|is_spendable_sentrix_address|ZERO_ADDRESS|zero[-_ ]address" crates/sentrix-nft || true
# 2) Inspect address checks inside sentrix-nft's collection operations.
# Print the exact guard clauses around mint/transfer/approve/set_approval_for_all.
rg -n --no-heading "pub fn (mint|transfer|approve|set_approval_for_all)\b" crates/sentrix-nft/src/collection.rs
for fn in mint transfer approve set_approval_for_all; do
echo "---- $fn guards ----"
rg -n --no-heading -C3 "${fn}\(" crates/sentrix-nft/src/collection.rs | head -n 80
done
# 3) Search for explicit empty checks and any regex/prefix checks in sentrix-nft.
rg -n --no-heading "is_empty\(|starts_with\(|0x0|0X0|address|spender|operator|owner" crates/sentrix-nft/src/collection.rs | head -n 200Repository: sentrix-labs/sentrix
Length of output: 5728
Validate Sentrix addresses at the NFT core boundary.
crates/sentrix-nft/src/collection.rs only guards against empty strings (to/from in transfer, spender in approve, owner/operator in set_approval_for_all) via *.is_empty() checks; these operation paths don’t enforce Sentrix address format and don’t reject zero/invalid-but-nonempty addresses. Add Sentrix address validation in crates/sentrix-core/src/nft.rs (e.g., is_valid_sentrix_address / is_spendable_sentrix_address or equivalent) before calling .mint(...), .transfer(...), .approve(...), and .set_approval_for_all(...).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/sentrix-core/src/nft.rs` around lines 95 - 134, The NFT core currently
calls collection_mut(...).mint/transfer/approve/set_approval_for_all without
verifying that input addresses are valid Sentrix addresses; add checks in
crates/sentrix-core/src/nft.rs before calling those methods: validate `to` in
TokenOp::MintNft, `from`/`to` in TokenOp::TransferNft, `spender` in
TokenOp::ApproveNft, and `operator` (and owner if applicable) in
TokenOp::SetApprovalForAll using an existing helper like
is_valid_sentrix_address or is_spendable_sentrix_address (or add one), and
return an appropriate Sentrix error (mapped via nft_err_to_sentrix or the same
error path) when validation fails, only calling
collection_mut(...).mint/transfer/approve/set_approval_for_all after the checks
pass.
github-actions Bot
pushed a commit
that referenced
this pull request
Jun 3, 2026
…#776) Native-module state (SRC-20 ContractRegistry + NFT NftRegistry) was absent from the STATE-FP fingerprint, so two validators could diverge purely in token/NFT state and still print identical [STATE-FP] lines (CodeRabbit #1 on #775). Close the debug-visibility gap consistently for both native modules with canonical, HashMap-order-independent hashing. - NftCollection/NftRegistry: canonical_hash() — fixed field order, all maps folded with keys sorted (read-only, no new mutable surface). - SRC20Contract/ContractRegistry: canonical_hash() — same approach. - block_executor: extract compute_state_fingerprint() (testable, no env gate) and fold both registries' canonical_hash into the combined fp. - nft apply path: validate to/from/spender/operator addresses via is_spendable/is_valid_sentrix_address before mutating (CodeRabbit #3), mirroring SRC-20 M-02. Scope: this is fingerprint (debug) commitment only. Consensus-enforced trie/state_root commitment of native-module state is the heavier fork-gated + migrated + soaked follow-up; canonical serialization here is its foundation. No state_root/trie/MDBX change, no fork gate, no consensus impact. Event-staging refactor (CodeRabbit #2, shared with SRC-20) stays deferred — fixing NFT alone would diverge from the SRC-20 precedent. Tests: sentrix-nft canonical_hash (order-independence, state sensitivity, owner distinction, empty stability); sentrix-core fingerprint tracks SRC-20 + NFT changes, replay-deterministic, order-independent, distinct states differ; invalid-address rejection at the apply boundary.
This was referenced Jun 3, 2026
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.
Wires the merged SRC-721
TokenOpvariants into the existing block execution path using thesentrix-nftdomain crate. Option A: follows the SRC-20 native-module precedent; state_root/trie commitment is explicitly deferred.What this does
Dispatches 6 SRC-721 ops through the existing 3-layer path:
DeployCollection(DeployNft),Mint(MintNft),Transfer(TransferNft),Burn(BurnNft),Approve(ApproveNft),SetApprovalForAllLayers wired (mirrors SRC-20):
validate_block— lazy working-clone dry-runBlockchain.nft_registry, emits via existingTokenOpEventAuthorization
tx.from_address(authenticated sender). NFT payloads carry nocaller.TransferNft.fromis treated as untrusted claimed data; the domain cross-checks it against the real owner.set_approval_for_all: owner pinned to sender — a payload can't grant approvals for another owner.tx.txid(SRC-20 precedent; no wall-clock, no randomness).NFT_TOKENOP_HEIGHT(defaultu64::MAX= disabled).nft_registryis serialized in the chain.db blob, NOT in the trie — identical to SRC-20contracts. A divergent registry is NOT caught by a state_root mismatch. → next branchfeat/native-module-state-commitment.DeployNftwire format has no transferability selector, so apply-path-deployed collections default to transferable. Soulbound enforcement is wired + tested; soulbound deployment via tx needs aDeployNftfield (follow-up). This matters for the Proof Asset product goal (validator/builder/contributor/bug-hunter proofs are inherently soulbound).Tests (20)
16 dispatch + 4 E2E through
add_block: deploy/mint/transfer, soulbound transfer rejected, approved spender, operator approval, owner-pinned set_approval_for_all, burn clears approval, strict no-reuse, max_supply counts ever-minted, deterministic replay, typed errors, pre-fork rejection, cross-node determinism, failed-block rollback.Coverage
sentrix-nftadded to thecargo-llvm-covpackage set (92.8% lines locally). The apply-path wiring itself lives insentrix-core, which stays in the EVM-excluded coverage set (revm instrumentation brittleness) — its tests run inci.yml'scargo test.Commands
cargo test -p sentrix-nft✅ (39 + 2 doctests)cargo test --workspace✅ (66 suites, 0 failures)cargo fmt --check✅cargo clippy --workspace --all-targets -- -D warnings✅Do not deploy. No RPC/explorer/wallet/marketplace/bridge/image-bytes. No trie/state-root/MDBX-table changes.
Summary by CodeRabbit
New Features
Tests