Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Import to Wallet GUI #650

Closed
wants to merge 7 commits into from
Closed

Conversation

KolbyML
Copy link
Contributor

@KolbyML KolbyML commented Aug 8, 2022

resolves #19

This PR does a few things

  • Refactors importmulti and importdescripters RPC code
  • Adds functions to interfaces
  • Adds GUI's for importpubkey, importprivkey, importaddress, importmulti, and importdescriptors RPCs
  • Adds QT Tests for all 5 import GUI's

Things to get this merged

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
image image
Import Private Key Dialog Import Address Dialog
image image
Import Multi Dialog scriptPubKey Tab Import Multi Dialog Descriptor Tab
image image
Import Descriptors Dialog
image

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 KolbyML force-pushed the import-gui-k branch 5 times, most recently from adf1a83 to 46fea25 Compare August 8, 2022 20:43
Copy link
Member

@achow101 achow101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Note: @KolbyML is my Summer of Bitcoin mentee.

src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved
src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved
src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved
src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved
src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved
src/wallet/imports.h Outdated Show resolved Hide resolved
src/qt/importdialog.cpp Outdated Show resolved Hide resolved
src/qt/importentry.cpp Outdated Show resolved Hide resolved
src/qt/importdialog.cpp Outdated Show resolved Hide resolved
src/qt/importdialog.cpp Outdated Show resolved Hide resolved
@KolbyML KolbyML force-pushed the import-gui-k branch 18 times, most recently from aaa73da to 91ab27a Compare August 9, 2022 21:25
@w0xlt
Copy link
Contributor

w0xlt commented Aug 9, 2022

Concept ACK.
Perhaps it would be better to split each menu item into its own PR.

@KolbyML KolbyML force-pushed the import-gui-k branch 3 times, most recently from bc90775 to 3662ce8 Compare August 9, 2022 23:02
Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

@pablomartin4btc
Copy link
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
Copy link
Contributor Author

KolbyML commented Sep 22, 2023

@pablomartin4btc sounds good I will try and get my 2 PR's rebased

@hebasto
Copy link
Member

hebasto commented Sep 22, 2023

@KolbyML

Suggesting to convert this PR to a draft while CI fails.

@KolbyML KolbyML marked this pull request as draft September 23, 2023 05:56
@DrahtBot
Copy link
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
Copy link
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
Copy link
Contributor Author

KolbyML commented Mar 20, 2024

I no longer have time to work on this so as the bot suggests I am going to close this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Import dialog
9 participants