fix: QR code not showing on top up#438
Conversation
WalkthroughParsed and validated the funding amount earlier, restructured QR rendering layout to a top-down centered flow, reduced post-QR spacing, routed AddressWithQRCode through the wallet selection/unlock path, and adjusted error-message handling. No public API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant UI as TopUpIdentityScreen
participant W as WalletSelector
participant QR as QR Renderer
rect rgba(230,240,255,0.5)
note over UI: Funding method dispatch
U->>UI: Select AddressWithQRCode
UI->>W: Show wallet selection/unlock (includes AddressWithQRCode)
W-->>UI: Wallet selected/unlocked
end
rect rgba(240,255,240,0.5)
note over UI: Validation then render
UI->>UI: Parse funding_amount → amount_dash
alt amount_dash <= 0.0 or invalid
UI-->>U: AppAction::None (no QR)
else valid
UI->>QR: render_qr_code(amount_dash) (inside top-down centered layout)
QR-->>U: Display QR (spacing 10.0)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
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: 1
🧹 Nitpick comments (6)
src/ui/identities/top_up_identity_screen/mod.rs (2)
491-518: Wallet unlock likely unnecessary for QR-only funding; gate unlock to balance/asset-lock paths.Address-with-QR only needs a receive address; unlocking the wallet may be overkill and adds friction. Keep wallet selection, but skip unlock for AddressWithQRCode.
Apply this diff:
- let (needed_unlock, just_unlocked) = self.render_wallet_unlock_if_needed(ui); - - if needed_unlock && !just_unlocked { - return; - } + if funding_method != FundingMethod::AddressWithQRCode { + let (needed_unlock, just_unlocked) = self.render_wallet_unlock_if_needed(ui); + if needed_unlock && !just_unlocked { + return; + } + }
495-505: Step heading/numbering UX: hide the “Choose the wallet” step when only one wallet auto-selects.render_wallet_selection(ui) already returns whether a choice was presented. Consider using it to increment the step and hide the heading when no user action is needed.
src/ui/identities/top_up_identity_screen/by_wallet_qr_code.rs (4)
26-39: Update core_has_funding_address after importing the address.When Some(false), you import the address but don’t set core_has_funding_address = Some(true), which can cause redundant imports if the funding_address is later reset.
Apply this diff:
.import_address( &receive_address, Some("Managed by Dash Evo Tool"), Some(false), ) .map_err(|e| e.to_string())?; + self.core_has_funding_address = Some(true);
111-121: Minor UX: show inline hint instead of returning early on invalid/zero amount.Right now the UI silently returns None; consider showing a small helper like “Enter a positive amount to generate a QR code.” Keeps the screen informative.
If you prefer, replace the early returns with a label:
- let duffs = self.funding_amount_exact.unwrap_or_else(|| { - (self.funding_amount.parse::<f64>().unwrap_or(0.0) * 1e8).round() as u64 - }); - if duffs == 0 { - return AppAction::None; - } + let duffs = self.funding_amount_exact.unwrap_or_else(|| { + (self.funding_amount.parse::<f64>().unwrap_or(0.0) * 1e8).round() as u64 + }); + if duffs == 0 { + ui.label("Enter a positive amount to generate a QR code."); + return AppAction::None; + }Also applies to: 124-134
77-83: Optional: cache the QR texture per pay_uri to avoid regenerating every frame.Generating and uploading the texture each frame can be costly. Cache TextureHandle keyed by pay_uri on the struct and reuse until pay_uri changes.
I can propose a small struct-level cache (last_qr_uri: String, last_qr_tex: TextureHandle) and the minimal glue code if you want.
86-86: Spacing consistency.You changed spacing after the QR from 15.0 to 10.0. Consider extracting constants for common paddings to keep screens consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/ui/identities/top_up_identity_screen/by_wallet_qr_code.rs(3 hunks)src/ui/identities/top_up_identity_screen/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Error handling uses
thiserrorfor structured error types.
Files:
src/ui/identities/top_up_identity_screen/mod.rssrc/ui/identities/top_up_identity_screen/by_wallet_qr_code.rs
🧬 Code graph analysis (1)
src/ui/identities/top_up_identity_screen/by_wallet_qr_code.rs (2)
src/ui/identities/top_up_identity_screen/mod.rs (1)
ui(419-553)src/ui/identities/add_new_identity_screen/mod.rs (1)
ui(940-1107)
⏰ 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). (1)
- GitHub Check: Clippy
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/ui/identities/top_up_identity_screen/by_wallet_qr_code.rs (1)
127-133: Use integer Duffs end-to-end; f64 and 4-decimal QR formatting can misstate funds.Parsing
self.funding_amountasf64and later formatting the QR amount to 4 decimals risks under/overpayment. Switch to integer Duffs (8 decimals) throughout this flow and reject zero. Also update the call at Line 138 and the QR URI builder to format exactly 8 fractional digits. This was already flagged previously; repeating here for this hunk.Apply this local diff for this hunk:
- let Ok(amount_dash) = self.funding_amount.parse::<f64>() else { - return AppAction::None; - }; - - if amount_dash <= 0.0 { - return AppAction::None; - } + let duffs = self.funding_amount_exact.unwrap_or_else(|| { + (self.funding_amount.parse::<f64>().unwrap_or(0.0) * 1e8).round() as u64 + }); + if duffs == 0 { + return AppAction::None; + }Changes required outside this hunk (for clarity, not a patch):
// 1) Update signature (Line 12) and use the imported Ui: fn render_qr_code(&mut self, ui: &mut Ui, amount_duffs: u64) -> Result<(), QRError> { /* ... */ } // 2) Update call (Line 138): if let Err(e) = self.render_qr_code(ui, duffs) { /* ... */ } // 3) Build URI with 8 decimals (Line 75 region): let major = amount_duffs / 100_000_000; let minor = amount_duffs % 100_000_000; let pay_uri = format!("{}?amount={}.{:08}", address.to_qr_uri(), major, minor); // 4) Button label and toast text (Lines 91, 104): if ui.button("Copy Payment URI").clicked() { /* ... */ } // ... ui.label("Payment URI copied to clipboard.");src/ui/identities/top_up_identity_screen/mod.rs (1)
312-312: Amount input: follow-up on integer Duffs parsing and 8-decimal limit.The text field is fine, but downstream logic still derives
funding_amount_exactviaf64(Lines 315–317), which can introduce rounding errors. Parse directly to integer Duffs and cap to 8 decimals; show a hint for expected precision. Reiterating prior feedback for consistency across the screen.Apply within this hunk (optional UX):
- ui.add(egui::TextEdit::singleline(&mut self.funding_amount).desired_width(100.0)) + ui.add( + egui::TextEdit::singleline(&mut self.funding_amount) + .desired_width(120.0) + .hint_text("e.g. 1.23456789") + )Outside this hunk, replace the
f64conversion (Lines 315–317) with an exact parser:fn parse_dash_to_duffs(s: &str) -> Option<u64> { let s = s.trim(); if s.is_empty() { return None; } let mut parts = s.split('.'); let major = parts.next()?.parse::<u64>().ok()?; let minor = match parts.next() { None => 0u64, Some(frac) => { if parts.next().is_some() { return None; } // multiple dots let mut frac = frac.trim().chars().take(8).collect::<String>(); while frac.len() < 8 { frac.push('0'); } frac.parse::<u64>().ok()? } }; major.checked_mul(100_000_000)?.checked_add(minor) } // Then: self.funding_amount_exact = parse_dash_to_duffs(&self.funding_amount);
🧹 Nitpick comments (1)
src/ui/identities/top_up_identity_screen/by_wallet_qr_code.rs (1)
8-8: Prefer structured errors overString; avoidunwrap/expectin UI path.
render_qr_codereturnsResult<(), String>and uses severalunwrap/expecton locks/RPC calls. Adopt athiserrorerror type and propagate via?to meet repo guidelines and avoid UI-thread panics.Example (outside this hunk):
use thiserror::Error; #[derive(Debug, Error)] pub enum QRError { #[error("No wallet selected")] NoWallet, #[error("Core RPC error: {0}")] CoreRpc(#[from] dash_sdk::dashcore_rpc::Error), #[error("Core client lock poisoned")] CoreLockPoisoned, #[error("QR generation failed")] QrGen, #[error("Clipboard: {0}")] Clipboard(String), } fn render_qr_code(&mut self, ui: &mut Ui, amount_duffs: u64) -> Result<(), QRError> { // replace `.expect("...")` with `.map_err(|_| QRError::CoreLockPoisoned)?;` // replace `.map_err(|e| e.to_string())?` with `?` using the `From` above // map QR errors to `QRError::QrGen` Ok(()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/ui/identities/top_up_identity_screen/by_wallet_qr_code.rs(3 hunks)src/ui/identities/top_up_identity_screen/mod.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Error handling uses
thiserrorfor structured error types.
Files:
src/ui/identities/top_up_identity_screen/mod.rssrc/ui/identities/top_up_identity_screen/by_wallet_qr_code.rs
🧠 Learnings (1)
📚 Learning: 2025-03-19T14:02:15.588Z
Learnt from: asalomeri
PR: dashpay/dash-evo-tool#196
File: src/aaaa:16-18
Timestamp: 2025-03-19T14:02:15.588Z
Learning: For Bitcoin transactions in this project, amounts should be parameterized instead of hardcoded, and proper error handling should be used with `?` instead of `unwrap()`.
Applied to files:
src/ui/identities/top_up_identity_screen/by_wallet_qr_code.rs
🧬 Code graph analysis (2)
src/ui/identities/top_up_identity_screen/mod.rs (3)
src/ui/tokens/tokens_screen/mod.rs (2)
ui(2469-2702)error_message(2852-2854)src/ui/identities/register_dpns_name_screen.rs (3)
ui(176-404)error_message(432-434)error_message(487-507)src/ui/identities/add_existing_identity_screen.rs (2)
ui(492-587)error_message(467-469)
src/ui/identities/top_up_identity_screen/by_wallet_qr_code.rs (1)
src/ui/identities/top_up_identity_screen/mod.rs (1)
ui(420-558)
⏰ 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). (1)
- GitHub Check: Clippy
🔇 Additional comments (7)
src/ui/identities/top_up_identity_screen/by_wallet_qr_code.rs (2)
135-137: Layout refactor: good fix for early-render freeze.Moving QR rendering into a top-down, centered layout after amount validation prevents premature QR generation and aligns with the PR’s goal.
86-86: Spacing tweak LGTM.Reducing the post-error space to 10.0 is fine and keeps the QR area tighter.
src/ui/identities/top_up_identity_screen/mod.rs (5)
28-28: Imports and Color32 usage LGTM.
Color32is now used for error labels;ComboBox,ScrollArea, andUiare correctly imported.
146-152: Helpful UX when only one wallet is loaded.Auto-selecting and clearly labeling the chosen wallet improves clarity.
158-161: Clear error when no wallets are available.Good use of a red label to block progress with actionable guidance.
492-495: Including QR funding in the wallet-selection/unlock gate is correct.This aligns the QR flow with other funding methods and prevents rendering before a usable wallet is ready.
535-537: Centralized error rendering LGTM.Showing
self.error_messageonce at the bottom avoids duplicated messaging in subviews.
2 participants
QR code was not showing up in the Top Up section because we tried to show it before we asked for the funding amount, making the entire screen "stuck"
This did not affect the QR code in the "New identity" screen
Summary by CodeRabbit
New Features
Bug Fixes
Style