fix: reset correct fields to close token remove popup successfully#236
Conversation
WalkthroughThe update refines the state management logic for closing the "Remove Token" confirmation popup in the tokens screen UI. Specifically, it corrects which internal state variables are reset when the popup is closed, ensuring that only the relevant flags and variables associated with the token removal popup are cleared, rather than those related to identity token balance removal. No public interfaces or exported entities are affected; the changes are limited to internal state handling within a private method. Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)
✨ 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:
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (55)
src/model/qualified_identity/qualified_identity_public_key.rs (1)
44-48:
⚠️ Potential issueSimplified address derivation but introduced potential panic condition.
The new implementation directly constructs a PublicKey from the raw key data and derives a P2PKH address, but uses
expect()which could cause a panic if the input is invalid.Consider using a pattern that handles invalid public key data gracefully rather than panicking:
- let pubkey = - PublicKey::from_slice(value.data().as_slice()).expect("Expected valid public key"); - - let address = Address::p2pkh(&pubkey, network); + let pubkey = match PublicKey::from_slice(value.data().as_slice()) { + Ok(key) => key, + Err(_) => return Self { + identity_public_key: value, + in_wallet_at_derivation_path: None, + }, + }; + + let address = Address::p2pkh(&pubkey, network);Committable suggestion skipped: line range outside the PR's diff.
src/backend_task/core/refresh_wallet_info.rs (2)
33-37:
⚠️ Potential issueAvoid panicking – propagate
RwLockpoisoning instead ofexpect()Using
expect("Core client lock was poisoned")will abort the whole async task, which is undesirable in long-running background workers. Propagate the error upward (or convert it into a retryable warning) so the caller can decide what to do.- match self - .core_client - .read() - .expect("Core client lock was poisoned") + let core_client = self + .core_client + .read() + .map_err(|e| format!("Core client lock poisoned: {e}"))?; + match core_client📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let core_client = self .core_client .read() .map_err(|e| format!("Core client lock poisoned: {e}"))?; match core_client .get_received_by_address(address, None)
58-66: 🛠️ Refactor suggestion
⚠️ Potential issueLock-ordering risk: acquire the
core_clientread-guard before the wallet write-guard
reload_utxosis called while holdingwallet.write()and simultaneously borrowingcore_client.read(). Other tasks may acquire these locks in the reverse order (core_client→wallet) and deadlock.Extract the
core_clientguard first, then pass a reference to it after obtaining the wallet lock:- let utxo_map = { - let mut wallet_guard = wallet.write().map_err(|e| e.to_string())?; - match wallet_guard.reload_utxos( - &self - .core_client - .read() - .expect("Core client lock was poisoned"), + let core_client = self + .core_client + .read() + .map_err(|e| format!("Core client lock poisoned: {e}"))?; + + let utxo_map = { + let mut wallet_guard = wallet.write().map_err(|e| e.to_string())?; + match wallet_guard.reload_utxos( + &core_client,This keeps a consistent lock order (
core_client→wallet) and eliminates potential deadlocks.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// first grab the core_client read-lock let core_client = self .core_client .read() .map_err(|e| format!("Core client lock poisoned: {e}"))?; // then grab the wallet write-lock and call reload_utxos using the // already-held core_client guard let utxo_map = { let mut wallet_guard = wallet.write().map_err(|e| e.to_string())?; match wallet_guard.reload_utxos( &core_client, self.network, Some(self), ) { // … } };src/backend_task/identity/transfer.rs (1)
19-22:
⚠️ Potential issuePropagate poisoning errors instead of
unwrap()
self.sdk.read().unwrap()will panic on a poisoned lock. Usemap_errso the calling UI can surface the error gracefully.- let sdk_guard = { - let guard = self.sdk.read().unwrap(); - guard.clone() - }; + let sdk_guard = self + .sdk + .read() + .map_err(|e| format!("SDK lock poisoned: {e}"))? + .clone();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let sdk_guard = self .sdk .read() .map_err(|e| format!("SDK lock poisoned: {e}"))? .clone();src/backend_task/identity/register_identity.rs (1)
121-124: 💡 Verification agent
🧩 Analysis chain
Avoid
unwrap()on poisoned lock – propagate or handle error gracefully
self.sdk.read().unwrap()will panic if the lock is poisoned, taking the whole task down.
Because this path is executed in the happy‐day flow of identity registration, consider either:
- returning an error (
map_err(|_| "SDK lock poisoned".into())?) so the caller can surface it, or- logging and falling back to a sane default instead of crashing.
Same applies to multiple
RwLockreads further below; a helper likefn read_sdk(&self) -> Result<Sdk, Error>would deduplicate the pattern.
🏁 Script executed:
#!/bin/bash # Search for all instances of `.read().unwrap()` in the identity registration file rg -n "read\(\)\.unwrap" -C2 src/backend_task/identity/register_identity.rsLength of output: 917
Avoid unwrap() on poisoned RwLock reads – handle or propagate errors
Unwinding on a poisoned lock can crash the entire registration task. Please replace all
.read().unwrap()calls with graceful error handling or propagation.Occurrences in
src/backend_task/identity/register_identity.rs:
- Line 122:
self.sdk.read().unwrap()- Line 138:
wallet.read().unwrap()- Line 336:
wallet.read().unwrap().seed_hash()- Line 347:
wallet.read().unwrap().seed_hash()Recommended refactor:
- Introduce helper methods, e.g.:
impl YourStruct { fn read_sdk(&self) -> Result<Sdk, Error> { self.sdk.read() .map(Clone::clone) .map_err(|_| Error::LockPoisoned("sdk".into())) } fn read_wallet(&self) -> Result<Wallet, Error> { self.wallet.read() .map(Clone::clone) .map_err(|_| Error::LockPoisoned("wallet".into())) } }- Update call sites:
let sdk = self.read_sdk()?; let wallet = self.read_wallet()?; let wallet_seed_hash = self.read_wallet()?.seed_hash();- This ensures any poisoned lock error is returned to the caller instead of panicking.
src/database/scheduled_votes.rs (1)
32-94: 🛠️ Refactor suggestion
Migration logic: ensure PRAGMA
foreign_keysis always re-enabledIf any SQL statement inside the transaction fails, the
foreign_keys = ONat line 91 is skipped, leaving the connection in an unsafe state.
Wrap the whole block in amatch tx_resultand re-enable foreign keys in theErrpath too, or use ascopeguard:let fk_guard = scopeguard::guard((), |_| { let _ = conn.execute("PRAGMA foreign_keys = ON", []); });This guarantees restoration even on early returns.
src/backend_task/document.rs (1)
13-20: 🛠️ Refactor suggestion
Consider whether all payload fields are really required for
BroadcastDocumentThe variant carries both a
DocumentTypeand an explicit[u8; 32]entropy slice, althoughDocument::put_to_platform_and_wait_for_responsecan usually derive theDocumentTypefrom theDocumentitself and will generate its own entropy whenNoneis supplied. Passing redundant data makes the API harder to reason about and increases the surface for misuse (e.g. mismatchingdoc_typeand the one embedded indocument).If you don’t have a strong reason to expose them, consider simplifying the tuple:
- BroadcastDocument( - Document, - [u8; 32], - DocumentType, - QualifiedIdentity, - IdentityPublicKey, - ), + BroadcastDocument( + Document, + QualifiedIdentity, + IdentityPublicKey, + ),and call
put_to_platform_and_wait_for_responsewithNoneforentropyanddocument.document_type()for the type.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.BroadcastDocument( Document, QualifiedIdentity, IdentityPublicKey, ), FetchDocuments(DocumentQuery),src/backend_task/identity/top_up_identity.rs (1)
34-37: 🛠️ Refactor suggestion
Avoid panicking on poisoned
RwLock– propagate the error instead
self.sdk.read().unwrap()will panic if another task poisoned the lock. In a long-running async application that’s hard to recover from. Prefer returning a descriptive error (or usingexpectconsistently with the other accesses):-let sdk = { - let guard = self.sdk.read().unwrap(); - guard.clone() -}; +let sdk = self + .sdk + .read() + .map_err(|_| "SDK lock was poisoned")? + .clone();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let sdk = self .sdk .read() .map_err(|_| "SDK lock was poisoned")? .clone();src/backend_task/register_contract.rs (1)
60-69: 🛠️ Refactor suggestion
Improve error message extraction for better resilience.
The current approach to extract the contract ID from the error message is fragile as it depends on the exact error message format. If the error message format changes, this extraction could fail.
Consider using a regex pattern to extract the ID more reliably:
- let id_str = error_string - .split(" ") - .last() - .ok_or("Failed to get contract ID from proof error")?; + let id_str = { + // Try to extract ID using regex for more resilience + let re = regex::Regex::new(r"([123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz]{34})").unwrap(); + if let Some(captures) = re.captures(&error_string) { + captures.get(1).map(|m| m.as_str()).ok_or("Failed to extract contract ID from error message")? + } else { + // Fallback to original method + error_string + .split(" ") + .last() + .ok_or("Failed to get contract ID from proof error")? + } + };This would require adding the regex crate to your dependencies.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// Try to extract ID using regex for more resilience let id_str = { let re = regex::Regex::new(r"([123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz]{34})").unwrap(); if let Some(captures) = re.captures(&error_string) { captures .get(1) .map(|m| m.as_str()) .ok_or("Failed to extract contract ID from error message")? } else { // Fallback to original method error_string .split(" ") .last() .ok_or("Failed to get contract ID from proof error")? } }; let id = Identifier::from_string(id_str, Encoding::Base58).map_err(|e| { format!( "Failed to convert contract ID from string to Identifier: {}", e.to_string() ) })?;Cargo.toml (1)
30-31: 💡 Verification agent
❓ Verification inconclusive
Avoid pinning to an RC version of
bincodeunless absolutely necessary
bincode = "=2.0.0-rc.3"locks the crate to a release-candidate that may receive breaking changes or yanks.
If the 2.0 API is needed, prefer the latest stable (currently 1.3) or track2.0.0-rc.*without the leading=so you can benefit from patch updates:-bincode = { version = "=2.0.0-rc.3", features = ["serde"] } +bincode = { version = "2.0.0-rc", features = ["serde"] }Otherwise, revert to
1.3until 2.0 is officially released.
Avoid pinning to an RC version of
bincodeLocking to a single prerelease (
=2.0.0-rc.3) risks yanks or breaking changes. If you need the 2.0 API, use a more flexible requirement that will pick up future rc patches (and eventually the stable 2.0.0):• File: Cargo.toml (lines 30–31)
-bincode = { version = "=2.0.0-rc.3", features = ["serde"] } +bincode = { version = "2.0.0-rc", features = ["serde"] }This will allow any
2.0.0-rc.*release (and later2.0.0) without rebinding your Cargo.lock.Otherwise, stick with the latest stable 1.x until 2.0 goes GA:
bincode = { version = "1.3", features = ["serde"] }src/backend_task/tokens/query_token_non_claimed_perpetual_distribution_rewards.rs (2)
34-38: 🛠️ Refactor suggestion
Blocking DB calls inside an async method can stall the async runtime
self.db.get_token_config_for_id(..)(and other DB helpers later in the function) appear to be synchronousrusqlitequeries.
Executing them directly insideasync fnblocks the entire Tokio worker thread.Consider off-loading to
spawn_blockingor migrating the DB layer to an async driver:let token_config = tokio::task::spawn_blocking({ let db = self.db.clone(); move || db.get_token_config_for_id(&token_id, self) }) .await .map_err(|e| format!("Join error: {e}"))? .map_err(|e| format!("Failed to get token config: {e}"))? .ok_or("Corrupted DET state: Token config not found in database")?;
114-123:
⚠️ Potential issueAvoid panicking with
expect(…)in backend pathsThe
expect("we should never get back an epoch so far in the future")will crash the whole task on an out-of-range epoch.
Since other errors are already propagated withErr(String), do the same here:- .expect("we should never get back an epoch so far in the future"), + .map_err(|_| "Epoch index overflow while converting to u16".to_owned())?,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let block_info = dash_sdk::dpp::block::block_info::BlockInfo { time_ms: current_epoch_with_metadata.1.time_ms, height: current_epoch_with_metadata.1.height, core_height: current_epoch_with_metadata.1.core_chain_locked_height, // This will not matter epoch: current_epoch_with_metadata .0 .index() .try_into() .map_err(|_| "Epoch index overflow while converting to u16".to_owned())?, };src/ui/tools/document_visualizer_screen.rs (1)
238-244:
⚠️ Potential issue
display_message()keeps showing stale JSON after an errorWhen an external task calls
display_message(.., MessageType::Error), you setparse_status = Errorbut leaveparsed_jsonuntouched, so the previous (possibly unrelated) successful result stays visible.Add
self.parsed_json = None;to prevent misleading output.- if matches!(t, crate::ui::MessageType::Error) { - self.parse_status = DocumentParseStatus::Error(msg.to_owned()); + if matches!(t, crate::ui::MessageType::Error) { + self.parsed_json = None; + self.parse_status = DocumentParseStatus::Error(msg.to_owned()); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.impl crate::ui::ScreenLike for DocumentVisualizerScreen { fn display_message(&mut self, msg: &str, t: crate::ui::MessageType) { if matches!(t, crate::ui::MessageType::Error) { self.parsed_json = None; self.parse_status = DocumentParseStatus::Error(msg.to_owned()); } } fn display_task_result(&mut self, _r: BackendTaskSuccessResult) {} }src/app_dir.rs (1)
47-61: 🛠️ Refactor suggestion
Handle unimplemented network cases more gracefully
Using
unimplemented!()will cause a runtime panic if an unsupported network is encountered. Consider returning an error instead to handle this case more gracefully.- _ => unimplemented!(), + _ => return Err(std::io::Error::new( + std::io::ErrorKind::Unsupported, + format!("Unsupported network type: {:?}", network) + )),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.pub fn core_cookie_path( network: Network, devnet_name: &Option<String>, ) -> Result<PathBuf, std::io::Error> { core_user_data_dir_path().map(|path| { let network_dir = match network { Network::Dash => "", Network::Testnet => "testnet3", Network::Devnet => devnet_name.as_deref().unwrap_or(""), Network::Regtest => "regtest", _ => return Err(std::io::Error::new( std::io::ErrorKind::Unsupported, format!("Unsupported network type: {:?}", network) )), }; path.join(network_dir).join(".cookie") }) }src/database/contracts.rs (2)
46-57: 🛠️ Refactor suggestion
Wrap token/contract insertion in a transaction for atomicity
A failure after some tokens are inserted (but before the loop finishes) will leave contract and token tables out of sync.
Starting aBEGIN IMMEDIATEtransaction before inserting the contract and committing after the tokens loop guarantees all-or-nothing persistence.If you’d like a sample diff just let me know.
62-68:
⚠️ Potential issuePropagate serialization errors instead of silently skipping tokens
bincode::encode_to_vecreturns aResult; converting it toOptionwith.ok()discards the error context and can hide real problems (e.g., schema incompatibilities). Bubble the error up instead:-let Some(serialized_token_configuration) = - bincode::encode_to_vec(&token_configuration, config).ok() -else { - // We should always be able to serialize - return Ok(()); -}; +let serialized_token_configuration = bincode::encode_to_vec(&token_configuration, config) + .map_err(|e| rusqlite::Error::ToSqlConversionFailure(Box::new(e)))?;This way the caller is immediately informed and the DB isn’t left in a partially-updated state.
src/context_provider.rs (1)
30-41:
⚠️ Potential issueHarden cookie parsing to avoid panics and stray whitespace
cookie.split(':')[1]panics if the cookie string is malformed, and trailing newlines are passed verbatim to RPC auth. A safer approach:-let cookie_parts: Vec<&str> = cookie.split(':').collect(); -let user = cookie_parts[0]; -let password = cookie_parts[1]; +let (user, password) = cookie + .trim() // remove \n + .split_once(':') // returns Option<(&str,&str)> + .ok_or("Invalid cookie format")?;This prevents out-of-bounds indexing and ensures clean credentials.
Committable suggestion skipped: line range outside the PR's diff.
src/ui/components/contract_chooser_panel.rs (1)
274-295: 🛠️ Refactor suggestion
to_json()every frame is expensive – cache on first expand
DataContract::to_jsonperforms validation & conversion; calling it each frame of an expanded panel can become costly with large contracts.Consider:
- Compute the pretty JSON string once on the first expansion and store it in a
HashMap<ContractId, String>insideAppContextor in a small LRU cache.- Render the cached string afterwards.
This keeps scrolling smooth and battery-friendly.
src/backend_task/contract.rs (3)
75-78: 🛠️ Refactor suggestion
Persist fetched contracts to keep backend & UI in sync
Unlike the
FetchContractsbranch, this variant does not store the freshly-fetched contract in the local database.
Down-stream code (token screens, balance refresh, etc.) expects the contract to be present, so a user who lands on this path may hit “contract not found” errors later.After building
token_infosyou can safely reuse the existing helper:self.db.insert_contract_if_not_exists( contract, None, NoTokensShouldBeAdded, self, )?;(Place it before populating
resultsto avoid double borrowing.)
94-118:
⚠️ Potential issueAvoid silently fabricating token IDs – handle the
Nonecase explicitly
contract.token_id(*token.0).unwrap_or_default()will fabricate a zero-identifier whenever the contract happens to miss a token-ID mapping.
If that situation ever arises (e.g. a partially-synced platform or an unexpected schema change) the UI will render a bogus “000…00” token ID, and any follow-up action (freeze, mint, etc.) will fail at runtime in much less obvious places.-let token_info = TokenInfo { - token_id: contract.token_id(*token.0).unwrap_or_default(), +let token_id = match contract.token_id(*token.0) { + Some(id) => id, + None => { + log::warn!( + "Contract {} is missing a token_id for position {} – skipping this token", + contract.id(), + token.0 + ); + continue; // skip this broken entry instead of forging an ID + } +}; + +let token_info = TokenInfo { + token_id,This guards against corrupted / incomplete contracts and prevents downstream crashes.
(Tagging as critical because the current behaviour can lead to persisting invalid data.)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.for token in contract.tokens() { let token_name = { let token_configuration = contract .expected_token_configuration(*token.0) .expect("Expected to get token configuration") .as_cow_v0(); let conventions = match &token_configuration.conventions { TokenConfigurationConvention::V0(conventions) => conventions, }; conventions .plural_form_by_language_code_or_default("en") .to_string() }; // Explicitly handle missing token_id instead of defaulting to zero let token_id = match contract.token_id(*token.0) { Some(id) => id, None => { log::warn!( "Contract {} is missing a token_id for position {} – skipping this token", contract.id(), token.0 ); continue; // skip this broken entry } }; let token_info = TokenInfo { token_id, token_name, data_contract_id: contract.id(), token_position: *token.0, description: token.1.description().clone(), }; token_infos.push(token_info); }
121-138: 🛠️ Refactor suggestion
Contracts without a description disappear from the result map
results.insert(..)is only executed whendocument_optionisSome(_).
If the Search-Contract doesn’t contain a matchingfullDescriptiondocument, the caller receives no entry at all – even though the contract and its token list were fetched successfully.Consider always inserting an entry and making the description optional:
-if let Some(document) = document_option { - let contract_description_info = ContractDescriptionInfo { … }; - results.insert(contract.id(), (Some(contract_description_info), token_infos)); -} +let description_info = document_option.map(|document| ContractDescriptionInfo { + data_contract_id: contract.id(), + description: document + .get("description") + .and_then(|v| v.as_text()) + .unwrap_or_default() + .to_string(), +}); + +results.insert(contract.id(), (description_info, token_infos));This way the caller can still display the token list and simply fall back to “No description available”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// Always produce an optional description instead of only inserting when Some let description_info = document_option.map(|document| ContractDescriptionInfo { data_contract_id: contract.id(), description: document .get("description") .and_then(|v| v.as_text()) .unwrap_or_default() .to_string(), }); results.insert( contract.id(), (description_info, token_infos), ); } } Ok(BackendTaskSuccessResult::ContractsWithDescriptions(results))src/ui/tokens/add_token_by_id_screen.rs (1)
148-157:
⚠️ Potential issueSuccess detection never triggers – token addition is stuck in “Searching”
display_messageonly transitions toCompletewhen the success message contains"AddToken", but the backend currently replies with"DataContract successfully saved".-MessageType::Success => { - if msg.contains("AddToken") { - self.status = AddTokenStatus::Complete; - } +MessageType::Success => { + // Any success originating from the SaveDataContract task finishes the flow + self.status = AddTokenStatus::Complete; }Without this fix the UI never shows the green “Token added successfully!” message and the screen isn’t popped.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.fn display_message(&mut self, msg: &str, msg_type: MessageType) { match msg_type { MessageType::Success => { // Any success originating from the SaveDataContract task finishes the flow self.status = AddTokenStatus::Complete; } MessageType::Error => { self.status = AddTokenStatus::Error(msg.to_owned()); self.error_message = Some(msg.to_owned()); } // … other message types … } }src/ui/tokens/freeze_tokens_screen.rs (1)
386-396:
⚠️ Potential issuePrevent runtime panics – disable “Freeze” button until prerequisites are met
Currently the button is always clickable, and the back-end call later does
self.selected_key.clone().expect("No key selected"), causing a hard crash
when the user forgets to pick a key or enters no identity.-let button = egui::Button::new(RichText::new("Freeze").color(Color32::WHITE)) - .fill(Color32::from_rgb(64, 64, 255)) - .corner_radius(3.0); - -if ui.add(button).clicked() { +let can_freeze = self.selected_key.is_some() && !self.freeze_identity_id.trim().is_empty(); +let button = egui::Button::new(RichText::new("Freeze").color(Color32::WHITE)) + .fill(Color32::from_rgb(64, 64, 255)) + .corner_radius(3.0); + +if ui.add_enabled(can_freeze, button).clicked() { self.show_confirmation_popup = true; }This delivers immediate UI feedback and removes the
expectpanic downstream.src/ui/tokens/resume_tokens_screen.rs (2)
41-46:
⚠️ Potential issueCompile-time type mismatch between selected key and key list
selected_keyis declared asOption<dash_sdk::platform::IdentityPublicKey>, yet all keys yielded by
identity.identity.public_keys()andavailable_authentication_keys()are DPP keys (dash_sdk::dpp::identity::IdentityPublicKey).
Because the two types are distinct,ui.selectable_value(&mut self.selected_key, Some(key.clone()), …)will not compile, nor will the later.expect("No key selected").- selected_key: Option<dash_sdk::platform::IdentityPublicKey>, + selected_key: Option<dash_sdk::dpp::identity::IdentityPublicKey>,and update every occurrence, or perform an explicit conversion (
platform::IdentityPublicKey::from(key.clone())) when pushing the value into theOption.Failing to reconcile the types will break the entire screen at build time.
Also applies to: 102-128
334-345: 🛠️ Refactor suggestion
Prevent “Resume” without a signing key
Users can click “Resume” even when
selected_keyisNone, which will panic insideshow_confirmation_popup(expect("No key selected")). Disable the button until a key has been chosen:- if ui.add(button).clicked() { - self.show_confirmation_popup = true; - } + let resume_clicked = ui.add_enabled(self.selected_key.is_some(), button).clicked(); + if resume_clicked { + self.show_confirmation_popup = true; + }src/ui/contracts_documents/register_contract_screen.rs (2)
42-45:
⚠️ Potential issueKey type mismatch identical to ResumeTokensScreen
selected_keyis a platform key whilequalified_identities[i].identity.public_keys()returns DPP keys, so the combobox comparison and later.unwrap()will not compile.Align the types (or convert) exactly as suggested for
ResumeTokensScreen.Also applies to: 60-73
335-344:
⚠️ Potential issueUnsafe unwraps on identity / key selection
When the “Register Contract” button is clicked the code unwraps both
selected_qualified_identityandselected_key. The earlier guard only checksselected_key;selected_qualified_identitycan still beNoneif the user cleared the dropdown.Add a defensive check:
- app_action = AppAction::BackendTask(BackendTask::ContractTask( - ContractTask::RegisterDataContract( - contract.clone(), - self.contract_alias_input.clone(), - self.selected_qualified_identity.clone().unwrap().0, - self.selected_key.clone().unwrap(), - ), - )); + if let (Some(qi), Some(key)) = (&self.selected_qualified_identity, &self.selected_key) { + app_action = AppAction::BackendTask(BackendTask::ContractTask( + ContractTask::RegisterDataContract( + contract.clone(), + self.contract_alias_input.clone(), + qi.0.clone(), + key.clone(), + ), + )); + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// Fire off a backend task if let (Some(qi), Some(key)) = (&self.selected_qualified_identity, &self.selected_key) { app_action = AppAction::BackendTask(BackendTask::ContractTask( ContractTask::RegisterDataContract( contract.clone(), self.contract_alias_input.clone(), qi.0.clone(), key.clone(), ), )); }src/ui/tokens/burn_tokens_screen.rs (2)
43-45:
⚠️ Potential issueSame key-type mismatch as above
selected_key: Option<dash_sdk::platform::IdentityPublicKey>but keys returned from identity are DPP keys, making all combobox operations uncompilable.Adopt the same fix as earlier.
Also applies to: 121-136
141-170: 🛠️ Refactor suggestion
Insufficient validation of burn amount
Only syntactic validity is checked. Burning more tokens than owned results in a backend failure that surfaces as an error toast.
Add a balance check before opening the confirmation popup:
- ui.text_edit_singleline(&mut self.amount_to_burn); + ui.text_edit_singleline(&mut self.amount_to_burn) + .hint_text(format!( + "Max: {}", + self.identity_token_balance.balance + ));and in the confirmation logic:
- let amount_ok = self.amount_to_burn.parse::<u64>().ok(); + let amount_ok = self.amount_to_burn + .parse::<u64>() + .ok() + .filter(|a| *a > 0 && *a <= self.identity_token_balance.balance);Rejecting impossible burns client-side improves UX and avoids unnecessary backend calls.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.fn render_amount_input(&mut self, ui: &mut Ui) { ui.horizontal(|ui| { ui.label("Amount to Burn:"); ui.text_edit_singleline(&mut self.amount_to_burn) .hint_text(format!( "Max: {}", self.identity_token_balance.balance )); }); } /// Renders a confirm popup with the final "Are you sure?" step fn show_confirmation_popup(&mut self, ui: &mut Ui) -> AppAction { let mut action = AppAction::None; let mut is_open = true; egui::Window::new("Confirm Burn") .collapsible(false) .open(&mut is_open) .show(ui.ctx(), |ui| { // Validate user input let amount_ok = self.amount_to_burn .parse::<u64>() .ok() .filter(|a| *a > 0 && *a <= self.identity_token_balance.balance); if amount_ok.is_none() { self.error_message = Some("Please enter a valid integer amount.".into()); self.status = BurnTokensStatus::ErrorMessage("Invalid amount".into()); self.show_confirmation_popup = false; return; } ui.label(format!( "Are you sure you want to burn {} tokens?", self.amount_to_burn )); ui.add_space(10.0);src/ui/tokens/claim_tokens_screen.rs (2)
67-74: 🛠️ Refactor suggestion
⚠️ Potential issueAvoid
expect()→ graceful fallback when identity is missing
expect("No local qualified identity …")will panic and take down the whole GUI if the identity is not found (e.g., DB corruption, concurrent deletion, wrong network). Return a dedicatedClaimTokensScreenerror/placeholder screen instead of crashing.- let identity = app_context - .load_local_qualified_identities() - .unwrap_or_default() - .into_iter() - .find(|id| id.identity.id() == identity_token_balance.identity_id) - .expect("No local qualified identity found for this token’s identity."); + let identity = app_context + .load_local_qualified_identities() + .unwrap_or_default() + .into_iter() + .find(|id| id.identity.id() == identity_token_balance.identity_id) + .ok_or_else(|| { + log::error!( + "Identity {} not found when opening ClaimTokensScreen", + identity_token_balance.identity_id + ); + return ClaimTokensScreen::dummy_error( + "Identity not found – try refreshing identities.".into(), + app_context, + ); + })?;(illustrative – adapt to constructor signature).
Committable suggestion skipped: line range outside the PR's diff.
232-243:
⚠️ Potential issueRuntime panic if
selected_keyisNone
expect("No key selected")will panic when the user opens the screen but never chooses a key (possible in developer mode after manually clearing the default). Validate before opening the confirmation window instead of panicking in the background task construction.- if self.distribution_type.is_none() { + if self.selected_key.is_none() { + self.status = ClaimTokensStatus::ErrorMessage( + "Select a signing key first.".into(), + ); + return; + } + if self.distribution_type.is_none() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if self.selected_key.is_none() { self.status = ClaimTokensStatus::ErrorMessage( "Select a signing key first.".into(), ); return; } if self.distribution_type.is_none() { // existing distribution‐type error handling return; } action = AppAction::BackendTasks( vec![ BackendTask::TokenTask(TokenTask::ClaimTokens { data_contract: self.token_contract.contract.clone(), token_position: self.identity_token_balance.token_position, actor_identity: self.identity.clone(), distribution_type, signing_key: self.selected_key.clone().expect("No key selected"), }), BackendTask::TokenTask(TokenTask::QueryMyTokenBalances), ], BackendTasksExecutionMode::Sequential,src/ui/contracts_documents/create_document_screen.rs (1)
334-340: 🛠️ Refactor suggestion
⚠️ Potential issueOptional fields with blank input trigger false-positive “MissingField” errors
field_inputsis pre-populated with""for every property. When an optional numeric/boolean/etc. field is left blank the later parser still attemptsparse::<…>(), causing an error. Skip empty strings for non-required fields:- let schema = doc_type + let schema = doc_type .properties() .get(name) .ok_or_else(|| format!("Unknown property {name}"))?; - - // convert UI string into proper Value + // For optional properties the user may leave the input empty. + if input_str.trim().is_empty() && !schema.required { + continue; + } + // convert UI string into proper ValueCommittable suggestion skipped: line range outside the PR's diff.
src/app.rs (1)
320-344: 🛠️ Refactor suggestion
⚠️ Potential issueSpawn ZMQ listeners only when an
AppContextexists for that network
devnet_core_zmq_listenerandlocal_core_zmq_listenerare spawned unconditionally even whendevnet_app_context/local_app_contextareNone.
This wastes threads and logs misleading errors because there is no configuration nor message consumer.- let devnet_core_zmq_listener = CoreZMQListener::spawn_listener( - Network::Devnet, - "tcp://127.0.0.1:23710", - core_message_sender.clone(), - devnet_tx_zmq_status_option, - ) - .expect("Failed to create devnet InstantSend listener"); + let devnet_core_zmq_listener = if devnet_app_context.is_some() { + CoreZMQListener::spawn_listener( + Network::Devnet, + "tcp://127.0.0.1:23710", + core_message_sender.clone(), + devnet_tx_zmq_status_option, + ) + .expect("Failed to create devnet InstantSend listener") + } else { + CoreZMQListener::disabled(Network::Devnet) + };Apply the same guard for
local_core_zmq_listener.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let devnet_tx_zmq_status_option = match devnet_app_context { Some(ref context) => Some(context.sx_zmq_status.clone()), None => None, }; let devnet_core_zmq_listener = if devnet_app_context.is_some() { CoreZMQListener::spawn_listener( Network::Devnet, "tcp://127.0.0.1:23710", core_message_sender.clone(), devnet_tx_zmq_status_option, ) .expect("Failed to create devnet InstantSend listener") } else { CoreZMQListener::disabled(Network::Devnet) }; let local_tx_zmq_status_option = match local_app_context { Some(ref context) => Some(context.sx_zmq_status.clone()), None => None, }; let local_core_zmq_listener = if local_app_context.is_some() { CoreZMQListener::spawn_listener( Network::Regtest, "tcp://127.0.0.1:20302", core_message_sender, local_tx_zmq_status_option, ) .expect("Failed to create local InstantSend listener") } else { CoreZMQListener::disabled(Network::Regtest) };src/ui/tokens/destroy_frozen_funds_screen.rs (2)
60-66:
⚠️ Potential issueField / method name collision will not compile
show_confirmation_popupis declared as a struct field (bool) and later re-used as a method name.
Fields and associated items share the same namespace in Rust; this causes a hard
compile-time error (duplicate definitions with name 'show_confirmation_popup').- show_confirmation_popup: bool, + show_confirmation_popup_flag: bool, // or rename the method insteadand update every occurrence (
self.show_confirmation_popup→self.show_confirmation_popup_flag).Also applies to: 155-156
82-83: 🛠️ Refactor suggestion
Unconditional
expect/unwrapmay crash the GUISeveral
.expect()/.unwrap()calls assume data/contracts/keys are always
present. Any out-of-sync state will panic the entire GUI:
- lines 82-83 – missing local identity
- lines 98-105 – wallet acquisition
- lines 197-216 – contract lookup &
selected_key.expectPrefer graceful user feedback:
-let data_contract = self - .app_context - .get_contracts(None, None) - .expect("Contracts not loaded") +let data_contract = match self.app_context.get_contracts(None, None) { + Ok(c) => c, + Err(e) => { + self.error_message = Some(format!("Failed to load contracts: {e}")); + self.status = DestroyFrozenFundsStatus::ErrorMessage(e.to_string()); + return AppAction::None; + } +};Repeat for other spots; replace
.expect()with user-visible error paths.Also applies to: 98-105, 197-216
src/ui/tokens/pause_tokens_screen.rs (2)
52-54:
⚠️ Potential issueDuplicate identifier
show_confirmation_popupSame compile-time issue as in the previous screen – rename either the field or the method.
Also applies to: 135-136
146-171: 🛠️ Refactor suggestion
selected_key.expect("No key selected")can panicDisable “Pause” until a key is chosen (see suggestion in destroy screen) or
surface a validation error instead of panicking.src/ui/tokens/mint_tokens_screen.rs (3)
171-178: 🛠️ Refactor suggestion
Input validation & overflow
amount_to_mint.parse::<u64>():
u64may overflow for large supplies; consideru128or decimal types.- Negative / zero amounts are currently accepted.
-let amount = self.amount_to_mint.parse::<u64>().ok(); +let amount = self.amount_to_mint + .parse::<u128>() + .ok() + .filter(|v| *v > 0);Fail early with a helpful message if validation fails.
Also applies to: 242-244
55-57:
⚠️ Potential issueName collision:
show_confirmation_popupThe struct field and method share the same identifier – compilation will fail.
Rename one of them consistently across all token screens.Also applies to: 163-164
242-243: 🛠️ Refactor suggestion
Potential panic on missing
selected_key
selected_key.expect("Expected a key")will crash the GUI.
Disable the “Mint” button until a key is chosen or return an error.src/ui/mod.rs (1)
304-337:
⚠️ Potential issueIncorrect screen construction for several Token‐related variants
FreezeTokensScreen,UnfreezeTokensScreen,PauseTokensScreen, andResumeTokensScreenare all routed toDestroyFrozenFundsScreen, andClaimTokensScreenis markedunreachable!().
This breaks navigation: the user will never reach the correct UI for those operations and will panic ifClaimTokensScreenis requested.- ScreenType::FreezeTokensScreen(identity_token_balance) => { - Screen::DestroyFrozenFundsScreen(DestroyFrozenFundsScreen::new( + ScreenType::FreezeTokensScreen(identity_token_balance) => { + Screen::FreezeTokensScreen(FreezeTokensScreen::new( ... } /* repeat the same pattern for the three cases below */ - ScreenType::UnfreezeTokensScreen(identity_token_balance) => { - Screen::DestroyFrozenFundsScreen(DestroyFrozenFundsScreen::new( + ScreenType::UnfreezeTokensScreen(identity_token_balance) => { + Screen::UnfreezeTokensScreen(UnfreezeTokensScreen::new( ... } - ScreenType::PauseTokensScreen(identity_token_balance) => { - Screen::DestroyFrozenFundsScreen(DestroyFrozenFundsScreen::new( + ScreenType::PauseTokensScreen(identity_token_balance) => { + Screen::PauseTokensScreen(PauseTokensScreen::new( ... } - ScreenType::ResumeTokensScreen(identity_token_balance) => { - Screen::DestroyFrozenFundsScreen(DestroyFrozenFundsScreen::new( + ScreenType::ResumeTokensScreen(identity_token_balance) => { + Screen::ResumeTokensScreen(ResumeTokensScreen::new( ... } - ScreenType::ClaimTokensScreen(_) => { - unreachable!(); - } + ScreenType::ClaimTokensScreen(identity_token_balance) => { + Screen::ClaimTokensScreen(ClaimTokensScreen::new( + identity_token_balance.clone(), + app_context, + )) + }Without this patch the UI will mislead users and can crash at runtime.
src/ui/tokens/transfer_tokens_screen.rs (3)
224-232: 🛠️ Refactor suggestion
⚠️ Potential issueUnchecked
amount.parse::<u64>().unwrap()may panicUser input can be non-numeric or exceed
u64, leading to a crash during confirmation.- amount: self.amount.parse().unwrap(), + amount: match self.amount.parse::<u64>() { + Ok(v) => v, + Err(_) => { + self.error_message = Some("Invalid amount".into()); + self.transfer_tokens_status = + TransferTokensStatus::ErrorMessage( + "Invalid amount".into(), + ); + return; + } + },Consider also validating
v > 0 && v <= self.max_amountbefore dispatching the backend task.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.vec![ BackendTask::TokenTask(TokenTask::TransferTokens { sending_identity: self.identity.clone(), recipient_id: identifier, amount: match self.amount.parse::<u64>() { Ok(v) => v, Err(_) => { self.error_message = Some("Invalid amount".into()); self.transfer_tokens_status = TransferTokensStatus::ErrorMessage( "Invalid amount".into(), ); return; } }, data_contract, token_position: self.identity_token_balance.token_position, signing_key: self.selected_key.clone().expect("Expected a key"), }),
68-76:
⚠️ Potential issue
max_amountis calculated across all tokens, not the selected one
find(|balance| balance.identity_id == identity.identity.id())returns the first match for the identity, which may belong to a different token.
Filter by bothidentity_idand the token’stoken_position(and/or contract id) to avoid overdrawing.- let max_amount = token_balances - .values() - .find(|balance| balance.identity_id == identity.identity.id()) + let max_amount = token_balances + .values() + .find(|balance| { + balance.identity_id == identity.identity.id() + && balance.token_position == identity_token_balance.token_position + }) .map(|balance| balance.balance) .unwrap_or(0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let token_balances = app_context .db .get_identity_token_balances(app_context) .expect("Token balances not loaded"); let max_amount = token_balances .values() .find(|balance| { balance.identity_id == identity.identity.id() && balance.token_position == identity_token_balance.token_position }) .map(|balance| balance.balance) .unwrap_or(0);
392-398: 🛠️ Refactor suggestion
selected_walletis never refreshed when the user changesselected_keyIf the user picks a different key after the screen is created,
render_wallet_unlock_if_needed()may still point to the old (orNone) wallet.Add a lightweight sync at the start of
ui():fn ui(&mut self, ctx: &Context) -> AppAction { + // Keep wallet in sync with the currently selected key + if let Some(key) = &self.selected_key { + let mut err = None; + self.selected_wallet = + get_selected_wallet(&self.identity, None, Some(key), &mut err); + if let Some(e) = err { + self.error_message = Some(e); + } + }This ensures the unlock dialog always targets the correct wallet.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.fn ui(&mut self, ctx: &Context) -> AppAction { // Keep wallet in sync with the currently selected key if let Some(key) = &self.selected_key { let mut err = None; self.selected_wallet = get_selected_wallet(&self.identity, None, Some(key), &mut err); if let Some(e) = err { self.error_message = Some(e); } } if self.selected_wallet.is_some() { let (needed_unlock, just_unlocked) = self.render_wallet_unlock_if_needed(ui); if needed_unlock && !just_unlocked { return; } } // ... rest of your UI rendering ... }src/ui/network_chooser_screen.rs (1)
320-327:
⚠️ Potential issueDestructive “Clear local Platform data” should confirm first
Wiping platform data is irreversible. Triggering it with a single click risks accidental data loss. Wrap the action in a confirmation dialog or at least a second “Are you sure?” button before dispatching
SystemTask::WipePlatformData.src/backend_task/tokens/mod.rs (2)
53-56: 🛠️ Refactor suggestion
Make
TokenTaskpublicly accessible
TokenTaskis declaredpub(crate), yet callers outsidebackend_task::tokensmay need to construct it (e.g. UI screens). If any module outside this sub-crate tries to use the enum, compilation will fail.-pub(crate) enum TokenTask { +pub enum TokenTask {Limiting visibility can be fine, but ensure the enclosing path matches all current and future call-sites.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.--- a/src/backend_task/tokens/mod.rs +++ b/src/backend_task/tokens/mod.rs @@ #[derive(Debug, Clone, PartialEq)] -pub(crate) enum TokenTask { +pub enum TokenTask { RegisterTokenContract { identity: QualifiedIdentity,
191-204:
⚠️ Potential issueAvoid panic when
token_namesis empty
register_data_contractindexestoken_names[0]; if the caller passes an empty vec the process panics. Validate early:- let data_contract = self + if token_names.is_empty() { + return Err("token_names must contain at least one (name, lang) pair".into()); + } + let data_contract = selfFailing fast with a clear error prevents unexpected crashes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if token_names.is_empty() { return Err("token_names must contain at least one (name, lang) pair".into()); } let data_contract = self .build_data_contract_v1_with_one_token( identity.identity.id().clone(), token_names.clone(), contract_keywords.to_vec(), token_description.clone(), *should_capitalize, *decimals, *base_supply, *max_supply, *start_paused, *keeps_history, *main_control_group, manual_minting_rules.clone(), )src/ui/tokens/unfreeze_tokens_screen.rs (2)
89-100: 🛠️ Refactor suggestion
Preserve
error_messagereturned byget_selected_wallet
error_messagepopulated insideget_selected_walletis immediately shadowed and lost.
Persisting it ensures the UI can surface wallet-related problems to the user.- let mut error_message = None; - let selected_wallet = - get_selected_wallet(&identity, None, possible_key.clone(), &mut error_message); + let mut error_message = None; + let selected_wallet = + get_selected_wallet(&identity, None, possible_key.clone(), &mut error_message); + + // Preserve any wallet-selection error we just collectedCommittable suggestion skipped: line range outside the PR's diff.
374-392:
⚠️ Potential issueAvoid panic when no signing key is selected
Unfreezeis always enabled; if the user hasn’t picked a key the backend dispatch will panic on
expect("No key selected"). Disable the button (or show a tooltip) untilself.selected_keyis
Some.- if ui.add(button).clicked() { - self.show_confirmation_popup = true; - } + let enabled = self.selected_key.is_some(); + if ui.add_enabled(enabled, button).clicked() { + self.show_confirmation_popup = true; + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// 2) Identity to unfreeze ui.heading("2. Enter the identity ID to unfreeze"); ui.add_space(5.0); self.render_unfreeze_identity_input(ui); ui.add_space(10.0); // Unfreeze button let button = egui::Button::new(RichText::new("Unfreeze").color(Color32::WHITE)) .fill(Color32::from_rgb(0, 128, 128)) .corner_radius(3.0); // Disable until a key is selected let enabled = self.selected_key.is_some(); if ui.add_enabled(enabled, button).clicked() { self.show_confirmation_popup = true; } // If user pressed "Unfreeze," show popup if self.show_confirmation_popup { action |= self.show_confirmation_popup(ui); }src/database/tokens.rs (3)
228-247:
⚠️ Potential issue
JOINmisses network filter – cross-network leakage possible
get_identity_token_balancesconstrainsidentity_token_balances.networkbut nottoken.network.
If the same token ID exists on multiple networks the query may return the wrong row.... JOIN token AS t ON b.token_id = t.id AND t.network = b.network
318-336: 🛠️ Refactor suggestion
Dropping and recreating
token_orderunconditionally causes silent data loss
initialize_token_order_tabledeletes existing user ordering every time it runs.
Consider running this only during explicit migrations, or checkPRAGMA user_versionbefore
destructive changes.
340-362: 🛠️ Refactor suggestion
token_orderis not scoped per-network
save_token_order/load_token_orderignorenetwork, so switching networks will mix orders and may
violate foreign-key constraints.
Add anetworkcolumn (part of PK) and include it in all INSERT/SELECT statements.Also applies to: 367-394
src/context.rs (3)
411-425: 🛠️ Refactor suggestion
get_contract_by_idignores in-memory system contractsThe method only queries the database; IDs matching DPNS, withdrawals, token-history or
keyword-search contracts will returnNoneunless those contracts were manually persisted.
Either:
- Check the four in-memory contracts before falling back to DB, or
- Ensure they are inserted into the DB at startup.
Failing to do so breaks callers that rely on this helper.
604-610:
⚠️ Potential issueError in serialising
TokenConfigurationis swallowedReturning
Ok(())when serialisation fails hides real problems and leaves DB rows incomplete.
Propagate the error (or at least log it) instead of silently succeeding.- let Some(serialized_token_configuration) = - bincode::encode_to_vec(&token_configuration, config).ok() - else { - // We should always be able to serialize - return Ok(()); - }; + let serialized_token_configuration = bincode::encode_to_vec(&token_configuration, config) + .map_err(|e| rusqlite::Error::ToSqlConversionFailure(Box::new(e)))?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let config = config::standard(); let serialized_token_configuration = bincode::encode_to_vec(&token_configuration, config) .map_err(|e| rusqlite::Error::ToSqlConversionFailure(Box::new(e)))?;
203-209: 💡 Verification agent
🧩 Analysis chain
Changed regtest identifier from
"regtest"to"local"– verify downstream usagesIf any DB rows, config files, or hard-coded paths still expect
"regtest", this rename will break
look-ups and path resolution. Please audit callers ofnetwork_string().
Run a quick search to confirm:
🏁 Script executed:
rg '"regtest"' -nLength of output: 80
Harmonize Regtest identifier in
app_dir.rsThe literal
"regtest"insrc/app_dir.rsstill diverges from the new"local"returned bynetwork_string(). This will break path lookups for Regtest.• File:
src/app_dir.rs, line 56
• Change the Regtest arm to match the new identifier- Network::Regtest => "regtest", + Network::Regtest => "local",Optionally, you could call
self.context.network_string()here instead of a hard-coded literal for future consistency.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@pauldelucia can you review? |
2 participants
This PR addresses an issue where closing the "Remove Token" confirmation popup via the "X" button did not reset the appropriate state variables.
Related to #233
Summary by CodeRabbit