-
Notifications
You must be signed in to change notification settings - Fork 189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(torii-sqlite): executor broker messages & ipfs logging #2923
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2923 +/- ##
==========================================
- Coverage 55.99% 55.80% -0.20%
==========================================
Files 449 444 -5
Lines 58020 57316 -704
==========================================
- Hits 32491 31986 -505
+ Misses 25529 25330 -199 ☔ View full report in Codecov by Sentry. |
WalkthroughOhayo, sensei! This pull request introduces modifications across multiple files in the Torii project, primarily focusing on simplifying IPFS content fetching and transaction management. The changes involve removing the Changes
Sequence DiagramsequenceDiagram
participant Caller
participant FetchFunction
participant IPFSClient
Caller->>FetchFunction: Call fetch_content_from_ipfs(cid)
FetchFunction->>IPFSClient: Attempt to fetch content
alt Fetch successful
IPFSClient-->>FetchFunction: Return content
FetchFunction-->>Caller: Return content
else Fetch failed
IPFSClient--xFetchFunction: Return error
FetchFunction->>FetchFunction: Retry fetch
alt Retries exhausted
FetchFunction--xCaller: Return error
end
end
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/torii/sqlite/src/utils.rs (1)
56-73
: Ohayo! Nice improvements to the IPFS fetching logic, sensei!The changes improve the function's interface and error reporting. The warning logs now provide better context with error details and remaining attempts.
Consider adding exponential backoff instead of fixed 3-second delays between retries:
- tokio::time::sleep(Duration::from_secs(3)).await; + let backoff = Duration::from_secs(2u64.pow(IPFS_CLIENT_MAX_RETRY - retries)); + tokio::time::sleep(backoff).await;crates/torii/sqlite/src/executor/erc.rs (1)
292-292
: Clean adaptation to the new IPFS fetching interface, sensei!The code correctly uses the simplified fetch_content_from_ipfs function while maintaining robust error handling.
Consider extracting the URI scheme handling into a separate function for better maintainability:
async fn fetch_by_scheme(uri: &str) -> Result<serde_json::Value> { match uri { uri if uri.starts_with("ipfs") => fetch_from_ipfs(uri).await, uri if uri.starts_with("http") || uri.starts_with("https") => fetch_from_http(uri).await, uri if uri.starts_with("data") => fetch_from_data_uri(uri).await, uri => Err(anyhow::anyhow!("Unsupported URI scheme: {}", uri)) } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/torii/indexer/src/processors/metadata_update.rs
(2 hunks)crates/torii/server/src/artifacts.rs
(2 hunks)crates/torii/sqlite/src/executor/erc.rs
(2 hunks)crates/torii/sqlite/src/executor/mod.rs
(1 hunks)crates/torii/sqlite/src/utils.rs
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: docs
- GitHub Check: build
- GitHub Check: ensure-wasm
🔇 Additional comments (4)
crates/torii/sqlite/src/executor/mod.rs (1)
779-781
: Ohayo, sensei! Moving message publishing after transaction commit ensures consistency.By sending broker messages only after the transaction has been successfully committed, we prevent potential inconsistencies where messages could be published for transactions that are not yet persisted or could fail. This change enhances data integrity across the system.
crates/torii/indexer/src/processors/metadata_update.rs (2)
109-109
: Ohayo, sensei! Simplified IPFS content fetching aligns with the updated interface.Removing the
retries
parameter from thefetch_content_from_ipfs
call in themetadata
function reflects the revised function signature and streamlines the code.
120-120
: Ohayo, sensei! Updatedfetch_image
function adheres to the new IPFS fetching logic.The code now correctly calls
fetch_content_from_ipfs
without theretries
parameter, aligning with the recently updated function definition and simplifying error handling.crates/torii/server/src/artifacts.rs (1)
207-207
: Ohayo, sensei! Updated IPFS image retrieval matches the new function signature.By removing the
retries
parameter from thefetch_content_from_ipfs
call, thefetch_and_process_image
function now aligns with the updated IPFS content fetching interface, ensuring consistency across the codebase.
Summary by CodeRabbit
Refactor
Bug Fixes
The changes primarily focus on streamlining IPFS content fetching and transaction management, with improved error logging and simplified function interfaces.