fix(torii-sqlite): erc721 tokens upsert do nothing #2986
Conversation
|
Ohayo, sensei! WalkthroughThis change modifies the SQL queries related to ERC721 token operations in the database. The token details query now fetches the name and symbol based on the contract address instead of requiring a separate fetch operation. Additionally, the insert operation for token metadata has been updated to include an Changes
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (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: 0
🧹 Nitpick comments (1)
crates/torii/sqlite/src/executor/mod.rs (1)
634-639: Consider adding transaction isolation level, sensei!While the query modification to use token ID is good, there's a potential race condition between the existence check and token registration. Consider using SERIALIZABLE isolation level or adding a unique constraint on the token ID.
-- Add unique constraint to prevent duplicate tokens ALTER TABLE tokens ADD CONSTRAINT unique_token_id UNIQUE (id);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/sqlite/src/executor/mod.rs(1 hunks)
🔇 Additional comments (1)
crates/torii/sqlite/src/executor/mod.rs (1)
624-632: Ohayo! Nice addition of token existence check, sensei!The token existence check efficiently prevents duplicate registrations using COUNT(*) query with early return pattern.
Larkooo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/torii/sqlite/src/executor/erc.rs (2)
342-344: Ohayo! The ON CONFLICT DO NOTHING addition looks good sensei!The change makes the token registration process more resilient by gracefully handling duplicate entries.
Consider adding debug logging when a conflict occurs to help with troubleshooting:
let query = sqlx::query( "INSERT INTO tokens (id, contract_address, name, symbol, decimals, metadata) VALUES \ - (?, ?, ?, ?, ?, ?) ON CONFLICT DO NOTHING", + (?, ?, ?, ?, ?, ?) ON CONFLICT DO NOTHING RETURNING ( + CASE + WHEN xmax::text::int > 0 THEN 1 + ELSE 0 + END + ) as skipped", )Then check the result:
let result = query.fetch_one(&mut *self.transaction).await?; let skipped: i32 = result.get("skipped"); if skipped > 0 { debug!( target: LOG_TARGET, token_id = %result.query.token_id, "Skipped duplicate token registration" ); }
342-344: Consider future-proofing with UPSERT sensei!If there's a future requirement to update existing token metadata, you might want to consider using
ON CONFLICT UPDATEinstead. This would allow updating fields like name, symbol, or metadata when they change.Here's an example of how it could look:
let query = sqlx::query( "INSERT INTO tokens (id, contract_address, name, symbol, decimals, metadata) VALUES \ - (?, ?, ?, ?, ?, ?) ON CONFLICT DO NOTHING", + (?, ?, ?, ?, ?, ?) ON CONFLICT (id) DO UPDATE SET + name = EXCLUDED.name, + symbol = EXCLUDED.symbol, + metadata = EXCLUDED.metadata + WHERE tokens.metadata IS NULL OR tokens.metadata = ''", )This would update existing records only if they have empty metadata, preventing overwrites of valid data.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/torii/sqlite/src/executor/erc.rs(1 hunks)crates/torii/sqlite/src/executor/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/sqlite/src/executor/mod.rs
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2986 +/- ##
==========================================
- Coverage 56.99% 56.97% -0.02%
==========================================
Files 425 426 +1
Lines 56303 56376 +73
==========================================
+ Hits 32088 32119 +31
- Misses 24215 24257 +42 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
Release Notes:
ON CONFLICT DO NOTHINGclause to the token insertion process.