after that is done I will split the GUI PR into (importpubkey, importprivkey, importaddress), importmulti, importdescriptors each PR will have its respective tests. I will rebase to remove the commit for the refactor in those PR’s it is in this PR so people can test. This is for easier code review
The dialogs are located under File -> Import to Wallet…
The options vary based on what is supported with your specific wallet type.
Dialogs are Located
Import Public Key Dialog
Import Private Key Dialog
Import Address Dialog
Import Multi Dialog scriptPubKey Tab
Import Multi Dialog Descriptor Tab
Import Descriptors Dialog
For Range before I had a lineedit with placeholders begin and end, @achow101 suggested I used QSpinBox, but it doesn’t have placeholder text. So Currently if both are default value it counts as no input. It would look very nice if I implemented a custom QAbstractSpinBox with placeholder text, but I am not sure if it is overkill for this PR.
KolbyML force-pushed
on Aug 8, 2022
KolbyML force-pushed
on Aug 8, 2022
KolbyML force-pushed
on Aug 8, 2022
in
src/wallet/rpc/backup.cpp:928
in
f718e24693outdated
In 3a75af09dbb6c182a86ce71b15fe0824d3c68ac5 “qt: implemented importpubkey, importprivkey, and importaddress GUI’s”
Since this is almost entirely copied from the RPC, I think this should be refactored. It would allow the both the RPC and the GUI to behave the same way with the locking - this implementation has us lock and release in each of the function calls, whereas the RPC holds the lock throughout. Additionally, doing so would let us expose fewer of these functions in the wallet interface.
in
src/qt/importdialog.cpp:193
in
3a75af09dboutdated
192+ // Add the wpkh script for this key if possible
193+ if (pubkey.IsCompressed()) {
194+ walletModel->wallet().ImportScripts({GetScriptForDestination(WitnessV0KeyHash(vchAddress))},
195+ 0 /* timestamp */);
196+ }
197+ }
In 3a75af09dbb6c182a86ce71b15fe0824d3c68ac5 “qt: implemented importpubkey, importprivkey, and importaddress GUI’s”
This is also copied from the RPC, I think it could be refactored as well.
For handling the errors, the refactored functions could throw the same kinds of exceptions that were defined for importmulti and importdescriptors, and this GUI code can catch those and turn them into the error message boxes as necessary.
in
src/qt/importdialog.cpp:579
in
c71f418f18outdated
715+ // If the last entry is about to be removed add an empty one
716+ if (ui->entries->count() == 1)
717+ addEntryDescriptors();
718+
719+ entry->deleteLater();
720+}
In c71f418f18000f537871f57a42ec76e0a25941e9 “qt: implemented importmulti and importdescriptors GUI’s”
These two functions are basically the same. I think you could just collapse them into a single removeEntry function. To figure which addEntry to call, you can look at entry’s entryPage member (which will need to be made public, or a getter added).
In c71f418f18000f537871f57a42ec76e0a25941e9 “qt: implemented importmulti and importdescriptors GUI’s”
These two functions are basically the same. You could collapse it into a single addEntry function that takes an EntryPage as a parameter to be passed to the ImportEntry constructor.
In c71f418f18000f537871f57a42ec76e0a25941e9 “qt: implemented importmulti and importdescriptors GUI’s”
Compiler warning here, also below for importDescriptors:
0qt/importdialog.cpp: In member function ‘void ImportDialog::on_Accept_clicked()’:
1qt/importdialog.cpp:496:31: warning: comparison of integer expressions of different signedness: ‘int’ and ‘std::vector<resultpdi>::size_type’ {aka ‘long unsigned int’} [-Wsign-compare]
2 496 | for (int i = 0; i < response.size(); i++) {
3 | ~~^~~~~~~~~~~~~~~~~
In c71f418f18000f537871f57a42ec76e0a25941e9 “qt: implemented importmulti and importdescriptors GUI’s”
This rescan check boilerplate could be deduplicated and placed at the top of the function. Since some of this is not checked for some of the imports, you can guard it with an if that checks the enum to determine whether the check should occur.
In 3a75af09dbb6c182a86ce71b15fe0824d3c68ac5 “qt: implemented importpubkey, importprivkey, and importaddress GUI’s”
Fetching the key and label are things that are shared for the pubkey, address, and privkey imports. This could also be deuplicated and placed at the top of the function, with the extraction of the text done only if the enum matches one of these three.
achow101
commented at 8:56 pm on August 8, 2022:
member
While the imports mostly work, I noticed that there is no feedback when it is successful. What I expected to happen was a dialog indicating success, and then the import dialog would close itself too.
I think you should also look into adapting a couple of the importmulti tests to be GUI unit tests that can test these new dialogs. It would be very helpful to have some automated testing of all of the things here rather than trying to do it by hand.
w0xlt
commented at 9:30 pm on August 9, 2022:
contributor
Concept ACK.
Perhaps it would be better to split each menu item into its own PR.
KolbyML force-pushed
on Aug 9, 2022
KolbyML force-pushed
on Aug 9, 2022
KolbyML force-pushed
on Aug 9, 2022
jarolrod
commented at 11:10 pm on August 9, 2022:
member
Awesome first contribution @KolbyML, you’re taking a big swing here! Concept ACK on adding this feature.
First, I would second @w0xlt suggestion of breaking each import into it’s own PR, this makes the changes digestible for reviewers.
Second, please note that any changes to the code outside of src/qt (excluding necessary build changes to build any new ui files) cannot be merged from this repo. Any refactoring changes to code outside of src/qt you want to propose in order to make this gui feature work should be opened up in the main repo: https://github.com/bitcoin/bitcoin
KolbyML force-pushed
on Aug 10, 2022
KolbyML force-pushed
on Aug 10, 2022
luke-jr changes_requested
luke-jr
commented at 2:40 am on August 10, 2022:
member
Import Public Key & Address: This doesn’t work as one would expect, and only makes sense as part of a larger entire-wallet watch-import. So too low-level for a GUI feature.
Import Private Key: This is a footgun. Instead, we should only support sweeping (as a basic end-user option) - and probably via the Receive tab.
Import Multi & Descriptors: Seems a bit weird in the GUI. What does “multi” mean here? But maybe this could be an advanced feature similar to coin control… (hidden by default)
KolbyML force-pushed
on Aug 10, 2022
KolbyML force-pushed
on Aug 10, 2022
KolbyML force-pushed
on Aug 10, 2022
KolbyML force-pushed
on Aug 10, 2022
KolbyML force-pushed
on Aug 10, 2022
KolbyML force-pushed
on Aug 10, 2022
KolbyML force-pushed
on Aug 10, 2022
KolbyML force-pushed
on Aug 10, 2022
KolbyML force-pushed
on Aug 10, 2022
KolbyML force-pushed
on Aug 10, 2022
KolbyML force-pushed
on Aug 10, 2022
KolbyML force-pushed
on Aug 10, 2022
KolbyML force-pushed
on Aug 10, 2022
KolbyML force-pushed
on Aug 10, 2022
KolbyML force-pushed
on Aug 10, 2022
KolbyML force-pushed
on Aug 10, 2022
KolbyML force-pushed
on Aug 11, 2022
KolbyML force-pushed
on Aug 11, 2022
KolbyML force-pushed
on Aug 12, 2022
KolbyML force-pushed
on Aug 12, 2022
hebasto renamed this:
qt, refactor: Add Import to Wallet GUI
Add Import to Wallet GUI
on Aug 15, 2022
hebasto added the label
Wallet
on Aug 15, 2022
KolbyML force-pushed
on Aug 16, 2022
KolbyML force-pushed
on Aug 16, 2022
KolbyML force-pushed
on Aug 17, 2022
KolbyML force-pushed
on Aug 17, 2022
KolbyML force-pushed
on Aug 17, 2022
KolbyML force-pushed
on Aug 17, 2022
KolbyML force-pushed
on Aug 17, 2022
KolbyML force-pushed
on Aug 17, 2022
KolbyML force-pushed
on Aug 17, 2022
KolbyML force-pushed
on Aug 17, 2022
KolbyML force-pushed
on Aug 17, 2022
KolbyML force-pushed
on Aug 17, 2022
KolbyML force-pushed
on Aug 17, 2022
KolbyML force-pushed
on Aug 19, 2022
KolbyML force-pushed
on Aug 19, 2022
KolbyML force-pushed
on Aug 19, 2022
KolbyML force-pushed
on Aug 19, 2022
KolbyML force-pushed
on Aug 19, 2022
KolbyML force-pushed
on Aug 19, 2022
KolbyML force-pushed
on Aug 19, 2022
KolbyML force-pushed
on Aug 19, 2022
KolbyML force-pushed
on Aug 19, 2022
KolbyML force-pushed
on Aug 19, 2022
KolbyML force-pushed
on Aug 19, 2022
KolbyML force-pushed
on Aug 19, 2022
KolbyML force-pushed
on Aug 19, 2022
KolbyML force-pushed
on Aug 19, 2022
KolbyML force-pushed
on Aug 19, 2022
KolbyML force-pushed
on Aug 19, 2022
KolbyML force-pushed
on Aug 19, 2022
KolbyML force-pushed
on Aug 19, 2022
KolbyML force-pushed
on Aug 20, 2022
KolbyML force-pushed
on Aug 20, 2022
KolbyML force-pushed
on Aug 20, 2022
KolbyML force-pushed
on Aug 20, 2022
KolbyML force-pushed
on Aug 20, 2022
KolbyML force-pushed
on Aug 20, 2022
KolbyML force-pushed
on Aug 20, 2022
KolbyML force-pushed
on Aug 20, 2022
KolbyML force-pushed
on Aug 20, 2022
KolbyML force-pushed
on Aug 20, 2022
KolbyML force-pushed
on Aug 20, 2022
KolbyML force-pushed
on Aug 20, 2022
KolbyML force-pushed
on Aug 20, 2022
KolbyML force-pushed
on Aug 20, 2022
KolbyML force-pushed
on Aug 20, 2022
KolbyML force-pushed
on Aug 20, 2022
KolbyML force-pushed
on Aug 20, 2022
KolbyML force-pushed
on Aug 20, 2022
KolbyML force-pushed
on Aug 20, 2022
KolbyML force-pushed
on Aug 20, 2022
KolbyML force-pushed
on Aug 20, 2022
KolbyML force-pushed
on Aug 20, 2022
KolbyML force-pushed
on Aug 24, 2022
KolbyML force-pushed
on Aug 24, 2022
KolbyML force-pushed
on Aug 24, 2022
KolbyML force-pushed
on Aug 24, 2022
KolbyML force-pushed
on Aug 25, 2022
KolbyML force-pushed
on Aug 25, 2022
KolbyML force-pushed
on Aug 25, 2022
KolbyML force-pushed
on Aug 25, 2022
KolbyML force-pushed
on Aug 25, 2022
KolbyML force-pushed
on Aug 25, 2022
KolbyML force-pushed
on Aug 25, 2022
KolbyML force-pushed
on Aug 25, 2022
hebasto
commented at 9:03 am on August 25, 2022:
member
It is not an appropriate usage of CI resources to debug your branch.
@hebasto oh, sorry for doing that. How should I debug the Win64 native [vs2022] linker error I have then going forward? Thank you for letting me know
hebasto
commented at 9:09 am on August 25, 2022:
member
How should I debug the Win64 native [vs2022] linker error I have then going forward? Thank you for letting me know
bitcoin/bitcoin#25929 should fix it
KolbyML
commented at 9:12 am on August 25, 2022:
contributor
How should I debug the Win64 native [vs2022] linker error I have then going forward? Thank you for letting me know
Thank you for letting me know that was driving me crazy. Cause it seemed like my code was fine. I will push my changes, then switch the PR to a draft then, since I think that won’t trigger CI. Worse comes to worse I will just work on a local branch like I was originally before working on the PR.
Edit: switch it to a draft till I am done implementing tests for importmulti/importdescriptors
KolbyML force-pushed
on Aug 25, 2022
hebasto
commented at 9:40 am on August 25, 2022:
member
Edit: switch it to a draft till I am done implementing tests for importmulti/importdescriptors
Just click “Convert to draft” :)
KolbyML marked this as a draft
on Aug 25, 2022
KolbyML force-pushed
on Aug 26, 2022
DrahtBot
commented at 1:27 pm on October 21, 2022:
contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviews
See the guideline for information on the review process.
#bitcoin/bitcoin/22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
DrahtBot added the label
Needs rebase
on Dec 6, 2022
KolbyML force-pushed
on Feb 18, 2023
KolbyML force-pushed
on Feb 19, 2023
KolbyML force-pushed
on Feb 19, 2023
KolbyML marked this as ready for review
on Feb 19, 2023
KolbyML force-pushed
on Feb 20, 2023
KolbyML force-pushed
on Feb 20, 2023
DrahtBot removed the label
Needs rebase
on Feb 20, 2023
KolbyML force-pushed
on Feb 20, 2023
KolbyML force-pushed
on Feb 20, 2023
KolbyML force-pushed
on Feb 21, 2023
KolbyML force-pushed
on Feb 21, 2023
KolbyML force-pushed
on Feb 21, 2023
KolbyML force-pushed
on Feb 21, 2023
KolbyML force-pushed
on Feb 21, 2023
KolbyML force-pushed
on Feb 21, 2023
hernanmarino
commented at 0:11 am on March 27, 2023:
contributor
Concept ACK, I’d really like to see this implemented.
DrahtBot added the label
Needs rebase
on Apr 12, 2023
KolbyML force-pushed
on Apr 12, 2023
KolbyML force-pushed
on Apr 12, 2023
KolbyML force-pushed
on Apr 12, 2023
DrahtBot removed the label
Needs rebase
on Apr 12, 2023
DrahtBot added the label
Needs rebase
on Apr 12, 2023
KolbyML force-pushed
on Apr 12, 2023
DrahtBot removed the label
Needs rebase
on Apr 13, 2023
DrahtBot added the label
Needs rebase
on May 17, 2023
refactor: importmulti and importdescriptors rpc5f0add38db
interfaces: Add functions for import gui9bc001c469
qt: implemented importpubkey, importprivkey, and importaddress GUI's0a5e42a496
qt: implemented importmulti and importdescriptors GUI'sd722316cc0
KolbyML force-pushed
on May 18, 2023
KolbyML force-pushed
on May 18, 2023
DrahtBot added the label
CI failed
on May 18, 2023
KolbyML force-pushed
on May 18, 2023
qt, test: add importmulti testsc29eb07af1
qt, test: add importdescriptors tests4e3be9f752
qt, test: add importpubkey, importprivkey, and importaddress testsb2b00af1d3
KolbyML force-pushed
on May 18, 2023
DrahtBot removed the label
Needs rebase
on May 18, 2023
DrahtBot added the label
Needs rebase
on Aug 17, 2023
DrahtBot
commented at 1:48 pm on August 17, 2023:
contributor
🐙 This pull request conflicts with the target branch and needs rebase.
pablomartin4btc
commented at 11:50 pm on September 21, 2023:
contributor
I’ll review this as soon as I can, in the meantime, could you please link the PR to the existent issue? (Writing any of these keywords somewhere in the description followed by #19 would do the trick, thx!)
KolbyML
commented at 0:40 am on September 22, 2023:
contributor
@pablomartin4btc sounds good I will try and get my 2 PR’s rebased
hebasto
commented at 6:17 pm on September 22, 2023:
member
Suggesting to convert this PR to a draft while CI fails.
KolbyML marked this as a draft
on Sep 23, 2023
DrahtBot
commented at 0:57 am on December 22, 2023:
contributor
There hasn’t been much activity lately and the patch still needs rebase. What is the status here?
Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.
DrahtBot
commented at 0:33 am on March 20, 2024:
contributor
⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?
Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.
KolbyML
commented at 0:37 am on March 20, 2024:
contributor
I no longer have time to work on this so as the bot suggests I am going to close this
KolbyML closed this
on Mar 20, 2024
hebasto added the label
Up for grabs
on Mar 22, 2024
This is a metadata mirror of the GitHub repository
bitcoin-core/gui.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2024-12-03 20:20 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me