test(torii): add metadata to erc1155 and update #3054
Conversation
|
Ohayo, sensei! Here’s the updated overview of the changes: WalkthroughA new external function Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant E as ERC1155 Contract
C->>E: Call token_uri(token_id)
E->>E: Build JSON with image URL using block number
E-->>C: Return ByteArray with JSON
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
examples/spawn-and-move/src/externals/erc1155.cairo (2)
64-68: Ohayo! Consider adding token_id validation, sensei! 🎭The token URI implementation is creative, using DiceBear for generating unique avatars. However, we should validate that the token_id exists before generating the URI.
#[external(v0)] fn token_uri(ref self: ContractState, token_id: u256) -> ByteArray { + // Verify token exists + assert(self.erc1155.balance_of(starknet::get_caller_address(), token_id) > 0, 'Token does not exist'); let tx_hash = starknet::get_tx_info().transaction_hash; format!("{{ \"image\": \"https://api.dicebear.com/9.x/lorelei-neutral/png?seed={:x}\" }}", tx_hash) }
60-63:❓ Verification inconclusive
Arigato for the clear testing disclaimer, sensei! 🛡️
The comment clearly indicates this is for testing. For production, consider:
- Rate limiting token_uri calls
- Adding access control for minting
- Implementing proper safe transfer checks
Let's verify the contract's testing nature is documented in the repository:
🏁 Script executed:
#!/bin/bash # Search for testing/example documentation rg -i "test|example" README.mdLength of output: 509
Ohayo, sensei! Thanks for the clear testing disclaimer.
Your inline comment in
examples/spawn-and-move/src/externals/erc1155.cairo(lines 60-63) clearly marks this implementation as for testing and quick minting only. However, our search in the project README didn't reveal additional documentation supporting this testing-only use. Please ensure that this intended use case is well communicated across the repository to avoid any deployment mix-ups later.For production, consider:
- Rate limiting token_uri calls to mitigate abuse.
- Adding access control for minting to secure token issuance.
- Implementing proper safe transfer checks to strengthen overall security.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
examples/spawn-and-move/Scarb.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
examples/spawn-and-move/src/externals/erc1155.cairo(1 hunks)
🔇 Additional comments (1)
examples/spawn-and-move/src/externals/erc1155.cairo (1)
10-10: Sugoi implementation of ERC4906 integration, sensei! ✨The ERC4906 component integration for metadata updates pairs nicely with the new token_uri function. This provides a complete solution for token metadata management.
Also applies to: 15-15, 23-23
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
examples/spawn-and-move/src/externals/erc1155.cairo (1)
64-64: Ohayo sensei! Let's add some documentation for our new function.Consider adding natspec documentation to match other functions and comply with standards:
#[external(v0)] +/// @notice Get the URI for a given token ID +/// @dev Returns a data URI containing token metadata in JSON format +/// @param token_id The ID of the token to query +/// @return ByteArray containing the token's metadata URI fn token_uri(ref self: ContractState, token_id: u256) -> ByteArray {examples/spawn-and-move/manifest_dev.json (1)
3918-3929: Ohayo, sensei! update_tokens_metadata Function Enhancement for ERC1155TokenThe addition of the
update_tokens_metadatafunction, which accepts an array (via the Span struct) of token IDs, is a nice touch for flexibility. It would be beneficial to have unit tests covering this new functionality if you haven’t added them already.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/spawn-and-move/manifest_dev.json(12 hunks)examples/spawn-and-move/src/externals/erc1155.cairo(1 hunks)
🔇 Additional comments (7)
examples/spawn-and-move/manifest_dev.json (7)
2426-2429: Ohayo, sensei! External Contract Details for ERC721Token UpdatedThe new
class_hashandaddressvalues for the ERC721Token (instance "Badge") are now updated. Please confirm that these values match your deployed contract details to ensure proper external integration.
2457-2506: Ohayo, sensei! Adding Metadata Update Functions for ERC721TokenNice work on adding the new metadata update functions to the ERC721Token ABI – namely,
update_token_metadata,update_batch_token_metadata, andupdate_tokens_metadata. The input types (usingcore::integer::u256and the new Span struct) look correct, but please double-check that these ABI entries fully match your contract implementation and caller expectations.
3771-3774: Ohayo, sensei! ERC1155Token External Contract Details UpdatedThe ERC1155Token (instance "Rewards") now reflects updated
class_hashandaddressvalues. Ensure these values are correctly deployed and that all downstream references are updated accordingly.
3790-3807: Ohayo, sensei! Updated ByteArray Struct in ERC1155Token ABIThe updated definition for
core::byte_array::ByteArrayin this segment now uses the expected members. Verify that this new structure is consistent with how thetoken_urifunction will assemble and return its JSON metadata payload.
3808-3823: Ohayo, sensei! New token_uri Function Added for ERC1155TokenThe introduction of the
token_urifunction here is great. It accepts atoken_idand returns aByteArraycontaining the JSON metadata, as intended. Everything appears to be in order with the function signature and return type.
3891-3901: Ohayo, sensei! New update_token_metadata Function for ERC1155TokenThe new function
update_token_metadatain the ERC1155Token section is a solid addition for updating individual token metadata. Please ensure that the metadata handling logic on the contract side (not shown here) is consistent with this ABI.
3903-3917: Ohayo, sensei! Batch Metadata Update Function for ERC1155TokenThe implementation of
update_batch_token_metadatato allow bulk updates is well integrated. Make sure the semantics of the "from" and "to" token IDs cover all edge cases, particularly with sequential token IDs.
| #[external(v0)] | ||
| fn token_uri(ref self: ContractState, token_id: u256) -> ByteArray { | ||
| let tx_hash = starknet::get_tx_info().transaction_hash; | ||
| format!("data:application/json,{{ \"image\": \"https://api.dicebear.com/9.x/lorelei-neutral/png?seed={}\" }}", tx_hash) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Let's make the token URI implementation more robust.
The current implementation has a few concerns:
- Using transaction hash means the same token could have different URIs in different transactions
- External dependency on dicebear service could be risky if the service changes/goes down
Consider this improved implementation:
#[external(v0)]
fn token_uri(ref self: ContractState, token_id: u256) -> ByteArray {
+ // Validate token exists
+ assert(self.erc1155._exists(token_id), 'ERC1155: URI query for nonexistent token');
+
+ // Use token_id for deterministic image generation
let tx_hash = starknet::get_tx_info().transaction_hash;
- format!("data:application/json,{{ \"image\": \"https://api.dicebear.com/9.x/lorelei-neutral/png?seed={}\" }}", tx_hash)
+ format!(
+ "data:application/json,{{\"name\":\"Token #{}\",\"image\":\"https://api.dicebear.com/9.x/lorelei-neutral/png?seed={}\"}}",
+ token_id,
+ token_id
+ )
}📝 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.
| #[external(v0)] | |
| fn token_uri(ref self: ContractState, token_id: u256) -> ByteArray { | |
| let tx_hash = starknet::get_tx_info().transaction_hash; | |
| format!("data:application/json,{{ \"image\": \"https://api.dicebear.com/9.x/lorelei-neutral/png?seed={}\" }}", tx_hash) | |
| } | |
| #[external(v0)] | |
| fn token_uri(ref self: ContractState, token_id: u256) -> ByteArray { | |
| // Validate token exists | |
| assert(self.erc1155._exists(token_id), 'ERC1155: URI query for nonexistent token'); | |
| // Use token_id for deterministic image generation | |
| let tx_hash = starknet::get_tx_info().transaction_hash; | |
| format!( | |
| "data:application/json,{{\"name\":\"Token #{}\",\"image\":\"https://api.dicebear.com/9.x/lorelei-neutral/png?seed={}\"}}", | |
| token_id, | |
| token_id | |
| ) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
crates/torii/sqlite/src/utils.rs (3)
63-63: Ohayo sensei! Consider using a more descriptive variable name.The variable name
backslash_countcould be more descriptive to indicate its purpose in tracking escape sequences.- let mut backslash_count = 0; + let mut escape_sequence_count = 0;
75-78: Consider extracting backslash handling into a separate function.The backslash handling logic could be clearer if extracted into a dedicated function.
+ fn handle_backslash(count: &mut usize) -> char { + *count += 1; + '\\' + }
79-104: Consider simplifying the nested conditionals.The deep nesting of conditionals makes the code harder to follow. Consider extracting the quote handling logic into a separate function.
+ fn handle_quote(chars: &mut std::iter::Peekable<std::str::Chars>, backslash_count: usize) -> (bool, String) { + if backslash_count % 2 != 0 { + return (true, "\"".to_string()); + } + + let mut temp_chars = chars.clone(); + while let Some(&next_c) = temp_chars.peek() { + if !next_c.is_whitespace() { + return match next_c { + ':' | ',' | '}' => (false, "\"".to_string()), + _ => (true, "\\\"".to_string()) + }; + } + temp_chars.next(); + } + (false, "\"".to_string()) + }crates/torii/indexer/src/test.rs (3)
933-935: Ohayo sensei! The test function looks good but could use more documentation.Consider adding a doc comment explaining the test's purpose and expected behavior.
+/// Tests the ERC1155 token metadata update functionality: +/// 1. Mints a new token +/// 2. Updates its metadata +/// 3. Verifies the metadata contains the correct image URL
966-976: Consider adding assertions for successful minting.The test should verify that the token was minted successfully before proceeding with metadata updates.
TransactionWaiter::new(tx.transaction_hash, &provider).await.unwrap(); + + // Verify successful minting + let balance = provider + .call( + Call { + to: rewards_address, + selector: get_selector_from_name("balance_of").unwrap(), + calldata: vec![account.address(), Felt::from(1), Felt::ZERO], + }, + BlockId::Latest, + ) + .await + .unwrap(); + assert_eq!(balance[0], Felt::from(1), "Token should be minted with amount 1");
1025-1028: Add more comprehensive metadata verification.The test only checks for the image URL. Consider verifying other metadata fields as well.
assert!(token.metadata.contains(&format!( "\"image\": \"https://api.dicebear.com/9.x/lorelei-neutral/png?seed={}\"", block_number ))); + // Verify other metadata fields + assert!(token.metadata.contains("\"name\"")); + assert!(token.metadata.contains("\"description\"")); + assert!(token.metadata.contains("\"attributes\""));examples/spawn-and-move/manifest_dev.json (2)
2457-2506: Ohayo sensei! New metadata update functions added for ERC721Token.
The ABI now includes additional functions:update_token_metadata,update_batch_token_metadata, andupdate_tokens_metadataalong with the associated struct forcore::array::Span::<core::integer::u256>. These additions align with enriched contract capabilities. Please ensure that these function signatures match the corresponding implementation in the contract and that comprehensive tests are in place for metadata updates.
3890-3929: Ohayo sensei! Added metadata update functions for ERC1155Token.
The ABI now includesupdate_token_metadata,update_batch_token_metadata, andupdate_tokens_metadatafor ERC1155Token. These functions are analogous to those for ERC721Token and aim to improve metadata management. Please confirm that the newly added functions’ inputs and state mutability are in line with your design intentions and that you have tests covering various metadata update scenarios.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
crates/torii/types-test/Scarb.lockis excluded by!**/*.lockexamples/simple/Scarb.lockis excluded by!**/*.lockspawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (4)
crates/torii/indexer/src/test.rs(1 hunks)crates/torii/sqlite/src/utils.rs(1 hunks)examples/spawn-and-move/manifest_dev.json(12 hunks)examples/spawn-and-move/src/externals/erc1155.cairo(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/spawn-and-move/src/externals/erc1155.cairo
🔇 Additional comments (3)
examples/spawn-and-move/manifest_dev.json (3)
2426-2429: Ohayo sensei! Review the updated ERC721Token deployment.
The external contract definition for ERC721Token now shows an updatedclass_hashandaddress(notice the tilde markers on these lines). Please double-check that these new values exactly match the intended deployed contract details.
3771-3774: Ohayo sensei! Confirm the updated ERC1155Token deployment details.
The external contract for ERC1155Token now shows an updatedclass_hashandaddress(with tilde markers on these lines) and uses the instance name"Rewards". Please verify that these deployment details are consistent with your intended network configuration.
3790-3823: Ohayo sensei! Newtoken_urifunction detected in ERC1155Token ABI.
A new functiontoken_uri(token_id: core::integer::u256)has been added that returns acore::byte_array::ByteArray. This is expected to wrap a JSON string containing an image URL based on the token ID. Ensure that the implementation in the corresponding Cairo contract (e.g. inerc1155.cairo) matches this ABI and that the JSON structure is validated in your tests.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3054 +/- ##
==========================================
+ Coverage 57.65% 57.82% +0.16%
==========================================
Files 439 439
Lines 59437 59543 +106
==========================================
+ Hits 34267 34429 +162
+ Misses 25170 25114 -56 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/torii/sqlite/src/utils.rs (2)
74-77: Consider adding a maximum backslash limit.While the backslash counting logic is correct, extremely long sequences of backslashes could lead to unnecessary counting. Consider adding a reasonable limit.
} else if c == '\\' { + if backslash_count < 1000 { // Arbitrary limit backslash_count += 1; + } result.push('\\');
271-281: Test coverage looks good!The test cases cover both unescaped and properly escaped scenarios.
However, consider adding these edge cases:
- Empty strings
- Multiple consecutive unescaped quotes
- Maximum backslash sequences
+ #[test] + fn test_sanitize_json_string_edge_cases() { + // Empty string + assert_eq!(sanitize_json_string(r#""""#), r#""""#); + + // Multiple consecutive unescaped quotes + assert_eq!( + sanitize_json_string(r#"{"key":""value""}"#), + r#"{"key":"\"value\""}"# + ); + + // Many backslashes + assert_eq!( + sanitize_json_string(r#"{"key":"\\\\"}"#), + r#"{"key":"\\\\"}"# + ); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/sqlite/src/utils.rs(1 hunks)
🔇 Additional comments (4)
crates/torii/sqlite/src/utils.rs (4)
63-63: Ohayo! Nice addition of backslash tracking, sensei!The introduction of
backslash_countimproves escape sequence handling.
66-73: LGTM! Clean handling of non-string context.The logic for handling characters outside of string context is clear and straightforward.
78-103: Excellent string boundary detection, sensei!The logic for detecting string boundaries by looking ahead at non-whitespace characters is robust and handles edge cases well.
104-112: Clean state management.Good practice resetting the backslash count after processing each character.
Summary by CodeRabbit