CEP-96 #632
CEP-96 #632
Conversation
Benchmark report
|
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Approve with suggestions
This PR introduces CEP-96 contract metadata functionality with no critical issues, but several improvements are suggested for flexibility, error handling, and documentation.
📄 Documentation Diagram
This diagram documents the new CEP-96 metadata integration workflow in the example contract.
sequenceDiagram
participant U as User
participant C as Cep96Cep95 Contract
participant T as Cep95 Module
participant M as Cep96 Module
U->>C: deploy with init params
C->>T: init(name, symbol)
C->>M: init(metadata...)
note over C,M: PR #35;632 adds CEP-96 metadata support
U->>C: mint(to, token_id, metadata)
C->>T: raw_mint(...)
U->>C: query contract_name()
C->>M: contract_name()
M-->>C: returns Option<String>
C-->>U: returns name
🌟 Strengths
- Clean implementation of the CEP-96 standard with functional examples.
- Passing tests demonstrate core functionality integration.
Since the PR has 5 findings, below is a summary table:
| Priority | File | Category | Impact Summary (≤12 words) | Anchors |
|---|---|---|---|---|
| P2 | modules/src/cep96/storage.rs | Architecture | Storage immutability may cause silent failures and inflexible updates. | symbol:Cep95::raw_mint, path:modules/src/cep95.rs |
| P2 | examples/src/contracts/cep96_cep95.rs | Correctness and Business Logic | Silent burn failure confuses users and complicates debugging. | path:examples/src/contracts/owned_cep95.rs, method:burn |
| P2 | modules/src/cep96.rs | Documentation | Lack of documentation increases integration risk and learning curve. | |
| P2 | modules/src/cep96.rs | Architecture | Inflexible initialization prevents metadata correction, risking incomplete contracts. | path:modules/src/cep96/storage.rs, method:set |
| P2 | examples/src/contracts/cep96_cep95.rs | Testing | Missing negative tests reduce robustness and error handling demonstration. | path:modules/src/cep95.rs, method:init |
🔍 Notable Themes
- Metadata Immutability: The storage design prevents updates, which could lead to permanently incorrect or incomplete metadata.
- Error Handling: Several functions lack clear error messages, potentially confusing users and complicating debugging.
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
| pub fn set(&self, value: String) { | ||
| if self.get().is_none() { | ||
| self.env().set_named_value(KEY_CONTRACT_NAME, value); | ||
| } | ||
| } |
There was a problem hiding this comment.
P2 | Confidence: High
The CEP-96 storage modules enforce immutability after the first set() call, ignoring subsequent attempts. This design is brittle and may lead to silent failures, especially when integrated with existing token modules like Cep95. The raw_mint method in CEP-95 (and related contracts) is likely to be called multiple times. If CEP-96 metadata were to be set or updated later (e.g., via an update_metadata function), the current set logic would fail silently, potentially confusing users and leaving the contract in an incorrect state. This inflexibility clashes with common NFT contract patterns where collection metadata may need to be corrected or updated.
| pub fn set(&self, value: String) { | |
| if self.get().is_none() { | |
| self.env().set_named_value(KEY_CONTRACT_NAME, value); | |
| } | |
| } | |
| pub fn set(&self, value: String) { | |
| self.env().set_named_value(KEY_CONTRACT_NAME, value); | |
| } |
Evidence: symbol:Cep95::raw_mint, path:modules/src/cep95.rs
| pub fn burn(&mut self, token_id: U256) { | ||
| let owner = self.token.owner_of(token_id); | ||
| let caller = self.env().caller(); | ||
| if Some(caller) == owner { | ||
| self.token.raw_burn(token_id); | ||
| } | ||
| } |
There was a problem hiding this comment.
P2 | Confidence: High
The burn function silently does nothing if the caller is not the token owner. This is inconsistent with the pattern established in the related owned_cep95.rs contract, where the function also does nothing on a non-owner call. However, this is a flawed pattern that can lead to user confusion and failed transactions without clear error messages. Burning is a critical, irreversible operation; the contract should either revert with a clear error (e.g., "Caller is not the token owner") or implement proper authorization logic (e.g., allowing an approved operator or a contract owner to burn). The silent failure masks intent and makes debugging difficult.
Code Suggestion:
pub fn burn(&mut self, token_id: U256) {
let owner = self.token.owner_of(token_id)
.unwrap_or_else(|| self.env().revert(Error::TokenNonExistent)); // Assuming an Error enum exists.
let caller = self.env().caller();
if caller != owner {
// Also consider checking `is_approved_for_all` or `approved_for` if operators should be allowed.
self.env().revert(Error::Unauthorized);
}
self.token.raw_burn(token_id);
}Evidence: path:examples/src/contracts/owned_cep95.rs, method:burn
| pub trait Cep96ContractMetadata { | ||
| /// Contract's human-readable name. | ||
| fn contract_name(&self) -> Option<String>; | ||
|
|
||
| /// Brief description of the contract. | ||
| fn contract_description(&self) -> Option<String>; | ||
|
|
||
| /// URI pointing to the contract's icon image. | ||
| fn contract_icon_uri(&self) -> Option<String>; | ||
|
|
There was a problem hiding this comment.
P2 | Confidence: High
The module starts with #![allow(missing_docs)], and the public trait Cep96ContractMetadata lacks comprehensive documentation. While the field names are descriptive, there is no documentation explaining the standard (CEP-96) this implements, the expected format of URIs (e.g., HTTPS, IPFS), maximum length constraints, or the mutability semantics (immutable after first set, as per the storage implementation). This lack of documentation increases the learning curve for developers and the risk of improper integration. Public traits that define a standard's interface should be thoroughly documented.
Code Suggestion:
//! Implementation of the Casper CEP-96 Standard: Contract Metadata.
//! Provides a standard interface for retrieving metadata about a smart contract (name, description, icon, project link).
/// Trait defining the CEP-96 standard interface for contract metadata.
/// All fields are optional as per the standard.
pub trait Cep96ContractMetadata {
/// Returns the human-readable name of the contract (e.g., "My NFT Collection").
/// Returns `None` if not set.
fn contract_name(&self) -> Option<String>;
/// Returns a brief description of the contract's purpose.
/// Returns `None` if not set.
fn contract_description(&self) -> Option<String>;
/// Returns a URI (e.g., HTTPS, IPFS) pointing to an icon image representing the contract.
/// The image should be square and at least 512x512 pixels.
/// Returns `None` if not set.
fn contract_icon_uri(&self) -> Option<String>;
/// Returns a URI (e.g., HTTPS, IPFS) pointing to the project's main website or documentation.
/// Returns `None` if not set.
fn contract_project_uri(&self) -> Option<String>;
}| } | ||
|
|
||
| impl Cep96 { | ||
| pub fn init( |
There was a problem hiding this comment.
P2 | Confidence: High
The init method only sets values that are provided as Some. Combined with the storage modules' logic that prevents updates after the first set, this creates an initialization pattern that cannot be corrected if a mistake is made (e.g., a typo in the name). If init is called with None for a field, that field can never be initialized later via the Cep96 module's API. This is a rigid architectural choice that could lead to permanently incomplete contract metadata. The design should either allow subsequent updates (changing the storage logic) or require all fields to be provided at initialization, failing otherwise.
Code Suggestion:
Option A (Allow updates, requires changing storage `set` logic):
pub fn init(...) { /* same, but storage set() allows overwrites */ }
// Later, add an `update_metadata` method with proper access control.
Option B (Require complete initialization):
pub fn init(
&self,
name: String,
description: String,
icon_uri: String,
project_uri: String
) {
self.contract_name.set(name);
self.contract_description.set(description);
self.contract_icon_uri.set(icon_uri);
self.contract_project_uri.set(project_uri);
}Evidence: path:modules/src/cep96/storage.rs, method:set
| #[odra::module] | ||
| impl Cep96Cep95 { | ||
| /// Initializes the contract with CEP-95 token params and CEP-96 metadata. | ||
| pub fn init( |
There was a problem hiding this comment.
P2 | Confidence: High
The example contract Cep96Cep95 initializes the Cep95 submodule with self.token.init(name, symbol). The related context for Cep95 (templates and other examples) shows that Cep95::init typically sets the name and symbol via self.name.set() and self.symbol.set(). The provided tests pass, indicating the integration works. However, this example does not demonstrate or test error scenarios, such as initializing metadata twice (which would be silently ignored) or unauthorized mint/burn attempts. The tests are positive cases only. Adding negative test cases would improve robustness and serve as better documentation for the module's behavior.
Benchmark report
|
Benchmark report
|
No description provided.