Feat/access control and ownership verification in nft contract #155
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request makes several updates and additions across configuration and NFT contract files. The changes include enhanced file type declarations in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NFTContract
participant Storage
participant Events
User->>NFTContract: initialize(admin)
NFTContract->>Storage: Check admin key & set admin/counter
NFTContract-->>User: Confirmation
User->>NFTContract: mint(to, name, description, attributes)
NFTContract->>Storage: Store token metadata & owner info
NFTContract->>Events: Emit mint event
NFTContract-->>User: Token minted
User->>NFTContract: transfer(from, to, token_id)
NFTContract->>Storage: Retrieve current token owner
NFTContract->>Storage: Update token owner data
NFTContract->>Events: Emit transfer event
NFTContract-->>User: Transfer confirmed
User->>NFTContract: token_metadata(token_id)
NFTContract->>Storage: Retrieve NFT details
NFTContract-->>User: Return NFTDetail(owner, metadata)
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
coderabbitai
bot
added
chore
![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
🔭 Outside diff range comments (1)
apps/contract/contracts/nft/src/types.rs (1)
21-28: 🛠️ Refactor suggestionAdd counter-related variant to DataKey enum.
The
COUNTER_KEYconstant is defined but there's no corresponding variant in theDataKeyenum for token counting functionality.pub enum DataKey { TokenOwner(u32), TokenMetadata(u32), TokenUri(u32), UserBalance(Address), + TokenCounter, }
🧹 Nitpick comments (15)
apps/contract/contracts/nft/src/lib.rs (7)
8-10: Leverage re-exports for convenience.
We could consider re-exporting selectively fromsoroban_sdkif these types are widely used throughout the codebase. This might simplify references in other modules while remaining organized.
12-13: Avoid wildcard imports for clarity.
Although this import is concise, wildcard imports can obscure naming conflicts in bigger projects and hamper readability. More explicit imports can reduce confusion as the codebase grows.
14-16: Add documentation.
We might consider adding a short doc comment (///) aboveNFTContractto explain its purpose, especially for developers new to this contract.
17-26: Check for admin authorization during initialization.
If administrative constraints apply to initializing the contract, consider callingadmin.require_auth()to prevent unauthorized parties from setting critical state.
28-61: Validate large attribute sets & return token_id.
We could enhance this method by adding checks on the size ofdescriptionorattributesto avoid excessive resource usage. Additionally, returning the minted token ID might improve usability for client applications.
63-91: Consider self-transfer handling.
We could clarify whether self-transfers (from == to) are allowed or disallowed. If they’re permitted, documenting any specific reason or effect (like a no-op) could prevent confusion.
93-101: Use typed errors instead of panics.
We might improve production stability by returning aResult<NFTDetail, NFTError>instead of panicking when a token or metadata is missing, ensuring the contract remains robust under unexpected conditions.apps/contract/contracts/nft/src/events.rs (2)
7-8: Event topic structuring.
We could enhance the mint event by including additional context such as basic token metadata. This extra detail might aid in block explorers or external listeners.
12-13: Detailed event data.
Similarly, consider capturing more transfer context (e.g., timestamp, reason) if it supports your monitoring or auditing needs.apps/contract/contracts/nft/src/types.rs (1)
3-4: Consider adding documentation for the constant keys.The constants
ADMIN_KEYandCOUNTER_KEYwould benefit from documentation comments explaining their purpose and usage in the contract.+/// Key used to store the contract admin address pub const ADMIN_KEY: Symbol = symbol_short!("ADMIN"); +/// Key used to track the total number of minted tokens pub const COUNTER_KEY: Symbol = symbol_short!("COUNTER");apps/contract/contracts/nft/src/test.rs (3)
83-84: Translate comments to English for consistency.The comments are in Spanish, which should be translated to English for consistency.
- // Intentar mint sin autorización - env.mock_auths(&[]); // Establecer autorizaciones vacías + // Attempt to mint without authorization + env.mock_auths(&[]); // Set empty authorizations
43-43: Use specific auth mocks instead of mock_all_auths.Consider using specific auth mocks with expected signers instead of
mock_all_auths()for more precise testing.- env.mock_all_auths(); + env.mock_auths(&[ + (&admin, NFTContract::initialize.type_name(), args.clone()), + // Add other expected auth checks + ]);Also applies to: 112-112, 124-124
27-63: Consider adding negative test cases to test_basic_flow.The basic flow test only covers the happy path. Consider adding negative test cases to verify error handling.
I can help generate additional test cases if you'd like.
apps/contract/contracts/nft/test_snapshots/test/test_transfer_ownership.1.json (2)
141-144: Consider validating ownership transfer constraints.The ownership transfer test shows correct balance updates, but we could enhance security by:
- Verifying that only the current owner can initiate transfers
- Adding checks for edge cases like self-transfers
Consider implementing these additional test cases:
- Unauthorized transfer attempt
- Self-transfer validation
- Transfer to zero address
Also applies to: 155-159
194-195: Consider persistent storage for critical data.The nonce entries are marked as temporary. For better security and auditability:
- Consider making authentication-related data persistent
- Add TTL validation for temporary entries
- "durability": "temporary" + "durability": "persistent"Also applies to: 227-228
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/contract/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.cursorrules(1 hunks)apps/contract/.cargo/config.toml(0 hunks)apps/contract/Cargo.toml(2 hunks)apps/contract/contracts/nft/Cargo.toml(1 hunks)apps/contract/contracts/nft/src/contract.rs(0 hunks)apps/contract/contracts/nft/src/distribution.rs(1 hunks)apps/contract/contracts/nft/src/errors.rs(1 hunks)apps/contract/contracts/nft/src/events.rs(1 hunks)apps/contract/contracts/nft/src/lib.rs(1 hunks)apps/contract/contracts/nft/src/storage.rs(1 hunks)apps/contract/contracts/nft/src/test.rs(1 hunks)apps/contract/contracts/nft/src/types.rs(1 hunks)apps/contract/contracts/nft/test_snapshots/test/test_admin_access_control.1.json(1 hunks)apps/contract/contracts/nft/test_snapshots/test/test_basic_flow.1.json(1 hunks)apps/contract/contracts/nft/test_snapshots/test/test_error_handling.1.json(1 hunks)apps/contract/contracts/nft/test_snapshots/test/test_transfer_ownership.1.json(1 hunks)
💤 Files with no reviewable changes (2)
- apps/contract/.cargo/config.toml
- apps/contract/contracts/nft/src/contract.rs
✅ Files skipped from review due to trivial changes (2)
- apps/contract/Cargo.toml
- .cursorrules
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.rs`: Review Rust files for memory safety and ownership...
**/*.rs: Review Rust files for memory safety and ownership patterns. Verify proper error handling using Result/Option types. Check trait implementations and lifetime annotations. Ensure thread safety in concurrent code and proper module organization.
apps/contract/contracts/nft/src/events.rsapps/contract/contracts/nft/src/errors.rsapps/contract/contracts/nft/src/lib.rsapps/contract/contracts/nft/src/types.rsapps/contract/contracts/nft/src/distribution.rsapps/contract/contracts/nft/src/test.rsapps/contract/contracts/nft/src/storage.rs
🧠 Learnings (2)
apps/contract/contracts/nft/src/lib.rs (2)
Learnt from: Bran18
PR: kindfi-org/kindfi#0
File: :0-0
Timestamp: 2025-02-12T22:34:18.818Z
Learning: The KindFi NFT contract should be implemented in Rust using Soroban SDK with:
1. Contract structure in src/contracts/ directory
2. Separate files for contract (nft.rs) and tests (tests/nft_test.rs)
3. Custom error handling for access control and ownership verification
4. Complete test coverage for all scenarios including error cases
Learnt from: Bran18
PR: kindfi-org/kindfi#0
File: :0-0
Timestamp: 2025-02-12T22:34:18.818Z
Learning: The NFT contract implementation in Rust should include:
1. Admin-only access control for minting operations
2. Ownership verification for token transfers
3. Balance tracking per address
4. Custom error handling for unauthorized access and invalid transfers
File structure: src/contracts/nft.rs for implementation and src/contracts/tests/nft_test.rs for tests
apps/contract/contracts/nft/src/test.rs (1)
Learnt from: Bran18
PR: kindfi-org/kindfi#0
File: :0-0
Timestamp: 2025-02-12T22:34:18.818Z
Learning: The NFT contract implementation in Rust should include:
1. Admin-only access control for minting operations
2. Ownership verification for token transfers
3. Balance tracking per address
4. Custom error handling for unauthorized access and invalid transfers
File structure: src/contracts/nft.rs for implementation and src/contracts/tests/nft_test.rs for tests
🔇 Additional comments (6)
apps/contract/contracts/nft/Cargo.toml (2)
5-5: Great addition of the publish flag!The
publish = falseflag is a good security practice that prevents accidental publishing of private packages.
9-9:❓ Verification inconclusive
Consider the implications of disabling doctests.
While disabling doctests can speed up the build process, they serve as valuable documentation and verification tools. Could we discuss the rationale behind this decision and whether selective doctest enabling might be beneficial for critical NFT operations?
Let's check if other contracts follow the same pattern:
🏁 Script executed:
#!/bin/bash # Description: Check doctest configuration in other contract Cargo.toml files fd -t f "Cargo.toml" apps/contract/contracts -x grep -H "doctest"Length of output: 192
Revisit the doctest configuration approach
Upon verification, we found that both the NFT and Auth contracts disable doctests (as seen in
apps/contract/contracts/nft/Cargo.tomlandapps/contract/contracts/auth-contracts/account/Cargo.toml). While this consistency might help speed up build times, it also removes an important layer of inline documentation and automated verification.
- Consider discussing the rationale behind completely disabling doctests.
- It may be worthwhile to evaluate if selective enabling of doctests, especially in critical areas such as NFT operations, could enhance code reliability without significantly impacting build performance.
apps/contract/contracts/nft/src/lib.rs (1)
104-105: Good test module structure.
Keeping tests in a dedicated module is a neat way to isolate and maintain test logic. This approach remains clear as the codebase expands.apps/contract/contracts/nft/src/errors.rs (1)
4-10:❓ Verification inconclusive
Revisit error code references.
Renumbering these variants may impact any code or documentation referencing their old numeric values. Consider verifying that all references have been updated to avoid inconsistencies.
🏁 Script executed:
#!/bin/bash # Searches the codebase for possible references to old NFCError numeric values. rg 'NFTError::(AlreadyInitialized|NotAuthorized|TokenAlreadyExists|TokenNotFound|NotTokenOwner).*100' -A 3 rg 'NFTError::(AlreadyInitialized|NotAuthorized|TokenAlreadyExists|TokenNotFound|NotTokenOwner).*101' -A 3 rg 'NFTError::(AlreadyInitialized|NotAuthorized|TokenAlreadyExists|TokenNotFound|NotTokenOwner).*102' -A 3 rg 'NFTError::(AlreadyInitialized|NotAuthorized|TokenAlreadyExists|TokenNotFound|NotTokenOwner).*103' -A 3 rg 'NFTError::(AlreadyInitialized|NotAuthorized|TokenAlreadyExists|TokenNotFound|NotTokenOwner).*104' -A 3Length of output: 544
Attention: Confirm All References to NFTError Numeric Values
The renumbering of the NFTError variants appears correct, but please ensure there are no lingering references to the old numeric values in any consumers of this module. Although our automated search did not produce any hits for numeric references (e.g., values 100–104), it’s advisable to manually verify all integration points, tests, and documentation that might reference these numeric codes.
- Action: Double-check integration tests, external modules, or documentation for assumptions about the original NFTError numeric values.
- Note: Even though the search didn't return matches, manual verification is encouraged to mitigate any risks of silent dependency breakage.
apps/contract/contracts/nft/src/events.rs (1)
1-1: Refined import.
Adoptingsymbol_shortneatly captures concise event labels, keeping event topics more readable.apps/contract/contracts/nft/test_snapshots/test/test_admin_access_control.1.json (1)
40-40:❓ Verification inconclusive
Consider adding WASM code validation.
The contract code hash is present but the actual WASM code is empty. We should ensure that the test snapshots include valid WASM code for comprehensive testing of the contract deployment and execution.
Also applies to: 56-57
🏁 Script executed:
#!/bin/bash # Description: Verify if the WASM code is present in the contract implementation ast-grep --pattern 'fn deploy($_, $_) { $$$ }'Length of output: 50
WASM Validation Uncertain – Manual Verification Recommended
I attempted to identify the deployment logic via the AST grep search, but no output was produced, suggesting that our current automated check isn’t confirming the presence of a deployment function with valid WASM code. The test snapshot currently shows a WASM hash corresponding to an empty file, which might not be suitable for thorough contract testing.
Consider manually verifying that:
- The contract deployment (or equivalent) logic actually includes validation or integration of non-empty, valid WASM code.
- The WASM code in the snapshot test—both at line 40 and lines 56-57—is updated to reflect real, deployable WASM content rather than merely an empty hash.
This manual verification will help ensure comprehensive testing of the contract deployment and execution.
| name = "kindfi-nft" | ||
| version = "0.1.0" | ||
| name = "nft" | ||
| version = "0.0.0" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider clarifying the version number strategy.
The version has been set to "0.0.0", which typically indicates pre-development stage. Given that this is implementing access control features, we might want to align this with a more appropriate semantic version.
Let's check the versioning strategy across other contracts:
🏁 Script executed:
#!/bin/bash
# Description: Check version numbers in other contract Cargo.toml files
fd -t f "Cargo.toml" apps/contract/contracts -x grep -H "^version"Length of output: 367
Clarify Versioning and Doctest Strategy for the NFT Contract
- The NFT contract's
Cargo.tomlspecifies the version as"0.0.0", which contrasts with the"0.1.0"versions used in other contracts. Consider verifying if this is an intentional marker for pre-development or if it should be aligned with the standard semantic versioning. - Additionally, since doctests have been disabled (
doctest = false), please document the rationale behind this choice to ensure clarity on how it affects code documentation and testing practices.
| if owner != from { | ||
| panic!("Not token owner"); | ||
| } | ||
|
|
||
| // Verify authorization | ||
| let sender = env.invoker(); | ||
| if sender != from { | ||
| panic!("Not authorized"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace panic with Result for error handling.
Consider using the Result type with custom errors instead of panic for better error handling and API consistency.
- if owner != from {
- panic!("Not token owner");
- }
+ if owner != from {
+ return Err(NFTError::NotTokenOwner);
+ }
// Verify authorization
let sender = env.invoker();
- if sender != from {
- panic!("Not authorized");
- }
+ if sender != from {
+ return Err(NFTError::NotAuthorized);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if owner != from { | |
| panic!("Not token owner"); | |
| } | |
| // Verify authorization | |
| let sender = env.invoker(); | |
| if sender != from { | |
| panic!("Not authorized"); | |
| } | |
| if owner != from { | |
| return Err(NFTError::NotTokenOwner); | |
| } | |
| // Verify authorization | |
| let sender = env.invoker(); | |
| if sender != from { | |
| return Err(NFTError::NotAuthorized); | |
| } |
| let token_key = symbol_short!(&format!("TOKEN_{}", token_id)); | ||
| let owner: Address = env.storage().instance().get(&token_key).unwrap_or_else(|| { | ||
| panic!("Token not found"); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace string formatting with DataKey enum for token key.
The current implementation uses string formatting to generate token keys, which is inconsistent with the DataKey enum and less efficient.
- let token_key = symbol_short!(&format!("TOKEN_{}", token_id));
- let owner: Address = env.storage().instance().get(&token_key).unwrap_or_else(|| {
- panic!("Token not found");
- });
+ let owner = env.storage().instance()
+ .get(&DataKey::TokenOwner(token_id))
+ .ok_or(NFTError::TokenNotFound)?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let token_key = symbol_short!(&format!("TOKEN_{}", token_id)); | |
| let owner: Address = env.storage().instance().get(&token_key).unwrap_or_else(|| { | |
| panic!("Token not found"); | |
| }); | |
| let owner = env.storage().instance() | |
| .get(&DataKey::TokenOwner(token_id)) | |
| .ok_or(NFTError::TokenNotFound)?; |
| pub fn get_admin(env: &Env) -> Address { | ||
| env.storage().instance().get(&ADMIN_KEY).unwrap() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add fallback for missing admin address.
The get_admin function uses unwrap which could panic if the admin address is not set.
- pub fn get_admin(env: &Env) -> Address {
- env.storage().instance().get(&ADMIN_KEY).unwrap()
+ pub fn get_admin(env: &Env) -> Result<Address, NFTError> {
+ env.storage().instance()
+ .get(&ADMIN_KEY)
+ .ok_or(NFTError::AdminNotSet)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn get_admin(env: &Env) -> Address { | |
| env.storage().instance().get(&ADMIN_KEY).unwrap() | |
| } | |
| pub fn get_admin(env: &Env) -> Result<Address, NFTError> { | |
| env.storage().instance() | |
| .get(&ADMIN_KEY) | |
| .ok_or(NFTError::AdminNotSet) | |
| } |
| pub fn decrement_balance(env: &Env, address: &Address) { | ||
| let balance: u32 = env | ||
| .storage() | ||
| .instance() | ||
| .get(&DataKey::UserBalance(address.clone())) | ||
| .unwrap_or(0); | ||
| env.storage() | ||
| .instance() | ||
| .set(&DataKey::UserBalance(address.clone()), &(balance - 1)); | ||
| } |
There was a problem hiding this comment.
Add underflow check in decrement_balance.
The decrement_balance function should check for underflow before decrementing the balance.
pub fn decrement_balance(env: &Env, address: &Address) {
let balance: u32 = env
.storage()
.instance()
.get(&DataKey::UserBalance(address.clone()))
.unwrap_or(0);
+ if balance == 0 {
+ panic!("Balance underflow");
+ }
env.storage()
.instance()
.set(&DataKey::UserBalance(address.clone()), &(balance - 1));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn decrement_balance(env: &Env, address: &Address) { | |
| let balance: u32 = env | |
| .storage() | |
| .instance() | |
| .get(&DataKey::UserBalance(address.clone())) | |
| .unwrap_or(0); | |
| env.storage() | |
| .instance() | |
| .set(&DataKey::UserBalance(address.clone()), &(balance - 1)); | |
| } | |
| pub fn decrement_balance(env: &Env, address: &Address) { | |
| let balance: u32 = env | |
| .storage() | |
| .instance() | |
| .get(&DataKey::UserBalance(address.clone())) | |
| .unwrap_or(0); | |
| if balance == 0 { | |
| panic!("Balance underflow"); | |
| } | |
| env.storage() | |
| .instance() | |
| .set(&DataKey::UserBalance(address.clone()), &(balance - 1)); | |
| } |
| "contract_fn": { | ||
| "contract_address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM", | ||
| "function_name": "", | ||
| "args": [] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing function invocation testing.
The function name is empty in the contract invocation. We could improve test coverage by:
- Adding specific function names for each test scenario
- Including relevant arguments for the function calls
"contract_fn": {
"contract_address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM",
- "function_name": "",
- "args": []
+ "function_name": "mint",
+ "args": ["Test Token", "Test Description", []]
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "contract_fn": { | |
| "contract_address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM", | |
| "function_name": "", | |
| "args": [] | |
| "contract_fn": { | |
| "contract_address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM", | |
| "function_name": "mint", | |
| "args": ["Test Token", "Test Description", []] |
| "auth": [ | ||
| [], | ||
| [] | ||
| ], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider expanding error handling test scenarios.
The current error handling test structure could be enhanced by adding more scenarios:
- Invalid token ID handling
- Overflow checks for counters
- Authorization failures
Would you like me to generate additional test scenarios for comprehensive error handling coverage?
Implement access control and ownership verification in nft contract
Summary by CodeRabbit
New Features
Refactor
Tests