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 advanced options on Recover Wallet tab #1376

Merged
merged 7 commits into from May 8, 2019

Conversation

Projects
None yet
4 participants
@guggero
Copy link
Contributor

commented Apr 26, 2019

This PR adds two new "advanced options" to the tab "Recover Wallet":

  • "Account Key Path": Useful if an existing BIP39 wallet should be imported that was created with a different software and used a different derivation path.
  • "Number of keys to generate": Determines how many keys will be generated/derived when the wallet is created. Useful if a wallet should be imported/restored that has been used a lot already.

Preview

Screenshot from 2019-04-30 22-54-01

I plan to add additional PRs with more advanced options in the "Recover Wallet" tab to support different derivation methods used by other wallet software. I hope that's in the general interest of the project since people might want to import their existing wallet rather than create a new one and send funds to it.

@nopara73 nopara73 requested a review from lontivero Apr 28, 2019

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Apr 28, 2019

First of all, I really like how you did the UX. I can really appreciate that someone doesn't add unnecessary complexity. In fact we could even steal this "Show advanced options" UX pattern and could use more heavily. Refer to the issue: #1369

@lontivero can you review the code?

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Apr 28, 2019

One last thing for the UX, the MinGapLimit should have somehow an enforced minimum: AbsoluteMinGapLimit and some enforced maximum, like 1000000(1 million).

@guggero Would you like to work on our recommendations or you would prefer if we take over from here?

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Apr 28, 2019

I hope that's in the general interest of the project since people might want to import their existing wallet rather than create a new one and send funds to it.

Ok, I see we are on the same page here:)

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Apr 28, 2019

Oh, one more thing. We can only monitor bech32 addresses. So the user may get confused if he writes m/44'/... and doesn't see his balance.

Added a note suggestion for this.

@guggero

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

@nopara73 thanks a lot for your feedback!
I'm happy this is a feature you agree with.
I will gladly implement your suggestions. It will be a busy week for me so it might take me a few days.
There seem to be build errors too, so I'll fix them as well.

@guggero guggero force-pushed the guggero:recover-advanced-options branch from 81aeb83 to dfd13f3 Apr 30, 2019

@guggero

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

I've rebased against master and addressed all your comments/suggestions. The test that I broke should now run fine too.
The screenshot shows how it currently looks.

@lontivero

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

I will review this tomorrow.

@@ -69,10 +75,11 @@ public class KeyManager
public bool IsHardwareWallet => EncryptedSecret is null && MasterFingerprint != null;
public HardwareWalletInfo HardwareWalletInfo { get; set; }

private const int AbsoluteMinGapLimit = 21;
public const int AbsoluteMinGapLimit = 21;
public const int AbsoluteMaxGapLimit = 1_000_000;

This comment has been minimized.

Copy link
@lontivero

lontivero May 1, 2019

Contributor

Uhmm... if someone sets the minGapLimit to a 1e6 it could take a long time to derivare that number of keys. Have you measured how much it takes?

This comment has been minimized.

Copy link
@guggero

guggero May 3, 2019

Author Contributor

No, I didn't try it myself. @nopara73 suggested to add the upper bound of 1 million. It seemed large to me too but who am I to disagreee ;-)

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented May 1, 2019

@lontivero @guggero Scream when you guys think it's ready and I'll do some final fixup before merging.

@guggero guggero force-pushed the guggero:recover-advanced-options branch 2 times, most recently from e47dbd9 to 046baf3 May 3, 2019

@guggero

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

Thanks @lontivero for the review!
I addresses most of your comments. Sorry I only found time to do so today.
If you agree with my comments @nopara73, I think the PR is ready to merge. Otherwise I'm happy to change the max value for MinGapLimit and/or use a NumericUpDown.

@lontivero

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

I think what needs to be addressed is #1376 (comment)

@guggero guggero force-pushed the guggero:recover-advanced-options branch from 046baf3 to c6f02b8 May 4, 2019

@guggero

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

@lontivero I have addressed that, there is a TryParseKeyPath function now.
I've also rebased to master.
@nopara73 I think this might be ready to be merged.

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented May 6, 2019

@guggero Awesome! I am on it.

nopara73 added some commits May 6, 2019

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented May 6, 2019

I removed the maxgaplimit from the KeyManager, and made it a UI concept, because I don't want to pay attention to the max gap limit inside the KeyManager: 7c46b4c

It's also not max gap limit, but max min gap limit:)

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented May 6, 2019

I added a custom keypath converter. NBitcoin's doesn't always work in the way we want.

@guggero

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

Cool, thanks a lot!

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented May 6, 2019

What's with the trickering of MinGapLimit +- 1? I don't understand why that's needed.

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented May 6, 2019

What's with the trickering of MinGapLimit +- 1? I don't understand why that's needed.

After we discussed this, it can be merged.

@guggero

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

Because in the KeyManager it's always .Count() < MinGapLimit and the default was 21 before. And at first I named it "number of keys to generate" and 21 wouldn't have been correct.
But with the wording you suggested it's not needed any more.

@guggero

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

I've removed the now unnecessary one-off corrections of MinGapLimit.

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented May 8, 2019

Awesome! Merging. Although we have a feature freeze (#1412), I make an exception for this PR, this looks really safe.

@nopara73 nopara73 merged commit 92b6085 into zkSNACKs:master May 8, 2019

4 checks passed

CodeFactor No issues found.
Details
Wasabi.Linux #20190506.12 succeeded
Details
Wasabi.Osx #20190506.12 succeeded
Details
Wasabi.Windows #20190506.12 succeeded
Details

@guggero guggero deleted the guggero:recover-advanced-options branch May 11, 2019

@guggero

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2019

Thanks for merging so quickly! And thanks a lot for making an exception about the feature freeze :-)

@MaxHillebrand

This comment has been minimized.

Copy link
Contributor

commented May 12, 2019

I found a little bug, not sure if I should open a new issue...

When typing in the mnemonic, and you have a typo, you can not move the cursor back to the misspelled word, neither by mouse clicking, nor by arrow key press.

When you click and select a word, and start typing, then the cursor jumps back to the end of the mnemonic and the letters are then there.

@MaxHillebrand

This comment has been minimized.

Copy link
Contributor

commented May 12, 2019

Since this specific PR is for only the advanced option, I guess it's best to open a new issue.
There is a closed issue with the same bug.

Sorry for spamming this PR...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.