feat(torii-indexer): eip 4906 update metadata processor #2984
Conversation
WalkthroughOhayo, sensei! This pull request introduces ERC-4906 support by adding new processors for both single and batch metadata updates, extending the event handling capabilities within the indexer engine. It refactors executor and SQLite functionality to improve NFT metadata registration and updating, and enhances spawn-and-move examples with new ERC4906 components and interfaces for emitting metadata events. Changes
Sequence Diagram(s)sequenceDiagram
participant B as Blockchain Event
participant E as Engine
participant P as ERC4906MetadataUpdateProcessor
participant D as Database/Sql
B->>E: Emit MetadataUpdate event
E->>P: Route event to processor
P->>D: Update NFT metadata record
D-->>P: Acknowledge update
P-->>E: Complete processing
sequenceDiagram
participant B as Blockchain Event
participant E as Engine
participant P as ERC4906BatchMetadataUpdateProcessor
participant D as Database/Sql
B->>E: Emit BatchMetadataUpdate event
E->>P: Route event to processor
P->>D: Loop: update metadata for each token in range
D-->>P: Acknowledge each update
P-->>E: Complete batch processing
Possibly related PRs
Suggested reviewers
🪧 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: 1
🧹 Nitpick comments (2)
crates/torii/indexer/src/processors/erc4906_metadata_update.rs (1)
45-66: Task identifier generation is clear, but watch out for collision risk.
Although aDefaultHasheris generally fine, collisions can occur. If collisions become problematic, consider using a more robust hashing approach (e.g., SHA-256).crates/torii/sqlite/src/executor/erc.rs (1)
236-265: Solid metadata fetching error handling.
The fallback to an empty string if an error occurs is user-friendly. Should you need to differentiate partial vs. total failures, consider logging more details or re-trying.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/torii/indexer/src/processors/erc4906_metadata_update.rs(1 hunks)crates/torii/indexer/src/processors/mod.rs(1 hunks)crates/torii/sqlite/src/executor/erc.rs(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/torii/indexer/src/processors/mod.rs
🔇 Additional comments (6)
crates/torii/indexer/src/processors/erc4906_metadata_update.rs (3)
1-11: Ohayo sensei! Imports look organized and consistent.
No issues found with these initial lines, and all dependencies seem necessary.
18-29: Good approach on setting the event key.
Returning"MetadataUpdate"clarifies intent and simplifies subsequent usage. No further adjustments needed.
31-43: Validation logic looks straightforward.
It properly distinguishes single vs. batch metadata updates by checking the event keys count and data emptiness.crates/torii/sqlite/src/executor/erc.rs (3)
183-234: Fetching token URI is well-structured, sensei!
The fallback totokenURIis clever, and the warning for both failures is helpful. Just ensure that the empty string fallback won't cause unexpected behaviors downstream.
267-287: Metadata update method looks clean.
Storing metadata based on the newly fetched URI is separated nicely. This improves maintainability.
289-313: Great use of functional decomposition inprocess_register_erc721_token_query.
The modular approach (fetching separately) streamlines the logic. No major issues here.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
crates/torii/indexer/src/processors/erc4906_metadata_update.rs (4)
30-42: Ohayo sensei, consider validating the batch rangeIn the batch update path (
event.keys.len() == 5), you might want to confirmfrom_token_id <= to_token_idin thisvalidatemethod to avoid silently skipping or handling invalid ranges.🧰 Tools
🪛 GitHub Actions: ci
[warning] 33-33: Line exceeds maximum length. Consider breaking the line for better readability.
48-65: Ohayo sensei, consider stronger hashingIf collisions for
task_identifierpose a risk, consider using a more robust cryptographic hash (e.g., SHA256) instead ofDefaultHasher.🧰 Tools
🪛 GitHub Actions: ci
[warning] 48-48: Line exceeds maximum length. Consider breaking the line for better readability.
67-116: Ohayo sensei, watch out for large batch loopsIf
to_token_id - from_token_idis very large, iterating over every token may impact performance. Consider batching or chunk-based database updates.🧰 Tools
🪛 GitHub Actions: ci
[warning] 80-80: Line exceeds maximum length. Consider breaking the line for better readability.
[warning] 93-93: Line exceeds maximum length. Consider breaking the line for better readability.
33-33: Ohayo sensei, break up lines to satisfy pipeline formattingThe pipeline flags lines 33, 48, 80, and 93 for exceeding the recommended maximum length. Consider splitting them for improved readability:
-if event.keys.len() == 3 && event.data.is_empty() { +if event.keys.len() == 3 + && event.data.is_empty() { return true; }(and similarly for lines 48, 80, and 93)
Also applies to: 48-48, 80-80, 93-93
🧰 Tools
🪛 GitHub Actions: ci
[warning] 33-33: Line exceeds maximum length. Consider breaking the line for better readability.
crates/torii/sqlite/src/erc.rs (1)
146-161: Ohayo sensei, nice straightforward update method
update_erc721_metadatais concise and effective. A small suggestion: adding logs on success or failure might help with debugging and tracing.crates/torii/sqlite/src/executor/erc.rs (2)
242-271: Ohayo sensei, graceful error fallbackReturning an empty string if the metadata fetch fails keeps the workflow moving. Optionally, you could log or meter repeated failures to spot systemic issues.
🧰 Tools
🪛 GitHub Actions: ci
[warning] 247-247: Line exceeds maximum length. Consider breaking the line for better readability.
247-247: Ohayo sensei, friendly reminder about line lengthThe pipeline flagged lines 247, 280, 302, and 309 for exceeding the recommended maximum. Splitting them bolsters clarity.
Also applies to: 280-280, 302-302, 309-309
🧰 Tools
🪛 GitHub Actions: ci
[warning] 247-247: Line exceeds maximum length. Consider breaking the line for better readability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/torii/indexer/src/processors/erc4906_metadata_update.rs(1 hunks)crates/torii/sqlite/src/erc.rs(2 hunks)crates/torii/sqlite/src/executor/erc.rs(5 hunks)crates/torii/sqlite/src/executor/mod.rs(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/indexer/src/processors/erc4906_metadata_update.rs
[warning] 33-33: Line exceeds maximum length. Consider breaking the line for better readability.
[warning] 48-48: Line exceeds maximum length. Consider breaking the line for better readability.
[warning] 80-80: Line exceeds maximum length. Consider breaking the line for better readability.
[warning] 93-93: Line exceeds maximum length. Consider breaking the line for better readability.
crates/torii/sqlite/src/executor/erc.rs
[warning] 247-247: Line exceeds maximum length. Consider breaking the line for better readability.
[warning] 280-280: Line exceeds maximum length. Consider breaking the line for better readability.
[warning] 302-302: Line exceeds maximum length. Consider breaking the line for better readability.
[warning] 309-309: Line exceeds maximum length. Consider breaking the line for better readability.
🔇 Additional comments (7)
crates/torii/indexer/src/processors/erc4906_metadata_update.rs (2)
25-28: Ohayo sensei, looks goodNo issues found with
event_key—it neatly identifies the MetadataUpdate event for further processing.
44-46: Ohayo sensei, task_priority seems well-chosenKeeping this processor at a slightly lower priority than transfer tasks sounds logical.
crates/torii/sqlite/src/executor/erc.rs (3)
48-52: Ohayo sensei, struct definition looks good
UpdateErc721MetadataQuerycleanly encapsulates the needed fields for metadata updates.
189-240: Ohayo sensei, dual-entry-point check is smartFalling back from
token_uritotokenURIpreserves flexibility. The warning and empty value fallback align well with robust error handling.
295-319: Ohayo sensei, cohesive registration flowThe
process_register_erc721_token_queryneatly pipes throughfetch_token_uriandfetch_token_metadata. Implementation looks consistent.🧰 Tools
🪛 GitHub Actions: ci
[warning] 302-302: Line exceeds maximum length. Consider breaking the line for better readability.
[warning] 309-309: Line exceeds maximum length. Consider breaking the line for better readability.
crates/torii/sqlite/src/executor/mod.rs (2)
119-119: LGTM! Clean enum variant addition.The new
UpdateErc721Metadatavariant is well-named and follows the established pattern.
766-775: Ohayo! Clean implementation of metadata update handling, sensei!The implementation follows the established patterns with proper logging, error handling, and duration tracking.
| pub async fn update_erc721_metadata( | ||
| &mut self, | ||
| contract_address: Felt, | ||
| token_id: U256, | ||
| provider: Arc<P>, | ||
| ) -> Result<()> { | ||
| let token_uri = Self::fetch_token_uri(&provider, contract_address, token_id).await?; | ||
| let metadata = Self::fetch_token_metadata(contract_address, token_id, &token_uri).await?; | ||
|
|
||
| // Update metadata in database | ||
| sqlx::query( | ||
| "UPDATE tokens SET metadata = ? WHERE contract_address = ? AND id LIKE ?", | ||
| ) | ||
| .bind(&metadata) | ||
| .bind(felt_to_sql_string(&contract_address)) | ||
| .bind(format!("%{}", u256_to_sql_string(&token_id))) | ||
| .execute(&mut *self.transaction) | ||
| .await?; | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Ohayo sensei, consider using an exact match instead of LIKE
Using LIKE '%... may inadvertently update multiple tokens if IDs share a suffix. Switching to an exact match:
- "UPDATE tokens SET metadata = ? WHERE contract_address = ? AND id LIKE ?"
+ "UPDATE tokens SET metadata = ? WHERE contract_address = ? AND id = ?"...avoids accidental collisions.
📝 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 async fn update_erc721_metadata( | |
| &mut self, | |
| contract_address: Felt, | |
| token_id: U256, | |
| provider: Arc<P>, | |
| ) -> Result<()> { | |
| let token_uri = Self::fetch_token_uri(&provider, contract_address, token_id).await?; | |
| let metadata = Self::fetch_token_metadata(contract_address, token_id, &token_uri).await?; | |
| // Update metadata in database | |
| sqlx::query( | |
| "UPDATE tokens SET metadata = ? WHERE contract_address = ? AND id LIKE ?", | |
| ) | |
| .bind(&metadata) | |
| .bind(felt_to_sql_string(&contract_address)) | |
| .bind(format!("%{}", u256_to_sql_string(&token_id))) | |
| .execute(&mut *self.transaction) | |
| .await?; | |
| Ok(()) | |
| } | |
| pub async fn update_erc721_metadata( | |
| &mut self, | |
| contract_address: Felt, | |
| token_id: U256, | |
| provider: Arc<P>, | |
| ) -> Result<()> { | |
| let token_uri = Self::fetch_token_uri(&provider, contract_address, token_id).await?; | |
| let metadata = Self::fetch_token_metadata(contract_address, token_id, &token_uri).await?; | |
| // Update metadata in database | |
| sqlx::query( | |
| "UPDATE tokens SET metadata = ? WHERE contract_address = ? AND id = ?", | |
| ) | |
| .bind(&metadata) | |
| .bind(felt_to_sql_string(&contract_address)) | |
| .bind(format!("%{}", u256_to_sql_string(&token_id))) | |
| .execute(&mut *self.transaction) | |
| .await?; | |
| Ok(()) | |
| } |
🧰 Tools
🪛 GitHub Actions: ci
[warning] 280-280: Line exceeds maximum length. Consider breaking the line for better readability.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/torii/indexer/src/processors/erc4906_metadata_update.rs (1)
25-43: Consider using constants for key lengths, sensei!The validation logic is correct but would be more maintainable with named constants instead of magic numbers.
+ const SINGLE_UPDATE_KEY_LENGTH: usize = 3; + const BATCH_UPDATE_KEY_LENGTH: usize = 5; + fn validate(&self, event: &Event) -> bool { // Single token metadata update: [hash(MetadataUpdate), token_id.low, token_id.high] - if event.keys.len() == 3 && event.data.is_empty() { + if event.keys.len() == Self::SINGLE_UPDATE_KEY_LENGTH && event.data.is_empty() { return true; } // Batch metadata update: [hash(BatchMetadataUpdate), from_token_id.low, from_token_id.high, // to_token_id.low, to_token_id.high] - if event.keys.len() == 5 && event.data.is_empty() { + if event.keys.len() == Self::BATCH_UPDATE_KEY_LENGTH && event.data.is_empty() { return true; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/torii/indexer/src/processors/erc4906_metadata_update.rs(1 hunks)crates/torii/indexer/src/processors/mod.rs(2 hunks)crates/torii/sqlite/src/executor/erc.rs(5 hunks)crates/torii/sqlite/src/executor/mod.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/torii/indexer/src/processors/mod.rs
- crates/torii/sqlite/src/executor/mod.rs
- crates/torii/sqlite/src/executor/erc.rs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: ensure-wasm
- GitHub Check: docs
- GitHub Check: clippy
- GitHub Check: build
🔇 Additional comments (3)
crates/torii/indexer/src/processors/erc4906_metadata_update.rs (3)
1-16: Ohayo sensei! The imports and constants look good!All necessary dependencies are properly imported and the log target is well-defined.
17-18: The struct declaration is clean and minimal!Good use of
DefaultandDebugderives for this unit struct.
49-64: Heads up on potential race conditions in batch updates, sensei!While serializing batch updates by contract address prevents overlapping range issues, it might create a bottleneck if multiple non-overlapping ranges could be processed in parallel. Consider if this level of serialization is necessary.
✅ Verification successful
Ohayo, sensei! Let's get a better look at the context within the file to ensure we're making an informed call on potential race conditions. I'm going to grab more lines around the
task_identifierfunction incrates/torii/indexer/src/processors/erc4906_metadata_update.rsfor deeper analysis.
Ohayo, sensei! After checking out the code context in
crates/torii/indexer/src/processors/erc4906_metadata_update.rs, it's clear that the design intentionally hashes only the contract address for batch updates. This ensures that batch updates for the same contract are serialized to prevent race conditions from overlapping token ranges. While this does mean that non-overlapping ranges can't be processed concurrently, the current approach is a deliberate trade-off to maintain safety. No changes are needed unless improved parallelism becomes a priority.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any other batch processors that might provide insights rg -A 5 "batch.*process|process.*batch" --type rustLength of output: 53
Script:
#!/bin/bash # Display relevant context around the task_identifier function in the ERC4906 metadata update processor sed -n '40,80p' crates/torii/indexer/src/processors/erc4906_metadata_update.rsLength of output: 1353
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2984 +/- ##
==========================================
- Coverage 57.76% 57.69% -0.07%
==========================================
Files 437 439 +2
Lines 59305 59411 +106
==========================================
+ Hits 34256 34280 +24
- Misses 25049 25131 +82 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/torii/sqlite/src/executor/mod.rs (1)
769-779: Consider enhancing error handling for metadata updates.The implementation looks good sensei, but consider adding more detailed error handling to provide better diagnostics when metadata updates fail.
QueryType::UpdateErc721Metadata(update_metadata) => { debug!(target: LOG_TARGET, "Updating ERC721 metadata."); let instant = Instant::now(); - self.update_erc721_metadata( - update_metadata.contract_address, - update_metadata.token_id, - self.provider.clone(), - ) - .await?; + match self.update_erc721_metadata( + update_metadata.contract_address, + update_metadata.token_id, + self.provider.clone(), + ).await { + Ok(_) => { + debug!(target: LOG_TARGET, duration = ?instant.elapsed(), "Updated ERC721 metadata."); + } + Err(e) => { + error!( + target: LOG_TARGET, + error = %e, + contract_address = %update_metadata.contract_address, + token_id = %update_metadata.token_id, + "Failed to update ERC721 metadata" + ); + return Err(e); + } + }; - debug!(target: LOG_TARGET, duration = ?instant.elapsed(), "Updated ERC721 metadata."); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/torii/sqlite/src/executor/erc.rs(8 hunks)crates/torii/sqlite/src/executor/mod.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/sqlite/src/executor/erc.rs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: clippy
- GitHub Check: build
- GitHub Check: docs
🔇 Additional comments (2)
crates/torii/sqlite/src/executor/mod.rs (2)
10-10: LGTM!Ohayo! The import is well-organized with other erc-related imports.
120-120: LGTM!The new query type follows the established pattern and is well-integrated into the enum.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
crates/torii/sqlite/src/executor/erc.rs (3)
338-412: Ohayo sensei, consider enhancing error logging.While the method is robust in trying multiple entry points, it could benefit from more detailed error logging to help with debugging.
Consider adding specific error messages for each failed attempt:
} else { + let token_uri_err = provider.call(/* token_uri call */).await.err(); + let tokenURI_err = provider.call(/* tokenURI call */).await.err(); + let uri_err = provider.call(/* uri call */).await.err(); warn!( contract_address = format!("{:#x}", contract_address), token_id = %token_id, + token_uri_error = ?token_uri_err, + tokenURI_error = ?tokenURI_err, + uri_error = ?uri_err, "Error fetching token URI, empty metadata will be used instead.", );
414-438: Consider adding metadata caching.To improve performance and reduce network calls, consider caching the fetched metadata.
This could be particularly beneficial for frequently accessed tokens or when the same metadata URL is used by multiple tokens (common in ERC-1155).
440-457: Consider implementing optimistic updates.To improve user experience, consider implementing optimistic updates through the
SimpleBroker.Add broker message dispatch:
.execute(&mut *self.transaction) .await?; + // Publish optimistic update + if let Ok(token) = sqlx::query_as::<_, Token>("SELECT * FROM tokens WHERE id = ?") + .bind(felt_and_u256_to_sql_string(&contract_address, &token_id)) + .fetch_one(&mut *self.transaction) + .await + { + SimpleBroker::publish(unsafe { + std::mem::transmute::<Token, OptimisticToken>(token.clone()) + }); + }crates/torii/sqlite/src/executor/mod.rs (1)
757-767: Enhance logging for better observability.Consider adding more detailed logging to help with monitoring and debugging.
Add more context to the logs:
- debug!(target: LOG_TARGET, "Updating ERC721 metadata."); + debug!( + target: LOG_TARGET, + contract_address = format!("{:#x}", update_metadata.contract_address), + token_id = %update_metadata.token_id, + "Updating ERC721 metadata" + );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/torii/indexer/src/processors/mod.rs(2 hunks)crates/torii/sqlite/src/erc.rs(5 hunks)crates/torii/sqlite/src/executor/erc.rs(4 hunks)crates/torii/sqlite/src/executor/mod.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/indexer/src/processors/mod.rs
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/sqlite/src/erc.rs
[error] 10-10: Rust formatting check failed. Multiple imports should be grouped together.
[error] 150-150: Rust formatting check failed. The UpdateNftMetadataQuery instantiation should be formatted on a single line.
🔇 Additional comments (4)
crates/torii/sqlite/src/erc.rs (2)
95-100: LGTM! Improved token ID handling.The changes standardize token ID generation using
felt_and_u256_to_sql_string, making the code more consistent and maintainable.Also applies to: 107-107
242-243: LGTM! Consistent parameter usage.The method now consistently uses the
idparameter throughout, improving code clarity and maintainability.Also applies to: 250-251, 260-260
crates/torii/sqlite/src/executor/erc.rs (1)
40-44: LGTM! Well-structured query type.The
UpdateNftMetadataQuerystruct is cleanly defined with appropriate fields for contract address and token ID.crates/torii/sqlite/src/executor/mod.rs (1)
121-121: LGTM! Clean enum extension.The new
UpdateNftMetadatavariant follows the existing pattern and is well-placed in the enum.
crates/torii/sqlite/src/erc.rs
Outdated
| pub async fn update_nft_metadata( | ||
| &mut self, | ||
| contract_address: Felt, | ||
| token_id: U256, | ||
| ) -> Result<()> { | ||
| self.executor.send(QueryMessage::new( | ||
| "".to_string(), | ||
| vec![], | ||
| QueryType::UpdateNftMetadata(UpdateNftMetadataQuery { | ||
| contract_address, | ||
| token_id, | ||
| }), | ||
| ))?; | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Ohayo sensei, let's fix the query formatting.
The UpdateNftMetadataQuery instantiation should be formatted on a single line.
Apply this diff to fix the formatting:
- QueryType::UpdateNftMetadata(UpdateNftMetadataQuery {
- contract_address,
- token_id,
- }),
+ QueryType::UpdateNftMetadata(UpdateNftMetadataQuery { contract_address, 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.
| pub async fn update_nft_metadata( | |
| &mut self, | |
| contract_address: Felt, | |
| token_id: U256, | |
| ) -> Result<()> { | |
| self.executor.send(QueryMessage::new( | |
| "".to_string(), | |
| vec![], | |
| QueryType::UpdateNftMetadata(UpdateNftMetadataQuery { | |
| contract_address, | |
| token_id, | |
| }), | |
| ))?; | |
| Ok(()) | |
| } | |
| pub async fn update_nft_metadata( | |
| &mut self, | |
| contract_address: Felt, | |
| token_id: U256, | |
| ) -> Result<()> { | |
| self.executor.send(QueryMessage::new( | |
| "".to_string(), | |
| vec![], | |
| QueryType::UpdateNftMetadata(UpdateNftMetadataQuery { contract_address, token_id }), | |
| ))?; | |
| Ok(()) | |
| } |
🧰 Tools
🪛 GitHub Actions: ci
[error] 150-150: Rust formatting check failed. The UpdateNftMetadataQuery instantiation should be formatted on a single line.
crates/torii/sqlite/src/erc.rs
Outdated
| @@ -10,6 +10,7 @@ | |||
| use super::utils::{u256_to_sql_string, I256}; | |||
| use super::{Sql, SQL_FELT_DELIMITER}; | |||
| use crate::constants::TOKEN_TRANSFER_TABLE; | |||
| use crate::executor::erc::UpdateNftMetadataQuery; | |||
There was a problem hiding this comment.
Ohayo sensei, let's group related imports together.
The import for UpdateNftMetadataQuery should be grouped with other imports from the executor module.
Apply this diff to fix the formatting:
-use crate::executor::erc::UpdateNftMetadataQuery;
use crate::executor::erc::RegisterNftTokenQuery;
+use crate::executor::erc::{RegisterNftTokenQuery, UpdateNftMetadataQuery};Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/torii/sqlite/src/erc.rs (1)
144-156: Ohayo sensei! Let's improve the query message construction.The empty string and empty vector arguments seem unnecessary for this query type.
Apply this diff to simplify the query message construction:
- self.executor.send(QueryMessage::new( - "".to_string(), - vec![], - QueryType::UpdateNftMetadata(UpdateNftMetadataQuery { contract_address, token_id }), - ))?; + self.executor.send(QueryMessage::new_simple( + QueryType::UpdateNftMetadata(UpdateNftMetadataQuery { contract_address, token_id }) + ))?;Note: This assumes the existence of a
new_simpleconstructor. If it doesn't exist, we could create one to handle cases where only the query type is needed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/sqlite/src/erc.rs(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: clippy
- GitHub Check: ensure-wasm
- GitHub Check: docs
🔇 Additional comments (3)
crates/torii/sqlite/src/erc.rs (3)
13-13: Ohayo sensei! The imports look good.The imports are correctly organized and grouped together.
94-99: Ohayo sensei! The NFT transfer handling looks good.The changes improve consistency by using a standardized token ID format throughout the method.
Also applies to: 106-106
238-238: Ohayo sensei! The parameter renaming looks good.The changes improve naming consistency throughout the codebase.
Also applies to: 246-246, 256-256
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
examples/spawn-and-move/src/externals/erc721.cairo (1)
75-112: Ohayo sensei, the new metadata update externals look solid.
Enforcingassert_only_ownerfosters security. Emitting separate events for single, batch, and multiple tokens provides needed flexibility. Make sure to thoroughly test these flows to confirm no corner cases (e.g., empty arrays) break the loop or validations.🧰 Tools
🪛 GitHub Actions: ci
[error] 76-76: Formatting check failed. Ensure there is a newline at the end of the file.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
examples/spawn-and-move/src/externals.cairo(1 hunks)examples/spawn-and-move/src/externals/components.cairo(1 hunks)examples/spawn-and-move/src/externals/components/erc4906.cairo(1 hunks)examples/spawn-and-move/src/externals/erc1155.cairo(4 hunks)examples/spawn-and-move/src/externals/erc721.cairo(4 hunks)
✅ Files skipped from review due to trivial changes (2)
- examples/spawn-and-move/src/externals/components.cairo
- examples/spawn-and-move/src/externals.cairo
🧰 Additional context used
🪛 GitHub Actions: ci
examples/spawn-and-move/src/externals/components/erc4906.cairo
[error] 34-34: Formatting check failed. Ensure there is a newline at the end of the file.
examples/spawn-and-move/src/externals/erc1155.cairo
[error] 106-106: Formatting check failed. Ensure there is a newline at the end of the file.
examples/spawn-and-move/src/externals/erc721.cairo
[error] 76-76: Formatting check failed. Ensure there is a newline at the end of the file.
🔇 Additional comments (10)
examples/spawn-and-move/src/externals/components/erc4906.cairo (3)
1-5: Ohayo sensei, the trait design looks clear and minimal.
This concise interface is straightforward and effectively outlines the two essential functions for metadata updates.
14-19: Ohayo sensei, event enumeration structure looks good.
Defining two event types with an explicit#[event]annotation helps keep them self-descriptive and distinct.
35-57: Ohayo sensei, the component implementation is consistent.
Usingemit_...methods for each event is a tidy approach that cleanly segregates logic. Theassertusage in line 50 is direct and effectively ensures valid token ID ranges.examples/spawn-and-move/src/externals/erc721.cairo (3)
10-15: Ohayo sensei, great addition of the ERC4906 component.
Importing and declaringERC4906Componentas a new component enhances metadata flexibility for ERC-721 tokens without cluttering the existing code.Also applies to: 22-23
37-38: Ohayo sensei, substorage setup is coherent.
Addingerc4906to the storage ensures all ERC4906-related state remains isolated and organized.
50-51: Ohayo sensei, enumerating ERC4906 events aligns the contract’s event flow.
Flattening these events helps external processors receive consistent logs. Nicely done!examples/spawn-and-move/src/externals/erc1155.cairo (4)
10-15: Ohayo sensei, the ERC4906 integration is consistent.
Similar to ERC-721, embeddingERC4906Componenthere paves the way for cross-standard metadata updates.Also applies to: 22-23
37-38: Ohayo sensei, substorage mapping maintains clarity.
Keeping each component’s storage separate is a neat approach for better code organization and maintainability.
50-51: Ohayo sensei, integrating ERC4906 events into the master event enum is well-executed.
Consistent event flattening ensures downstream tooling sees all events in a uniform manner.
105-142: Ohayo sensei, the new token metadata update functions look robust and consistent with ownership checks.
The logic to loop over an array of token IDs ensures flexible updates, while also preserving the single-token and range-based updates. Good coverage!🧰 Tools
🪛 GitHub Actions: ci
[error] 106-106: Formatting check failed. Ensure there is a newline at the end of the file.
| #[key] | ||
| token_id: u256, | ||
| } | ||
|
|
There was a problem hiding this comment.
Add a newline at the end of the file.
The pipeline complains about formatting on line 34, indicating there's no trailing newline. Fixing this is required to pass CI.
... end of file ...
+📝 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.
| // ... (existing file content remains unchanged) | |
🧰 Tools
🪛 GitHub Actions: ci
[error] 34-34: Formatting check failed. Ensure there is a newline at the end of the file.
| @@ -63,5 +71,43 @@ mod ERC721Token { | |||
| fn mint(ref self: ContractState, token_id: u256) { | |||
| self.erc721.mint(starknet::get_caller_address(), token_id); | |||
| } | |||
|
|
|||
| #[external(v0)] | |||
| fn update_token_metadata(ref self: ContractState, token_id: u256) { | |||
There was a problem hiding this comment.
Add a newline at the end of the file.
The pipeline highlights a formatting check failure at line 76. Ensure there’s exactly one trailing newline to pass CI.
... end of file ...
+📝 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.
| fn update_token_metadata(ref self: ContractState, token_id: u256) { | |
| fn update_token_metadata(ref self: ContractState, token_id: u256) { | |
| } |
🧰 Tools
🪛 GitHub Actions: ci
[error] 76-76: Formatting check failed. Ensure there is a newline at the end of the file.
| @@ -93,5 +101,44 @@ mod ERC1155Token { | |||
| // Seems to not be supported by default dojo account. | |||
| // self.erc1155.batch_mint_with_acceptance_check(account, token_ids, values, data); | |||
| } | |||
|
|
|||
| #[external(v0)] | |||
| fn update_token_metadata(ref self: ContractState, token_id: u256) { | |||
There was a problem hiding this comment.
Add a newline at the end of the file.
The formatting error at line 106 indicates no trailing newline is present. Adding it will resolve the pipeline failure.
... end of file ...
+📝 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.
| fn update_token_metadata(ref self: ContractState, token_id: u256) { | |
| fn update_token_metadata(ref self: ContractState, token_id: u256) { | |
| } |
🧰 Tools
🪛 GitHub Actions: ci
[error] 106-106: Formatting check failed. Ensure there is a newline at the end of the file.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
crates/torii/indexer/src/processors/erc4906_metadata_update_batch.rs (2)
27-31: Ohayo sensei, consider thorough error logging in validation.While limiting the event to exactly five keys with no data is correct per ERC-4906, it might be helpful to log an explicit error if the event fails validation, providing clearer debugging info.
59-62: Ohayo sensei, beware of large token ranges in the loop.Looping from
from_token_idtoto_token_idcould be costly if the range is large. Consider introducing safeguards or batching to avoid long-running tasks or massive DB writes.crates/torii/sqlite/src/executor/erc.rs (2)
334-409: Ohayo sensei, consider strengthening error-handling or fallback logic.When calling the fallback entry points (
token_uri,tokenURI,uri), you rely on the last successful call or an empty string if all fail. For better resilience, consider logging or distinguishing which call succeeded or failed.
437-454: Ohayo sensei, nice direct update query!
update_erc721_metadatauses an exact match onidin theUPDATE tokensquery. This approach avoids collisions that can arise from wildcard usage. Also, handle potential network or parsing failures by verifying partial updates don’t leave you in an inconsistent state.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/torii/indexer/src/processors/erc4906_metadata_update.rs(1 hunks)crates/torii/indexer/src/processors/erc4906_metadata_update_batch.rs(1 hunks)crates/torii/indexer/src/processors/mod.rs(2 hunks)crates/torii/sqlite/src/executor/erc.rs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/indexer/src/processors/mod.rs
- crates/torii/indexer/src/processors/erc4906_metadata_update.rs
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/indexer/src/processors/erc4906_metadata_update_batch.rs
[error] 72-72: Rust formatting check failed. Please run 'rustfmt' to format the code.
🔇 Additional comments (6)
crates/torii/indexer/src/processors/erc4906_metadata_update_batch.rs (2)
15-17: Ohayo sensei, nice dedicated struct for the processor!Defining
Erc4906BatchMetadataUpdateProcessorin its own struct nicely encapsulates the batch metadata update logic. This approach aids readability and future expansions for ERC-4906.
72-72: Ohayo sensei, please fix the Rust formatting issue.Rustfmt is failing on line 72. Running
cargo fmtorrustfmtlocally should resolve this pipeline failure.🧰 Tools
🪛 GitHub Actions: ci
[error] 72-72: Rust formatting check failed. Please run 'rustfmt' to format the code.
crates/torii/sqlite/src/executor/erc.rs (4)
21-22: Ohayo sensei, thanks for centralizing these conversions!Bundling conversions (e.g.
felt_and_u256_to_sql_stringandsql_string_to_u256) in the utils fosters consistency and prevents scattered inline conversions across the codebase.
40-44: Ohayo sensei, this new struct addition is clean and straightforward.
UpdateNftMetadataQueryneatly packages the NFT metadata update requirements. This ensures clarity when passing these details around.
216-229: Ohayo sensei, great reusability!Using the new
fetch_token_uriandfetch_token_metadatamethods inprocess_register_nft_token_querylowers duplication and promotes code cleanliness.
411-435: Ohayo sensei, nicely isolated metadata fetch approach.
fetch_token_metadatacleanly separates remote or data-URI retrieval from the rest of the logic. Just mind potential large metadata payloads or malformed JSON structures.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/torii/sqlite/src/executor/erc.rs (1)
411-435: Consider caching failed metadata fetches.To prevent repeated failed requests for the same token URI, consider implementing a temporary cache for failed fetches.
+use std::collections::HashSet; +use std::sync::Mutex; + +lazy_static! { + static ref FAILED_URIS: Mutex<HashSet<String>> = Mutex::new(HashSet::new()); +} + async fn fetch_token_metadata( contract_address: Felt, token_id: U256, token_uri: &str, ) -> Result<String> { if token_uri.is_empty() { return Ok("".to_string()); } + + // Check if URI is in failed cache + if FAILED_URIS.lock().unwrap().contains(token_uri) { + return Ok("".to_string()); + } let metadata = Self::fetch_metadata(token_uri).await; match metadata { Ok(metadata) => { serde_json::to_string(&metadata).context("Failed to serialize metadata") } Err(_) => { + // Cache failed URI + FAILED_URIS.lock().unwrap().insert(token_uri.to_string()); warn!( contract_address = format!("{:#x}", contract_address), token_id = %token_id, token_uri = %token_uri, "Error fetching metadata, empty metadata will be used instead.", ); Ok("".to_string()) } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
crates/torii/indexer/src/engine.rs(2 hunks)crates/torii/indexer/src/processors/erc4906_batch_metadata_update.rs(1 hunks)crates/torii/indexer/src/processors/erc4906_metadata_update.rs(1 hunks)crates/torii/indexer/src/processors/mod.rs(2 hunks)crates/torii/sqlite/src/erc.rs(5 hunks)crates/torii/sqlite/src/executor/erc.rs(8 hunks)crates/torii/sqlite/src/executor/mod.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/torii/indexer/src/processors/mod.rs
- crates/torii/indexer/src/processors/erc4906_metadata_update.rs
- crates/torii/indexer/src/engine.rs
🔇 Additional comments (7)
crates/torii/indexer/src/processors/erc4906_batch_metadata_update.rs (4)
23-25: LGTM! Event key matches ERC-4906 specification.The event key correctly matches the ERC-4906 BatchMetadataUpdate event name.
27-31: LGTM! Validation matches ERC-4906 event structure.The validation correctly checks for the expected BatchMetadataUpdate event structure with 5 keys (event hash + token ID ranges) and empty data.
33-35: LGTM! Appropriate task priority.Priority level 2 is suitable for metadata updates, allowing them to proceed without blocking critical operations.
37-39: LGTM! Sequential processing ensures ordered updates.Using TASK_ID_SEQUENTIAL ensures batch updates are processed in order, maintaining data consistency.
crates/torii/sqlite/src/erc.rs (1)
141-153: LGTM! Clean implementation of NFT metadata update.The update_nft_metadata method is well-structured and follows the codebase's patterns for database operations.
crates/torii/sqlite/src/executor/erc.rs (1)
335-409: LGTM! Robust token URI fetching implementation.The fetch_token_uri implementation:
- Tries multiple entry point names (token_uri, tokenURI, uri)
- Handles both ByteArray and Array return types
- Includes helpful debug logging
- Gracefully handles failures
crates/torii/sqlite/src/executor/mod.rs (1)
757-767: LGTM! Clean integration of NFT metadata updates.The UpdateNftMetadata query type is well-integrated into the existing query handling system with appropriate logging and error handling.
| async fn process( | ||
| &self, | ||
| _world: &WorldContractReader<P>, | ||
| db: &mut Sql, | ||
| _block_number: u64, | ||
| _block_timestamp: u64, | ||
| _event_id: &str, | ||
| event: &Event, | ||
| _config: &EventProcessorConfig, | ||
| ) -> Result<(), Error> { | ||
| let token_address = event.from_address; | ||
| let from_token_id = U256Cainome::cairo_deserialize(&event.keys, 1)?; | ||
| let from_token_id = U256::from_words(from_token_id.low, from_token_id.high); | ||
|
|
||
| let to_token_id = U256Cainome::cairo_deserialize(&event.keys, 3)?; | ||
| let to_token_id = U256::from_words(to_token_id.low, to_token_id.high); | ||
|
|
||
| let mut token_id = from_token_id; | ||
| while token_id <= to_token_id { | ||
| db.update_nft_metadata(token_address, token_id).await?; | ||
| token_id += U256::from(1u8); | ||
| } | ||
|
|
||
| debug!( | ||
| target: LOG_TARGET, | ||
| token_address = ?token_address, | ||
| from_token_id = ?from_token_id, | ||
| to_token_id = ?to_token_id, | ||
| "NFT metadata updated for token range" | ||
| ); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Consider adding range size limits.
The batch update processor doesn't limit the token ID range size. Large ranges could:
- Cause excessive database operations
- Lead to timeouts or resource exhaustion
- Impact system performance
Consider adding a reasonable maximum range size (e.g., 1000 tokens).
async fn process(
&self,
_world: &WorldContractReader<P>,
db: &mut Sql,
_block_number: u64,
_block_timestamp: u64,
_event_id: &str,
event: &Event,
_config: &EventProcessorConfig,
) -> Result<(), Error> {
let token_address = event.from_address;
let from_token_id = U256Cainome::cairo_deserialize(&event.keys, 1)?;
let from_token_id = U256::from_words(from_token_id.low, from_token_id.high);
let to_token_id = U256Cainome::cairo_deserialize(&event.keys, 3)?;
let to_token_id = U256::from_words(to_token_id.low, to_token_id.high);
+ // Limit maximum range size to prevent resource exhaustion
+ const MAX_RANGE_SIZE: U256 = U256::from(1000u32);
+ if to_token_id - from_token_id > MAX_RANGE_SIZE {
+ return Err(anyhow::anyhow!(
+ "Token ID range too large. Maximum allowed: {}", MAX_RANGE_SIZE
+ ));
+ }
let mut token_id = from_token_id;
while token_id <= to_token_id {
db.update_nft_metadata(token_address, token_id).await?;
token_id += U256::from(1u8);
}📝 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.
| async fn process( | |
| &self, | |
| _world: &WorldContractReader<P>, | |
| db: &mut Sql, | |
| _block_number: u64, | |
| _block_timestamp: u64, | |
| _event_id: &str, | |
| event: &Event, | |
| _config: &EventProcessorConfig, | |
| ) -> Result<(), Error> { | |
| let token_address = event.from_address; | |
| let from_token_id = U256Cainome::cairo_deserialize(&event.keys, 1)?; | |
| let from_token_id = U256::from_words(from_token_id.low, from_token_id.high); | |
| let to_token_id = U256Cainome::cairo_deserialize(&event.keys, 3)?; | |
| let to_token_id = U256::from_words(to_token_id.low, to_token_id.high); | |
| let mut token_id = from_token_id; | |
| while token_id <= to_token_id { | |
| db.update_nft_metadata(token_address, token_id).await?; | |
| token_id += U256::from(1u8); | |
| } | |
| debug!( | |
| target: LOG_TARGET, | |
| token_address = ?token_address, | |
| from_token_id = ?from_token_id, | |
| to_token_id = ?to_token_id, | |
| "NFT metadata updated for token range" | |
| ); | |
| Ok(()) | |
| } | |
| async fn process( | |
| &self, | |
| _world: &WorldContractReader<P>, | |
| db: &mut Sql, | |
| _block_number: u64, | |
| _block_timestamp: u64, | |
| _event_id: &str, | |
| event: &Event, | |
| _config: &EventProcessorConfig, | |
| ) -> Result<(), Error> { | |
| let token_address = event.from_address; | |
| let from_token_id = U256Cainome::cairo_deserialize(&event.keys, 1)?; | |
| let from_token_id = U256::from_words(from_token_id.low, from_token_id.high); | |
| let to_token_id = U256Cainome::cairo_deserialize(&event.keys, 3)?; | |
| let to_token_id = U256::from_words(to_token_id.low, to_token_id.high); | |
| // Limit maximum range size to prevent resource exhaustion | |
| const MAX_RANGE_SIZE: U256 = U256::from(1000u32); | |
| if to_token_id - from_token_id > MAX_RANGE_SIZE { | |
| return Err(anyhow::anyhow!( | |
| "Token ID range too large. Maximum allowed: {}", | |
| MAX_RANGE_SIZE | |
| )); | |
| } | |
| let mut token_id = from_token_id; | |
| while token_id <= to_token_id { | |
| db.update_nft_metadata(token_address, token_id).await?; | |
| token_id += U256::from(1u8); | |
| } | |
| debug!( | |
| target: LOG_TARGET, | |
| token_address = ?token_address, | |
| from_token_id = ?from_token_id, | |
| to_token_id = ?to_token_id, | |
| "NFT metadata updated for token range" | |
| ); | |
| Ok(()) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
examples/spawn-and-move/src/externals/erc721.cairo (1)
95-109: Consider using array iteration methods, sensei!The loop implementation could be simplified using array iteration methods. Also, consider adding a check for empty token_ids array.
- let mut i: usize = 0; - loop { - if i >= token_ids.len() { - break; - } - self.erc4906.emit_metadata_update(*token_ids.at(i)); - i += 1; - } + assert(token_ids.len() > 0, 'Empty token_ids'); + token_ids.iter().for_each(|token_id| { + self.erc4906.emit_metadata_update(*token_id); + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (5)
examples/spawn-and-move/src/externals.cairo(1 hunks)examples/spawn-and-move/src/externals/components.cairo(1 hunks)examples/spawn-and-move/src/externals/components/erc4906.cairo(1 hunks)examples/spawn-and-move/src/externals/erc1155.cairo(4 hunks)examples/spawn-and-move/src/externals/erc721.cairo(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/spawn-and-move/src/externals.cairo
- examples/spawn-and-move/src/externals/components.cairo
- examples/spawn-and-move/src/externals/components/erc4906.cairo
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: docs
- GitHub Check: build
- GitHub Check: ensure-wasm
- GitHub Check: clippy
🔇 Additional comments (3)
examples/spawn-and-move/src/externals/erc721.cairo (2)
10-15: Ohayo sensei! Clean component integration!The ERC4906 component is integrated following the same pattern as other components, maintaining consistency.
22-23: Implementation looks great, sensei!The ERC4906MixinImpl is properly implemented with the correct attributes.
examples/spawn-and-move/src/externals/erc1155.cairo (1)
125-140: Same array iteration improvement applies here, sensei!Consider the same array iteration improvement as suggested for the ERC721 implementation.
| #[external(v0)] | ||
| fn update_token_metadata(ref self: ContractState, token_id: u256) { | ||
| // Only owner can update metadata | ||
| self.ownable.assert_only_owner(); | ||
|
|
||
| // Emit metadata update event | ||
| self.erc4906.emit_metadata_update(token_id); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Ohayo! Consider adding metadata validation, sensei!
The function emits the event but doesn't verify if the token exists. Consider adding a check to ensure the token exists before emitting the update event.
🏁 Script executed:
#!/bin/bash
# Search for token existence checks in similar functions
ast-grep --pattern 'fn $FUNC($_, token_id: u256) {
$$$
self.erc721._exists(token_id)
$$$
}'Length of output: 99
Ohayo sensei – Token Existence Check Needed!
We've re-run our searches (using ast-grep and rg) and found no evidence of token existence checks (i.e. calls to self.erc721._exists(token_id)) in similar metadata update functions. This confirms that the function:
#[external(v0)]
fn update_token_metadata(ref self: ContractState, token_id: u256) {
// Only owner can update metadata
self.ownable.assert_only_owner();
// Emit metadata update event
self.erc4906.emit_metadata_update(token_id);
}does indeed emit the metadata update event without verifying that the token exists. To prevent potential issues (like emitting updates for non-existent tokens), it's advisable to add a token existence check before proceeding with the event emission. For instance, you could consider:
#[external(v0)]
fn update_token_metadata(ref self: ContractState, token_id: u256) {
// Only owner can update metadata
self.ownable.assert_only_owner();
+
+ // Verify that the token exists; handle non-existent tokens gracefully
+ if (!self.erc721._exists(token_id)) {
+ // Optionally: raise an error or return early
+ return;
+ }
// Emit metadata update event
self.erc4906.emit_metadata_update(token_id);
}Please consider adding this token existence validation to strengthen the update process.
| #[external(v0)] | ||
| fn update_token_metadata(ref self: ContractState, token_id: u256) { | ||
| // Only owner can update metadata | ||
| self.ownable.assert_only_owner(); | ||
|
|
||
| // Emit metadata update event | ||
| self.erc4906.emit_metadata_update(token_id); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Verify token existence for ERC1155, sensei!
Similar to ERC721, consider adding token existence verification. For ERC1155, you might want to check if the token has ever been minted.
🏁 Script executed:
#!/bin/bash
# Search for token existence checks in ERC1155 implementations
ast-grep --pattern 'fn $FUNC($_, token_id: u256) {
$$$
self.erc1155.total_supply(token_id)
$$$
}'Length of output: 105
Ohayo, sensei! Attention to ERC1155 token metadata update logic needed.
It appears that the current implementation in
examples/spawn-and-move/src/externals/erc1155.cairo (lines 105-112) does not verify that the token has been minted (or exists) before updating its metadata. Unlike the ERC721 approach where token existence is checked, this ERC1155 function lacks a guard to ensure that metadata updates are performed only on tokens with a positive supply.
Actionable suggestions:
- Insert token existence verification: Before calling
self.erc4906.emit_metadata_update(token_id), add a check to confirm that the token has been minted. For example:let supply = self.erc1155.total_supply(token_id); // Only proceed if token exists (supply > 0) assert supply > 0, 'Token does not exist';
- Review similar patterns in ERC721 implementations: Ensure consistency and possibly reuse proven logic from the ERC721 token methods.
Please verify and implement these changes accordingly.
Summary by CodeRabbit
New Features
Refactor