v0.9.3 to master#458
Conversation
* fix: top up with qr code wasnt working * fmt and clippy * coderabbit
* fix: dash-qt path not detected correctly * fix: correct handling of dash-qt path clear button * chore: copilot review
…and others) (#406) * build(deps): update all rust dependencies * chore: replace depreacated ui.close_menu() * chore: upgrade Popup in identities screen * fix: actions menu items width too low * chore: all warnings fixed * chore: close identity actions on click outside --------- Co-authored-by: pauldelucia <pauldelucia2@gmail.com>
* refactor: unified identity selection * feat: identity selector implementation on multiple screens * feat: add label to IdentitySelector * chore: I think final * Update src/ui/components/identity_selector.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* feat: amount input, first building version * test(amount): fixed tests * chore: I think final * chore: my_tokens display correct amount * chore: transfer tokens update * chore: hide unit on rewards estimate column * chore: two new helper methods * chore: I think finals * cargo fmt * feat: component trait * impl Component for AmountInput * chore: updated component trait * chore: update for egui enabled state mgmt * doc: component design pattern doc * chore: component design pattern continued * chore: amount improvements * chore: copilot review * chore: amount improvements * backport: amount component from * chore: fix imports * chore: refactor * chore: futher refactor * chore: further refactor based on feedback * doc: simplified component design pattern description * chore: peer review * doc: update docs * chore: amount input
* fix: failing tests * gha: run tests * fix: tests fail due to lack of .env file
…ns (#419) * feat: amount input, first building version * test(amount): fixed tests * chore: I think final * chore: my_tokens display correct amount * chore: transfer tokens update * chore: hide unit on rewards estimate column * chore: two new helper methods * chore: I think finals * cargo fmt * feat: component trait * impl Component for AmountInput * chore: updated component trait * chore: update for egui enabled state mgmt * doc: component design pattern doc * chore: component design pattern continued * chore: amount improvements * chore: copilot review * chore: amount improvements * refactor: mint and burn token screens use AmountInput * chore: use AmountInput on token creator screen * fix: burn error handling * feat: errors displayed in the AmountInput component * fix: vertical align of amount input * backport: amount component from * chore: fix imports * chore: refactor * chore: futher refactor * chore: further refactor based on feedback * doc: simplified component design pattern description * chore: peer review * doc: update docs * chore: amount input * chore: fixes after merge * chore: self-review * feat: amout set decimal places + rename label => with_label * refactor: amount input init on token screen * chore: fix token creator layout * chore: format base amount with leading zeros in confirmation * chore: base supply 0 by default
* feat: add network field to identity structures * fix
* feat: add display for private key in both WIF and Hex formats * fix: display private keys as normal text and depend on color theme --------- Co-authored-by: pauldelucia <pauldelucia2@gmail.com>
* feat: allow setting ZMQ URI * chore: rename zmq_endpoint to core_zmq_endpoint
* refactor: unified alert window * chore: update CLAUDE to create reusable components whenever appropriate * feat: correct confirm dialog on set token price screen * chore: remove callbacks which are overkill * chore: cargo fmt * chore: doctest fix * chore: impl Component for ConfirmationDialog * chore: use WidgetText * feat: Add Escape key handling for confirmation dialog * chore: button inactive when in progress * chore: fixes after merge * chore: some theme improvements * fmt and visual appeal --------- Co-authored-by: pauldelucia <pauldelucia2@gmail.com>
* feat: nicer contract chooser panel * fmt * clippy
* feat: nicer expanding tabs in token creator * fix
* feat: implement new ConfirmationDialog component throughout app * fix: remove backup files
* a lot of work on DML viewer * more work * more work * more work * more work * more work * more work * more work * more work * ui improvements * ui improvements * optimizations * fast start * more work * more work * more work * fmt * much more work * fixes * updates * fix * fmt * revert * update for dashcore 40 * params for testnet * added testnet diff * fixes * clippy * more clippy * fixes * clean UI * use backend tasks * backend messages * coderabbit suggestions to add timeouts and prevent infinite loops * transient and fatal errors * update dash core configs for testnet * fmt * fix timeout * fix h-3 error * clear state when switching networks --------- Co-authored-by: pauldelucia <pauldelucia2@gmail.com>
The 1.0-dev branch builds were still reporting as 0.9.0
…ser panel (#441) * feat: add left panel button labels and create contract subscreen chooser panel * fmt
* feat: left panel scroll * clippy
* feat: add existing identity by wallet + identity index * fmt * remove logging * update derivation index label * add robustness * fix: screen button labels display hard to see in dark mode * feat: left panel scroll (#443) * feat: left panel scroll * clippy * feat: load all identities up to an index * clippy * fix
* feat: remove wallet * clippy * Clarify wallet removal warning message Updated warning message for wallet removal to clarify that keys will no longer work unless the wallet is re-imported.
* fix: identity creation handling * cleanup * cleanup * fix identity topup too * remove note
… and UI updates (#449) * fix: keyword search not displaying errors * update platform version and keyword search cleanup * fix
…412) * fix: inactive button on set price group action * chore: apply review feedback * refactor: use token amount input * chore: update AmountInput * chore: tiered pricing * chore: direct purchase * chore: remove total agreed price * chore: max amount input defaults to max_credits * fmt * fix * clippy --------- Co-authored-by: pauldelucia <pauldelucia2@gmail.com> Co-authored-by: Claude <noreply@anthropic.com>
* works * works * fix * ok * ok * ok * ok * ok * fix verification message showing in generation screen * remove advanced settings. set default 16 grinding bits and 128 bit security * ok * lock * ok * clippy * fmt * clippy and fmt * pin grovestark dep * fix * ok * clippy * rename file * remove ProofData * cleanup * fix * rename GroveStark to GroveSTARK * rename zk_proofs_screen to grovestark_screen * rename ZKProofs tool subscreen to GroveSTARK * move grovestark prover to model * rename ContractsDashpayComingSoon to ContractsDashpay * rename ContractsDashpay to Dashpay * rename dashpay stuff * fixes * ok * update grovestark rev * update
WalkthroughAdds per-network core ZMQ config, Rust/tooling bumps, new GroveSTARK prover and MNList runners, a Core P2P handler, typed Amount and UI component primitives (AmountInput, ConfirmationDialog, IdentitySelector, Component trait), settings caching, network-aware identity/wallet flows, and broad UI/backend refactors. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI Screen
participant Comp as AmountInput
participant Dialog as ConfirmationDialog
participant Backend as BackendTask runner
UI->>Comp: render / user types amount
Comp-->>UI: validated Amount or error
UI->>UI: validate identity/key selection
UI->>Dialog: create/show dialog (lazy)
Dialog-->>UI: user confirms or cancels
alt user confirms
UI->>Backend: dispatch BackendTask with typed Amount & params
Backend-->>UI: task queued / result
else user cancels
UI->>UI: clear dialog
end
sequenceDiagram
participant App as Application
participant Ctx as AppContext
participant DB as Database
App->>Ctx: get_settings()
alt cache hit
Ctx-->>App: cached Settings
else cache miss
Ctx->>DB: load settings
DB-->>Ctx: Settings
Ctx->>Ctx: store cache
Ctx-->>App: Settings
end
App->>Ctx: update_dash_core_execution_settings(...)
Ctx->>Ctx: invalidate_settings_cache()
Ctx->>DB: persist changes
DB-->>Ctx: Result
sequenceDiagram
participant App as App
participant P2P as CoreP2PHandler
participant Core as Dash P2P Node
App->>P2P: new(network, port)
P2P->>Core: TCP connect
Core-->>P2P: socket OK
P2P->>Core: send Version
Core-->>P2P: recv Version
P2P->>Core: send Verack
Core-->>P2P: recv Verack
Note right of P2P: handshake complete
App->>P2P: get_dml_diff(...)
P2P->>Core: send GetMnListDiff
Core-->>P2P: recv MnListDiff
P2P-->>App: return MnListDiff
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)src/components/core_zmq_listener.rs (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (3)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 45
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (22)
src/ui/tokens/tokens_screen/data_contract_json_pop_up.rs (1)
62-62: Fix incorrect method call: should useavailable_width()notavailable_height().Line 62 uses
available_height()to constrain the maximum width, which is a logic error. This should beavailable_width()to properly constrain the horizontal dimension.Apply this diff:
- .max_width(ui.available_height() - 20.0) + .max_width(ui.available_width() - 20.0)src/ui/components/left_panel.rs (1)
20-21: Per‑frame texture thrash: use persisted cache, not temp.get_temp/insert_temp data is cleared every frame; icons will reload and re-upload to GPU continuously. Persist across frames to avoid jank and leaks.
Apply this change:
- ctx.data_mut(|d| d.get_temp::<TextureHandle>(egui::Id::new(path))) + ctx.data_mut(|d| d.get_persisted::<TextureHandle>(egui::Id::new(path))) @@ - // Cache the texture - ctx.data_mut(|d| d.insert_temp(egui::Id::new(path), texture.clone())); + // Persist for the app lifetime (survives frames) + ctx.data_mut(|d| d.insert_persisted(egui::Id::new(path), texture.clone()));Alternatively, store TextureHandle(s) in AppContext or use egui_extras::RetainedImage.
Also applies to: 30-38
src/backend_task/tokens/query_tokens.rs (1)
29-31: Avoid panics in backend tasks: propagate query construction errors instead of expect().Replace expect("create query")/expect("create desc query") with map_err and early-return. Panics here would crash the UI thread path.
Apply this diff:
- let mut kw_query = - DocumentQuery::new(self.keyword_search_contract.clone(), "contractKeywords") - .expect("create query"); + let mut kw_query = + DocumentQuery::new(self.keyword_search_contract.clone(), "contractKeywords") + .map_err(|e| format!("create keywords query: {e}"))?; … - let mut desc_query = - DocumentQuery::new(self.keyword_search_contract.clone(), "shortDescription") - .expect("create desc query"); + let mut desc_query = + DocumentQuery::new(self.keyword_search_contract.clone(), "shortDescription") + .map_err(|e| format!("create description query: {e}"))?;Also applies to: 71-73
src/components/core_zmq_listener.rs (1)
47-56: Critical: topic mismatch (subscribing “rawchainlock”, handling “rawchainlocksig”).Currently you subscribe to CHAIN_LOCKED_BLOCK_MSG="rawchainlock" but match on "rawchainlocksig", so ChainLock messages are never handled. Update the constant and subscription.
Apply this diff:
-#[cfg(not(target_os = "windows"))] -pub const CHAIN_LOCKED_BLOCK_MSG: &[u8; 12] = b"rawchainlock"; +#[cfg(not(target_os = "windows"))] +pub const CHAIN_LOCKED_BLOCK_MSG: &[u8; 14] = b"rawchainlocksig"; … - // Subscribe to the "rawtxlocksig" events. + // Subscribe to the chainlock signature events. socket .set_subscribe(CHAIN_LOCKED_BLOCK_MSG) - .expect("Failed to subscribe to rawchainlock"); + .expect("Failed to subscribe to rawchainlocksig");Also applies to: 99-107, 140-181
src/ui/tokens/tokens_screen/distributions.rs (3)
550-551: Also saturate products in Stepwise preview text.Same overflow risk for block/time/epoch = step_position * interval.
Apply this diff:
- let block = step_position * perpetual_dist_interval_input; + let block = step_position.saturating_mul(perpetual_dist_interval_input); … - let time = step_position * perpetual_dist_interval_input; + let time = step_position.saturating_mul(perpetual_dist_interval_input); … - let epoch = step_position * perpetual_dist_interval_input; + let epoch = step_position.saturating_mul(perpetual_dist_interval_input);Also applies to: 564-565, 578-579
787-790: Fix sanitizer type: base amount (b) is u64 but sanitize_i64 is used.Use sanitize_u64 to match the specified type.
Apply this diff:
- ui.text_edit_singleline(&mut self.exp_b_input); - sanitize_i64(&mut self.exp_b_input); + ui.text_edit_singleline(&mut self.exp_b_input); + sanitize_u64(&mut self.exp_b_input);
196-335: Info text for StepDecreasing describes a Random function.The section under “# StepDecreasingAmount” explains random emissions, which contradicts the selected function and the UI (Random not supported). Replace with a correct description of step‑decreasing emissions.
Apply this diff (illustrative replacement):
-# StepDecreasingAmount - -Emits a random number of tokens within a specified range. -… -`f(x) ∈ [min, max]` +# StepDecreasingAmount + +Emits a fixed amount that decreases by a ratio each interval up to optional bounds. + +Formula (per interval i ≥ s): + f(i) = max(min_value, b · (1 − n/d)^{i − s}) + +Parameters: + - n/d: decrease per interval (0 < n < d ≤ 65_535) + - s: optional start period offset + - b: initial emission amount + - min_value: optional floor + +Use cases: + - Tapering rewards over time + - Emission halving-like schedulessrc/ui/tokens/unfreeze_tokens_screen.rs (1)
522-529: Copy/paste typo: says “Mint” on Unfreeze screen.Change text to “Unfreeze.”
- "You are signing an existing group Mint so you are not allowed to put a note.", + "You are signing an existing group Unfreeze so you are not allowed to put a note.",src/model/qualified_identity/encrypted_key_storage.rs (3)
28-33: Make serialization portable and bounded (use u32, add cap).Using usize in encoded length is arch‑dependent and unbounded. Switch to u32 with a sane max to avoid compatibility/OOM issues.
--- a/src/model/qualified_identity/encrypted_key_storage.rs +++ b/src/model/qualified_identity/encrypted_key_storage.rs @@ -26,9 +26,14 @@ impl Encode for WalletDerivationPath { - // Encode the length of the `DerivationPath` - self.derivation_path.len().encode(encoder)?; + // Encode the length of the `DerivationPath` (portable) + let path_len_u32 = u32::try_from(self.derivation_path.len()) + .map_err(|_| EncodeError::OtherString("DerivationPath too long".into()))?; + path_len_u32.encode(encoder)?; @@ -60,7 +65,12 @@ impl Decode for WalletDerivationPath { - // Decode the length of the `DerivationPath` - let path_len = usize::decode(decoder)?; + // Decode the length of the `DerivationPath` (portable) + bound check + let path_len_u32 = u32::decode(decoder)?; + let path_len = path_len_u32 as usize; + // hard cap prevents pathological allocations + const MAX_PATH_LEN: usize = 128; + if path_len > MAX_PATH_LEN { + return Err(DecodeError::OtherString("DerivationPath too long".into())); + } @@ -100,7 +110,12 @@ impl<'de> BorrowDecode<'de> for WalletDerivationPath { - // Decode the length of the `DerivationPath` - let path_len = usize::decode(decoder)?; + // Decode the length of the `DerivationPath` (portable) + bound check + let path_len_u32 = u32::decode(decoder)?; + let path_len = path_len_u32 as usize; + const MAX_PATH_LEN: usize = 128; + if path_len > MAX_PATH_LEN { + return Err(DecodeError::OtherString("DerivationPath too long".into())); + }Also applies to: 62-69, 100-107
141-166: Do not print private keys. Redact clear key material.fmt::Display currently hex‑prints clear keys, which can leak secrets via logs/UI. Redact instead.
impl fmt::Display for PrivateKeyData { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - PrivateKeyData::Clear(data) => { - write!(f, "Clear({})", hex::encode(data)) - } + PrivateKeyData::Clear(_data) => write!(f, "Clear([redacted])"), PrivateKeyData::Encrypted(data) => { write!(f, "Encrypted({} bytes)", data.len()) } - PrivateKeyData::AlwaysClear(data) => { - write!(f, "Clear({})", hex::encode(data)) - } + PrivateKeyData::AlwaysClear(_data) => write!(f, "Clear([redacted])"), PrivateKeyData::AtWalletDerivationPath(WalletDerivationPath { wallet_seed_hash: wallet_seed, derivation_path, }) => { write!( f, "AtWalletDerivationPath({}/{})", hex::encode(wallet_seed), derivation_path ) } } } }
251-275: Adopt structured errors (thiserror) instead ofString.Public APIs return Result<_, String>. Per repo guideline, prefer
thiserrorenums for clarity and downstream handling. I can draft a minimalKeyStorageErrorand update signatures if desired.As per coding guidelines.
Also applies to: 277-323
src/ui/identities/add_new_identity_screen/by_wallet_qr_code.rs (1)
101-107: Fix label/content mismatch.Button copies the full payment URI; label says “Copy Address”.
- if ui.button("Copy Address").clicked() { + if ui.button("Copy Payment URI").clicked() { if let Err(e) = copy_to_clipboard(pay_uri.as_str()) {src/ui/tokens/tokens_screen/groups.rs (1)
180-240: Fix removal loop: removing then incrementing skips the next member.When you remove at index j, the next element shifts into j. Incrementing unconditionally skips it.
- let mut j = 0; - while j < group_ui.members.len() { - let mut member = group_ui.members[j].clone(); + let mut j = 0; + while j < group_ui.members.len() { + let mut member = group_ui.members[j].clone(); + let mut removed = false; @@ - if ui.button("Remove Member").clicked() { - group_ui.members.remove(j);// return so we skip the assignment at the end - } else { - // Only assign back if we didn’t remove - group_ui.members[j] = member; - } + if ui.button("Remove Member").clicked() { + group_ui.members.remove(j); + removed = true; + } else { + group_ui.members[j] = member; + } }); - j += 1; + if !removed { + j += 1; + } }src/ui/tokens/direct_token_purchase_screen.rs (1)
360-365: Fix completion trigger typo.“purchaseed” prevents status from reaching Complete.
- if message.contains("Successfully purchaseed tokens") || message == "PurchaseTokens" + if message.contains("Successfully purchased tokens") || message == "PurchaseTokens"src/ui/identities/top_up_identity_screen/by_wallet_qr_code.rs (3)
16-23: Release wallet lock before Core RPC to avoid lock-order deadlocks.You hold a write lock on Wallet while also taking a read lock on core_client and performing RPC. This mixed lock ordering across subsystems risks deadlock and blocks the UI thread. Drop the wallet lock immediately after deriving the receive address.
Apply this minimal change:
let receive_address = wallet.receive_address( self.app_context.network, false, Some(&self.app_context), )?; + // Release wallet lock before contacting Core + drop(wallet); // Import address to Core if needed for monitoring let core_client = selfAlso applies to: 25-31
73-79: Button label/content mismatch (address vs URI).You copy pay_uri but the button says "Copy Address". Either copy address.to_string() or rename the button.
Apply one of:
- if ui.button("Copy Address").clicked() { - if let Err(e) = copy_to_clipboard(pay_uri.as_str()) { + if ui.button("Copy Payment URI").clicked() { + if let Err(e) = copy_to_clipboard(pay_uri.as_str()) {or
- if ui.button("Copy Address").clicked() { - if let Err(e) = copy_to_clipboard(pay_uri.as_str()) { + if ui.button("Copy Address").clicked() { + if let Err(e) = copy_to_clipboard(address.to_string().as_str()) {
57-57: Increase amount precision from 4 to 8 decimal places to match Dash payment URI specification.Dash URIs should support up to 8 decimal places (smallest unit is the duff = 10^-8 Dash). The current format
{:.4}truncates precision and can cause payment errors.Update both occurrences:
- src/ui/identities/top_up_identity_screen/by_wallet_qr_code.rs:57
- src/ui/identities/add_new_identity_screen/by_wallet_qr_code.rs:84
Change:
format!("{}?amount={:.4}", ...)→format!("{}?amount={:.8}", ...)src/ui/identities/keys/key_info_screen.rs (1)
593-616: Bug: use-after-move on Result (compile error).validation_result is moved by if let Err(err) pattern, then unwrapped. Capture once and branch on the boolean.
Apply:
- let validation_result = self - .key - .validate_private_key_bytes(&private_key_bytes, self.app_context.network); - if let Err(err) = validation_result { - self.error_message = Some(format!("Issue verifying private key {}", err)); - } else if validation_result.unwrap() { + let is_valid = match self + .key + .validate_private_key_bytes(&private_key_bytes, self.app_context.network) + { + Ok(v) => v, + Err(err) => { + self.error_message = Some(format!("Issue verifying private key {}", err)); + return; + } + }; + if is_valid { // If valid, store the private key in the context and reset the input fieldsrc/model/qualified_identity/mod.rs (1)
606-611: Don’t panic on inconsistent wallet index.
This will crash the UI. Return an error instead so callers can surface it gracefully.Apply:
- if self.wallet_index.is_some_and(|v| v != wallet_index) { - panic!( - "Inconsistent wallet index found: {:?} vs {:?}", - self.wallet_index, wallet_index - ); - }; + if self.wallet_index.is_some_and(|v| v != wallet_index) { + return Err(format!( + "Inconsistent wallet index found: {:?} vs {:?}", + self.wallet_index, wallet_index + )); + };src/ui/tokens/pause_tokens_screen.rs (1)
92-111: User-facing text refers to “Burning” on a Pause screen.
Replace “Burning” with “Pausing” and adjust the permission text accordingly.Suggested replacements:
- "Pausing is not allowed on this token"
- "You are not allowed to pause this token. Only the contract owner is."
- "You are not allowed to pause this token."
- error_message = Some("Burning is not allowed on this token".to_string()); + error_message = Some("Pausing is not allowed on this token".to_string()); ... - "You are not allowed to burn this token. Only the contract owner is." + "You are not allowed to pause this token. Only the contract owner is." ... - error_message = Some("You are not allowed to burn this token".to_string()); + error_message = Some("You are not allowed to pause this token".to_string());Also applies to: 96-105, 107-112
src/ui/identities/add_existing_identity_screen.rs (1)
668-693: Time unit mismatch: TimestampMillis but storing seconds.AddIdentityStatus::WaitingForResult(TimestampMillis) is populated with as_secs(), yet later treated as seconds. Store milliseconds and compute elapsed accordingly to match the type.
- let now = SystemTime::now() - .duration_since(UNIX_EPOCH) - .expect("Time went backwards") - .as_secs(); + let now = SystemTime::now() + .duration_since(UNIX_EPOCH) + .expect("Time went backwards") + .as_millis() as u64; self.add_identity_status = AddIdentityStatus::WaitingForResult(now);And when displaying elapsed:
- let now = SystemTime::now() + let now = SystemTime::now() .duration_since(UNIX_EPOCH) .expect("Time went backwards") - .as_secs(); - let elapsed_seconds = now - start_time; + .as_millis() as u64; + let elapsed_ms = now - start_time; + let elapsed_seconds = elapsed_ms / 1000;src/ui/components/contract_chooser_panel.rs (1)
542-604: Context menu never opens (state is never set)
chooser_state.show_context_menu/right_click_contract_id/context_menu_positionare read but never set after replacing collapsing headers. This regresses right-click functionality.Wire secondary clicks on the contract header to set these fields.
- if render_collapsing_header(ui, &display_name, is_expanded, is_selected_contract, 0) { + let header_clicked = render_collapsing_header(ui, &display_name, is_expanded, is_selected_contract, 0); + // Detect right-click anywhere on the last header row + if ui.ctx().input(|i| i.pointer.secondary_clicked()) { + if let Some(pos) = ui.ctx().pointer_interact_pos() { + chooser_state.right_click_contract_id = Some(contract_id.clone()); + chooser_state.context_menu_position = pos; + chooser_state.show_context_menu = true; + } + } + if header_clicked { if is_expanded { chooser_state.expanded_contracts.remove(&contract_id); } else { chooser_state.expanded_contracts.insert(contract_id.clone()); } }Also, close the menu using the actual window rect:
- egui::Window::new("Contract Menu") - ... - .show(ctx, |ui| { ... }); + if let Some(win) = egui::Window::new("Contract Menu") + .id(egui::Id::new("contract_context_menu")) + .title_bar(false) + .resizable(false) + .collapsible(false) + .fixed_pos(chooser_state.context_menu_position) + .show(ctx, |ui| { /* ... */ }) { + if ctx.input(|i| i.pointer.any_click()) { + if let Some(pos) = ctx.pointer_interact_pos() { + if !win.response.rect.contains(pos) { + chooser_state.show_context_menu = false; + } + } + } + }
| - name: Install Rust toolchain | ||
| uses: actions-rs/toolchain@v1 | ||
| with: | ||
| toolchain: stable | ||
| override: true |
There was a problem hiding this comment.
Replace deprecated actions-rs/toolchain action.
The actions-rs/toolchain@v1 action is no longer maintained and its runner is too old for GitHub Actions. Consider using dtolnay/rust-toolchain instead, which is actively maintained.
Apply this diff:
- - name: Install Rust toolchain
- uses: actions-rs/toolchain@v1
- with:
- toolchain: stable
- override: true
+ - name: Install Rust toolchain
+ uses: dtolnay/rust-toolchain@stable📝 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.
| - name: Install Rust toolchain | |
| uses: actions-rs/toolchain@v1 | |
| with: | |
| toolchain: stable | |
| override: true | |
| - name: Install Rust toolchain | |
| uses: dtolnay/rust-toolchain@stable |
🧰 Tools
🪛 actionlint (1.7.8)
35-35: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In .github/workflows/tests.yml around lines 34 to 38, the workflow uses the
deprecated actions-rs/toolchain@v1; replace that step with the maintained
dtolnay/rust-toolchain action. Change the uses to dtolnay/rust-toolchain@v1 and
pass the equivalent inputs to select the stable toolchain (e.g., toolchain:
stable) and preserve the override behavior using the new action's inputs
(consult dtolnay/rust-toolchain docs for exact input names), ensuring the
workflow uses the up-to-date runner support.
| - name: Run tests | ||
| uses: actions-rs/cargo@v1 | ||
| with: | ||
| command: test | ||
| args: --all-features --workspace | ||
|
|
||
| - name: Run doc tests | ||
| uses: actions-rs/cargo@v1 | ||
| with: | ||
| command: test | ||
| args: --doc --all-features --workspace |
There was a problem hiding this comment.
Replace deprecated actions-rs/cargo actions.
The actions-rs/cargo@v1 action is no longer maintained and its runner is too old for GitHub Actions. Replace with direct cargo commands.
Apply this diff:
- name: Run tests
- uses: actions-rs/cargo@v1
- with:
- command: test
- args: --all-features --workspace
+ run: cargo test --all-features --workspace
- name: Run doc tests
- uses: actions-rs/cargo@v1
- with:
- command: test
- args: --doc --all-features --workspace
+ run: cargo test --doc --all-features --workspace📝 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.
| - name: Run tests | |
| uses: actions-rs/cargo@v1 | |
| with: | |
| command: test | |
| args: --all-features --workspace | |
| - name: Run doc tests | |
| uses: actions-rs/cargo@v1 | |
| with: | |
| command: test | |
| args: --doc --all-features --workspace | |
| - name: Run tests | |
| run: cargo test --all-features --workspace | |
| - name: Run doc tests | |
| run: cargo test --doc --all-features --workspace |
🧰 Tools
🪛 actionlint (1.7.8)
55-55: the runner of "actions-rs/cargo@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
61-61: the runner of "actions-rs/cargo@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
.github/workflows/tests.yml around lines 54 to 64: the workflow uses the
deprecated actions-rs/cargo@v1 action for running tests; replace those steps
with direct run steps that invoke the cargo CLI (e.g., use a step with name "Run
tests" and run: cargo test --all-features --workspace, and a step with name "Run
doc tests" and run: cargo test --doc --all-features --workspace), ensuring the
steps run on the default runner environment and keeping any needed environment
or working-directory settings from the original workflow.
|
|
||
| ### 2. Lazily initialize the component | ||
|
|
||
| Inside your screen's `show()` method or simiar: |
There was a problem hiding this comment.
Fix typo: “simiar” → “similar”.
Minor doc polish for clarity.
Apply this diff:
-Inside your screen's `show()` method or simiar:
+Inside your screen's `show()` method or similar:📝 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.
| Inside your screen's `show()` method or simiar: | |
| Inside your screen's `show()` method or similar: |
🧰 Tools
🪛 LanguageTool
[grammar] ~27-~27: Ensure spelling is correct
Context: ...Inside your screen's show() method or simiar: rust let amount_widget = self.amount_widget.get_or_insert_with(|| { AmountInput::new(amount_type) .with_label("Amount:") }); ### 3. Show component and handle updates Af...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In doc/COMPONENT_DESIGN_PATTERN.md around line 27, there's a typo: change
"simiar" to "similar" in the sentence "Inside your screen's `show()` method or
simiar:" so it reads "Inside your screen's `show()` method or similar:" and
ensure spacing/punctuation remains consistent.
| if let (Network::Testnet, Some(testnet_app_context)) = | ||
| (chosen_network, testnet_app_context.as_ref()) | ||
| { | ||
| identities_screen = IdentitiesScreen::new(testnet_app_context); | ||
| dpns_active_contests_screen = | ||
| DPNSScreen::new(testnet_app_context, DPNSSubscreen::Active); | ||
| dpns_past_contests_screen = DPNSScreen::new(testnet_app_context, DPNSSubscreen::Past); | ||
| dpns_my_usernames_screen = DPNSScreen::new(testnet_app_context, DPNSSubscreen::Owned); | ||
| dpns_scheduled_votes_screen = | ||
| DPNSScreen::new(testnet_app_context, DPNSSubscreen::ScheduledVotes); | ||
| transition_visualizer_screen = TransitionVisualizerScreen::new(testnet_app_context); | ||
| proof_visualizer_screen = ProofVisualizerScreen::new(testnet_app_context); | ||
| document_visualizer_screen = DocumentVisualizerScreen::new(testnet_app_context); | ||
| contract_visualizer_screen = ContractVisualizerScreen::new(testnet_app_context); | ||
| document_query_screen = DocumentQueryScreen::new(testnet_app_context); | ||
| grovestark_screen = GroveSTARKScreen::new(testnet_app_context); | ||
| wallets_balances_screen = WalletsBalancesScreen::new(testnet_app_context); | ||
| proof_log_screen = ProofLogScreen::new(testnet_app_context); | ||
| platform_info_screen = PlatformInfoScreen::new(testnet_app_context); | ||
| masternode_list_diff_screen = MasternodeListDiffScreen::new(testnet_app_context); | ||
| contracts_dashpay_screen = DashpayScreen::new(testnet_app_context); | ||
| tokens_balances_screen = | ||
| TokensScreen::new(testnet_app_context, TokensSubscreen::MyTokens); | ||
| token_search_screen = | ||
| TokensScreen::new(testnet_app_context, TokensSubscreen::SearchTokens); | ||
| token_creator_screen = | ||
| TokensScreen::new(testnet_app_context, TokensSubscreen::TokenCreator); | ||
| } else if let (Network::Devnet, Some(devnet_app_context)) = | ||
| (chosen_network, devnet_app_context.as_ref()) | ||
| { | ||
| identities_screen = IdentitiesScreen::new(devnet_app_context); | ||
| dpns_active_contests_screen = | ||
| DPNSScreen::new(devnet_app_context, DPNSSubscreen::Active); | ||
| dpns_past_contests_screen = DPNSScreen::new(devnet_app_context, DPNSSubscreen::Past); | ||
| dpns_my_usernames_screen = DPNSScreen::new(devnet_app_context, DPNSSubscreen::Owned); | ||
| dpns_scheduled_votes_screen = | ||
| DPNSScreen::new(devnet_app_context, DPNSSubscreen::ScheduledVotes); | ||
| transition_visualizer_screen = TransitionVisualizerScreen::new(devnet_app_context); | ||
| proof_visualizer_screen = ProofVisualizerScreen::new(devnet_app_context); | ||
| document_visualizer_screen = DocumentVisualizerScreen::new(devnet_app_context); | ||
| document_query_screen = DocumentQueryScreen::new(devnet_app_context); | ||
| masternode_list_diff_screen = MasternodeListDiffScreen::new(devnet_app_context); | ||
| contract_visualizer_screen = ContractVisualizerScreen::new(devnet_app_context); | ||
| grovestark_screen = GroveSTARKScreen::new(devnet_app_context); | ||
| wallets_balances_screen = WalletsBalancesScreen::new(devnet_app_context); | ||
| proof_log_screen = ProofLogScreen::new(devnet_app_context); | ||
| platform_info_screen = PlatformInfoScreen::new(devnet_app_context); | ||
| tokens_balances_screen = | ||
| TokensScreen::new(devnet_app_context, TokensSubscreen::MyTokens); | ||
| token_search_screen = | ||
| TokensScreen::new(devnet_app_context, TokensSubscreen::SearchTokens); | ||
| token_creator_screen = | ||
| TokensScreen::new(devnet_app_context, TokensSubscreen::TokenCreator); | ||
| } else if let (Network::Regtest, Some(local_app_context)) = | ||
| (chosen_network, local_app_context.as_ref()) | ||
| { | ||
| identities_screen = IdentitiesScreen::new(local_app_context); | ||
| dpns_active_contests_screen = DPNSScreen::new(local_app_context, DPNSSubscreen::Active); | ||
| dpns_past_contests_screen = DPNSScreen::new(local_app_context, DPNSSubscreen::Past); | ||
| dpns_my_usernames_screen = DPNSScreen::new(local_app_context, DPNSSubscreen::Owned); | ||
| dpns_scheduled_votes_screen = | ||
| DPNSScreen::new(local_app_context, DPNSSubscreen::ScheduledVotes); | ||
| transition_visualizer_screen = TransitionVisualizerScreen::new(local_app_context); | ||
| proof_visualizer_screen = ProofVisualizerScreen::new(local_app_context); | ||
| document_visualizer_screen = DocumentVisualizerScreen::new(local_app_context); | ||
| contract_visualizer_screen = ContractVisualizerScreen::new(local_app_context); | ||
| document_query_screen = DocumentQueryScreen::new(local_app_context); | ||
| grovestark_screen = GroveSTARKScreen::new(local_app_context); | ||
| wallets_balances_screen = WalletsBalancesScreen::new(local_app_context); | ||
| masternode_list_diff_screen = MasternodeListDiffScreen::new(local_app_context); | ||
| proof_log_screen = ProofLogScreen::new(local_app_context); | ||
| platform_info_screen = PlatformInfoScreen::new(local_app_context); | ||
| contracts_dashpay_screen = DashpayScreen::new(local_app_context); | ||
| tokens_balances_screen = | ||
| TokensScreen::new(local_app_context, TokensSubscreen::MyTokens); | ||
| token_search_screen = | ||
| TokensScreen::new(local_app_context, TokensSubscreen::SearchTokens); | ||
| token_creator_screen = | ||
| TokensScreen::new(local_app_context, TokensSubscreen::TokenCreator); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Fallback if chosen_network context is unavailable.
If settings.network is Testnet/Devnet/Regtest but corresponding AppContext::new returned None, current_app_context() will later panic on expect(...). Add a fallback to Dash (and persist the change) to avoid boot-time crashes.
- } else if let (Network::Regtest, Some(local_app_context)) = (chosen_network, local_app_context.as_ref()) {
+ } else if let (Network::Regtest, Some(local_app_context)) = (chosen_network, local_app_context.as_ref()) {
// ... existing branches ...
+ } else {
+ // Fallback to mainnet if selected network is unavailable
+ network_chooser_screen.current_network = Network::Dash;
+ // Optionally: persist change
+ let _ = mainnet_app_context.update_settings(settings.root_screen_type);
+ // Screens already constructed for mainnet above
+ }🏁 Script executed:
#!/bin/bash
# Find the current_app_context() function implementation and its usages
rg -nP "fn\s+current_app_context\s*\(" -A 10Length of output: 1542
🏁 Script executed:
#!/bin/bash
# Check broader context around the reviewed code section in app.rs
# Get lines 200-350 to see initialization and what happens after the match
sed -n '200,350p' src/app.rsLength of output: 9202
🏁 Script executed:
#!/bin/bash
# Search for all invocations of current_app_context
rg -nC 3 "current_app_context\(\)"Length of output: 7044
🏁 Script executed:
#!/bin/bash
# Look for mainnet initialization to see if there's a fallback default
rg -nP "mainnet_app_context.*=" -B 2 -A 2 | head -50Length of output: 615
🏁 Script executed:
#!/bin/bash
# Check what happens if all contexts are None - look for panic or expect calls
rg -nP "expect\(|panic\(" src/app.rs | head -20Length of output: 921
🏁 Script executed:
#!/bin/bash
# Find AppContext::new() implementation to see what causes None
rg -nP "impl\s+AppContext" -A 5 | head -40Length of output: 2633
🏁 Script executed:
#!/bin/bash
# Search for AppContext::new() definition
rg -nP "fn new\(" -B 5 -A 15 src/ --type=rs | grep -A 15 "AppContext::new"Length of output: 26
🏁 Script executed:
#!/bin/bash
# Check where AppContext::new returns Option or Result
fd -e rs -type f | xargs grep -l "AppContext::new" | head -5Length of output: 170
🏁 Script executed:
#!/bin/bash
# Look at the full initialization in app.rs, especially lines around 171-200
sed -n '160,200p' src/app.rsLength of output: 1332
🏁 Script executed:
#!/bin/bash
# Check what happens after the if/else branches - lines 330-380
sed -n '330,380p' src/app.rsLength of output: 2211
🏁 Script executed:
#!/bin/bash
# Find where self.chosen_network is assigned
rg -nP "chosen_network\s*=" src/app.rs | head -20Length of output: 93
🏁 Script executed:
#!/bin/bash
# Search for App struct definition to see chosen_network field
rg -nP "struct\s+App\s*\{" -A 30 src/app.rsLength of output: 0
🏁 Script executed:
#!/bin/bash
# Look for lines after the if/else chain to see if chosen_network is explicitly set
sed -n '240,250p' src/app.rsLength of output: 412
🏁 Script executed:
#!/bin/bash
# Check if NetworkChooserScreen.new initializes chosen_network and how
rg -nP "chosen_network" src/app.rs -B 2 -A 2Length of output: 1982
Add else clause to reset network fallback, but ensure local chosen_network variable is also updated.
The issue is confirmed: if settings.network persists Testnet/Devnet/Regtest but that context is None, current_app_context() will panic on expect(). However, the suggested fix is incomplete—it updates network_chooser_screen.current_network and settings, but the local chosen_network variable (line 248) is never reset. This variable is assigned to self.chosen_network at line 489, so without resetting it, self.chosen_network retains the unavailable network value and the panic still occurs.
The else clause must also include: chosen_network = Network::Dash;
🤖 Prompt for AI Agents
In src/app.rs around lines 251 to 330, the added else branch resets
network_chooser_screen.current_network and settings but fails to update the
local chosen_network variable, so self.chosen_network (set later) can still hold
an unavailable network and cause a panic; modify that else branch to also set
chosen_network = Network::Dash (the local variable declared earlier) so the
function and eventual assignment to self.chosen_network reflect the fallback
network.
| pub async fn run_grovestark_task( | ||
| task: GroveSTARKTask, | ||
| sdk: &Sdk, | ||
| ) -> Result<BackendTaskSuccessResult, String> { | ||
| match task { |
There was a problem hiding this comment.
Harden error handling and secrets: stop deriving Debug/PartialEq on private keys; use typed errors.
- Avoid logging/comparing secrets: remove Debug/PartialEq; provide redacted Debug impl.
- Switch from String errors to a thiserror enum per guidelines.
+use thiserror::Error;
+use std::fmt;
@@
-pub async fn run_grovestark_task(
+pub async fn run_grovestark_task(
task: GroveSTARKTask,
sdk: &Sdk,
-) -> Result<BackendTaskSuccessResult, String> {
+) -> Result<BackendTaskSuccessResult, GroveStarkTaskError> {
@@
- Ok(proof_data) => Ok(BackendTaskSuccessResult::GeneratedZKProof(proof_data)),
- Err(e) => Err(format!("Failed to generate proof: {}", e)),
+ Ok(proof_data) => Ok(BackendTaskSuccessResult::GeneratedZKProof(proof_data)),
+ Err(e) => Err(GroveStarkTaskError::Generate(e.to_string())),
}
@@
- Ok(is_valid) => Ok(BackendTaskSuccessResult::VerifiedZKProof(
+ Ok(is_valid) => Ok(BackendTaskSuccessResult::VerifiedZKProof(
is_valid, proof_data,
)),
- Err(e) => Err(format!("Failed to verify proof: {}", e)),
+ Err(e) => Err(GroveStarkTaskError::Verify(e.to_string())),
}
}
}
}
-#[derive(Debug, Clone, PartialEq)]
+#[derive(Clone)]
pub enum GroveSTARKTask {
@@
VerifyProof {
proof_data: ProofDataOutput,
},
}
+
+#[derive(Debug, Error)]
+pub enum GroveStarkTaskError {
+ #[error("Proof generation failed: {0}")]
+ Generate(String),
+ #[error("Proof verification failed: {0}")]
+ Verify(String),
+}
+
+// Redacted Debug to avoid leaking keys
+impl fmt::Debug for GroveSTARKTask {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ match self {
+ GroveSTARKTask::GenerateProof {
+ identity_id,
+ contract_id,
+ document_type,
+ document_id,
+ key_id,
+ ..
+ } => f
+ .debug_struct("GroveSTARKTask::GenerateProof")
+ .field("identity_id", identity_id)
+ .field("contract_id", contract_id)
+ .field("document_type", document_type)
+ .field("document_id", document_id)
+ .field("key_id", key_id)
+ .field("private_key", &"<redacted>")
+ .field("public_key", &"<redacted>")
+ .finish(),
+ GroveSTARKTask::VerifyProof { .. } => {
+ f.debug_struct("GroveSTARKTask::VerifyProof").finish()
+ }
+ }
+ }
+}Also applies to: 34-36, 41-46, 51-65
🤖 Prompt for AI Agents
In src/backend_task/grovestark.rs around lines 5-9 (and also apply to lines
34-36, 41-46, 51-65), the code exposes secrets by deriving Debug/PartialEq on
private key types and returns plain String errors; remove Debug and PartialEq
derives from any private key structs, implement a custom Debug that redacts
secret material (e.g., prints "<redacted>") and keep Eq/PartialEq only where
safe, and replace functions returning Result<..., String> with a typed error
enum using thiserror::Error to categorize failures (parsing, crypto, sdk, etc.);
update all error returns and matches to use the new enum and map underlying
errors into it with contextual messages, ensuring no secret values are included
in error Debug/Display output.
| AppAction::BackendTask(BackendTask::TokenTask(Box::new(TokenTask::PauseTokens { | ||
| actor_identity: self.identity.clone(), | ||
| data_contract, | ||
| token_position: self.identity_token_info.token_position, | ||
| signing_key: self.selected_key.clone().expect("No key selected"), | ||
| public_note: if self.group_action_id.is_some() { | ||
| None | ||
| } else { | ||
| self.public_note.clone() | ||
| }, | ||
| group_info, | ||
| }))) |
There was a problem hiding this comment.
Gracefully handle missing signing key instead of expect.
Avoid crashing when no key is selected.
Apply:
- signing_key: self.selected_key.clone().expect("No key selected"),
+ signing_key: match self.selected_key.clone() {
+ Some(k) => k,
+ None => {
+ self.error_message = Some("Please select a signing key.".into());
+ self.status = PauseTokensStatus::ErrorMessage("No key selected".into());
+ return AppAction::None;
+ }
+ },Also consider disabling the “Pause” button until a key is selected.
📝 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.
| AppAction::BackendTask(BackendTask::TokenTask(Box::new(TokenTask::PauseTokens { | |
| actor_identity: self.identity.clone(), | |
| data_contract, | |
| token_position: self.identity_token_info.token_position, | |
| signing_key: self.selected_key.clone().expect("No key selected"), | |
| public_note: if self.group_action_id.is_some() { | |
| None | |
| } else { | |
| self.public_note.clone() | |
| }, | |
| group_info, | |
| }))) | |
| AppAction::BackendTask(BackendTask::TokenTask(Box::new(TokenTask::PauseTokens { | |
| actor_identity: self.identity.clone(), | |
| data_contract, | |
| token_position: self.identity_token_info.token_position, | |
| signing_key: match self.selected_key.clone() { | |
| Some(k) => k, | |
| None => { | |
| self.error_message = Some("Please select a signing key.".into()); | |
| self.status = PauseTokensStatus::ErrorMessage("No key selected".into()); | |
| return AppAction::None; | |
| } | |
| }, | |
| public_note: if self.group_action_id.is_some() { | |
| None | |
| } else { | |
| self.public_note.clone() | |
| }, | |
| group_info, | |
| }))) |
🤖 Prompt for AI Agents
In src/ui/tokens/pause_tokens_screen.rs around lines 230 to 241, the code uses
expect("No key selected") when building the PauseTokens task which will panic if
no signing key is chosen; change this to handle the missing key gracefully by
returning early (e.g., show an error/notification or enqueue a UI-level failure
action) instead of panicking, and ensure the Pause action is only constructed
when selected_key.is_some(); additionally, disable or make the “Pause” button
inert in the UI when selected_key.is_none() so the user cannot trigger the
action without a signing key.
| Some(TokenPricingSchedule::SinglePrice(price_per_smallest_unit)) => { | ||
| // Convert price per smallest unit back to price per token for display | ||
| let price_per_token = price_per_smallest_unit * decimal_multiplier; | ||
| let amount = | ||
| Amount::new(price_per_token, DASH_DECIMAL_PLACES).with_unit_name("DASH"); | ||
| (Some(amount), vec![(None, None)]) | ||
| } |
There was a problem hiding this comment.
Fix potential u64 overflow when converting per-unit price to per-token
Multiplying price_per_smallest_unit by decimal_multiplier in u64 can overflow. Use checked/saturating math as used elsewhere (e.g., purchase screen).
- let price_per_token = price_per_smallest_unit * decimal_multiplier;
+ let price_per_token = (price_per_smallest_unit as u128)
+ .saturating_mul(decimal_multiplier as u128)
+ .min(u64::MAX as u128) as u64;Apply similarly for tiered prices (lines 311-319).
Also applies to: 311-319
| perpetual_dist_interval_input * step_count_input; | ||
| if amount == 1 { | ||
| "Every Block".to_string() | ||
| } else { | ||
| format!("Every {} Blocks", amount) | ||
| } | ||
| } | ||
| PerpetualDistributionIntervalTypeUI::TimeBased => { | ||
| let amount = perpetual_dist_interval_input * step_count_input; | ||
| format!("Every {} {}", amount, self.perpetual_dist_interval_unit.capitalized_label_for_num_amount(amount)) | ||
| let amount = | ||
| perpetual_dist_interval_input * step_count_input; | ||
| format!( | ||
| "Every {} {}", | ||
| amount, | ||
| self.perpetual_dist_interval_unit | ||
| .capitalized_label_for_num_amount(amount) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Use saturating_mul for interval math to avoid overflow panics.
Multiplying two user-entered u64s can overflow (panic in debug, wrap in release). Use saturating_mul or checked_mul with a safe fallback.
Apply this diff:
- let amount =
- perpetual_dist_interval_input * step_count_input;
+ let amount = perpetual_dist_interval_input
+ .saturating_mul(step_count_input);
…
- let amount =
- perpetual_dist_interval_input * step_count_input;
+ let amount = perpetual_dist_interval_input
+ .saturating_mul(step_count_input);
…
- let amount =
- perpetual_dist_interval_input * step_count_input;
+ let amount = perpetual_dist_interval_input
+ .saturating_mul(step_count_input);Also applies to: 398-405, 409-415
🤖 Prompt for AI Agents
In src/ui/tokens/tokens_screen/distributions.rs around lines 390-406 (and also
at 398-405 and 409-415), the code multiplies two user-provided u64 values and
can panic on overflow; replace plain multiplication with saturating_mul (or use
checked_mul with a safe fallback) when computing amount (e.g., amount =
perpetual_dist_interval_input.saturating_mul(step_count_input)) so the UI uses a
saturating result instead of panicking or wrapping; apply the same change to the
other specified line ranges.
| let known_identities = app_context | ||
| .load_local_qualified_identities() | ||
| .expect("Identities not loaded"); | ||
|
|
||
| let friend_identities: Vec<(String, Identifier)> = all_identities | ||
| .iter() | ||
| .filter(|id| id.identity.id() != identity_token_balance.identity_id) | ||
| .map(|id| { | ||
| let alias = id | ||
| .alias | ||
| .clone() | ||
| .unwrap_or_else(|| id.identity.id().to_string(Encoding::Base58)); | ||
| (alias, id.identity.id()) | ||
| }) | ||
| .collect(); | ||
|
|
||
| let identity = all_identities | ||
| let identity = known_identities | ||
| .iter() | ||
| .find(|identity| identity.identity.id() == identity_token_balance.identity_id) | ||
| .expect("Identity not found") | ||
| .clone(); | ||
| let max_amount = identity_token_balance.balance; | ||
| let max_amount = Amount::from(&identity_token_balance); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid panics during screen init; surface a UI error instead
.expect("Identities not loaded") and .expect("Identity not found") will crash the app. Propagate into transfer_tokens_status and show an error.
I can provide a small patch to convert these into user-visible errors and a disabled screen state.
| let frozen_identities = app_context | ||
| .load_local_qualified_identities() | ||
| .expect("Identities not loaded"); | ||
|
|
There was a problem hiding this comment.
Avoid panic on identities load; surface error to the UI.
Replace expect("Identities not loaded") with handled error (set self.error_message/status) so the screen doesn’t crash in release.
- let frozen_identities = app_context
- .load_local_qualified_identities()
- .expect("Identities not loaded");
+ let frozen_identities = match app_context.load_local_qualified_identities() {
+ Ok(ids) => ids,
+ Err(e) => {
+ tracing::error!("Failed to load identities: {e}");
+ Vec::new()
+ }
+ };📝 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 frozen_identities = app_context | |
| .load_local_qualified_identities() | |
| .expect("Identities not loaded"); | |
| let frozen_identities = match app_context.load_local_qualified_identities() { | |
| Ok(ids) => ids, | |
| Err(e) => { | |
| tracing::error!("Failed to load identities: {e}"); | |
| Vec::new() | |
| } | |
| }; |
🤖 Prompt for AI Agents
In src/ui/tokens/unfreeze_tokens_screen.rs around lines 85 to 88, the call to
app_context.load_local_qualified_identities() uses expect(...) which will panic
on error; replace this with proper Result handling: match or if let Err(e) => {
self.error_message = Some(e.to_string()); self.status = Some(<appropriate error
status>); return early from the function so the UI shows the error instead of
crashing }, and on Ok(identities) continue normal flow. Ensure you convert the
error to a user-facing string and set the existing UI fields
(error_message/status) used by this screen.
There was a problem hiding this comment.
Actionable comments posted: 28
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (20)
src/ui/tokens/tokens_screen/token_creator.rs (1)
1113-1183: Remove duplicate assignment.Line 1174 duplicates the assignment on line 1171. Both set
show_token_creator_confirmation_popup = false.Apply this diff:
action = AppAction::BackendTasks(tasks, BackendTasksExecutionMode::Sequential); self.show_token_creator_confirmation_popup = false; let now = Utc::now().timestamp() as u64; self.token_creator_status = TokenCreatorStatus::WaitingForResult(now); - self.show_token_creator_confirmation_popup = false; self.token_creator_confirmation_dialog = None; }src/components/core_zmq_listener.rs (1)
140-180: Non-Windows path updated correctly (except error message).The non-Windows implementation properly decodes both Block and ChainLock and sends them together. However, the error message at Line 167 mentions "InstantLock" when it should say "ChainLock".
snap/snapcraft.yaml (2)
161-166: Build may fail if ImageMagick isn’t present.convert is used to create a placeholder icon, but imagemagick isn’t listed in build-packages. Either add it or avoid the dynamic generation.
Apply:
build-packages: - build-essential - pkg-config ... - xkb-data + - imagemagickOr remove the placeholder branch and make the icon a required file.
216-226: Add integrity verification for Dash tarball download.Dash publishes official SHA256SUMS.asc and GPG signatures on GitHub releases, which must be verified before extraction. The current code downloads and extracts without any verification, creating a supply chain vulnerability.
Add SHA256 verification at lines 216-226 in snap/snapcraft.yaml:
# Download from official Dash releases curl -L "https://github.com/dashpay/dash/releases/download/v${DASH_VERSION}/dashcore-${DASH_VERSION}-${DASH_ARCH}-linux-gnu.tar.gz" -o dash.tar.gz + curl -L "https://github.com/dashpay/dash/releases/download/v${DASH_VERSION}/SHA256SUMS.asc" -o SHA256SUMS.asc + sha256sum -c <(grep "dashcore-${DASH_VERSION}-${DASH_ARCH}-linux-gnu.tar.gz" SHA256SUMS.asc) || exit 1 tar -xzf dash.tar.gzFor stronger security, verify the GPG signature on SHA256SUMS.asc using Dash Core developer keys published in the release notes.
src/config.rs (1)
53-62: Returning reference to a temporary (&None) — change return type.
_ => &Nonereturns a reference to a temporary and will not compile safely. ReturnOption<&NetworkConfig>instead.Apply this diff (callers must switch to matching on
Option<&NetworkConfig>):- pub fn config_for_network(&self, network: Network) -> &Option<NetworkConfig> { - match network { - Network::Dash => &self.mainnet_config, - Network::Testnet => &self.testnet_config, - Network::Devnet => &self.devnet_config, - Network::Regtest => &self.local_config, - _ => &None, - } - } + pub fn config_for_network(&self, network: Network) -> Option<&NetworkConfig> { + match network { + Network::Dash => self.mainnet_config.as_ref(), + Network::Testnet => self.testnet_config.as_ref(), + Network::Devnet => self.devnet_config.as_ref(), + Network::Regtest => self.local_config.as_ref(), + _ => None, + } + }As per coding guidelines, config responsibilities belong here; this API tweak prevents invalid borrows. Based on learnings.
src/ui/contracts_documents/update_contract_screen.rs (1)
196-203: Avoidunwrap()on identity/key — guard explicitly to prevent UI panic.Even with upstream checks, selection can be None; unwraps may crash.
Apply this diff to bail out safely:
- if ui.add(button).clicked() { - // Fire off a backend task - app_action = AppAction::BackendTask(BackendTask::ContractTask(Box::new( - ContractTask::UpdateDataContract( - (**contract).clone(), - self.selected_qualified_identity.clone().unwrap(), // unwrap should be safe here - self.selected_key.clone().unwrap(), // unwrap should be safe here - ), - ))); - } + if ui.add(button).clicked() { + if let (Some(qi), Some(key)) = + (self.selected_qualified_identity.clone(), self.selected_key.clone()) + { + app_action = AppAction::BackendTask(BackendTask::ContractTask(Box::new( + ContractTask::UpdateDataContract((**contract).clone(), qi, key), + ))); + } else { + ui.colored_label(Color32::DARK_RED, "Select identity and critical key first."); + } + }src/ui/identities/add_new_identity_screen/by_using_unused_asset_lock.rs (1)
56-63: Prevent panic when proof is None; disable or handle selection safely.Selecting an asset lock with no proof will panic at runtime due to
expect("Asset lock proof is required"). Guard the button and handle the None case.- // Button to select this asset lock - if ui.button("Select").clicked() { - // Update the selected asset lock - self.funding_asset_lock = Some(( - tx.clone(), - proof.clone().expect("Asset lock proof is required"), - address.clone(), - )); - - // Update the step to ready to create identity - let mut step = self.step.write().unwrap(); - *step = WalletFundedScreenStep::ReadyToCreate; - } + // Only allow selecting entries that have a proof + let can_select = proof.is_some(); + let clicked = ui.add_enabled(can_select, egui::Button::new("Select")).clicked(); + if clicked { + if let Some(proof) = proof.clone() { + self.funding_asset_lock = Some((tx.clone(), proof, address.clone())); + let mut step = self.step.write().unwrap(); + *step = WalletFundedScreenStep::ReadyToCreate; + } + } else if !can_select { + ui.label(egui::RichText::new("Missing asset lock proof").color(egui::Color32::RED)); + }src/database/identities.rs (1)
168-181: Set in-memory network: LGTM; avoid panicking decode on corrupt rows.Assigning
identity.network = app_context.networkensures consistency. However,QualifiedIdentity::from_bytes(&data)currently panics on decode failures, which can crash the app on corrupted DB rows. Prefer a fallible decode and bubble up an error or skip the row.Example approach:
- Introduce
QualifiedIdentity::try_from_bytes(&[u8]) -> Result<Self, DecodeError>.- Replace calls here with
match ... { Ok(qi) => qi, Err(e) => return Err(rusqlite::Error::UserFunctionError(Box::new(e))) }.Based on relevant snippet in src/model/qualified_identity/mod.rs.
src/ui/tokens/tokens_screen/data_contract_json_pop_up.rs (1)
61-63: Fix width constraint: use available_width, not available_height.Likely a copy/paste typo; constraining width by height warps the layout.
- .max_width(ui.available_height() - 20.0) + .max_width(ui.available_width() - 20.0)src/ui/tokens/tokens_screen/groups.rs (2)
231-239: Removing a member skips the next entry (while-loop index bug).After removing
members[j], the code still doesj += 1, so the next member shifts intojand is skipped. Fix by tracking removal and not incrementingjwhen removed.- if ui.button("Remove Member").clicked() { - group_ui.members.remove(j);// return so we skip the assignment at the end - } else { - // Only assign back if we didn’t remove - group_ui.members[j] = member; - } + let mut removed = false; + if ui.button("Remove Member").clicked() { + group_ui.members.remove(j); + removed = true; + } else { + // Only assign back if we didn’t remove + group_ui.members[j] = member; + } }); - j += 1; + if removed { + continue; // re-check current index since items shifted left + } + j += 1;
43-64: Duplicate member handling contradicts the UI (“later entry ignored”).
BTreeMap::insertoverwrites the earlier member with the later duplicate. The UI says later duplicates are ignored. Preserve the first occurrence instead.- // Insert into the map - members_map.insert(id, power); + // Insert into the map; ignore later duplicates to match the UI message + use std::collections::btree_map::Entry; + match members_map.entry(id) { + Entry::Vacant(v) => { + v.insert(power); + } + Entry::Occupied(_) => { + // later duplicate: ignore + } + }src/ui/identities/add_new_identity_screen/by_wallet_qr_code.rs (1)
101-116: Button label vs copied content mismatch (URI vs address).You copy the full payment URI (with amount), but the UI says “Copy Address”. Align the label/feedback.
- if ui.button("Copy Address").clicked() { + if ui.button("Copy Payment URI").clicked() { if let Err(e) = copy_to_clipboard(pay_uri.as_str()) { self.copied_to_clipboard = Some(Some(e)); } else { self.copied_to_clipboard = Some(None); } } @@ - if let Some(error) = error { + if let Some(error) = error { ui.label(format!("Failed to copy to clipboard: {}", error)); } else { - ui.label("Address copied to clipboard."); + ui.label("Payment URI copied to clipboard."); }src/ui/tokens/tokens_screen/distributions.rs (2)
786-790: Type mismatch: label says u64 but sanitization allows signed.“Base amount (b, u64)” uses sanitize_i64, allowing negatives. Switch to sanitize_u64.
- ui.text_edit_singleline(&mut self.exp_b_input); - sanitize_i64(&mut self.exp_b_input); + ui.text_edit_singleline(&mut self.exp_b_input); + sanitize_u64(&mut self.exp_b_input);
215-256: Doc string mismatch for StepDecreasingAmount.The popup text under StepDecreasingAmount describes a Random function. Update to the correct step-decreasing description to avoid confusion.
src/ui/identities/top_up_identity_screen/by_wallet_qr_code.rs (1)
73-79: Minor UX: button copies URI, label says “Copy Address”.Either change the label to “Copy Payment URI” or copy only the address.
src/ui/identities/identities_screen.rs (1)
681-793: Update Keys popup to egui 0.32's new Popup API with menu semantics.The suggested changes are valid for egui 0.32. Apply the modernization to use
Popup::menu()and the full namespace pathegui::containers::popup::PopupCloseBehaviorfor stability. Replaceui.close_kind(egui::UiKind::Menu)withui.close()since menus close by default in 0.32.Apply:
- let popup_id = ui.make_persistent_id(format!("keys_popup_{}", qualified_identity.identity.id().to_string(Encoding::Base58))); - egui::Popup::from_toggle_button_response(&button_response).id(popup_id) - .close_behavior(egui::PopupCloseBehavior::CloseOnClickOutside) + let popup_id = ui.make_persistent_id(format!( + "keys_popup_{}", + qualified_identity.identity.id().to_string(Encoding::Base58) + )); + egui::containers::popup::Popup::menu(&button_response) + .id(popup_id) + .close_behavior(egui::containers::popup::PopupCloseBehavior::CloseOnClickOutside) .show(|ui| { ... })Replace
ui.close_kind(egui::UiKind::Menu)withui.close()for consistency with the new menu API.Note: The Actions popup at lines 624-625 uses the same deprecated pattern and should also be updated in a separate change.
src/ui/tokens/direct_token_purchase_screen.rs (1)
362-365: Fix success message match typo (“purchaseed”).The extra “e” can cause success detection to fail silently.
- if message.contains("Successfully purchaseed tokens") || message == "PurchaseTokens" + if message.contains("Successfully purchased tokens") || message == "PurchaseTokens"src/backend_task/mod.rs (1)
170-213: Adopt structured errors withthiserrorper coding guidelines.The review comment correctly identifies that backend_task violates the codebase guideline for Rust files, which mandates structured error handling using
thiserror. This pattern is already established in the codebase (ConfigError, WalletError).Current violations in backend_task/mod.rs:
run_backend_task()returnsResult<_, String>(line 174)run_backend_tasks_sequential/concurrent()returnVec<Result<_, String>>(lines 140, 156)- All submodule task runners use String errors
Implementation requires:
- Define BackendTaskError enum in src/backend_task/mod.rs
- Update run_backend_task and all task runner signatures across submodules
- Update app.rs From impl (lines 52-56) to handle BackendTaskError
- Ensure error propagation from submodules
src/ui/tokens/tokens_screen/mod.rs (2)
1873-1903: Wrong gating for StepDecreasing min_value and max_interval_count.Both options are gated by
step_decreasing_start_period_offset_input.is_empty()instead of their own inputs, causing valid values to be ignored.Apply this fix:
- min_value: if self.step_decreasing_start_period_offset_input.is_empty() { + min_value: if self.step_decreasing_min_value_input.is_empty() { None } else { match self.step_decreasing_min_value_input.parse::<u64>() { Ok(0) => None, Ok(v) => Some(v), Err(_) => { return Err( "Invalid min value for StepDecreasingAmount distribution. Put 0 for None." .to_string(), ); } } }, - max_interval_count: if self.step_decreasing_start_period_offset_input.is_empty() + max_interval_count: if self.step_decreasing_max_interval_count_input.is_empty() { None } else { match self.step_decreasing_max_interval_count_input.parse::<u16>() { Ok(0) => None, Ok(v) => Some(v), Err(_) => { return Err( "Invalid max interval count for StepDecreasingAmount distribution. Put 0 for None." .to_string(), ); } } },
1627-1629: Token reordering is a no‑op (assignment commented out).
reorder_vec_tobuildsreorderedbut never assigns it, so UI reordering and persisted order never take effect.// Replace the original with the reordered map - //self.my_tokens = reordered; + self.my_tokens = reordered;
♻️ Duplicate comments (41)
src/backend_task/tokens/transfer_tokens.rs (1)
102-136: Duplication persists: extract common balance-update logic into a helper function.The identical pattern-matching and balance-update logic in both the
HistoricalDocument(lines 102-136) andGroupActionWithDocument(lines 151-185) variants still exists. The only difference is the error message context. This violates DRY principles and increases maintenance burden.The refactoring suggestion from the previous review remains valid and should be implemented to reduce duplication.
Also applies to: 151-185
src/backend_task/identity/register_dpns_name.rs (2)
60-78: Set creator_id to match owner_id for proper document creator tracking.This critical issue was previously flagged but remains unresolved. In DPP DocumentV0,
creator_idrepresents the Identity that submitted/created the document and should matchowner_id. Settingcreator_id: Noneon line 63 whileowner_idis set toqualified_identity.identity.id()is incorrect.Apply this diff to fix the preorder document initialization:
let preorder_document = Document::V0(DocumentV0 { id: preorder_id, owner_id: qualified_identity.identity.id(), - creator_id: None, + creator_id: Some(qualified_identity.identity.id()), properties: BTreeMap::from([( "saltedDomainHash".to_string(), salted_domain_hash.into(),
79-121: Set creator_id to match owner_id for proper document creator tracking.Same critical issue in the domain document initialization. Line 82 sets
creator_id: Nonewhileowner_idis set toqualified_identity.identity.id(). This must be corrected to ensure proper document creator identity recording per DPP DocumentV0 semantics.Apply this diff to fix the domain document initialization:
let domain_document = Document::V0(DocumentV0 { id: domain_id, owner_id: qualified_identity.identity.id(), - creator_id: None, + creator_id: Some(qualified_identity.identity.id()), properties: BTreeMap::from([ ("parentDomainName".to_string(), "dash".into()),src/model/wallet/encryption.rs (1)
2-2: aes-gcm Nonce handling is incorrect; restoreNonce::from_slice(0.10.x API).
encrypt/decryptexpect&Nonce, not a converted slice. This will not compile with aes-gcm 0.10.x; import and useNonce::from_slice.Apply this diff:
-use aes_gcm::{Aes256Gcm, KeyInit}; +use aes_gcm::{Aes256Gcm, KeyInit, Nonce}; @@ - let encrypted_seed = cipher - .encrypt(nonce.as_slice().into(), message) + let encrypted_seed = cipher + .encrypt(Nonce::from_slice(&nonce), message) .map_err(|e| e.to_string())?; @@ - let seed = cipher - .decrypt(self.nonce.as_slice().into(), self.encrypted_seed.as_slice()) + let seed = cipher + .decrypt(Nonce::from_slice(&self.nonce), self.encrypted_seed.as_slice()) .map_err(|e| e.to_string())?;Also consider migrating to a typed error (thiserror) and zeroizing sensitive material after use.
Run to confirm crate version:
#!/bin/bash rg -nP '^\s*aes-gcm\s*=\s*' Cargo.toml rg -nP '^\s*aead\s*=\s*' Cargo.tomlAlso applies to: 53-54, 88-89
src/components/core_zmq_listener.rs (2)
342-369: Critical: enum payload updated but Windows path still sends old arity.ZMQMessage::ChainLockedBlock now expects (Block, ChainLock). Windows code sends only Block, which won't compile and is semantically incomplete. Decode ChainLock and send both; also align the topic to "rawchainlocksig".
165-170: Fix misleading log label: ChainLock vs InstantLock.This path decodes ChainLock; the error message mentions InstantLock. Update the label to avoid confusion during debugging.
src/ui/identities/funding_common.rs (1)
56-94: Don't panic on poisoned locks; handle gracefully.Using expect() on RwLock poisoning can crash the app in a UI flow. The existing review comment already covers this issue. Return None or log and continue instead of panicking.
.github/workflows/tests.yml (2)
34-38: Replace deprecated actions-rs/toolchain with dtolnay/rust-toolchain.The actions-rs toolchain action is unmaintained and too old for current runners. Switch to
dtolnay/rust-toolchain@stable.- - name: Install Rust toolchain - uses: actions-rs/toolchain@v1 - with: - toolchain: stable - override: true + - name: Install Rust toolchain + uses: dtolnay/rust-toolchain@stable + with: + toolchain: stable(Based on actionlint hints.)
54-59: Replace deprecated actions-rs/cargo steps with direct cargo invocations.Use
run: cargo ...instead ofactions-rs/cargo@v1to avoid runner incompatibilities.- name: Run tests - uses: actions-rs/cargo@v1 - with: - command: test - args: --all-features --workspace + run: cargo test --all-features --workspace - name: Run doc tests - uses: actions-rs/cargo@v1 - with: - command: test - args: --doc --all-features --workspace + run: cargo test --doc --all-features --workspaceOptional: add
--lockedif you want to enforce Cargo.lock.Also applies to: 60-64
src/ui/components/left_panel.rs (2)
235-250: Make network display match exhaustive to catch future variants.The wildcard arm in the network match won't force updates when new Network variants (e.g., Signet) are added. Use an exhaustive match to ensure compile-time coverage.
Consider extracting to a helper method with exhaustive matching:
impl Network { fn display_name_and_color(&self) -> (&str, Color32) { match self { Network::Dash => ("Mainnet", DashColors::DASH_BLUE), Network::Testnet => ("Testnet", Color32::from_rgb(255, 165, 0)), Network::Devnet => ("Devnet", Color32::DARK_RED), Network::Regtest => ("Local Network", Color32::from_rgb(139, 69, 19)), // Compiler will error here when new variants are added } } }
153-153: Unify inconsistent navigation actions.The code uses
SetMainScreenThenGoToMainScreenwhen texture is loaded (line 153) but falls back toSetMainScreenwhen texture is unavailable (lines 179, 194). Since texture availability is a UI rendering concern, both paths should trigger the same action for consistent navigation.Apply this diff to unify all action assignments:
if added.clicked() { action = AppAction::SetMainScreenThenGoToMainScreen( *screen_type, ); // ...later in fallback branches: - action = AppAction::SetMainScreen(*screen_type); + action = AppAction::SetMainScreenThenGoToMainScreen(*screen_type); // ...and: - action = AppAction::SetMainScreen(*screen_type); + action = AppAction::SetMainScreenThenGoToMainScreen(*screen_type);Also applies to: 179-179, 194-194
doc/COMPONENT_DESIGN_PATTERN.md (1)
27-27: Fix typo: "simiar" → "similar".Minor documentation polish for clarity.
Apply this diff:
-Inside your screen's `show()` method or simiar: +Inside your screen's `show()` method or similar:src/ui/contracts_documents/group_actions_screen.rs (1)
546-557: Replace.expect()with error handling to avoid UI panics.The
.expect("Failed to create identity selector")call on line 553 will crash the UI ifIdentitySelector::new(...).selected_identity(...)returns an error. Instead, handle the Result and display an inline error message.Apply this diff:
- ui.add( - IdentitySelector::new( - "group_actions_identity_selector", - &mut self.selected_identity_str, - &self.qualified_identities, - ) - .selected_identity(&mut self.selected_identity) - .expect("Failed to create identity selector") - .other_option(false) - .width(250.0) - .label("Identity:"), - ); + match IdentitySelector::new( + "group_actions_identity_selector", + &mut self.selected_identity_str, + &self.qualified_identities, + ) + .selected_identity(&mut self.selected_identity) { + Ok(selector) => { + ui.add( + selector + .other_option(false) + .width(250.0) + .label("Identity:") + ); + } + Err(e) => { + ui.colored_label(Color32::RED, format!("Failed to init identity selector: {e}")); + } + }src/ui/components/top_panel.rs (1)
332-352: Close docs popup after selection for consistent UX.The Documents popup doesn't close after selection (line 349 has commented-out
ui.close()), while the Contracts popup does close (line 386). Both should behave identically.Apply this diff:
for (text, da) in doc_actions { if ui.button(text).clicked() { action = da.create_action(app_context); - // ui.close(); + ui.close(); } }src/ui/identities/withdraw_screen.rs (2)
201-208: Remove .expect on withdrawal_amount in dialog (panic risk).This can panic if state drifts. Build the message with a safe fallback or abort opening the dialog. Same issue was flagged earlier.
Apply:
- let dialog = self.confirmation_dialog.get_or_insert_with(|| { - ConfirmationDialog::new( - "Confirm Withdrawal".to_string(), - format!( - "Are you sure you want to withdraw {} to {}", - self.withdrawal_amount - .as_ref() - .expect("Withdrawal amount should be present"), - message_address - ), - ) - .danger_mode(true) // Withdrawal is a destructive operation - }); + let dialog = self.confirmation_dialog.get_or_insert_with(|| { + let amount_display = self + .withdrawal_amount + .as_ref() + .map(|a| a.to_string()) + .unwrap_or_else(|| "unknown amount".to_string()); + ConfirmationDialog::new( + "Confirm Withdrawal".to_string(), + format!( + "Are you sure you want to withdraw {} to {}", + amount_display, + message_address + ), + ) + .danger_mode(true) + });
221-225: Remove .expect before sending backend task (panic risk).Gracefully handle missing amount; set error and stop instead of panicking. Same as prior review.
- let credits = self - .withdrawal_amount - .as_ref() - .expect("Withdrawal amount should be present") - .value() as u128; + let credits = match self.withdrawal_amount.as_ref() { + Some(amount) => amount.value(), + None => { + self.withdraw_from_identity_status = WithdrawFromIdentityStatus::ErrorMessage( + "No withdrawal amount specified".to_string(), + ); + self.confirmation_dialog = None; + return AppAction::None; + } + };src/ui/components/contracts_subscreen_chooser_panel.rs (1)
63-66: Fix inner_margin type: Margin::same expects f32, not i8.The i8 cast will not compile. Pass f32.
- .inner_margin(Margin::same(Spacing::XL as i8)) + .inner_margin(Margin::same(Spacing::XL))src/ui/tokens/claim_tokens_screen.rs (1)
186-189: Eliminate defaulting and panics in confirmation path. Validate Options before enqueuing tasks.Still using
unwrap_or(...)fordistribution_typeandexpect("No key selected")forselected_key. Handle both gracefully and keep UI responsive.- let distribution_type = self - .distribution_type - .unwrap_or(TokenDistributionType::Perpetual); + let Some(distribution_type) = self.distribution_type else { + self.status = ClaimTokensStatus::ErrorMessage("Please select a distribution type.".into()); + return AppAction::None; + }; @@ - Some(ConfirmationStatus::Confirmed) => { + Some(ConfirmationStatus::Confirmed) => { self.confirmation_dialog = None; let now = SystemTime::now() .duration_since(UNIX_EPOCH) .expect("Time went backwards") .as_secs(); self.status = ClaimTokensStatus::WaitingForResult(now); + let Some(signing_key) = self.selected_key.clone() else { + self.status = ClaimTokensStatus::ErrorMessage("Select a signing key first.".into()); + return AppAction::None; + }; + AppAction::BackendTasks( vec![ BackendTask::TokenTask(Box::new(TokenTask::ClaimTokens { data_contract: Arc::new(self.token_contract.contract.clone()), token_position: self.identity_token_basic_info.token_position, actor_identity: self.identity.clone(), distribution_type, - signing_key: self.selected_key.clone().expect("No key selected"), + signing_key, public_note: self.public_note.clone(), })), BackendTask::TokenTask(Box::new(TokenTask::QueryMyTokenBalances)), ], BackendTasksExecutionMode::Sequential, )Also applies to: 197-220
src/ui/components/tools_subscreen_chooser_panel.rs (1)
85-85: Fix type cast: use f32 for margins.
Margin::same(Spacing::XL as i8)should beMargin::same(Spacing::XL)sinceSpacing::XLis f32, not i8. Casting to i8 will truncate the value and produce incorrect margins.Apply this diff:
- .inner_margin(Margin::same(Spacing::XL as i8)) + .inner_margin(Margin::same(Spacing::XL))src/model/qualified_identity/mod.rs (1)
229-233: Remove conflicting AsRef impl.Rust's standard library already provides
impl<T: ?Sized> AsRef<T> for T. This manual implementation conflicts and will cause a compilation error.Apply this diff:
-impl AsRef<QualifiedIdentity> for QualifiedIdentity { - fn as_ref(&self) -> &QualifiedIdentity { - self - } -}src/backend_task/grovestark.rs (2)
5-9: Use typed errors instead of String per coding guidelines.The function returns
Result<BackendTaskSuccessResult, String>, but the coding guidelines specify that error handling should usethiserrorfor structured error types. String errors lack context and cannot be easily pattern-matched by callers.As per coding guidelines, apply this pattern:
use thiserror::Error; #[derive(Debug, Error)] pub enum GroveStarkTaskError { #[error("Failed to generate proof: {0}")] ProofGeneration(String), #[error("Failed to verify proof: {0}")] ProofVerification(String), } pub async fn run_grovestark_task( task: GroveSTARKTask, sdk: &Sdk, ) -> Result<BackendTaskSuccessResult, GroveStarkTaskError> { match task { GroveSTARKTask::GenerateProof { .. } => { // ... match prover.generate_proof(...).await { Ok(proof_data) => Ok(BackendTaskSuccessResult::GeneratedZKProof(proof_data)), Err(e) => Err(GroveStarkTaskError::ProofGeneration(e.to_string())), } } GroveSTARKTask::VerifyProof { proof_data } => { // ... match prover.verify_proof(&proof_data) { Ok(is_valid) => Ok(BackendTaskSuccessResult::VerifiedZKProof(is_valid, proof_data)), Err(e) => Err(GroveStarkTaskError::ProofVerification(e.to_string())), } } } }Also applies to: 34-36, 41-46
51-65: CRITICAL: Remove Debug/PartialEq derives to prevent secret exposure.
GroveSTARKTask::GenerateProofcontainsprivate_key: [u8; 32]andpublic_key: [u8; 32]. DerivingDebugwill print these secrets in logs, error messages, and panic output. DerivingPartialEqon secrets can leak timing information.Apply this fix:
-#[derive(Debug, Clone, PartialEq)] +#[derive(Clone)] pub enum GroveSTARKTask { GenerateProof { identity_id: String, contract_id: String, document_type: String, document_id: String, key_id: u32, private_key: [u8; 32], public_key: [u8; 32], }, VerifyProof { proof_data: ProofDataOutput, }, } + +// Redacted Debug to prevent secret leakage +impl std::fmt::Debug for GroveSTARKTask { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + GroveSTARKTask::GenerateProof { + identity_id, + contract_id, + document_type, + document_id, + key_id, + .. + } => f + .debug_struct("GenerateProof") + .field("identity_id", identity_id) + .field("contract_id", contract_id) + .field("document_type", document_type) + .field("document_id", document_id) + .field("key_id", key_id) + .field("private_key", &"<redacted>") + .field("public_key", &"<redacted>") + .finish(), + GroveSTARKTask::VerifyProof { proof_data } => f + .debug_struct("VerifyProof") + .field("proof_data", proof_data) + .finish(), + } + } +}src/ui/components/identity_selector.rs (1)
262-271: Compile error: unwrap_or on non-Option InnerResponse.ComboBox::show_ui returns InnerResponse; inner is bool, not Option.
- let combo_changed = combo_response.inner.unwrap_or(false); + let combo_changed = combo_response.inner;src/ui/identities/transfer_screen.rs (1)
177-200: Type consistency: avoid u64 → u128 → u64 roundtrips for Credits.Keep Credits as Credits throughout.
- // Use the amount directly since it's already an Amount struct - let credits = self.amount.as_ref().map(|v| v.value()).unwrap_or_default() as u128; - if credits == 0 { + // Use Credits directly (Amount.value() returns Credits) + let credits: Credits = self.amount.as_ref().map(|v| v.value()).unwrap_or(0); + if credits == 0 { self.error_message = Some("Amount must be greater than 0".to_string()); self.transfer_credits_status = TransferCreditsStatus::ErrorMessage("Amount must be greater than 0".to_string()); self.confirmation_popup = false; return AppAction::None; } @@ - credits as Credits, + credits,src/app.rs (1)
247-250: Add mainnet fallback when selected network context is unavailable (and update local chosen_network).Avoid expect() panics in current_app_context() at startup when a persisted network has no context. Also update the local chosen_network variable; otherwise self.chosen_network stays invalid. — As per coding guidelines (central state/screen mgmt in app.rs).
- let selected_main_screen = settings.root_screen_type; - let chosen_network = settings.network; + let selected_main_screen = settings.root_screen_type; + let mut chosen_network = settings.network; network_chooser_screen.current_network = chosen_network; @@ } else if let (Network::Regtest, Some(local_app_context)) = (chosen_network, local_app_context.as_ref()) { // ... existing branches ... - } + } else { + // Fallback to mainnet if selected network is unavailable + network_chooser_screen.current_network = Network::Dash; + chosen_network = Network::Dash; + // Persist settings using mainnet context (persists network/root screen) + let _ = mainnet_app_context.update_settings(selected_main_screen); + }Also applies to: 251-330
src/backend_task/mnlist.rs (3)
37-41: Blocking I/O inside async; switch to spawn_blocking or make sync; use typed errors.CoreP2PHandler and RpcApi calls are synchronous, blocking the async executor. Move blocking regions to tokio::task::spawn_blocking or make run_mnlist_task synchronous. Also replace Result<_, String> with a thiserror-typed error per guidelines.
Also applies to: 49-76, 117-127
81-82: Avoid unwrap() on core_client lock.Map poisoned RwLock to an error instead of panicking.
- let client = app.core_client.read().unwrap(); + let client = app.core_client.read().map_err(|_| "core_client lock poisoned".to_string())?;Prefer a typed error over String.
83-114: Range/window off-by-one; propagate errors from RPC calls.Use an inclusive range sized ≤ max_blocks and fail fast on get_block_hash errors instead of silently skipping.
- let end_height = start_height.saturating_add(max_blocks).min(block_height); + let end_height = start_height.saturating_add(max_blocks.saturating_sub(1)).min(block_height); - for h in start_height..end_height { - if let Ok(bh2) = client.get_block_hash(h) { + for h in start_height..=end_height { + let bh2 = client.get_block_hash(h) + .map_err(|e| format!("get_block_hash({h}) failed: {e}"))?; let bh = BlockHash::from_byte_array(bh2.to_byte_array()); if let Ok(block) = client.get_block(&bh2) { … } else { … } - } }Also consider logging when get_block fails.
src/backend_task/identity/load_identity_from_wallet.rs (1)
292-331: Don’t use string matching for control flow; use typed errors.Matching on error.starts_with("No identity found…") is brittle. Return a dedicated error enum (e.g., IdentityLoadError::NotFound { index }) from load_user_identity_from_wallet and match on it here.
src/ui/tokens/tokens_screen/distributions.rs (1)
381-406: Use saturating_mul for interval math to avoid overflow.Multiplying two u64s can overflow. Use saturating_mul (or checked_mul with fallback).
- let amount = perpetual_dist_interval_input * step_count_input; + let amount = perpetual_dist_interval_input.saturating_mul(step_count_input);Apply to BlockBased, TimeBased, and EpochBased branches.
Also applies to: 408-415
src/ui/tokens/destroy_frozen_funds_screen.rs (1)
296-311: Don’t panic on missing signing key.expect("No key selected") will crash if the user confirms without a key. Validate and surface an error; optionally disable the Destroy button until both frozen_identity_id parses and a key is selected.
- signing_key: self.selected_key.clone().expect("No key selected"), + signing_key: match self.selected_key.clone() { + Some(k) => k, + None => { + self.error_message = Some("Please select a signing key.".into()); + self.status = DestroyFrozenFundsStatus::ErrorMessage("No key selected".into()); + return AppAction::None; + } + },src/ui/tokens/pause_tokens_screen.rs (1)
230-241: Handle missing signing key without panicking.Replace expect("No key selected") with validation that sets an error and returns early; also disable the Pause button until a key is chosen.
- signing_key: self.selected_key.clone().expect("No key selected"), + signing_key: match self.selected_key.clone() { + Some(k) => k, + None => { + self.error_message = Some("Please select a signing key.".into()); + self.status = PauseTokensStatus::ErrorMessage("No key selected".into()); + return AppAction::None; + } + },src/ui/tokens/direct_token_purchase_screen.rs (2)
289-300: Avoid panic on missing signing key in confirm path.Replace expect("Expected a key") with a graceful check and error state before dispatch.
- signing_key: self.selected_key.clone().expect("Expected a key"), + // Guard: require a selected signing key + signing_key: { + let Some(signing_key) = self.selected_key.clone() else { + self.error_message = Some("Select a signing key first.".into()); + self.status = PurchaseTokensStatus::ErrorMessage("No signing key selected".into()); + return AppAction::None; + }; + signing_key + },
519-521: Remove duplicate “DASH” in price label.Amount already carries the unit. Show it once.
- ui.label(format!("{} DASH ({} credits)",dash_amount, calculated_price_credits)); + ui.label(format!("{} ({} credits)", dash_amount, calculated_price_credits));src/ui/components/confirmation_dialog.rs (2)
37-43: Blocker: returning reference to temporaryNone.This borrows a temporary and won’t compile. Always return a ref to the stored Option.
Apply:
- fn changed_value(&self) -> &Option<Self::DomainType> { - if self.has_changed() { - &self.dialog_response - } else { - &None - } - } + fn changed_value(&self) -> &Option<Self::DomainType> { + &self.dialog_response + }
1-1: Blocker: invalidWidgetTexthandling (match on &WidgetText; Arc not Into).Match on a value (clone) and construct
WidgetTextdirectly; dropArc.Apply:
- use std::sync::Arc; + // removed: Arc no longer needed- let confirm_label = if let WidgetText::RichText(rich_text) = - confirm_text - { - // preserve rich text formatting - rich_text.clone() - } else { - Arc::new(egui::RichText::new(confirm_text.text()).color(text_color)) - }; + let confirm_label: WidgetText = match confirm_text.clone() { + WidgetText::RichText(rt) => WidgetText::RichText(rt), + other => egui::RichText::new(other.text()) + .color(text_color) + .into(), + };- let cancel_label = if let WidgetText::RichText(rich_text) = cancel_text - { - // preserve rich text formatting - rich_text.clone() - } else { - egui::RichText::new(cancel_text.text()) - .color(ComponentStyles::secondary_button_text()) - .into() - }; + let cancel_label: WidgetText = match cancel_text.clone() { + WidgetText::RichText(rt) => WidgetText::RichText(rt), + other => egui::RichText::new(other.text()) + .color(ComponentStyles::secondary_button_text()) + .into(), + };Also applies to: 212-220, 241-250
src/ui/tokens/mint_tokens_screen.rs (1)
248-252: Treat empty recipient as “issuer (self)” and pass None to backend.Empty recipient should mint to issuer; avoid parsing empty string and improve confirmation text.
Apply:
@@ - let msg = format!( - "Are you sure you want to mint {} tokens to {}?", - self.amount.clone().unwrap_or(Amount::new(0, 0)), - self.recipient_identity_id - ); + let recipient_display = if self.recipient_identity_id.trim().is_empty() { + "issuer (self)".to_string() + } else { + self.recipient_identity_id.clone() + }; + let msg = format!( + "Are you sure you want to mint {} tokens to {}?", + self.amount.clone().unwrap_or(Amount::new(0, 0)), + recipient_display + ); @@ - let parsed_receiver_id = Identifier::from_string_try_encodings( - &self.recipient_identity_id, - &[ - dash_sdk::dpp::platform_value::string_encoding::Encoding::Base58, - dash_sdk::dpp::platform_value::string_encoding::Encoding::Hex, - ], - ); - - if parsed_receiver_id.is_err() { - self.status = MintTokensStatus::ErrorMessage("Invalid receiver".into()); - self.error_message = Some("Invalid receiver".into()); - return AppAction::None; - } - - let receiver_id = parsed_receiver_id.unwrap(); + let receiver_id_opt = if self.recipient_identity_id.trim().is_empty() { + None + } else { + match Identifier::from_string_try_encodings( + &self.recipient_identity_id, + &[ + dash_sdk::dpp::platform_value::string_encoding::Encoding::Base58, + dash_sdk::dpp::platform_value::string_encoding::Encoding::Hex, + ], + ) { + Ok(id) => Some(id), + Err(_) => { + self.status = MintTokensStatus::ErrorMessage("Invalid receiver".into()); + self.error_message = Some("Invalid receiver".into()); + return AppAction::None; + } + } + }; @@ - recipient_id: Some(receiver_id), + recipient_id: receiver_id_opt, amount: self.amount.clone().unwrap_or(Amount::new(0, 0)).value(), group_info, }))) @@ - let msg = format!( - "Are you sure you want to mint {} tokens to {}?", - self.amount.clone().unwrap_or(Amount::new(0, 0)), - self.recipient_identity_id - ); + let recipient_display = if self.recipient_identity_id.trim().is_empty() { + "issuer (self)".to_string() + } else { + self.recipient_identity_id.clone() + }; + let msg = format!( + "Are you sure you want to mint {} tokens to {}?", + self.amount.clone().unwrap_or(Amount::new(0, 0)), + recipient_display + );Also applies to: 281-296, 320-333, 638-648
src/ui/tokens/set_token_price_screen.rs (1)
293-299: Overflow risk converting price-per-unit to price-per-token.Use wider type and saturate/clamp to u64.
Apply:
- let price_per_token = price_per_smallest_unit * decimal_multiplier; + let price_per_token = (price_per_smallest_unit as u128) + .saturating_mul(decimal_multiplier as u128) + .min(u64::MAX as u128) as u64; @@ - let price_per_token = price_per_smallest_unit * decimal_multiplier; + let price_per_token = (*price_per_smallest_unit as u128) + .saturating_mul(decimal_multiplier as u128) + .min(u64::MAX as u128) as u64;Also applies to: 311-319
src/ui/tokens/freeze_tokens_screen.rs (1)
559-562: Bug: confirmation dialog never created on click.You reset to None and never instantiate; popup never appears.
Apply:
- if ui.add(button).clicked() { - // Initialize confirmation dialog when button is clicked - self.confirmation_dialog = None; // Reset for fresh dialog - } + if ui.add(button).clicked() { + self.confirmation_dialog = Some( + ConfirmationDialog::new( + "Confirm Freeze", + format!( + "Are you sure you want to freeze identity {}?", + self.freeze_identity_id + ), + ) + .confirm_text(Some("Confirm")) + .cancel_text(Some("Cancel")), + ); + }src/ui/contracts_documents/document_action_screen.rs (1)
360-367: Compile error: passing &mut of a temporary to TextEditTextEdit::multiline needs &mut String; &mut json.clone() is a temporary and won’t compile. Use a real mutable String or a non-editable widget.
Apply one of these:
Option A (non-editable code label):
- if let Ok(json) = serde_json::to_string_pretty(doc) { - ui.add( - egui::TextEdit::multiline(&mut json.clone()) - .font(egui::TextStyle::Monospace) - .desired_rows(10) - .desired_width(380.0) - .interactive(false), - ); + if let Ok(json) = serde_json::to_string_pretty(doc) { + ui.code(json);Option B (editable-looking, but disabled):
- if let Ok(json) = serde_json::to_string_pretty(doc) { - ui.add( - egui::TextEdit::multiline(&mut json.clone()) + if let Ok(mut json_str) = serde_json::to_string_pretty(doc) { + ui.add( + egui::TextEdit::multiline(&mut json_str) .font(egui::TextStyle::Monospace) .desired_rows(10) .desired_width(380.0) .interactive(false), );src/ui/tokens/transfer_tokens_screen.rs (1)
67-75: Avoid panics during screen init; surface a UI error insteadload_local_qualified_identities().expect(...) and find(...).expect(...) will crash the app if data is unavailable. Return a screen with a user-visible error state (e.g., TransferTokensStatus::ErrorMessage) and disable actions instead.
I can provide a small patch to replace these expects with graceful fallbacks and an error message.
| - name: Install system dependencies | ||
| run: | | ||
| sudo apt-get update | ||
| sudo apt-get install -y build-essential pkg-config clang cmake libsqlite3-dev | ||
|
|
||
| - name: Install protoc | ||
| run: | | ||
| curl -OL https://github.com/protocolbuffers/protobuf/releases/download/v25.2/protoc-25.2-linux-x86_64.zip | ||
| sudo unzip -o protoc-25.2-linux-x86_64.zip -d /usr/local bin/protoc | ||
| sudo unzip -o protoc-25.2-linux-x86_64.zip -d /usr/local 'include/*' | ||
| rm -f protoc-25.2-linux-x86_64.zip | ||
| env: | ||
| PROTOC: /usr/local/bin/protoc | ||
|
|
There was a problem hiding this comment.
Install unzip before using it; optionally export PROTOC at job scope.
unzip is used but not installed; add it to apt-get to avoid failures. Exporting PROTOC at job-level is optional since /usr/local/bin is in PATH.
- name: Install system dependencies
run: |
sudo apt-get update
- sudo apt-get install -y build-essential pkg-config clang cmake libsqlite3-dev
+ sudo apt-get install -y build-essential pkg-config clang cmake libsqlite3-dev unzipOptionally set at job env:
env:
CARGO_TERM_COLOR: always
PROTOC: /usr/local/bin/protoc🤖 Prompt for AI Agents
.github/workflows/tests.yml around lines 40 to 53: the workflow calls unzip but
never installs it and sets PROTOC only in the step env; update the "Install
system dependencies" step to include unzip in the apt-get install list (e.g.,
add unzip to build-essential pkg-config clang cmake libsqlite3-dev) so the
subsequent step can run, and optionally move or add PROTOC=/usr/local/bin/protoc
under the job-level env block so the variable is available to all steps.
| zmqpubrawtxlocksig=tcp://0.0.0.0:23709 | ||
| zmqpubrawchainlock=tcp://0.0.0.0:23709 No newline at end of file | ||
| zmqpubrawchainlocksig=tcp://0.0.0.0:23709 No newline at end of file |
There was a problem hiding this comment.
Open ZMQ endpoints on 0.0.0.0 — restrict scope or document firewalling.
Exposing ZMQ to all interfaces can leak mempool/chain data and invite scraping. Prefer localhost unless remote subscribers are required.
Apply this diff to bind locally (adjust if you truly need remote subscribers):
- zmqpubrawtxlocksig=tcp://0.0.0.0:23709
- zmqpubrawchainlocksig=tcp://0.0.0.0:23709
+ zmqpubrawtxlocksig=tcp://127.0.0.1:23709
+ zmqpubrawchainlocksig=tcp://127.0.0.1:23709🤖 Prompt for AI Agents
In dash_core_configs/testnet.conf around lines 9 to 10, the ZMQ endpoints are
bound to 0.0.0.0 which exposes mempool/chain data to all interfaces; change the
bindings to localhost (127.0.0.1) so ZMQ only listens locally, or if remote
subscribers are required, leave as-is but document and enforce firewall rules
and IP-restricted access for those ports to limit exposure.
| && let (Value::Identifier(owner_bytes), Value::U64(amount)) = | ||
| (owner_value, amount_value) | ||
| { | ||
| if let Ok(owner_id) = Identifier::from_bytes(owner_bytes) { | ||
| if let Err(e) = self | ||
| .insert_token_identity_balance(&token_id, &owner_id, *amount) | ||
| { | ||
| eprintln!( | ||
| "Failed to update token balance from historical document: {}", | ||
| e | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| && let Ok(owner_id) = Identifier::from_bytes(owner_bytes) | ||
| && let Err(e) = | ||
| self.insert_token_identity_balance(&token_id, &owner_id, *amount) | ||
| { | ||
| eprintln!( | ||
| "Failed to update token balance from historical document: {}", | ||
| e | ||
| ); |
There was a problem hiding this comment.
Log missing/invalid data and parse failures (HistoricalDocument).
Only insert failures are logged; other failures are silent.
- if let (Some(owner_value), Some(amount_value)) =
- (document.get("ownerId"), document.get("amount"))
- && let (Value::Identifier(owner_bytes), Value::U64(amount)) =
- (owner_value, amount_value)
- && let Ok(owner_id) = Identifier::from_bytes(owner_bytes)
- && let Err(e) =
- self.insert_token_identity_balance(&token_id, &owner_id, *amount)
- {
- eprintln!(
- "Failed to update token balance from historical document: {}",
- e
- );
- }
+ if let (Some(owner_value), Some(amount_value)) =
+ (document.get("ownerId"), document.get("amount"))
+ && let (Value::Identifier(owner_bytes), Value::U64(amount)) =
+ (owner_value, amount_value)
+ && let Ok(owner_id) = Identifier::from_bytes(owner_bytes)
+ {
+ if let Err(e) = self.insert_token_identity_balance(
+ &token_id,
+ &owner_id,
+ *amount,
+ ) {
+ eprintln!(
+ "Failed to update token balance from historical document: {}",
+ e
+ );
+ }
+ } else {
+ eprintln!(
+ "Historical document missing/invalid ownerId or amount; skipping balance update"
+ );
+ }Committable suggestion skipped: line range outside the PR's diff.
| (owner_value, amount_value) | ||
| { | ||
| if let Ok(owner_id) = Identifier::from_bytes(owner_bytes) { | ||
| if let Err(e) = self | ||
| .insert_token_identity_balance(&token_id, &owner_id, *amount) | ||
| { | ||
| eprintln!( | ||
| "Failed to update token balance from group action document: {}", | ||
| e | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| && let Ok(owner_id) = Identifier::from_bytes(owner_bytes) | ||
| && let Err(e) = | ||
| self.insert_token_identity_balance(&token_id, &owner_id, *amount) | ||
| { | ||
| eprintln!( | ||
| "Failed to update token balance from group action document: {}", | ||
| e | ||
| ); |
There was a problem hiding this comment.
Mirror the fix for GroupActionWithDocument.
Add else logging; move insert error check into the then-branch.
- if let (Some(owner_value), Some(amount_value)) =
- (document.get("ownerId"), document.get("amount"))
- && let (Value::Identifier(owner_bytes), Value::U64(amount)) =
- (owner_value, amount_value)
- && let Ok(owner_id) = Identifier::from_bytes(owner_bytes)
- && let Err(e) =
- self.insert_token_identity_balance(&token_id, &owner_id, *amount)
- {
- eprintln!(
- "Failed to update token balance from group action document: {}",
- e
- );
- }
+ if let (Some(owner_value), Some(amount_value)) =
+ (document.get("ownerId"), document.get("amount"))
+ && let (Value::Identifier(owner_bytes), Value::U64(amount)) =
+ (owner_value, amount_value)
+ && let Ok(owner_id) = Identifier::from_bytes(owner_bytes)
+ {
+ if let Err(e) =
+ self.insert_token_identity_balance(&token_id, &owner_id, *amount)
+ {
+ eprintln!(
+ "Failed to update token balance from group action document: {}",
+ e
+ );
+ }
+ } else {
+ eprintln!(
+ "Group action document missing/invalid ownerId or amount; skipping balance update"
+ );
+ }| if let (Some(claimer_value), Some(amount_value)) = | ||
| (document.get("claimerId"), document.get("amount")) | ||
| { | ||
| if let (Value::Identifier(claimer_bytes), Value::U64(amount)) = | ||
| && let (Value::Identifier(claimer_bytes), Value::U64(amount)) = | ||
| (claimer_value, amount_value) | ||
| { | ||
| if let Ok(claimer_id) = Identifier::from_bytes(claimer_bytes) { | ||
| if let Err(e) = self.insert_token_identity_balance( | ||
| &token_id, | ||
| &claimer_id, | ||
| *amount, | ||
| ) { | ||
| eprintln!( | ||
| "Failed to update token balance from claim document: {}", | ||
| e | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| && let Ok(claimer_id) = Identifier::from_bytes(claimer_bytes) | ||
| && let Err(e) = | ||
| self.insert_token_identity_balance(&token_id, &claimer_id, *amount) | ||
| { | ||
| eprintln!("Failed to update token balance from claim document: {}", e); | ||
| } |
There was a problem hiding this comment.
Don’t swallow validation/parse failures (ClaimResult::Document).
Only insertion failures are logged; missing/invalid claimerId or amount, or parse failure, produce no log.
- if let (Some(claimer_value), Some(amount_value)) =
- (document.get("claimerId"), document.get("amount"))
- && let (Value::Identifier(claimer_bytes), Value::U64(amount)) =
- (claimer_value, amount_value)
- && let Ok(claimer_id) = Identifier::from_bytes(claimer_bytes)
- && let Err(e) =
- self.insert_token_identity_balance(&token_id, &claimer_id, *amount)
- {
- eprintln!("Failed to update token balance from claim document: {}", e);
- }
+ if let (Some(claimer_value), Some(amount_value)) =
+ (document.get("claimerId"), document.get("amount"))
+ && let (Value::Identifier(claimer_bytes), Value::U64(amount)) =
+ (claimer_value, amount_value)
+ && let Ok(claimer_id) = Identifier::from_bytes(claimer_bytes)
+ {
+ if let Err(e) =
+ self.insert_token_identity_balance(&token_id, &claimer_id, *amount)
+ {
+ eprintln!(
+ "Failed to update token balance from claim document: {}",
+ e
+ );
+ }
+ } else {
+ eprintln!(
+ "Claim document missing/invalid claimerId or amount; skipping balance update"
+ );
+ }🤖 Prompt for AI Agents
In src/backend_task/tokens/claim_tokens.rs around lines 74 to 83, the current
chained let-guards only log insertion errors and silently ignore missing/invalid
fields or parse failures; update the code to explicitly handle and log each
failure path: check for presence of "claimerId" and "amount" and log if either
is missing, validate that they are the expected Value variants and log
descriptive errors when they are not, handle Identifier::from_bytes errors and
log the parse error, then proceed to call insert_token_identity_balance and log
any insertion error as before; ensure each branch uses clear, contextual error
messages so missing/invalid/parse failures are not swallowed.
| BackendTask::TokenTask(Box::new(TokenTask::BurnTokens { | ||
| owner_identity: self.identity_token_info.identity.clone(), | ||
| data_contract, | ||
| token_position: self.identity_token_info.token_position, | ||
| signing_key: self.selected_key.clone().expect("Expected a key"), | ||
| public_note: if self.group_action_id.is_some() { | ||
| None | ||
| } else { | ||
| self.public_note.clone() | ||
| }, | ||
| amount: amount.value(), | ||
| group_info, | ||
| })), |
There was a problem hiding this comment.
Avoid panic on missing signing key in confirm path.
Replace expect("Expected a key") with a guard and user-facing error.
- signing_key: self.selected_key.clone().expect("Expected a key"),
+ signing_key: {
+ let Some(signing_key) = self.selected_key.clone() else {
+ self.error_message = Some("Select a signing key first.".into());
+ self.status = BurnTokensStatus::ErrorMessage("No signing key selected".into());
+ return AppAction::None;
+ };
+ signing_key
+ },📝 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.
| BackendTask::TokenTask(Box::new(TokenTask::BurnTokens { | |
| owner_identity: self.identity_token_info.identity.clone(), | |
| data_contract, | |
| token_position: self.identity_token_info.token_position, | |
| signing_key: self.selected_key.clone().expect("Expected a key"), | |
| public_note: if self.group_action_id.is_some() { | |
| None | |
| } else { | |
| self.public_note.clone() | |
| }, | |
| amount: amount.value(), | |
| group_info, | |
| })), | |
| BackendTask::TokenTask(Box::new(TokenTask::BurnTokens { | |
| owner_identity: self.identity_token_info.identity.clone(), | |
| data_contract, | |
| token_position: self.identity_token_info.token_position, | |
| signing_key: { | |
| let Some(signing_key) = self.selected_key.clone() else { | |
| self.error_message = Some("Select a signing key first.".into()); | |
| self.status = BurnTokensStatus::ErrorMessage("No signing key selected".into()); | |
| return AppAction::None; | |
| }; | |
| signing_key | |
| }, | |
| public_note: if self.group_action_id.is_some() { | |
| None | |
| } else { | |
| self.public_note.clone() | |
| }, | |
| amount: amount.value(), | |
| group_info, | |
| })), |
🤖 Prompt for AI Agents
In src/ui/tokens/burn_tokens_screen.rs around lines 287 to 299, the code panics
by using expect("Expected a key") when building the BurnTokens task; instead,
guard against a missing signing_key and show a user-facing error (or set an
error state) rather than panicking. Before creating BackendTask::TokenTask,
check if self.selected_key.is_some(); if not, abort building the task and update
the UI state to display an appropriate error message (or return an Err) so the
confirm path fails gracefully; only construct and enqueue the BurnTokens task
when a signing key is present.
| AppAction::BackendTask(BackendTask::TokenTask(Box::new(TokenTask::ResumeTokens { | ||
| actor_identity: self.identity.clone(), | ||
| data_contract, | ||
| token_position: self.identity_token_info.token_position, | ||
| signing_key: self.selected_key.clone().expect("No key selected"), | ||
| public_note: if self.group_action_id.is_some() { | ||
| None | ||
| } else { | ||
| self.public_note.clone() | ||
| }, | ||
| group_info, | ||
| }))) |
There was a problem hiding this comment.
Gracefully handle missing signing key in Resume flow.
Same issue: replace expect("No key selected") with a checked branch that sets an error and returns without crashing.
- signing_key: self.selected_key.clone().expect("No key selected"),
+ signing_key: match self.selected_key.clone() {
+ Some(k) => k,
+ None => {
+ self.error_message = Some("Please select a signing key.".into());
+ self.status = ResumeTokensStatus::ErrorMessage("No key selected".into());
+ return AppAction::None;
+ }
+ },Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/ui/tokens/resume_tokens_screen.rs around lines 230 to 241, the code
currently calls expect("No key selected") when building the ResumeTokens task
which will panic if no signing key is selected; instead, check self.selected_key
first and if None set an appropriate error state on the UI (e.g., self.error =
Some("No signing key selected".into())) and return early so the AppAction is not
constructed, and only build and dispatch the BackendTask when a signing_key is
present (use the cloned key from the Some variant).
| Frame::group(ui.style()) | ||
| .stroke(visuals.widgets.noninteractive.bg_stroke) | ||
| .fill(visuals.extreme_bg_color) | ||
| .inner_margin(Margin::same(12)) | ||
| .show(ui, |ui| { |
There was a problem hiding this comment.
Use f32 for Margin::same.
Literal 12 is an integer; Margin::same requires f32.
- .inner_margin(Margin::same(12))
+ .inner_margin(Margin::same(12.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.
| Frame::group(ui.style()) | |
| .stroke(visuals.widgets.noninteractive.bg_stroke) | |
| .fill(visuals.extreme_bg_color) | |
| .inner_margin(Margin::same(12)) | |
| .show(ui, |ui| { | |
| Frame::group(ui.style()) | |
| .stroke(visuals.widgets.noninteractive.bg_stroke) | |
| .fill(visuals.extreme_bg_color) | |
| .inner_margin(Margin::same(12.0)) | |
| .show(ui, |ui| { |
🤖 Prompt for AI Agents
In src/ui/tokens/tokens_screen/contract_details.rs around lines 63 to 67, the
call Margin::same(12) passes an integer literal but the function expects an f32;
replace the integer literal with a floating-point literal such as
Margin::same(12.0) (or Margin::same(12.0_f32)) so the type matches and the code
compiles.
| let decimals = self.decimals_input.parse::<u8>().unwrap_or(0); | ||
| let input = self | ||
| .base_supply_input | ||
| .get_or_insert_with(|| AmountInput::new(Amount::new(0, decimals))); | ||
|
|
||
| if decimals != input.decimal_places() { | ||
| // Update decimals; it will change actual value but I guess this is what user expects | ||
| input.set_decimal_places(decimals); | ||
| } | ||
|
|
||
| let response = input.show(ui); | ||
| response.inner.update(&mut self.base_supply_amount); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Clamp decimals and allow zero base supply.
Parsing decimals to 0 by default is surprising; also, base supply of 0 should be permitted in creator flows.
- let decimals = self.decimals_input.parse::<u8>().unwrap_or(0);
+ // Clamp to a reasonable range; use DEFAULT_DECIMALS if parse fails
+ let decimals = self
+ .decimals_input
+ .parse::<u8>()
+ .unwrap_or(DEFAULT_DECIMALS)
+ .min(18);
@@
- if decimals != input.decimal_places() {
+ if decimals != input.decimal_places() {
// Update decimals; it will change actual value but I guess this is what user expects
input.set_decimal_places(decimals);
}
+ // Allow zero base supply
+ input.set_min_amount(Some(0));Please confirm the platform’s maximum supported decimals for tokens. If it differs from 18, adjust the clamp accordingly.
🌐 Web query:
What is the allowed range (min/max) for token decimals on Dash Platform tokens?
💡 Result:
Allowed decimals: minimum 0, maximum 16. [1][2]
Sources:
[1] Dash Platform — Contract Tokens (Decimal places constraint).
[2] Dash Platform — Tokens (Decimal precision default and configuration).
🏁 Script executed:
#!/bin/bash
# Find DEFAULT_DECIMALS constant definition
rg -n "DEFAULT_DECIMALS" --type rustLength of output: 382
🏁 Script executed:
#!/bin/bash
# Find set_min_amount method definition
rg -n "set_min_amount" --type rust -C 2Length of output: 391
🏁 Script executed:
#!/bin/bash
# Check context around the code in question
cat src/ui/tokens/tokens_screen/mod.rs | sed -n '2500,2530p'Length of output: 1238
Update decimals clamping to match Dash Platform constraint (max 16, not 18).
Per Dash Platform specifications, token decimals have a maximum of 16. Update the suggested clamping accordingly:
- let decimals = self.decimals_input.parse::<u8>().unwrap_or(0);
+ // Clamp to Dash Platform range [0, 16]; use DEFAULT_DECIMALS if parse fails
+ let decimals = self
+ .decimals_input
+ .parse::<u8>()
+ .unwrap_or(DEFAULT_DECIMALS)
+ .min(16);The rest of the review's suggestions are valid: DEFAULT_DECIMALS (8) is defined at line 78, and set_min_amount() exists in src/ui/components/amount_input.rs:227 and accepts Option<Credits>. Allowing zero base supply via input.set_min_amount(Some(0)) is correctly proposed.
📝 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 decimals = self.decimals_input.parse::<u8>().unwrap_or(0); | |
| let input = self | |
| .base_supply_input | |
| .get_or_insert_with(|| AmountInput::new(Amount::new(0, decimals))); | |
| if decimals != input.decimal_places() { | |
| // Update decimals; it will change actual value but I guess this is what user expects | |
| input.set_decimal_places(decimals); | |
| } | |
| let response = input.show(ui); | |
| response.inner.update(&mut self.base_supply_amount); | |
| } | |
| // Clamp to Dash Platform range [0, 16]; use DEFAULT_DECIMALS if parse fails | |
| let decimals = self | |
| .decimals_input | |
| .parse::<u8>() | |
| .unwrap_or(DEFAULT_DECIMALS) | |
| .min(16); | |
| let input = self | |
| .base_supply_input | |
| .get_or_insert_with(|| AmountInput::new(Amount::new(0, decimals))); | |
| if decimals != input.decimal_places() { | |
| // Update decimals; it will change actual value but I guess this is what user expects | |
| input.set_decimal_places(decimals); | |
| } | |
| let response = input.show(ui); | |
| response.inner.update(&mut self.base_supply_amount); | |
| } |
🤖 Prompt for AI Agents
In src/ui/tokens/tokens_screen/mod.rs around lines 2509 to 2521, update the
decimals handling to clamp to Dash Platform's max of 16 (not 18) and use
DEFAULT_DECIMALS as the fallback; specifically, parse decimals, clamp to 0..=16,
create the AmountInput with Amount::new(0, decimals) using DEFAULT_DECIMALS when
parsing fails, call input.set_decimal_places(clamped_decimals) when changed, and
ensure zero base supply is allowed by calling input.set_min_amount(Some(0)).
| let data_contract = Arc::new( | ||
| self.app_context | ||
| .get_unqualified_contract_by_id(&self.identity_token_balance.data_contract_id) | ||
| .expect("Failed to get data contract") | ||
| .expect("Data contract not found"), | ||
| ); | ||
|
|
||
| AppAction::BackendTask(BackendTask::TokenTask(Box::new( |
There was a problem hiding this comment.
Don’t unwrap contract lookup; convert to user-visible error
get_unqualified_contract_by_id(...).expect(...).expect(...) can panic. Handle both Result and Option, set an error status, and bail.
- let data_contract = Arc::new(
- self.app_context
- .get_unqualified_contract_by_id(&self.identity_token_balance.data_contract_id)
- .expect("Failed to get data contract")
- .expect("Data contract not found"),
- );
+ let data_contract = match self
+ .app_context
+ .get_unqualified_contract_by_id(&self.identity_token_balance.data_contract_id)
+ {
+ Ok(Some(dc)) => Arc::new(dc),
+ Ok(None) => {
+ self.transfer_tokens_status =
+ TransferTokensStatus::ErrorMessage("Data contract not found".into());
+ return AppAction::None;
+ }
+ Err(e) => {
+ self.transfer_tokens_status = TransferTokensStatus::ErrorMessage(format!(
+ "Failed to get data contract: {e}"
+ ));
+ return AppAction::None;
+ }
+ };🤖 Prompt for AI Agents
In src/ui/tokens/transfer_tokens_screen.rs around lines 216 to 223, the chained
expect calls on get_unqualified_contract_by_id can panic; replace them with
proper error handling by matching the Result first and the Option second: if the
Result is Err, set the screen's error status/message accordingly and return/bail
from the function; if the Result is Ok(None), set a "Data contract not found"
error status/message and return/bail; only when you have Ok(Some(contract)) wrap
it in Arc and continue to create the BackendTask. Ensure any early returns clean
up or reset UI state as needed.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
119-129: Improved build logging and diagnostics with strict shell settings.The build step now enforces strict shell behavior (
set -euo pipefail), captures output to per-platform logs, and sets environment variables for better error diagnostics. This is solid for CI/CD observability.Minor note on environment variable syntax: Line 129 uses a ternary operator (
matrix.target == 'x86_64-pc-windows-gnu' && '1' || '') to conditionally setBLST_PORTABLE. While functional, consider using an explicit GitHub Actionsif:condition at the step level for improved clarity. However, the current approach is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release.yml(3 hunks)
🔇 Additional comments (1)
.github/workflows/release.yml (1)
142-147: Good practice: Uploading per-platform build logs as artifacts.The new step to upload build logs ensures diagnostics are preserved for all builds (using
if: always()), enabling efficient post-mortem analysis. This complements the error summarization and improves observability across platforms.
| - name: Install essentials | ||
| if: ${{ runner.os == 'Linux' }} | ||
| run: sudo apt-get update && sudo apt-get install -y build-essential pkg-config clang cmake unzip libsqlite3-dev gcc-mingw-w64 mingw-w64 libsqlite3-dev mingw-w64-x86-64-dev gcc-aarch64-linux-gnu zip && uname -a && cargo clean | ||
| run: sudo apt-get update && sudo apt-get install -y build-essential pkg-config clang cmake unzip libsqlite3-dev gcc-mingw-w64 mingw-w64 libsqlite3-dev mingw-w64-x86-64-dev gcc-aarch64-linux-gnu zip nasm && uname -a && cargo clean |
There was a problem hiding this comment.
Remove duplicate libsqlite3-dev package from install list.
Line 81 specifies libsqlite3-dev twice in the apt-get install command, which is redundant.
Apply this diff to deduplicate the package:
- run: sudo apt-get update && sudo apt-get install -y build-essential pkg-config clang cmake unzip libsqlite3-dev gcc-mingw-w64 mingw-w64 libsqlite3-dev mingw-w64-x86-64-dev gcc-aarch64-linux-gnu zip nasm && uname -a && cargo clean
+ run: sudo apt-get update && sudo apt-get install -y build-essential pkg-config clang cmake unzip libsqlite3-dev gcc-mingw-w64 mingw-w64 mingw-w64-x86-64-dev gcc-aarch64-linux-gnu zip nasm && uname -a && cargo clean📝 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.
| run: sudo apt-get update && sudo apt-get install -y build-essential pkg-config clang cmake unzip libsqlite3-dev gcc-mingw-w64 mingw-w64 libsqlite3-dev mingw-w64-x86-64-dev gcc-aarch64-linux-gnu zip nasm && uname -a && cargo clean | |
| run: sudo apt-get update && sudo apt-get install -y build-essential pkg-config clang cmake unzip libsqlite3-dev gcc-mingw-w64 mingw-w64 mingw-w64-x86-64-dev gcc-aarch64-linux-gnu zip nasm && uname -a && cargo clean |
🤖 Prompt for AI Agents
.github/workflows/release.yml around line 81: the apt-get install command lists
libsqlite3-dev twice; remove the duplicated libsqlite3-dev entry so each package
appears only once in the install list, leaving the rest of the packages and
command intact.
| - name: Summarize build errors | ||
| if: failure() | ||
| run: | | ||
| echo "::group::Detected error lines" | ||
| # Try to extract the most relevant error lines from the log | ||
| grep -nE "^error(\[[A-Z0-9]+\])?:|^thread '.*' panicked at|^note:|^= note:" -m 200 build-${{ matrix.platform }}.log || true | ||
| echo "::endgroup::" | ||
| echo "::group::Last 200 lines of build output" | ||
| tail -n 200 build-${{ matrix.platform }}.log || true | ||
| echo "::endgroup::" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify grep flag behavior in error summarization step.
Line 136 uses grep -m 200, which stops after 200 matches, not 200 lines. Depending on the log structure, this could truncate error output unexpectedly if there are many error/note lines. Confirm this aligns with intent; otherwise, consider removing the -m flag or increasing the limit if needed.
🏁 Script executed:
#!/bin/bash
# Description: Verify grep -m 200 behavior on a sample Rust build log
# Test: Build a Rust project and capture error patterns to understand match density
# Create a test log with multiple error patterns
cat > test_build.log << 'EOF'
error[E0425]: cannot find value `undefined` in this scope
--> src/main.rs:10:5
|
10 | undefined_var;
| ^^^^^^^^^^^^^ not found in this scope
note: that this error is documented as follows:
= note: this is expected behavior
error[E0382]: use of moved value: `x`
--> src/main.rs:20:10
|
20 | let y = x;
| ^ value used after move
= note: move occurs because `x` has type `Vec<i32>`, which does not implement the `Copy` trait
EOF
# Simulate the grep used in the workflow
echo "=== Testing grep -m 200 ==="
grep -nE "^error(\[[A-Z0-9]+\])?:|^thread '.*' panicked at|^note:|^= note:" -m 200 test_build.log | wc -l
echo "Lines matched: $(grep -nE "^error(\[[A-Z0-9]+\])?:|^thread '.*' panicked at|^note:|^= note:" -m 200 test_build.log | wc -l)"Length of output: 46
Let me verify this with a more realistic test case that simulates a build log with multiple errors:
Let me test with a more realistic scenario with many Rust compilation errors:
Based on the verification, I can now generate the final rewritten comment. The review comment's concern is factually correct—grep -m NUM stops after NUM matching lines. The script confirmed this behavior. However, I should note that the workflow also has a fallback tail -n 200 command that captures the last 200 lines of the log, providing additional error context.
Update the grep command to remove the -m 200 limit or document the truncation risk.
The -m 200 flag stops after 200 matching lines, not 200 file lines. Given that dozens or hundreds of build errors can occur during refactoring, and each Rust error typically spans multiple matching lines (error declaration, location info, and notes), a single build could generate more than 200 matches, causing the error summary to truncate unexpectedly. Consider removing -m 200 to capture all error patterns, or if truncation is intentional, add a comment explaining the limit.
🤖 Prompt for AI Agents
.github/workflows/release.yml around lines 131 to 140: the grep command uses -m
200 which limits matches to 200 matching lines (not file lines) and can truncate
multi-line Rust errors; remove the -m 200 option so grep captures all matching
error/note/panic lines, or if you want a deliberate cap, replace it with a
comment explaining truncation and choose a more appropriate strategy (e.g., pipe
through head/tail after matching or output match count) so the summarized errors
are not accidentally incomplete.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/release.yml (2)
94-94: Remove duplicatelibsqlite3-devpackage from install list.Line 94 specifies
libsqlite3-devtwice in the apt-get install command. This was flagged in a previous review. Apply this diff to deduplicate:- run: sudo apt-get update && sudo apt-get install -y build-essential pkg-config clang cmake unzip libsqlite3-dev gcc-mingw-w64 mingw-w64 libsqlite3-dev mingw-w64-x86-64-dev gcc-aarch64-linux-gnu zip nasm && uname -a && cargo clean + run: sudo apt-get update && sudo apt-get install -y build-essential pkg-config clang cmake unzip libsqlite3-dev gcc-mingw-w64 mingw-w64 mingw-w64-x86-64-dev gcc-aarch64-linux-gnu zip nasm && uname -a && cargo clean
157-168: Address unresolved grep-m 200truncation issue from prior review.Line 163 uses
grep -m 200, which was flagged as a critical issue in a previous review. The-mflag limits matches to 200 matching lines (not file lines). During heavy build failures with dozens of Rust errors, each spanning multiple lines (error declaration, location, notes), this truncates error output unexpectedly.The fallback
tail -n 200provides partial mitigation but doesn't solve the grep truncation. Either:
- Remove the
-m 200limit to capture all matching error patterns, or- If truncation is intentional, add a comment explaining the limit and document the trade-off.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release.yml(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build and Release Dash Evo Tool (linux-arm64, ubuntu-22.04-arm, aarch64-unknown-linux-gnu, arm64-...
- GitHub Check: Build and Release Dash Evo Tool (Windows, windows-latest, x86_64-pc-windows-msvc, windows, .exe)
- GitHub Check: Build and Release Dash Evo Tool (macos-arm64, macos-latest, aarch64-apple-darwin, arm64-mac)
- GitHub Check: Build and Release Dash Evo Tool (linux-x86_64, ubuntu-22.04, x86_64-unknown-linux-gnu, x86_64-linux)
- GitHub Check: Build and Release Dash Evo Tool (macos-x86_64, macos-13, x86_64-apple-darwin, x86_64-mac)
- GitHub Check: Test Suite
- GitHub Check: Clippy
🔇 Additional comments (4)
.github/workflows/release.yml (4)
44-48: Verify obsolete Windows GNU protoc and libsql steps are intentionally disabled or should be removed.The matrix target changed from
x86_64-pc-windows-gnutox86_64-pc-windows-msvc. However, the workflow still contains steps that conditionally target the old GNU variant:
- Line 109-112: "Install protoc (Windows)" targets
x86_64-pc-windows-gnu- Line 139-140: "Windows libsql" targets
x86_64-pc-windows-gnuThese steps will never execute with the new matrix. Confirm whether they should be removed or maintained for future GNU support.
56-67: LGTM!Windows-specific setup is well-structured: properly configures long paths, sets Cargo/Rustup homes, and persists environment variables for downstream steps. PowerShell usage is appropriate here.
114-125: LGTM!Windows MSVC protoc installation is properly structured: correct error handling, version consistency (v25.2), and proper PATH configuration using
$env:GITHUB_PATH. Implementation follows PowerShell best practices.
176-188: LGTM!Platform-specific packaging is well-structured: Windows uses PowerShell with proper error handling and file cleanup, Linux/macOS use standard
zipcommand. Packaging logic correctly targets appropriate shells and maintains consistent artifact naming.
| shell: bash | ||
| run: | | ||
| cargo build --release --target ${{ matrix.target }} | ||
| set -euo pipefail | ||
| echo "::group::Cargo build output (${{ matrix.target }})" | ||
| cargo build --release --target ${{ matrix.target }} 2>&1 | tee build-${{ matrix.platform }}.log | ||
| echo "::endgroup::" | ||
| mv target/${{ matrix.target }}/release/dash-evo-tool${{ matrix.ext }} dash-evo-tool/dash-evo-tool${{ matrix.ext }} | ||
| env: | ||
| CC_x86_64_pc_windows_gnu: x86_64-w64-mingw32-gcc | ||
| AR_x86_64_pc_windows_gnu: x86_64-w64-mingw32-ar | ||
| CFLAGS_x86_64_pc_windows_gnu: "-O2" | ||
| RUST_BACKTRACE: "1" | ||
| BLST_PORTABLE: ${{ matrix.target == 'x86_64-pc-windows-gnu' && '1' || '' }} |
There was a problem hiding this comment.
🧩 Analysis chain
Verify bash shell compatibility with Windows paths and remove dead BLST_PORTABLE condition.
Two concerns:
-
Shell compatibility on Windows:
shell: bashon line 143 relies on Git Bash being available. While this typically works, verify that Cargo/Rust paths and build artifacts are correctly resolved on Windows. Consider using PowerShell for Windows and bash for Unix platforms for better path handling consistency. -
Dead code: Line 155 sets
BLST_PORTABLEconditionally onx86_64-pc-windows-gnu, but this target is no longer in the matrix (switched to MSVC on line 46). This condition will never be true. Remove the condition or update it if BLST_PORTABLE is needed for the MSVC target.
🏁 Script executed:
#!/bin/bash
# Examine the GitHub Actions workflow file structure
cat .github/workflows/release.yml | head -60
echo "---"
# Show lines around 143-155 and context
sed -n '130,165p' .github/workflows/release.ymlLength of output: 3505
🏁 Script executed:
#!/bin/bash
# Check for x86_64-pc-windows-gnu in the matrix
echo "=== Checking for x86_64-pc-windows-gnu in workflow ==="
grep -n "x86_64-pc-windows-gnu" .github/workflows/release.yml
echo ""
echo "=== Checking matrix strategy section ==="
sed -n '/strategy:/,/^[^ ]/p' .github/workflows/release.yml | head -50Length of output: 2093
🏁 Script executed:
#!/bin/bash
# Check all uses of matrix.target in the workflow
echo "=== All matrix.target references ==="
grep -n "matrix\.target" .github/workflows/release.ymlLength of output: 864
🏁 Script executed:
#!/bin/bash
# Check if BLST_PORTABLE is used elsewhere
echo "=== All BLST_PORTABLE references ==="
grep -n "BLST_PORTABLE" .github/workflows/release.ymlLength of output: 126
🏁 Script executed:
#!/bin/bash
# Get full matrix definition
echo "=== Extracting matrix definition ==="
python3 << 'EOF'
import re
with open('.github/workflows/release.yml', 'r') as f:
content = f.read()
# Find the matrix section
matrix_match = re.search(r'strategy:\s*matrix:(.*?)(?=\n[^ ]|\Z)', content, re.DOTALL)
if matrix_match:
matrix_section = matrix_match.group(1)
lines = matrix_section.split('\n')
for i, line in enumerate(lines[:40], 1): # First 40 lines of matrix
print(f"{i:3}: {line}")
EOFLength of output: 932
🏁 Script executed:
#!/bin/bash
# Check build step location and context
echo "=== Build step and surrounding context ==="
sed -n '135,160p' .github/workflows/release.yml | cat -nLength of output: 1468
🏁 Script executed:
#!/bin/bash
# Check shell: bash usage in workflow
echo "=== All shell specifications ==="
grep -n "shell:" .github/workflows/release.ymlLength of output: 192
🏁 Script executed:
#!/bin/bash
# Check matrix platform values
echo "=== Checking matrix.platform usage ==="
grep -n "matrix\.platform" .github/workflows/release.ymlLength of output: 1044
Remove dead code targeting x86_64-pc-windows-gnu—this target is no longer in the matrix.
The BLST_PORTABLE condition at line 155 references x86_64-pc-windows-gnu, which has been replaced with x86_64-pc-windows-msvc in the matrix. This condition will never be true and should be removed.
Additionally, clean up other dead code targeting the old gnu target:
- Environment variables at lines 151–153:
CC_x86_64_pc_windows_gnu,AR_x86_64_pc_windows_gnu,CFLAGS_x86_64_pc_windows_gnu - The "Windows libsql" step at line 139, which is gated on the removed target
- The "Install protoc (Windows libsql)" step at line 109
The shell: bash at line 143 is appropriate and works correctly on GitHub Actions Windows runners.
4 participants
Summary by CodeRabbit
New Features
Improvements
Chores