fix(torii): atomic token registry cache to fix race condition - #3124
Conversation
|
Ohayo, sensei! Below is the detailed overview of the changes: WalkthroughThis pull request updates the token ID management in the local cache for token transfers. The Changes
Sequence Diagram(s)sequenceDiagram
participant T as ERC Transfer Handler
participant C as LocalCache
T->>C: try_register_token_id(token_id)
alt Token ID not registered
C-->>T: true
T->>T: Register token metadata
else Already registered
C-->>T: false
T->>T: Skip metadata registration
end
Possibly related PRs
Suggested Reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🪧 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: 2
🧹 Nitpick comments (2)
crates/torii/sqlite/src/executor/mod.rs (1)
813-813: Ohayo! Simplified error handling is cleaner, but less informativeThe removal of
.with_context()makes the code cleaner but sacrifices detailed error information. This is acceptable since the SQL query is straightforward, but future developers may have slightly less context when debugging if this query fails.If detailed error context is valuable for troubleshooting, you could consider:
-let token = query.fetch_one(&mut **tx).await?; +let token = query.fetch_one(&mut **tx).await + .with_context(|| format!("Failed to register ERC20 token with ID {}", ®ister_erc20_token.token_id))?;crates/torii/sqlite/src/erc.rs (1)
1-304: Overall architecture significantly improvedOhayo, sensei! The change from checking and then registering to an atomic optimistic registration approach is an excellent architectural improvement. This resolves the race condition that could occur when multiple threads try to register the same token ID simultaneously.
This implementation follows the "optimistic concurrency control" pattern, which is a great fit for this use case. The use of a Mutex for atomic operations ensures thread safety while the optimistic approach minimizes lock contention by only acquiring the lock once instead of multiple times.
🧰 Tools
🪛 GitHub Actions: ci
[error] 40-40: Rust formatting check failed. Please run 'rustfmt' to format the code.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/torii/sqlite/src/cache.rs(3 hunks)crates/torii/sqlite/src/erc.rs(2 hunks)crates/torii/sqlite/src/executor/mod.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/sqlite/src/erc.rs
[error] 40-40: Rust formatting check failed. Please run 'rustfmt' to format the code.
crates/torii/sqlite/src/cache.rs
[error] 144-144: Rust formatting check failed. Please run 'rustfmt' to format the code.
🔇 Additional comments (7)
crates/torii/sqlite/src/cache.rs (5)
130-131: Good transition from RwLock to Mutex for atomic operations!The switch from
RwLocktoMutexis appropriate for this use case, sensei! The comment clearly explains the reasoning - we need atomic operations because of parallelized token transfers and registrations.
147-147: Clean initialization update for MutexProperly updated the initialization to use
Mutex::new().
152-152: Updated method to use Mutex lockThe
contains_token_idmethod has been correctly updated to uselock()instead ofread()to match the new mutex-based implementation.
156-157: Updated register method to use Mutex lockThe
register_token_idmethod has been correctly updated to uselock()instead ofwrite().
159-167: Excellent atomic registration method!This new
try_register_token_idmethod elegantly implements an atomic check-and-set operation, which is perfect for preventing race conditions in concurrent token registration, sensei!crates/torii/sqlite/src/erc.rs (2)
43-46: Excellent implementation of optimistic registration!This change uses the new atomic
try_register_token_idmethod instead of separate check-then-register calls, which effectively fixes the race condition. The logic has been inverted (using!before the method call) to maintain the same semantic meaning as before.
98-103: Consistent implementation for NFT transfersYou've applied the same optimistic registration pattern for NFT tokens as well, maintaining consistency across the codebase. The comments are also helpful in explaining the approach.
| use starknet::providers::{Provider, ProviderError}; | ||
| use starknet_crypto::Felt; | ||
| use tokio::sync::RwLock; | ||
| use tokio::sync::{Mutex, RwLock}; |
There was a problem hiding this comment.
Fix the formatting issue
The pipeline is failing due to formatting issues.
Please run rustfmt on this file to fix the formatting as indicated by the CI error.
| // optimistically add the token_id to cache | ||
| // this cache is used while applying the cache diff | ||
| // so we need to make sure that all RegisterErc*Token queries | ||
| // are applied before the cache diff is applied | ||
| let token_exists: bool = !self.local_cache.try_register_token_id(token_id.to_string()).await; |
There was a problem hiding this comment.
Great comments explaining the optimization!
These comments clearly explain the rationale behind the optimistic registration approach. Ohayo, sensei! You've made the code more understandable.
However, please run rustfmt on this file to fix the formatting issue reported by CI.
🧰 Tools
🪛 GitHub Actions: ci
[error] 40-40: Rust formatting check failed. Please run 'rustfmt' to format the code.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3124 +/- ##
=======================================
Coverage 56.81% 56.81%
=======================================
Files 442 442
Lines 61561 61564 +3
=======================================
+ Hits 34975 34977 +2
- Misses 26586 26587 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
glihm
changed the title
2 participants
Summary by CodeRabbit
dope.toml, eliminating outdated settings related to blockchain interaction.