fix: top up with qr code wasnt working#410
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the QR code top-up flow by displaying the QR step properly, importing the Dash Core address dynamically, and validating the funding amount before rendering the QR.
- Display “AddressWithQRCode” step with an info tooltip
- Replace
core_has_funding_addressflag with dynamicget_address_infocheck and import - Validate and handle invalid or zero amounts before calling
render_qr_code
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/ui/identities/top_up_identity_screen/mod.rs | Adds UI block for the QR code funding method and tooltip |
| src/ui/identities/top_up_identity_screen/by_wallet_qr_code.rs | Refactors QR code rendering: dynamic import, amount checks, and error handling |
Comments suppressed due to low confidence (2)
src/ui/identities/top_up_identity_screen/by_wallet_qr_code.rs:32
- [nitpick] The variable name
infois ambiguous. Consider renaming it toaddress_infofor clarity.
.get_address_info(&receive_address)
src/ui/identities/top_up_identity_screen/by_wallet_qr_code.rs:53
- [nitpick] The error message "No wallet selected" may not provide sufficient guidance to the user. Consider making it more descriptive, e.g., "Please select a wallet to generate a QR code."
return Err("No wallet selected".to_string());
| "The selected wallet will be used to:\n\n\ | ||
| • Generate a receive address for the QR code\n\ | ||
| • Hold the private keys needed to create the asset lock transaction\n\ | ||
| • Sign and broadcast the transaction to top up your identity\n\n\ | ||
| The wallet must have control over the funds to create the asset lock \ | ||
| that credits your identity on the Dash Platform.", |
There was a problem hiding this comment.
[nitpick] The long tooltip string makes the UI code verbose and harder to read. Consider extracting this text into a named constant or external resource for clarity and easier localization.
| "The selected wallet will be used to:\n\n\ | |
| • Generate a receive address for the QR code\n\ | |
| • Hold the private keys needed to create the asset lock transaction\n\ | |
| • Sign and broadcast the transaction to top up your identity\n\n\ | |
| The wallet must have control over the funds to create the asset lock \ | |
| that credits your identity on the Dash Platform.", | |
| TOOLTIP_TEXT, |
| .app_context | ||
| .core_client | ||
| .read() | ||
| .expect("Core client lock was poisoned"); |
There was a problem hiding this comment.
Using expect on a lock acquisition can cause a panic if the lock is poisoned. Consider propagating this error gracefully or handling lock poisoning to avoid unexpected crashes.
| .expect("Core client lock was poisoned"); | |
| .map_err(|_| "Core client lock was poisoned".to_string())?; |
* fix: top up with qr code wasnt working (#410) * fix: top up with qr code wasnt working * fmt and clippy * coderabbit * fix: dash-qt path autodetection fails (#409) * fix: dash-qt path not detected correctly * fix: correct handling of dash-qt path clear button * chore: copilot review * fix: change of wallet on top up does not update the address (#414) * build(deps): upgrade build dependencies (egui 0.32.0, rusqlite 0.37, 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 selector (#408) * 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> * fix: settings are read from db 20 times a second (#420) * feat: wallet unlock on wallet screen (#415) * refactor: Amount type and AmountInput component (#417) * 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 * test: run tests using github actions and fix failing tests (#422) * fix: failing tests * gha: run tests * fix: tests fail due to lack of .env file * fix: incorrect decimals handling on token create, mint and burn screens (#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 (#423) * feat: add network field to identity structures * fix * feat: add display for private key in both WIF and Hex formats (#424) * 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 in .env (#425) * feat: allow setting ZMQ URI * chore: rename zmq_endpoint to core_zmq_endpoint * fix: failing test in token screen needed update * fix: update CI to use rust version 1.88 to match rust-toolchain * feat: better display in key selector (#429) * refactor: unified confirmation dialog (#413) * 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 (#426) * feat: nicer contract chooser panel * fmt * clippy * fix: token purchasability was not refreshing unless app restart (#432) * fix: token action buttons alignment * feat: nicer expanding tabs in token creator (#431) * feat: nicer expanding tabs in token creator * fix * feat: implement new ConfirmationDialog component throughout app (#430) * feat: implement new ConfirmationDialog component throughout app * fix: remove backup files * fix: confirmation dialog on withdrawal screen always showing "loading" * feat: Dash Masternode List and Quorum Viewer (#428) * 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> * chore: bump version to 1.0.0-dev (#439) The 1.0-dev branch builds were still reporting as 0.9.0 * feat: add left panel button labels and create contract subscreen chooser panel (#441) * feat: add left panel button labels and create contract subscreen chooser panel * fmt * feat: clarify identity index description for identity creation * fix: screen button labels display hard to see in dark mode * feat: left panel scroll (#443) * feat: left panel scroll * clippy * feat: add existing identity by wallet + identity index (#444) * 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 (#445) * 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 and top-up via QR code handling (#447) * fix: identity creation handling * cleanup * cleanup * fix identity topup too * remove note * fix: keyword search required new platform version plus error handling and UI updates (#449) * fix: keyword search not displaying errors * update platform version and keyword search cleanup * fix * fix: handle decimals and fix schedules on price and purchase screens (#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> * fix: showing used asset locks as unused (#448) * chore: remove unused task sender * chore: update to Platform V10 * feat: GroveSTARK ZK proofs (#450) * 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 * chore: update grovestark rev * chore: bump version * update platform rev * clippy * fix * fix * fix release * fix release * fix release --------- Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: QuantumExplorer <quantum@dash.org> Co-authored-by: thephez <thephez@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
There were a few issues.
It wasn't even displaying to begin with because of an early return.
Then it was automatically saying "No wallet selected"
Then the QR code wasn't working - it had Dash Core sending funds to itself.