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

feat: import wallet #621

Merged
merged 58 commits into from Sep 8, 2023
Merged

feat: import wallet #621

merged 58 commits into from Sep 8, 2023

Conversation

theborakompanioni
Copy link
Collaborator

@theborakompanioni theborakompanioni commented Jun 5, 2023

Closes #137.

Adds the ability to import an existing wallet in Jam.

  • Customize options (plain input of gaplimit and blockheight)
  • Reset gaplimit to original value
  • Add "back" button
  • Prevent importing if rescan is active
  • Call /display after rescanning finished
  • Test on mainnet

How to test

After checking out the branch, execute npm run regtest:rebuild in order to rebuild your dev environment to get the lastest upstream changes from JM (the "import wallet" endpoint is only on master and not released yet). Otherwise you will get "Error while importing the wallet. Reason: Method Not Allowed" when trying to import a wallet.

@theborakompanioni theborakompanioni added the enhancement New feature or request label Jun 5, 2023
@theborakompanioni theborakompanioni self-assigned this Jun 5, 2023
@theborakompanioni theborakompanioni force-pushed the feat/137-import-wallet branch 3 times, most recently from da937a6 to 860f2d4 Compare July 6, 2023 19:59
@theborakompanioni
Copy link
Collaborator Author

theborakompanioni commented Aug 13, 2023

Hey @editwentyone

This is now functional and can be tested.

Please note, that you have to run npm run regtest:rebuild if you have not done so since JoinMarket-Org/joinmarket-clientserver#1525 has be merged.

From my point of view the only thing that is missing the the blockheight-as-date-picker component (Currently, it is a normal number input). If that is okay with you, I'll tackle this in a follow-up PR.

What is still missing but needed before merging:

  • Show options only with jm backenv version 0.9.10 (or 0.9.10dev) or greater
  • Call /display after rescanning finished
  • Improve or hide "rescan" page
  • Test on mainnet

Also, this has not been tested on mainnet yet.

Any feedback to the current state would be highly appreciated 🙏

@theborakompanioni
Copy link
Collaborator Author

theborakompanioni commented Aug 13, 2023

Follow-up PRs for stuff noticed during development:

@theborakompanioni theborakompanioni force-pushed the feat/137-import-wallet branch 2 times, most recently from df4d666 to 37b171e Compare August 17, 2023 12:58
@theborakompanioni
Copy link
Collaborator Author

theborakompanioni commented Aug 17, 2023

Rebased on master.

This is now "complete" and is ready for review.
Suggestions for wording/phrases improvements (in translation file) would be highly appreciated. But of course, the proper functioning of the new feature should be the main focus. 🙏

I will create issues for following related tasks which will be done in follow-up PRs:

As well as for additional tasks that came up during development:

Addendum: Test on mainnet is not yet done and still to be performed.

@theborakompanioni theborakompanioni marked this pull request as ready for review August 17, 2023 20:12
@climardo
Copy link

👏 congrats. ive been following this pr since i had to do this manually and ive been looking forward to this. going to try and run this to give feedback (im a newb)

@theborakompanioni
Copy link
Collaborator Author

theborakompanioni commented Aug 17, 2023

👏 congrats. ive been following this pr since i had to do this manually and ive been looking forward to this. going to try and run this to give feedback (im a newb)

This is so nice. ANY feedback is highly welcome! You can start with setting up the regtest environment. If you need any help or assistance, just ping me! 🙏

Edit: Sorry that it has taken so long to implement this basic feature.. 😬

@theborakompanioni
Copy link
Collaborator Author

Rebased on master (7a6b920)

@theborakompanioni
Copy link
Collaborator Author

Rebased on master (7a6b920)

Rebased again on current master (4d0479e)

Copy link

@editwentyone editwentyone left a comment

Choose a reason for hiding this comment

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

cACK, on testnet, not main net

@theborakompanioni theborakompanioni removed this from the v0.1.6 - TBD milestone Sep 7, 2023
@theborakompanioni
Copy link
Collaborator Author

With help of some users, this has been tested on mainnet with a moderately used wallets. Rescanning from block 481824 (segwit activation) took around ~8 hours on average (on a raspberry). Wallet has been successfully imported. However, tests were always done on the Bitcoin Core instance, that was already aware of the wallet. It should work on pristine Bitcoin Core instances as well, but we have to wait for feedback to verify.

@theborakompanioni theborakompanioni merged commit 028f321 into master Sep 8, 2023
3 checks passed
@theborakompanioni theborakompanioni deleted the feat/137-import-wallet branch September 8, 2023 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Import wallet using seedphrase
4 participants