fix: replace token properly after config update#270
Conversation
WalkthroughThe updates introduce a new public method, Changes
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🔭 Outside diff range comments (1)
src/backend_task/tokens/update_token_config.rs (1)
110-161: 💡 Verification agent❓ Verification inconclusive
Make contract replacement + token update atomic to avoid partial DB state
The sequence
replace_contract → remove_token → insert_token
spans three independent writes. If the process crashes or any step errors, the local DB can become inconsistent:
- contract updated but old token still present (if
remove_tokenfails);- token deleted with no replacement (if
insert_tokenfails);- token definition updated while contract is still old (if
replace_contractfails after the deletion in a future refactor).Wrapping the three statements in a single SQL transaction eliminates the risk:
- // Then replace the contract in the local database - self.replace_contract(...)?; - self.remove_token(...)?; - self.insert_token(...)?; + self.db.with_transaction(|tx, ctx| { + ctx.replace_contract_tx(tx, contract_id, &data_contract)?; + ctx.remove_token_tx(tx, &identity_token_balance.token_id)?; + ctx.insert_token_tx( + tx, + &identity_token_balance.token_id, + &identity_token_balance.token_alias, + token.clone(), + &identity_token_balance.data_contract_id, + identity_token_balance.token_position, + ) + })(Names
*_txare illustrative; adapt to your DB helper API).Additionally, you reuse
identity_token_balance.token_idwhen reinserting. If the token ID were changed by the config update (unlikely but possible if the token list was recreated), this would re-insert stale data. Consider pulling the ID fromtoken.id()(if available) or at least assert equality and surface a clear error when they differ.
Wrap contract replacement and token updates in a single transaction
Performing
replace_contract,remove_token, andinsert_tokenas three separate calls can leave the database in an inconsistent state if one of them fails. Wrap these operations in your DB crate’s transaction API so that either all three succeed or none are applied. For example, insrc/backend_task/tokens/update_token_config.rsaround lines 110–161:- // Then replace the contract in the local database - self.replace_contract(identity_token_balance.data_contract_id, &data_contract)? - .map_err(|e| format!("Error replacing contract: {}", e))?; - - self.remove_token(&identity_token_balance.token_id)? - .map_err(|e| format!("Error removing token: {}", e))?; - - self.insert_token( - &identity_token_balance.token_id, - &identity_token_balance.token_alias, - token.clone(), - &identity_token_balance.data_contract_id, - identity_token_balance.token_position, - )? - .map_err(|e| format!("Error inserting token: {}", e))?; + // Atomically replace the contract and update the token + self.db.transaction::<_, _, _>(|tx| { + tx.replace_contract_tx(identity_token_balance.data_contract_id, &data_contract)?; + tx.remove_token_tx(&identity_token_balance.token_id)?; + tx.insert_token_tx( + &identity_token_balance.token_id, + &identity_token_balance.token_alias, + token.clone(), + &identity_token_balance.data_contract_id, + identity_token_balance.token_position, + )?; + Ok(()) + })?;Additionally, rather than reusing the old
identity_token_balance.token_id, consider pulling the ID from the freshly fetchedtoken(e.g.token.id()) or assert that they match, so you never insert a stale identifier when the platform’s token list has been modified.
🧹 Nitpick comments (1)
src/context.rs (1)
668-670: Consider transactional removal and clearer naming for newly-addedremove_tokenThe helper simply delegates to the DB layer, which is fine, but:
- Removing a token is often followed by inserting a replacement (see
update_token_config). If the later insert fails, the token will be permanently missing. Exposing a DB-level transaction (or accepting a&rusqlite::Transaction) would allow callers to make the whole “remove → insert” sequence atomic.- The crate already exposes
remove_token_balance; adding another publicremove_tokenwith a different semantic can be confusing. A more explicit name such asdelete_token_by_id(orpurge_token_definition) would make intent unmistakable.Neither point is blocking, but addressing them will strengthen consistency and failure-safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
src/backend_task/tokens/update_token_config.rs(3 hunks)src/context.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/context.rs (1)
src/database/tokens.rs (1)
remove_token(318-332)
2 participants
We were replacing the contract from the local DB after config update, which used to replace tokens too, but not anymore, so now we replace the tokens too.
Summary by CodeRabbit
New Features
Bug Fixes
Other