feat(nft): sentrix-nft crate — native Proof Asset NFT (pure domain logic)#774
Merged
Conversation
Extracts native NFT logic into a standalone workspace crate, sentrix-nft, as a Proof Asset / reputation layer (validator/builder/contributor proofs, soulbound badges) — not a marketplace. Crate (pure domain only; no storage/trie/consensus/RPC/marketplace/bridge): - collection.rs NftCollection + all token state transitions - token.rs NftToken (transferable/frozen/burned, optional uri+hashes) - registry.rs NftRegistry, deterministic SRC721_ id from sha256(creator|seed) - events.rs NftEvent (typed, returned by every state change) - error.rs NftError (typed, replaces stringly errors) Model: collection-scoped, String addresses + u64 token ids (matches SRC-20 native rail). Soulbound supported (transferable=false); soulbound transfer fails with a typed NotTransferable error and no approval can bypass it. Per-token + collection freeze, metadata lock. Metadata is URI-only — no image bytes, no IPFS/Arweave upload logic. Intentional behavior changes from the earlier in-core nft.rs: - errors: SentrixError → typed NftError - token model: parallel HashMaps → NftToken struct (per-token soulbound/freeze) - STRICT NO-REUSE: a burned token id is retired forever (tombstone). The earlier code allowed re-minting a burned id; that behavior and its test are replaced, per the reputation-layer audit-trail requirement. - max_supply now counts ever-minted (burning never frees a slot) Integration: sentrix-core depends on sentrix-nft; crates/sentrix-core/src/nft.rs is now a thin facade (pub use sentrix_nft::*) plus nft_err_to_sentrix() to map NftError into SentrixError at the core boundary (free fn — orphan rule). No block-execution/trie/RPC wiring yet (deferred). sentrix-nft has zero sentrix-* dependencies (no cycle risk). Tests: 36 unit + 2 doctests in sentrix-nft, + 2 facade tests in core. The earlier 21 in-core tests are superseded by these (same behaviors + soulbound, freeze, metadata-lock, operator, strict-no-reuse). Full workspace test passes; cargo fmt --check and clippy --workspace -D warnings clean.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR introduces a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 5
🤖 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-nft/src/collection.rs`:
- Around line 200-223: The state mutations in mint/transfer/burn are not atomic:
code in functions like mint (where you call self.tokens.insert(...), update
self.balances, self.total_supply, self.total_minted) mutates storage before
performing fallible checked_add/checked_sub, so an error can leave
partially-applied state; change each affected function (mint, transfer, burn) to
perform all validation and arithmetic using local/temporary variables (e.g.,
compute new_balance = bal.checked_add(1)? / checked_sub(1)? and
new_total_supply/new_total_minted using checked_add/checked_sub and return
errors if any) and only after all checks succeed commit the writes (insert
NftToken, set self.balances[to] = new_balance, set self.total_supply =
new_total_supply, set self.total_minted = new_total_minted) to ensure atomic
state transitions.
- Around line 20-62: The struct currently exposes mutable invariants (tokens,
balances, token_approvals, operator_approvals, total_supply, total_minted,
frozen, metadata_mutable, default_transferable) as pub; change those fields to
private (remove pub) on NftCollection and provide a controlled API (impl) with
safe methods such as mint, burn, transfer, approve_token, set_operator_approval,
freeze_collection/unfreeze_collection, update_metadata, and read-only getters
for public info (id, creator, admin, name, symbol, etc.); ensure all state
transitions go through these methods so invariants (max_supply, never-reuse
token ids, balance counts, approval semantics, freeze checks, metadata
mutability) are enforced inside the corresponding methods (e.g.,
NftCollection::mint, ::transfer, ::burn, ::approve_token).
- Around line 432-456: The update_collection_metadata function currently allows
changes even after lock_metadata is called because it never checks the
metadata_mutable flag; add a guard at the top of update_collection_metadata
(after the frozen/admin checks) that returns Err(NftError::MetadataLocked) (or
add that error variant if missing) when self.metadata_mutable is false, and
apply the identical metadata_mutable check to the corresponding token-level
updater (e.g., update_token_metadata or the function covering lines 473-481) so
token metadata cannot be changed after lock_metadata either.
- Around line 340-357: The set_approval_for_all function currently allows anyone
to set approvals for any owner; enforce that only the owner can change their
approvals by validating the caller identity before modifying operator_approvals.
Update set_approval_for_all (or its call sites) to accept or obtain the
authenticated caller (e.g., a caller: &str parameter or fetch from the execution
context) and add a check like caller == owner (returning NftError::Unauthorized
or similar) before the entry/insert into self.operator_approvals; keep the
existing empty/operator equality checks and then perform the mutation only after
the ownership check.
In `@crates/sentrix-nft/src/registry.rs`:
- Around line 16-19: compute_collection_id currently builds the preimage with a
simple "|" separator which is ambiguous when inputs contain "|" and can lead to
hash collisions; change it to use an unambiguous, canonical encoding (e.g.,
length-prefixed concatenation or a binary serializer like bincode/serde) so the
preimage is always uniquely recoverable from (creator, seed). Update
compute_collection_id to build the payload by writing the creator length then
the creator bytes then the seed length then the seed bytes (or use
bincode::serialize) before hashing; apply the same change to the other similar
function referenced at lines 52-54 so both ID derivations use the same
unambiguous encoding.
🪄 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: c646770c-5817-43c5-b24b-d90060f71af5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (12)
Cargo.tomlcrates/sentrix-core/Cargo.tomlcrates/sentrix-core/src/lib.rscrates/sentrix-core/src/nft.rscrates/sentrix-nft/Cargo.tomlcrates/sentrix-nft/README.mdcrates/sentrix-nft/src/collection.rscrates/sentrix-nft/src/error.rscrates/sentrix-nft/src/events.rscrates/sentrix-nft/src/lib.rscrates/sentrix-nft/src/registry.rscrates/sentrix-nft/src/token.rs
Self-reviewed with the code-review skill BEFORE pushing this time.
CodeRabbit findings (all 5 valid, all fixed):
1. set_approval_for_all now takes caller and enforces caller == owner —
previously any caller could set operator approvals for any owner.
2. compute_collection_id uses length-prefixed hashing
(len(creator)‖creator‖len(seed)‖seed) instead of "{creator}|{seed}" —
the separator was ambiguous: ("a|b","c") and ("a","b|c") collided.
Regression test pins the disambiguation.
3. NftCollection invariant fields are private with read-only getters.
4. mint/transfer/burn compute all checked arithmetic into locals and only
commit after every fallible step succeeds — no partial state on overflow.
5. update_collection_metadata now enforces metadata_mutable (was only on the
token path) — lock_metadata can no longer be bypassed at collection level.
Additional gaps found by self-review (same class as #3, fixed):
- NftToken invariant fields (owner/uri/transferable/frozen/burned) are now
private with getters + pub(crate) setters used only by NftCollection.
uri_hash/metadata_hash stay public (no behavioral invariant).
- NftRegistry.collections is now private (deploy_collection is the only way
to create a collection; serde still persists it).
Deliberately NOT changed (design calls, not bugs): approve/burn do not check
the collection/token frozen flag — freeze blocks transfers; whether it should
also block burns/approvals is a policy decision, left as-is to avoid an
unrequested behavior change.
owner_of now returns Option<&str> (was Option<&String>) since token owner is
read through a getter. New regression tests: set_approval_for_all non-owner
rejected, collection-id separator collision, collection metadata lock.
cargo test (workspace) pass; cargo fmt --check + clippy --workspace -D warnings clean.
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.
Summary
New workspace crate
sentrix-nft— native NFT as a Proof Asset / reputation layer (validator/builder/contributor/bug-hunter/genesis proofs, soulbound badges). Not a marketplace, not a JPEG/PFP collection tool. EVM ERC-721/1155 remain a separate path.Pure domain logic only: no MDBX, trie, block execution, RPC, networking, consensus, marketplace, or bridge. Metadata is URI-only — no image bytes, no IPFS/Arweave upload logic.
Crate layout
collection.rs—NftCollection+ all token state transitionstoken.rs—NftToken(transferable/frozen/burned, optional uri + integrity hashes)registry.rs—NftRegistry, deterministicSRC721_<sha256(creator|seed)>idevents.rs—NftEvent(typed, returned by every state change)error.rs—NftError(typed)Design
Stringaddresses +u64token ids (matches the SRC-20 native rail).transferable=false; transfer fails with typedNotTransferable— no approval bypasses it (checked before authorization).max_supplycounts ever-minted (burning never frees a slot).Intentional behavior changes vs the earlier in-core
nft.rsSentrixError→ typedNftError.NftTokenstruct (enables per-token soulbound/freeze).Integration (thin)
sentrix-coredepends onsentrix-nft;sentrix-core/src/nft.rsis now a facade (pub use sentrix_nft::*) +nft_err_to_sentrix()to bridge errors at the core boundary. No block-execution/trie/RPC wiring yet (deferred).sentrix-nfthas zero sentrix-* deps → no cycle.Tests / gate
sentrix-nft; 2 facade tests in core. Earlier 21 in-core tests superseded (same behaviors + soulbound, freeze, metadata-lock, operator, strict-no-reuse).cargo test(workspace): pass ·cargo fmt --check: clean ·cargo clippy --workspace --all-targets -- -D warnings: cleanDeferred
MDBX/state integration, trie/state-root commitment, native NFT tx execution, JSON-RPC, explorer/wallet, ERC-721 adapter.
Summary by CodeRabbit
New Features
Documentation
Chores