feat(torii): deduped concurrent metadata updates - #3154
Conversation
|
Ohayo sensei! Below is the updated summary of the pull request: WalkthroughThis pull request refines the NFT metadata update process. It introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Executor
participant Task
participant DB
participant Broker
Client->>Executor: Request NFT metadata update
Executor->>Executor: Check if token update already in progress
Executor->>Task: Spawn handle_update_nft_metadata(UpdateNftMetadata)
Task->>DB: Query/update NFT metadata in database
DB-->>Task: Return updated metadata
Task->>Broker: Publish updated token information
Task-->>Executor: Return update result
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code Graph Analysis (1)crates/torii/sqlite/src/executor/erc.rs (1)
🔇 Additional comments (4)
🪧 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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/torii/sqlite/src/executor/erc.rs(5 hunks)crates/torii/sqlite/src/executor/mod.rs(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
crates/torii/sqlite/src/executor/mod.rs (2)
crates/torii/sqlite/src/utils.rs (2)
felt_and_u256_to_sql_string(44-46)felt_to_sql_string(40-42)crates/torii/sqlite/src/executor/erc.rs (3)
handle_update_nft_metadata(440-459)token_id(78-78)token_id(113-113)
🔇 Additional comments (12)
crates/torii/sqlite/src/executor/mod.rs (7)
10-10: Ohayo! Updated imports for the new struct.You've properly updated the imports to include the new
UpdateNftMetadatastruct that replaces the previousUpdateNftMetadataQuerytype. This aligns well with the refactoring approach, sensei!
150-153: Nice addition of task management fields!Ohayo! These new fields are excellent additions to support concurrent processing of metadata updates:
metadata_update_taskstracks the asynchronous tasks for metadata updatesmetadata_update_tokensmaintains a set of token IDs currently being processed, which enables deduplicationThis approach will prevent redundant work when multiple requests come in for the same token, sensei!
267-268: Proper initialization of new fields, sensei!The new fields are correctly initialized in the
newmethod, maintaining consistency with other collection initializations in this struct.
297-300: Good handling of task results in the run loop!Ohayo! This branch in the select macro correctly processes completed metadata update tasks by:
- Awaiting the next completed task
- Unwrapping both the outer and inner Result (with
??)- Passing the result to
handle_update_nft_metadataThis ensures metadata updates are processed as soon as they complete, sensei!
792-795: Excellent deduplication implementation!The token ID check prevents duplicate processing of the same token, which is the core improvement mentioned in the PR title. This efficiently prevents wasted work when multiple requests come in for the same token.
797-819: Well-structured async task for metadata updates!Ohayo! The task spawning implementation is excellent:
- Properly acquires a semaphore permit to limit concurrent tasks
- Fetches token URI and metadata asynchronously
- Constructs the UpdateNftMetadata with the results
- Properly releases the permit with
drop(permit)This approach keeps the main executor loop responsive while metadata fetching happens in the background, sensei!
845-848: Good cleanup of pending tasks during execution!This loop ensures that any pending metadata update tasks are completed and processed before the transaction is committed, maintaining data consistency, sensei!
crates/torii/sqlite/src/executor/erc.rs (5)
40-44: Well-designed struct for metadata updates!Ohayo, sensei! The new
UpdateNftMetadatastruct is well-designed and encapsulates exactly what's needed for a metadata update operation. This makes the code more maintainable and the parameter passing more explicit.
338-338: Good move making fetch_token_uri public!Making this method public allows it to be called from the spawned tasks in mod.rs, supporting the new asynchronous approach. This is a sensible change that improves code organization, sensei!
414-414: Good move making fetch_token_metadata public!Ohayo! Similar to fetch_token_uri, making this method public supports the new asynchronous processing approach. The change is consistent with the overall refactoring strategy, sensei!
440-443: Great method signature simplification!Renaming to
handle_update_nft_metadataand using the new struct as a parameter makes the method signature cleaner and more maintainable. This is a good example of the Single Responsibility Principle in action, sensei!
445-456: Clean implementation using the new struct!Ohayo! The implementation has been nicely updated to use the new
UpdateNftMetadatastruct:
- Properly binds the metadata and token_id fields for the update query
- Handles the optional result correctly
- Logs success and publishes the update when a token is found
This maintains all the functionality while making the code cleaner, sensei!
|
|
||
| let metadata_semaphore = self.metadata_semaphore.clone(); | ||
| let provider = self.provider.clone(); | ||
|
|
||
| let token_id = felt_and_u256_to_sql_string(&update_metadata.contract_address, &update_metadata.token_id); | ||
| if self.metadata_update_tokens.contains(&token_id) { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| self.metadata_update_tasks.spawn(async move { | ||
| let permit = metadata_semaphore.acquire().await.unwrap(); | ||
|
|
||
| let token_uri = Self::fetch_token_uri( | ||
| &provider, | ||
| update_metadata.contract_address, | ||
| update_metadata.token_id, | ||
| ) | ||
| .await?; | ||
| let metadata = Self::fetch_token_metadata( | ||
| update_metadata.contract_address, | ||
| update_metadata.token_id, | ||
| &token_uri, | ||
| ) | ||
| .await?; | ||
|
|
||
|
|
||
| drop(permit); | ||
| Ok(UpdateNftMetadata { | ||
| token_id, | ||
| metadata, | ||
| }) | ||
| }); |
There was a problem hiding this comment.
Consider cleaning up the tokens set after processing
Ohayo, sensei! While you're adding tokens to the metadata_update_tokens set, I don't see where they're being removed once processing is complete. This could potentially lead to a memory leak if tokens are never removed from the set.
Consider adding code to remove the token ID from the set after processing:
pub async fn handle_update_nft_metadata(
&mut self,
update_metadata: UpdateNftMetadata,
) -> Result<()> {
+ // Remove token from the set of tokens being processed
+ self.metadata_update_tokens.remove(&update_metadata.token_id);
// Update metadata in database
let token = sqlx::query_as::<_, Token>("UPDATE tokens SET metadata = ? WHERE id = ? RETURNING *")Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/torii/sqlite/src/executor/mod.rs (1)
794-817: Consider cleaning up the tokens set after processingOhayo, sensei! While you're adding tokens to the
metadata_update_tokensset, I don't see where they're being removed once processing is complete. This could potentially lead to a memory leak if tokens are never removed from the set.Consider adding code to remove the token ID from the set after processing:
pub async fn handle_update_nft_metadata( &mut self, update_metadata: UpdateNftMetadata, ) -> Result<()> { + // Remove token from the set of tokens being processed + self.metadata_update_tokens.remove(&update_metadata.token_id); // Update metadata in database let token = sqlx::query_as::<_, Token>("UPDATE tokens SET metadata = ? WHERE id = ? RETURNING *")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
crates/torii/sqlite/src/executor/erc.rs(5 hunks)crates/torii/sqlite/src/executor/mod.rs(8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
crates/torii/sqlite/src/executor/mod.rs (2)
crates/torii/sqlite/src/executor/erc.rs (6)
sqlx(287-287)sqlx(424-424)handle_update_nft_metadata(419-438)fetch_token_metadata(391-417)token_id(78-78)token_id(113-113)crates/torii/sqlite/src/utils.rs (2)
felt_and_u256_to_sql_string(44-46)felt_to_sql_string(40-42)
🪛 GitHub Actions: ci
crates/torii/sqlite/src/executor/mod.rs
[error] 14-14: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 27-27: Rust formatting check failed. Please run 'rustfmt' to format the code.
crates/torii/sqlite/src/executor/erc.rs
[error] 18-18: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 421-421: Rust formatting check failed. Please run 'rustfmt' to format the code.
🔇 Additional comments (9)
crates/torii/sqlite/src/executor/mod.rs (5)
10-10: Updated imports to include the newUpdateNftMetadatastruct.Ohayo, sensei! The import statement now includes the new
UpdateNftMetadatastruct from the erc module, which will be used for handling NFT metadata updates with the new deduplication mechanism.
151-153: Great addition of fields for async task management!Ohayo, sensei! These new fields are excellent additions:
metadata_update_taskshelps manage asynchronous metadata update operationsmetadata_update_tokenstracks which tokens are already being processed to prevent duplicate updatesThis aligns perfectly with the PR objective of deduplicating concurrent metadata updates.
267-268: New fields properly initialized in the constructor.Ohayo! These new fields are correctly initialized in the
newmethod, withmetadata_update_tasksas a newJoinSetandmetadata_update_tokensas an emptyHashSet. Good work, sensei!
297-300: Excellent addition to process metadata update tasks.Ohayo, sensei! This new branch in the
runmethod elegantly handles results frommetadata_update_tasksby invokinghandle_update_nft_metadata. This ensures that asynchronous metadata updates are properly processed in the main event loop.
843-846: Great handling of pending metadata update tasks.Ohayo, sensei! The
executemethod now correctly processes any pending metadata update tasks, ensuring all updates are completed before finishing execution. This is good practice to ensure no tasks are left hanging.crates/torii/sqlite/src/executor/erc.rs (4)
40-44: Well-designed new struct for metadata updates.Ohayo, sensei! The new
UpdateNftMetadatastruct is cleanly designed with the necessary fields:
token_id: A string identifier for the tokenmetadata: The token's metadata as a stringThis struct encapsulates the data needed for updating NFT metadata and supports the deduplication mechanism in the main executor.
315-315: Good exposure of methods as public APIs.Ohayo, sensei! Making these methods public is appropriate as they're now used outside this module. This change enhances the API's flexibility and supports the new asynchronous metadata update workflow.
Also applies to: 391-391, 419-419
396-397: Excellent refactoring of token metadata fetching.Ohayo! Having
fetch_token_metadatacallfetch_token_uriinternally is a great refactoring that:
- Streamlines the code flow
- Reduces duplication
- Makes the dependency between these methods explicit
This is clean, maintainable code, sensei!
419-438: Great simplification of the update method.Ohayo, sensei! The
handle_update_nft_metadatamethod now accepts a singleUpdateNftMetadatastruct parameter instead of multiple separate parameters. This is a good API design that:
- Simplifies the method signature
- Makes it clear what data is needed for updates
- Makes the code more maintainable
The implementation is also clean, updating the database, logging the result, and publishing the updated token.
🧰 Tools
🪛 GitHub Actions: ci
[error] 421-421: Rust formatting check failed. Please run 'rustfmt' to format the code.
| use sqlx::{FromRow, Pool, Sqlite, Transaction as SqlxTransaction}; | ||
| use starknet::core::types::requests::CallRequest; | ||
| use starknet::core::types::{BlockId, BlockTag, Felt, FunctionCall}; | ||
| use starknet::core::utils::{get_selector_from_name, parse_cairo_short_string}; |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Fix formatting issues flagged by CI.
Ohayo, sensei! The CI pipeline has flagged formatting issues on lines 14 and 27. Please run rustfmt to fix these issues.
Also applies to: 27-27
🏁 Script executed:
#!/bin/bash
# Run rustfmt to fix formatting issues
rustfmt crates/torii/sqlite/src/executor/mod.rsLength of output: 147
Ohayo, sensei!
The CI pipeline flagged formatting issues on lines 14 and 27 of crates/torii/sqlite/src/executor/mod.rs. However, when attempting to run rustfmt, the command wasn’t found in the environment. It appears that rustfmt isn’t installed or isn’t accessible at the moment.
To address this, please:
- Ensure that
rustfmtis installed (for example, by runningrustup component add rustfmt). - Re-run the formatting command on the file:
rustfmt crates/torii/sqlite/src/executor/mod.rs
Once rustfmt is properly installed, its execution should auto-format the file and clear the CI formatting issues.
🧰 Tools
🪛 GitHub Actions: ci
[error] 14-14: Rust formatting check failed. Please run 'rustfmt' to format the code.
2 participants
Summary by CodeRabbit