fix: import UserDirs#212
Conversation
WalkthroughThis update introduces comprehensive support for token management and multi-network operations within the application. Major changes include the addition of new backend and UI modules for token operations (such as minting, burning, freezing, unfreezing, pausing, resuming, transferring, and claiming tokens), as well as the ability to register data contracts directly from the UI. The application now supports Devnet and Regtest (Local) networks alongside Mainnet and Testnet, with corresponding configuration, context, and UI enhancements. Database schema migrations and new tables support token metadata, balances, and ordering. The codebase also improves thread safety via locking, refines error handling, and updates UI components for better user interaction and navigation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant AppContext
participant BackendTask
participant Database
participant DashSDK
User->>UI: Initiate token action (e.g., Mint/Burn/Transfer)
UI->>AppContext: Dispatch BackendTask::TokenTask
AppContext->>BackendTask: Run token task (e.g., mint_tokens)
BackendTask->>DashSDK: Build and sign token state transition
DashSDK-->>BackendTask: Signed transition or error
BackendTask->>DashSDK: Broadcast transition
DashSDK-->>BackendTask: Proof/result or error
BackendTask->>Database: Update token balances/metadata
BackendTask-->>AppContext: Return BackendTaskSuccessResult
AppContext-->>UI: Display result/status
UI-->>User: Show confirmation, error, or success
sequenceDiagram
participant User
participant UI
participant AppContext
participant Database
User->>UI: Register new data contract
UI->>AppContext: Dispatch BackendTask::RegisterDataContract
AppContext->>DashSDK: Submit contract to platform
DashSDK-->>AppContext: Confirmation or error
AppContext->>Database: Insert contract and tokens
Database-->>AppContext: Success/Failure
AppContext-->>UI: Display registration result
UI-->>User: Show contract registration status
Possibly related PRs
Poem
✨ 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 (57)
.github/workflows/release.yml (1)
33-33:
⚠️ Potential issueWarning: Unknown GitHub Actions runner label.
The label "ubuntu-22.04-arm" is not a standard GitHub-hosted runner. GitHub's standard runners include combinations like "ubuntu-latest" and "self-hosted" or architecture-specific labels like "linux" and "arm64".
If this is a custom self-hosted runner, you should define it in an actionlint.yaml configuration file to avoid warnings. Otherwise, consider using one of GitHub's officially supported runner labels such as "ubuntu-latest" with architecture labels or a properly specified self-hosted runner.
🧰 Tools
🪛 actionlint (1.7.4)
33-33: label "ubuntu-22.04-arm" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
src/model/qualified_identity/qualified_identity_public_key.rs (1)
44-47:
⚠️ Potential issueRefactored address derivation method with potential error handling issue.
The code now explicitly constructs a
PublicKeyfrom raw data and then derives an address, which is more explicit but usesexpect()which could cause a panic with invalid keys.Consider using proper error handling instead of
expect()to gracefully handle invalid public keys:- 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(e) => { + eprintln!("Invalid public key: {}", e); + 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 issueAdded explicit locking for thread-safe core client access.
The code now correctly acquires a read lock on the core client before making RPC calls, enhancing thread safety.
However, using
expect()for lock errors is problematic as it will crash the application if the lock is poisoned. Consider proper error handling:- match self - .core_client - .read() - .expect("Core client lock was poisoned") - .get_received_by_address(address, None) + match self + .core_client + .read() + .map_err(|e| format!("Failed to acquire core client lock: {}", e.to_string()))? + .get_received_by_address(address, 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.match self .core_client .read() .map_err(|e| format!("Failed to acquire core client lock: {}", e.to_string()))? .get_received_by_address(address, None)
59-65:
⚠️ Potential issueImproved thread-safe access to core client in reload_utxos.
The code now acquires a read lock on the core client before passing it to
reload_utxos, but similar to above, it usesexpect()which could crash the application.Replace with proper error handling:
- match wallet_guard.reload_utxos( - &self - .core_client - .read() - .expect("Core client lock was poisoned"), - self.network, - Some(self), - ) { + match wallet_guard.reload_utxos( + &self + .core_client + .read() + .map_err(|e| format!("Failed to acquire core client lock: {}", e.to_string()))?, + self.network, + Some(self), + ) {📝 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.match wallet_guard.reload_utxos( &self .core_client .read() .map_err(|e| format!("Failed to acquire core client lock: {}", e.to_string()))?, self.network, Some(self), ) { // …src/backend_task/identity/load_identity_from_wallet.rs (1)
32-34: 🛠️ Refactor suggestion
Use read lock instead of write lock for immutable access
identity_authentication_ecdsa_public_keyonly needs an immutable borrow, yet a write lock is taken:let wallet = wallet_arc_ref.wallet.write().unwrap();Switch to
read()to maximise concurrency and avoid unnecessary blocking of other threads:- let wallet = wallet_arc_ref.wallet.write().unwrap(); + let wallet = wallet_arc_ref.wallet.read().expect("wallet read lock poisoned");src/backend_task/identity/top_up_identity.rs (1)
63-66: 🛠️ Refactor suggestion
Hold core-client read lock only as long as necessary
get_raw_transaction_infocan be time-consuming and will block all writers while the read lock is held.
Consider:let core_client = self.core_client.read() .expect("Core client lock poisoned") .clone(); // clone the RPC handle (cheap) drop(core_client_lock); // release lock early let raw_info = core_client.get_raw_transaction_info(&tx_id, None)?;This minimises contention if other tasks need to mutate
core_client.src/ui/wallets/wallets_screen/mod.rs (1)
557-571: 🛠️ Refactor suggestion
Enhanced asset lock functionality with proper action handling.
The method now returns an
AppActioninstead of(), allowing it to trigger a backend task when the "Find asset locks" button is clicked. This improves the UI feedback loop and enables refreshing wallet info directly from this UI component.The early return pattern here is good, but consider adding a test to verify that the action is properly propagated to the backend task system.
fn render_wallet_asset_locks(&mut self, ui: &mut Ui) -> AppAction { let mut app_action = AppAction::None; if let Some(arc_wallet) = &self.selected_wallet { let wallet = arc_wallet.read().unwrap(); if wallet.unused_asset_locks.is_empty() { ui.label("No asset locks available."); if ui.button("Find asset locks").clicked() { app_action = AppAction::BackendTask(BackendTask::CoreTask( CoreTask::RefreshWalletInfo(arc_wallet.clone()), )) - }; + } return app_action; }📝 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_wallet_asset_locks(&mut self, ui: &mut Ui) -> AppAction { let mut app_action = AppAction::None; if let Some(arc_wallet) = &self.selected_wallet { let wallet = arc_wallet.read().unwrap(); if wallet.unused_asset_locks.is_empty() { ui.label("No asset locks available."); if ui.button("Find asset locks").clicked() { app_action = AppAction::BackendTask(BackendTask::CoreTask( CoreTask::RefreshWalletInfo(arc_wallet.clone()), )) } return app_action; } // ... rest of rendering when there are locks } app_action }src/backend_task/identity/register_identity.rs (2)
149-151: 🛠️ Refactor suggestion
Factor out repeated core-client access & eliminate
expectpanicsThe same three lines (
self.core_client.read().expect("…")) appear at least three times.
Besides the panic risk discussed above, the repetition is a code-smell and makes future changes harder.fn core_client(&self) -> Result<Arc<CoreClient>, String> { self.core_client .read() .map_err(|_| "Core client lock poisoned".to_owned()) .map(|g| g.clone()) }Then:
-self.core_client - .read() - .expect("Core client lock was poisoned") +self.core_client()?This removes the panic, shortens call-sites, and centralises the error message.
Also applies to: 221-223, 288-290
121-124: 🛠️ Refactor suggestion
⚠️ Potential issueAvoid panicking on poisoned lock – return a typed error instead
RwLock::read().unwrap()will panic if the lock is poisoned. In a long-running background task this will bring the entire process down, the opposite of what we want from a backend worker. Consider mapping the poisoned-lock case into your existingResult<String>flow instead of panicking:-let guard = self.sdk.read().unwrap(); -guard.clone() +let sdk = self + .sdk + .read() + .map_err(|_| "SDK lock poisoned".to_owned())? + .clone(); +sdk📝 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 = { let sdk = self .sdk .read() .map_err(|_| "SDK lock poisoned".to_owned())? .clone(); sdk };src/backend_task/tokens/resume_tokens.rs (1)
14-48: 🛠️ Refactor suggestion
Unused
senderparameter – either use it or drop it
_senderis accepted but never utilised. This is easy to forget and may hide logic that was supposed to report progress back to the UI. Options:
- Send a success/failure
TaskResultexactly as the other token tasks do.- Remove the parameter to keep the signature minimal.
- sdk: &Sdk, - _sender: mpsc::Sender<TaskResult>, + sdk: &Sdk,If you choose (1), remember to
.await sender.send(…)before returning.📝 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 AppContext { pub async fn resume_tokens( &self, actor_identity: &QualifiedIdentity, data_contract: &DataContract, token_position: u16, signing_key: IdentityPublicKey, sdk: &Sdk, ) -> Result<BackendTaskSuccessResult, String> { // Use .resume(...) constructor let builder = TokenEmergencyActionTransitionBuilder::resume( data_contract, token_position, actor_identity.identity.id(), ); // Optionally chain .with_public_note(...).with_settings(...), etc. let state_transition = builder .sign(sdk, &signing_key, actor_identity, self.platform_version) .await .map_err(|e| format!("Error signing Resume Tokens transition: {}", e.to_string()))?; // Broadcast let _proof_result = state_transition .broadcast_and_wait::<StateTransitionProofResult>(sdk, None) .await .map_err(|e| format!("Error broadcasting Resume Tokens transition: {}", e))?; // Return success Ok(BackendTaskSuccessResult::Message( "ResumeTokens".to_string(), )) } }src/database/scheduled_votes.rs (1)
24-26:
⚠️ Potential issue
networkshould be part of the primary key to avoid collisions across networksAs defined,
(identity_id, contested_name)must be unique globally, even though the table is scoped per network elsewhere in the code (WHERE network = ?).
Running the app on Mainnet and Testnet simultaneously will causePRIMARY KEYconflicts.-PRIMARY KEY (identity_id, contested_name), +PRIMARY KEY (identity_id, contested_name, 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.PRIMARY KEY (identity_id, contested_name, network), FOREIGN KEY (identity_id) REFERENCES identity(id) ON DELETE CASCADE )",src/backend_task/tokens/burn_tokens.rs (1)
20-24:
⚠️ Potential issueValidate
amountbefore constructing the state transition
TokenBurnTransitionBuilder::newwill happily accept anamountof0, which produces a no-op state transition that still costs fees.
Consider short-circuiting or returning an error ifamount == 0(and optionally if it exceeds the owner’s balance) to save users from unnecessary fees.- amount: u64, + amount: u64, ) -> Result<BackendTaskSuccessResult, String> { + if amount == 0 { + return Err("Burn amount must be greater than zero".into()); + }📝 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.signing_key: IdentityPublicKey, amount: u64, sdk: &Sdk, _sender: mpsc::Sender<TaskResult>, ) -> Result<BackendTaskSuccessResult, String> { if amount == 0 { return Err("Burn amount must be greater than zero".into()); }src/backend_task/tokens/query_tokens.rs (1)
45-56: 🛠️ Refactor suggestion
Avoid unnecessary second-phase queries for duplicate contract IDs
If several
contractKeywordsdocuments point to the same contract, the current logic will query itsshortDescriptionrepeatedly.
De-duplicate before the loop to save bandwidth and reduce latency.- let mut contract_ids: Vec<Identifier> = Vec::with_capacity(kw_docs.len()); + let mut contract_ids: Vec<Identifier> = Vec::with_capacity(kw_docs.len()); + let mut seen = HashSet::with_capacity(kw_docs.len()); ... - contract_ids.push( + let cid = cid_val + .to_identifier() + .map_err(|e| format!("Bad contractId: {e}"))?; + if seen.insert(cid) { + contract_ids.push(cid); + }📝 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 mut contract_ids: Vec<Identifier> = Vec::with_capacity(kw_docs.len()); let mut seen = HashSet::with_capacity(kw_docs.len()); for (_doc_id, doc_opt) in kw_docs.iter() { if let Some(doc) = doc_opt { if let Some(cid_val) = doc.get("contractId") { let cid = cid_val .to_identifier() .map_err(|e| format!("Bad contractId: {e}"))?; if seen.insert(cid) { contract_ids.push(cid); } } } }src/backend_task/register_contract.rs (1)
59-69:
⚠️ Potential issueBrittle string-splitting may extract the wrong contract ID
Relying on
error_string.split(" ").last()assumes the platform error message always ends with the Base58 contract ID, with no trailing punctuation or newline.
If the format changes (or a log prefix/suffix is added) we’ll parse garbage and break the recovery path.Consider using a regex with an explicit Base58 capture group or exposing the contract ID in the SDK error type.
-let id_str = error_string - .split(" ") - .last() - .ok_or("Failed to get contract ID from proof error")?; +let id_re = regex::Regex::new(r"[1-9A-HJ-NP-Za-km-z]{42,44}$") + .expect("hard-coded regex is valid"); +let id_str = id_re + .find(&error_string) + .map(|m| m.as_str()) + .ok_or("Failed to extract contract ID from proof error")?;(You’d need to add
regex = "1"toCargo.toml.)
This keeps the heuristic local but far more robust.📝 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 error_string = e.to_string(); let id_re = regex::Regex::new(r"[1-9A-HJ-NP-Za-km-z]{42,44}$") .expect("hard-coded regex is valid"); let id_str = id_re .find(&error_string) .map(|m| m.as_str()) .ok_or("Failed to extract 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() ) })?;src/database/asset_lock_transaction.rs (1)
331-378:
⚠️ Potential issueMigration drops secondary indexes & triggers
The
CREATE TABLE …block recreates only the columns and FKs; any indexes, triggers, or CHECK constraints added later onasset_lock_transactionare silently lost.Before dropping the old table, enumerate
PRAGMA index_list/PRAGMA trigger_listand recreate them, or usesqlite_schemaintrospection:-- pseudo-code for idx in pragma_index_list("asset_lock_transaction_old") { … }Losing indexes can degrade performance and break invariants.
src/database/contracts.rs (1)
61-67:
⚠️ Potential issueSwallowing serialization failure hides real problems
encode_to_vecshould never fail for a well-formedTokenConfiguration.
Silently returningOk(())masks bugs and leaves the DB in a partial state.-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)) + })?;Propagating the error lets the caller decide whether to retry, alert, or rollback.
📝 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)))?;src/database/identities.rs (2)
318-330: 🛠️ Refactor suggestion
Dropping and recreating
identity_orderevery init loses user customisation
Runninginitialize_identity_order_tableon every start obliterates the saved ordering. Preserve data unless you’re inside a migration:-// Drop the table if it already exists -conn.execute("DROP TABLE IF EXISTS identity_order", [])?; - -// Recreate with foreign key enforcement -conn.execute( - "CREATE TABLE identity_order ( ... )", - [], -)?; +conn.execute( + "CREATE TABLE IF NOT EXISTS identity_order( + pos INTEGER NOT NULL PRIMARY KEY, + identity_id BLOB NOT NULL, + FOREIGN KEY(identity_id) REFERENCES identity(id) ON DELETE CASCADE + )", + [], +)?;📝 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.conn.execute( "CREATE TABLE IF NOT EXISTS identity_order( pos INTEGER NOT NULL PRIMARY KEY, identity_id BLOB NOT NULL, FOREIGN KEY(identity_id) REFERENCES identity(id) ON DELETE CASCADE )", [], )?;
34-42:
⚠️ Potential issue
get_aliassilently eats every database error
.ok()converts all SQL errors (IO, malformed DB, lock poisoning…) intoNone, so callers can’t distinguish “alias is NULL” from “disk I/O failure”. Either bubble the error or mapQueryReturnedNoRowsexplicitly.- let mut stmt = conn.prepare("SELECT alias FROM identity WHERE id = ?")?; - let alias: Option<String> = stmt.query_row(params![id], |row| row.get(0)).ok(); - Ok(alias) + let mut stmt = conn.prepare("SELECT alias FROM identity WHERE id = ?")?; + match stmt.query_row(params![id], |row| row.get::<_, Option<String>>(0)) { + Ok(alias) => Ok(alias), + Err(rusqlite::Error::QueryReturnedNoRows) => Ok(None), + Err(e) => Err(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.pub fn get_alias(&self, identifier: &Identifier) -> rusqlite::Result<Option<String>> { let id = identifier.to_vec(); let conn = self.conn.lock().unwrap(); let mut stmt = conn.prepare("SELECT alias FROM identity WHERE id = ?")?; match stmt.query_row(params![id], |row| row.get::<_, Option<String>>(0)) { Ok(alias) => Ok(alias), Err(rusqlite::Error::QueryReturnedNoRows) => Ok(None), Err(e) => Err(e), } }src/backend_task/tokens/query_my_token_balances.rs (2)
84-88: 🛠️ Refactor suggestion
Emit a single refresh event per successful query
TaskResult::Refreshis sent once per token; with large wallets this may flood the UI/event-loop, causing unnecessary redraws and contention on the channel.- for balance in token_balances.iter() { - ... - sender.send(TaskResult::Refresh).await?; - } + for balance in token_balances.iter() { + ... + } + // Notify once after DB batch finished + sender.send(TaskResult::Refresh).await?;Apply the same batching strategy in
query_token_balance.Also applies to: 135-139
32-44: 🛠️ Refactor suggestion
Unnecessary allocation of unused tuple elements
token_infoscapturesdata_contract_idandtoken_position, yet those fields are never used after construction.
Keeping them inflates memory-usage and obscures the real intention, because onlytoken_idis needed to build the query.- let token_infos = self - .identity_token_balances()? - ... - .map(|t| (t.token_id.clone(), - t.data_contract_id.clone(), - t.token_position)) + let token_ids: Vec<Identifier> = self + .identity_token_balances()? + ... + .map(|t| t.token_id.clone()) + .collect();This removes two needless
clones per record and simplifies the subsequent logic (you can drop the extracollect::<Vec<_>>()/ re-collect step entirely).Committable suggestion skipped: line range outside the PR's diff.
src/app_dir.rs (2)
52-60: 🛠️ Refactor suggestion
Handle unknown networks gracefully
Using
unimplemented!()will panic in production if the enum gains a new variant.
Return anErrwithErrorKind::InvalidInput(or use_ => return Err(...)) to fail gracefully and keep your function’sResultcontract.
30-33:
⚠️ Potential issueCompile-time bug:
and_thenclosure returns wrong type
Option::and_thenexpects the closure to return anotherOption, butto_owned().into()yields a plainPathBuf.
This fails to compile onlinuxtargets.- UserDirs::new() - .and_then(|dirs| dirs.home_dir().to_owned().into()) - .map(|home_dir| home_dir.join(".dashcore")) + UserDirs::new() + .map(|dirs| dirs.home_dir().to_path_buf()) + .map(|home_dir| home_dir.join(".dashcore"))Alternatively, keep
and_thenand wrap the value withSome(...).📝 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.UserDirs::new() .map(|dirs| dirs.home_dir().to_path_buf()) .map(|home_dir| home_dir.join(".dashcore")) .ok_or_else(|| {src/config.rs (1)
274-274:
⚠️ Potential issueAvoid panicking when parsing DAPI addresses
expect("Could not parse DAPI addresses")will crash the application on a configuration typo.
Please propagate a properResultinstead, bubbling the error up toConfig::load().- AddressList::from_str(&self.dapi_addresses).expect("Could not parse DAPI addresses") + AddressList::from_str(&self.dapi_addresses) + .map_err(|e| format!("Invalid DAPI address list: {}", e))?You can then change the caller to handle the returned
Result<AddressList, String>.src/backend_task/contract.rs (2)
95-109: 🛠️ Refactor suggestion
expectcall may panic during token-name extraction
expected_token_configuration(*token.0).expect("Expected to get token configuration")
will abort the whole backend if the contract is malformed. Propagate the error instead:- let token_configuration = contract - .expected_token_configuration(*token.0) - .expect("Expected to get token configuration") - .as_cow_v0(); + let token_configuration = contract + .expected_token_configuration(*token.0) + .ok_or_else(|| format!("Missing token configuration at position {}", token.0))? + .as_cow_v0();Return the error upstream or mark the
TokenInfoas invalid rather than panicking.📝 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_name = { let token_configuration = contract .expected_token_configuration(*token.0) .ok_or_else(|| format!("Missing token configuration at position {}", token.0))? .as_cow_v0(); let conventions = match &token_configuration.conventions { TokenConfigurationConvention::V0(conventions) => conventions, }; conventions .plural_form_by_language_code_or_default("en") .to_string() };
67-141:
⚠️ Potential issueContracts lacking a description are omitted from the result map
results.insert(..)is executed only whendocument_option.is_some().
Callers ofContractsWithDescriptionswill receive no entry at all for contracts whose description document is missing, making it impossible to distinguish “contract exists but no description” from “fetch failed”.A safer pattern:
- if let Some(document) = document_option { + let contract_description = document_option.map(|d| { + ContractDescriptionInfo { + data_contract_id: contract.id(), + description: d + .get("description") + .and_then(|v| v.as_text()) + .unwrap_or_default() + .to_string(), + } + }); + + // Always insert – description may be None results.insert( contract.id(), - (Some(contract_description_info), token_infos), + (contract_description, token_infos), ); - }This preserves the one-to-one mapping and lets the UI show “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.let document_option = Document::fetch(sdk, document_query) .await .map_err(|e| format!("Error fetching description: {}", e))?; let mut token_infos = vec![]; 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() }; let token_info = TokenInfo { token_id: contract.token_id(*token.0).unwrap_or_default(), token_name, data_contract_id: contract.id(), token_position: *token.0, description: token.1.description().clone(), }; token_infos.push(token_info); } - if let Some(document) = document_option { - let contract_description_info = 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(), - (Some(contract_description_info), token_infos), - ); - } + let contract_description = document_option.map(|d| { + ContractDescriptionInfo { + data_contract_id: contract.id(), + description: d + .get("description") + .and_then(|v| v.as_text()) + .unwrap_or_default() + .to_string(), + } + }); + // Always insert – description may be None + results.insert( + contract.id(), + (contract_description, token_infos), + );src/backend_task/core/mod.rs (1)
111-141: 🛠️ Refactor suggestion
Underlying RPC-client errors are swallowed
When both cookie and user/pass authentication fail, the second
Client::newerror is mapped to a generic message, discarding valuable diagnostics.- let client = match Client::new(&addr, Auth::CookieFile(cookie_path.clone())) { + let client = match Client::new(&addr, Auth::CookieFile(cookie_path.clone())) { Ok(client) => Ok(client), Err(first_err) => { tracing::info!( "Failed to authenticate using .cookie file at {:?}, falling back to user/pass", cookie_path ); - Client::new( + Client::new( &addr, Auth::UserPass( network_config.core_rpc_user.to_string(), network_config.core_rpc_password.to_string(), ), - ) + ).map_err(|second_err| { + format!( + "Failed to create {} client. Cookie auth error: {}. User/pass auth error: {}", + network, first_err, second_err + ) + }) } - } - .map_err(|_| format!("Failed to create {} client", network.to_string()))?; + }?;This surfaces both failure reasons and eases debugging misconfigurations.
📝 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 cookie authentication first let client = match Client::new(&addr, Auth::CookieFile(cookie_path.clone())) { Ok(client) => Ok(client), Err(first_err) => { tracing::info!( "Failed to authenticate using .cookie file at {:?}, falling back to user/pass", cookie_path ); Client::new( &addr, Auth::UserPass( network_config.core_rpc_user.to_string(), network_config.core_rpc_password.to_string(), ), ).map_err(|second_err| { format!( "Failed to create {} client. Cookie auth error: {}. User/pass auth error: {}", network, first_err, second_err ) }) } }?;src/ui/tokens/view_token_claims_screen.rs (1)
88-105: 🛠️ Refactor suggestion
Refresh button triggers an un-scoped query
FetchDocuments(self.new_claims_query.clone())runs without anywhereclause that restricts results to:• the current identity (
identity_token_balance.identity_id)
• the specific token (identity_token_balance.token_id)This will fetch all claim documents in the contract and could be expensive on large datasets. Consider adding a precise filter before dispatching:
- DesiredAppAction::BackendTask(BackendTask::DocumentTask( - DocumentTask::FetchDocuments(self.new_claims_query.clone()), - )), + DesiredAppAction::BackendTask(BackendTask::DocumentTask( + DocumentTask::FetchDocuments({ + let mut q = self.new_claims_query.clone(); + q.where_clauses.push(( + "identityId".into(), + Operator::Equal(identity_token_balance.identity_id), + )); + q + }), + )),Committable suggestion skipped: line range outside the PR's diff.
src/context_provider.rs (2)
66-67: 🛠️ Refactor suggestion
Hold write-lock for the shortest time possible
let sdk = app_context.sdk.write()acquires a write lock and then callssdk.set_context_provider(self.clone()).
Ifset_context_providerinternally tries to lockapp_context(directly or indirectly) you may hit a dead-lock becauseapp_context’sMutexis still held bybind_app_context.Release the
app_contextlock before taking the SDK lock or clone the necessary data beforehand:- drop(ac); - - let sdk = app_context.sdk.write().expect("lock poisoned"); - sdk.set_context_provider(self.clone()); + drop(ac); // release app_context mutex + + { + let mut sdk = app_context.sdk.write().expect("lock poisoned"); + sdk.set_context_provider(self.clone()); + } // write-lock released hereCommittable suggestion skipped: line range outside the PR's diff.
30-41:
⚠️ Potential issueDefensive parsing of Core
.cookiefile
cookie_parts[0]/[1]will panic if the cookie file is malformed or contains trailing new-lines; additionally,read_to_stringkeeps the line-ending which contaminates the password.Recommend using
splitn+trimwith graceful fallback:- let cookie = std::fs::read_to_string(cookie_path); - let (user, pass) = if let Ok(cookie) = cookie { - // split the cookie at ":", first part is user (__cookie__), second part is password - let cookie_parts: Vec<&str> = cookie.split(':').collect(); - let user = cookie_parts[0]; - let password = cookie_parts[1]; - (user.to_string(), password.to_string()) + let cookie = std::fs::read_to_string(&cookie_path); + let (user, pass) = if let Ok(cookie) = cookie { + match cookie.trim_end().splitn(2, ':').collect::<Vec<_>>()[..] { + [u, p] => (u.to_owned(), p.to_owned()), + _ => { + tracing::warn!("Malformed cookie in {:?}", cookie_path); + (config.core_rpc_user.clone(), config.core_rpc_password.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 cookie_path = core_cookie_path(network, &config.devnet_name).expect("Failed to get core cookie path"); // Read the cookie from disk - let cookie = std::fs::read_to_string(cookie_path); - let (user, pass) = if let Ok(cookie) = cookie { - // split the cookie at ":", first part is user (__cookie__), second part is password - let cookie_parts: Vec<&str> = cookie.split(':').collect(); - let user = cookie_parts[0]; - let password = cookie_parts[1]; - (user.to_string(), password.to_string()) + let cookie = std::fs::read_to_string(&cookie_path); + let (user, pass) = if let Ok(cookie) = cookie { + match cookie.trim_end().splitn(2, ':').collect::<Vec<_>>()[..] { + [u, p] => (u.to_owned(), p.to_owned()), + _ => { + tracing::warn!("Malformed cookie in {:?}", cookie_path); + (config.core_rpc_user.clone(), config.core_rpc_password.clone()) + } + } } else { // … }src/ui/dpns/dpns_contested_names_screen.rs (1)
346-373: 🛠️ Refactor suggestion
Consolidate & harden the filter-term normalisation
The three filter blocks (Active, Past & Owned) are copy-pasted with only tiny variations:
- They all lower-case and replace
o/O → 0,l → 1.- Upper-case
L(and possiblyI/i) is not translated so e.g. searching for “Leet” fails.- Any future tweak will have to be applied in three places → maintenance risk.
Recommend extracting a small helper and extending the char-mapping:
+fn normalise_filter(term: &str) -> String { + term.to_lowercase() + .chars() + .map(|c| match c { + 'o' | '0' => '0', + 'l' | 'i' | '1' => '1', // treat visually-similar glyphs + _ => c, + }) + .collect() +}Then each table becomes as simple as:
let filter = normalise_filter(&self.active_filter_term); if !filter.is_empty() { cn.retain(|c| c.normalized_contested_name.to_lowercase().contains(&filter)); }This removes the duplication and fixes the missed upper-case
L.Also applies to: 698-726, 860-873
src/ui/tokens/resume_tokens_screen.rs (1)
129-176:
⚠️ Potential issue
Confirmcan panic when no key is selected
self.selected_keyis optional, yet the “Confirm” handler blindly unwraps:signing_key: self.selected_key.clone().expect("No key selected"),If the user never picked a key (or the default search failed) the application will crash.
Suggested guard:
-if ui.button("Confirm").clicked() { +if ui.button("Confirm").clicked() { + if self.selected_key.is_none() { + self.error_message = Some("Please select a signing key first.".into()); + return action; // keep the popup open + }…or disable the button entirely until a key is chosen:
-let confirm = ui.button("Confirm"); -if confirm.clicked() { … } +let confirm = ui.add_enabled(self.selected_key.is_some(), egui::Button::new("Confirm")); +if confirm.clicked() { … }src/ui/tokens/burn_tokens_screen.rs (1)
146-216:
⚠️ Potential issueMissing input validation can crash or burn an impossible amount
Issues inside the confirmation popup:
self.selected_key.expect(..)– same panic risk as in Resume Tokens.amount_to_burn.parse::<u64>()succeeds but the parsed value may exceed the holder’s balance (or be0), yet the burn is still dispatched.- When
amount_to_burnis invalid you immediately close the popup (self.show_confirmation_popup = false), leaving the user without context.Recommended quick fixes:
-let amount_ok = self.amount_to_burn.parse::<u64>().ok(); -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; -} +let amount_ok = match self.amount_to_burn.trim().parse::<u64>() { + Ok(amt) if amt > 0 && amt <= self.identity_token_balance.balance => amt, + _ => { + self.error_message = Some(format!( + "Please enter a number between 1 and {}.", + self.identity_token_balance.balance + )); + return action; // keep popup open + } +};And gate the dispatch behind
self.selected_key.is_some()using the same pattern suggested for the resume screen.src/ui/tokens/unfreeze_tokens_screen.rs (3)
193-199:
⚠️ Potential issueRuntime panic on missing key
self.selected_key.clone().expect("No key selected")will bring down the async worker thread. Replace withif let Some(key) = &self.selected_key { … } else { … }and propagate an error back to the UI.
76-90:
⚠️ Potential issue
possible_keycan beNone– protect later.expect()
possible_keyis optional. When the user has zero auth keys the “Unfreeze” button remains enabled, butself.selected_key.clone().expect("No key selected")(l. 197) will panic the first time they press it.Disable the button or show validation feedback until a key is picked:
-let button = egui::Button::new(RichText::new("Unfreeze").color(Color32::WHITE)) - .fill(Color32::from_rgb(0, 128, 128)) - .corner_radius(3.0); - -if ui.add(button).clicked() { - self.show_confirmation_popup = true; -} +let key_selected = self.selected_key.is_some(); +let button = egui::Button::new(RichText::new("Unfreeze").color(Color32::WHITE)) + .fill(Color32::from_rgb(0, 128, 128)) + .corner_radius(3.0) + .enabled(key_selected); + +if key_selected && ui.add(button).clicked() { + self.show_confirmation_popup = true; +}Remember to repeat the check inside
show_confirmation_popupas a final guard.Committable suggestion skipped: line range outside the PR's diff.
68-74: 🛠️ Refactor suggestion
⚠️ Potential issueAvoid
expect→ surface a user-friendly error instead of panicking
expect("No local qualified identity…")will crash the whole UI when the identity is missing (e.g. user deleted it from disk).
Return to a previous screen or render an error banner instead, so the app stays responsive.-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(|| { + format!( + "No local qualified identity found for identity {}", + identity_token_balance.identity_id + ) + })?;(If you prefer not to propagate errors, at least set
self.status = ErrorMessage(...)and stay on the screen.)📝 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 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(|| { format!( "No local qualified identity found for identity {}", identity_token_balance.identity_id ) })?;src/ui/tokens/pause_tokens_screen.rs (2)
162-169:
⚠️ Potential issuePotential panic on missing signing key
self.selected_key.clone().expect("No key selected")will crash if the user hasn’t picked a key. Either:
- Disable the “Pause” button until a key is chosen (preferred – the UI already shows a ComboBox).
- Or convert the absence to an
ErrorMessagestate instead of panicking.
65-71:
⚠️ Potential issueGracefully handle missing identity instead of panicking
Same concern as in the Unfreeze screen: crashing the UI is harsh. Convert this into a recoverable error.
src/backend_task/tokens/mod.rs (1)
523-528: 🛠️ Refactor suggestion
Hard-coding token position
0may collide with existing tokensInserting every token at
TokenContractPosition::from(0u16)will fail if the contract is extended with more tokens later.
Allow the caller to specify the desired position or computetokens.len()to append safely.src/ui/tokens/destroy_frozen_funds_screen.rs (3)
196-204: 🛠️ Refactor suggestion
⚠️ Potential issueGracefully handle missing/ unloaded contracts
If
get_contractsreturnsError the sought-for contract is not yet cached, the chain ofexpect(..)will panic. Prefer surfacing a user-visible error and allowing them to retry once the contract list is downloaded.Suggestion (sketch):
let data_contract = match self .app_context .get_contracts(None, None) .and_then(|v| { v.into_iter() .find(|c| c.contract.id() == self.identity_token_balance.data_contract_id) .map(|c| c.contract.clone()) .ok_or_else(|| anyhow!("Data contract not found")) }) { Ok(c) => c, Err(e) => { self.error_message = Some(format!("Cannot load contract: {e}")); self.status = DestroyFrozenFundsStatus::ErrorMessage("Contract not available".into()); return AppAction::None; } };
213-214: 🛠️ Refactor suggestion
⚠️ Potential issue
expectonselected_keyrisks panic – validate earlierIf the user never chooses a key, clicking “Destroy” will crash the app. Disable the button until a key is chosen or convert the
expectinto a graceful error path.
74-82: 🛠️ Refactor suggestion
⚠️ Potential issueAvoid crashing the whole UI when the local identity is missing
expect("No local qualified identity…")will panic and terminate the entire GUI if the token’s owner identity is not present locally (e.g. user restored the wallet on a different machine). Consider returning a screen-level error instead and letting the user resolve the situation.- 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 matching 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); + + let identity = match identity { + Some(id) => id, + None => { + return Self { + error_message: Some("Local identity for this token not found".into()), + status: DestroyFrozenFundsStatus::ErrorMessage( + "Local identity missing".into(), + ), + // ...keep the remaining struct initialisation unchanged + ..Default::default() // or mirror the manual fields + }; + } + };📝 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.// Locate the local identity that owns the token contract let identity = app_context .load_local_qualified_identities() .unwrap_or_default() .into_iter() .find(|id| id.identity.id() == identity_token_balance.identity_id); let identity = match identity { Some(id) => id, None => { return Self { error_message: Some("Local identity for this token not found".into()), status: DestroyFrozenFundsStatus::ErrorMessage( "Local identity missing".into(), ), ..Default::default() }; } }; // Grab a suitable keysrc/ui/tokens/mint_tokens_screen.rs (1)
68-74: 🛠️ Refactor suggestion
⚠️ Potential issueAvoid panicking when no local identity matches
Same risk pattern as in the destroy-funds screen:
expect("No local qualified identity…")will abort the UI. Return a screen with an error banner instead.src/ui/contracts_documents/register_contract_screen.rs (1)
341-342: 🛠️ Refactor suggestion
Unchecked
unwrapmay panic when no key or identity selected
selected_qualified_identityandselected_keyare force-unwrapped. If the user deselects them (or a future UI change allows “none”), the app will crash. Either disable the “Register Contract” button until both areSome, or handleNonegracefully.src/ui/tokens/freeze_tokens_screen.rs (2)
184-202:
⚠️ Potential issueUnchecked
.expect()can still trigger a panicInside the confirmation path the code dereferences several
Options withexpect()(data_contractandselected_key).
The UI does try to set an error message earlier, but a race condition (or a programmer-mode key deletion) can still make theseOptionsNone, crashing the whole app.Replace these
expect()s with a user-visible error and keep the application alive.
68-75: 🛠️ Refactor suggestion
Avoid hard-panicking when the identity cannot be found
expect("No local qualified identity found for this token")will crash the entire UI if the local DB ever becomes inconsistent (e.g. the balance row survives while the identity was deleted).
Return an error message and gracefully navigate the user back instead of panicking.-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"); +let identity = match app_context + .load_local_qualified_identities() + .unwrap_or_default() + .into_iter() + .find(|id| id.identity.id() == identity_token_balance.identity_id) +{ + Some(id) => id, + None => { + log::error!("Identity {:#x?} is missing for token balance", identity_token_balance.identity_id); + return Self::show_missing_identity_error(app_context.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 identity = match app_context .load_local_qualified_identities() .unwrap_or_default() .into_iter() .find(|id| id.identity.id() == identity_token_balance.identity_id) { Some(id) => id, None => { log::error!( "Identity {:#x?} is missing for token balance", identity_token_balance.identity_id ); return Self::show_missing_identity_error(app_context.clone()); } }; let identity_clone = identity.identity.clone();src/ui/tokens/transfer_tokens_screen.rs (2)
67-75:
⚠️ Potential issue
max_amountmay reference a different tokenThe lookup only filters by
identity_id; if the same identity owns multiple tokens, the first match may not correspond to the currently selectedidentity_token_balance, leading to an incorrect “Max” value and potential rejection from the backend.Prefer using the balance that matches both
identity_idand the(data_contract_id, token_position)pair (or just reuseidentity_token_balance.balance).
222-233:
⚠️ Potential issueUnhandled parse errors on
amountcan crash the UI
self.amount.parse().unwrap()will panic on non-numeric input, empty string, or values >u64::MAX.
Validate the amount and surface a user-friendly error instead.-let parsed_amount = self.amount.parse().unwrap(); +let parsed_amount = match self.amount.parse::<u64>() { + Ok(a) if a > 0 && a <= self.max_amount => a, + _ => { + self.error_message = Some("Enter a valid amount ≤ available balance".into()); + self.transfer_tokens_status = + TransferTokensStatus::ErrorMessage("Invalid amount".into()); + return AppAction::None; + } +}; ... - amount: self.amount.parse().unwrap(), + amount: parsed_amount,Committable suggestion skipped: line range outside the PR's diff.
src/ui/tokens/claim_tokens_screen.rs (2)
244-267: 🛠️ Refactor suggestion
Replace
expectpanics with graceful error propagation
show_confirmation_popupuses severalexpectcalls that will abort the whole application on a missing contract or key.
Consider surfacing these problems viaClaimTokensStatus::ErrorMessageinstead, so the user receives feedback instead of a crash.Example (only one shown – apply the same pattern to the others):
- let data_contract = self - .app_context - .get_contracts(None, None) - .expect("Contracts not loaded") - .iter() - .find(|c| c.contract.id() == self.identity_token_balance.data_contract_id) - .expect("Data contract not found") - .contract - .clone(); + let data_contract = match self + .app_context + .get_contracts(None, None) + .ok() + .and_then(|v| { + v.into_iter() + .find(|c| c.contract.id() == self.identity_token_balance.data_contract_id) + }) { + Some(c) => c.contract.clone(), + None => { + self.status = ClaimTokensStatus::ErrorMessage("Data-contract not found".into()); + return action; // early-out with no backend tasks enqueued + } + };📝 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 data_contract = match self .app_context .get_contracts(None, None) .ok() .and_then(|v| { v.into_iter() .find(|c| c.contract.id() == self.identity_token_balance.data_contract_id) }) { Some(c) => c.contract.clone(), None => { self.status = ClaimTokensStatus::ErrorMessage("Data-contract not found".into()); return action; // early-out with no backend tasks enqueued } }; action = AppAction::BackendTasks( vec![ BackendTask::TokenTask(TokenTask::ClaimTokens { data_contract, 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, );
430-437:
⚠️ Potential issueValidate
selected_keybefore opening the confirmation dialogThe click handler only checks
distribution_type.
Ifself.selected_keyis stillNone, the subsequentexpect("No key selected")inshow_confirmation_popupwill panic and crash the UI.if ui.add(button).clicked() { - if self.distribution_type.is_none() { + if self.selected_key.is_none() { + self.status = ClaimTokensStatus::ErrorMessage( + "Please select a signing key.".to_string(), + ); + return; + } + if self.distribution_type.is_none() { self.status = ClaimTokensStatus::ErrorMessage( "Please select a distribution type.".to_string(), ); return; - } else { - self.show_confirmation_popup = true; - } + } + self.show_confirmation_popup = true; }src/context.rs (2)
596-622:
⚠️ Potential issueFail fast if token configuration cannot be serialized
insert_tokensilently swallows a bincode failure by returningOk(()).
This hides real errors and leaves the database in an inconsistent state (token row missing).Replace with an explicit error:
-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| format!("Failed to serialize token configuration: {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.pub fn insert_token( &self, token_id: &Identifier, token_name: &str, token_configuration: TokenConfiguration, contract_id: &Identifier, token_position: u16, ) -> Result<()> { let config = config::standard(); let serialized_token_configuration = bincode::encode_to_vec(&token_configuration, config) .map_err(|e| format!("Failed to serialize token configuration: {e}"))?; self.db.insert_token( token_id, token_name, serialized_token_configuration.as_slice(), contract_id, token_position, self, )?; Ok(()) }
411-425: 🛠️ Refactor suggestion
get_contract_by_iddoesn’t look into in-memory system contracts
get_contract_by_idreturnsNonewhen the record is missing from SQLite, but it never checks the four pre-loaded system contracts (DPNS, TokenHistory, Withdrawals, KeywordSearch).
Any UI that relies on this helper will fail to resolve those contracts even though they are available in memory.Consider:
-// If the contract is not found in the database, return None -if contract.is_none() { - return Ok(None); -} - -// If the contract is found, return it -Ok(Some(contract.unwrap())) +if let Some(c) = contract { + return Ok(Some(c)); +} + +// Check in-memory system contracts +for c in [ + &self.dpns_contract, + &self.token_history_contract, + &self.withdraws_contract, + &self.keyword_search_contract, +] { + if c.id() == contract_id { + return Ok(Some(QualifiedContract { + contract: c.as_ref().clone(), + alias: None, + })); + } +} +Ok(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.pub fn get_contract_by_id( &self, contract_id: &Identifier, ) -> Result<Option<QualifiedContract>> { // Get the contract from the database let contract = self.db.get_contract_by_id(*contract_id, self)?; // First, return anything the DB knows about if let Some(c) = contract { return Ok(Some(c)); } // Otherwise, check the in-memory system contracts for c in [ &self.dpns_contract, &self.token_history_contract, &self.withdraws_contract, &self.keyword_search_contract, ] { if c.id() == contract_id { return Ok(Some(QualifiedContract { contract: c.as_ref().clone(), alias: None, })); } } // Not found anywhere Ok(None) }src/ui/mod.rs (1)
294-317:
⚠️ Potential issueIncorrect screen routing – several token actions open the wrong UI
ScreenType::create_screenmaps Freeze, Unfreeze, Pause, and Resume actions toDestroyFrozenFundsScreen, which makes the corresponding menu items open the wrong screen.-ScreenType::FreezeTokensScreen(identity_token_balance) => { - Screen::DestroyFrozenFundsScreen(DestroyFrozenFundsScreen::new( +ScreenType::FreezeTokensScreen(identity_token_balance) => { + Screen::FreezeTokensScreen(FreezeTokensScreen::new( identity_token_balance.clone(), app_context, )) } ScreenType::UnfreezeTokensScreen(identity_token_balance) => { - Screen::DestroyFrozenFundsScreen(DestroyFrozenFundsScreen::new( + Screen::UnfreezeTokensScreen(UnfreezeTokensScreen::new( identity_token_balance.clone(), app_context, )) } ScreenType::PauseTokensScreen(identity_token_balance) => { - Screen::DestroyFrozenFundsScreen(DestroyFrozenFundsScreen::new( + Screen::PauseTokensScreen(PauseTokensScreen::new( identity_token_balance.clone(), app_context, )) } ScreenType::ResumeTokensScreen(identity_token_balance) => { - Screen::DestroyFrozenFundsScreen(DestroyFrozenFundsScreen::new( + Screen::ResumeTokensScreen(ResumeTokensScreen::new( identity_token_balance.clone(), app_context, )) }Fixing this restores the expected navigation flow.
📝 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.ScreenType::DestroyFrozenFundsScreen(identity_token_balance) => { Screen::DestroyFrozenFundsScreen(DestroyFrozenFundsScreen::new( identity_token_balance.clone(), app_context, )) } ScreenType::FreezeTokensScreen(identity_token_balance) => { Screen::FreezeTokensScreen(FreezeTokensScreen::new( identity_token_balance.clone(), app_context, )) } ScreenType::UnfreezeTokensScreen(identity_token_balance) => { Screen::UnfreezeTokensScreen(UnfreezeTokensScreen::new( identity_token_balance.clone(), app_context, )) } ScreenType::PauseTokensScreen(identity_token_balance) => { Screen::PauseTokensScreen(PauseTokensScreen::new( identity_token_balance.clone(), app_context, )) } ScreenType::ResumeTokensScreen(identity_token_balance) => { Screen::ResumeTokensScreen(ResumeTokensScreen::new( identity_token_balance.clone(), app_context, )) }src/database/tokens.rs (4)
326-336: 🛠️ Refactor suggestion
token_ordertable missesnetwork; order persists across networksOrdering should be per-identity and per-network.
Without anetworkcolumn, switching networks will mix Devnet/Mainnet rows and the UNIQUE constraintPRIMARY KEY(pos, token_id)may break.-CREATE TABLE token_order ( - pos INTEGER NOT NULL, - token_id BLOB NOT NULL, - identity_id BLOB NOT NULL, +CREATE TABLE token_order ( + pos INTEGER NOT NULL, + token_id BLOB NOT NULL, + identity_id BLOB NOT NULL, + network TEXT NOT NULL, PRIMARY KEY(pos, token_id, identity_id, network), ... -INSERT INTO token_order (pos, token_id, identity_id) - VALUES (?1, ?2, ?3) +INSERT INTO token_order (pos, token_id, identity_id, network) + VALUES (?1, ?2, ?3, ?4)
save_token_order/load_token_ordermust be updated accordingly.Also applies to: 350-360, 366-394
17-29: 🛠️ Refactor suggestion
⚠️ Potential issueAdd
networkto PRIMARY KEY &ON CONFLICTto avoid cross-network data corruption
idalone is the primary key, yet we store the same token ID on multiple networks.
Currently an insert on Devnet would silently overwrite a Mainnet row with the sameidbecauseON CONFLICT(id)has nonetworkdiscriminator.-CREATE TABLE IF NOT EXISTS token ( - id BLOB PRIMARY KEY, +CREATE TABLE IF NOT EXISTS token ( + id BLOB NOT NULL, + network TEXT NOT NULL, token_alias TEXT NOT NULL, token_config BLOB NOT NULL, data_contract_id BLOB NOT NULL, token_position INTEGER NOT NULL, - network TEXT NOT NULL, + PRIMARY KEY(id, network), ... -ON CONFLICT(id) DO UPDATE SET +ON CONFLICT(id, network) DO UPDATE SETSame composite key should be reflected in every
SELECT/DELETEand foreign key definition.Failing to fix this will cause panics or, worse, users seeing the wrong token metadata when they switch networks.
Also applies to: 105-124
139-147: 🛠️ Refactor suggestion
Foreign-key inconsistency between
identity_token_balancesandtoken
identity_token_balancesincludesnetworkin its PK but referencestoken(id)withoutnetwork.
If the sameidexists on two networks foreign-key checks can link to the wrong row.Recommendation: reference the composite
(id, network)once the previous comment is addressed.
191-207: 🛠️ Refactor suggestion
Using
expect()may panic on malformed DB rows
Identifier::from_vec(..).expect("Failed to parse …")will abort the whole UI if a single record is corrupt.
Prefer graceful handling:-let token_id = token_id_res.expect("Failed to parse token ID"); +let token_id = match token_id_res { + Ok(id) => id, + Err(e) => { + log::warn!("Skipping token row with invalid id: {e}"); + continue; + } +};Repeat for other
expectcalls.Also applies to: 265-268
src/app.rs (1)
314-339: 🛠️ Refactor suggestion
Listeners are spawned even when AppContext is
None
devnet_core_zmq_listener/local_core_zmq_listenerare created unconditionally.
If the config is missing, the app context isNonebut the listener still binds the port, wasting resources and potentially exposing an unused socket.Consider skipping listener creation when the corresponding
AppContextisNone.
2 participants
Builds of v0.9-dev (629a55a) failed for me on Ubuntu 22.04 with the following error without this change:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores