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

multi: error if restore seed/pubkey already exists for decred wallet #15

Closed

Conversation

ukane-philemon
Copy link

@ukane-philemon ukane-philemon commented Aug 15, 2024

Issue Number (if Applicable): Fixes #

Description

Check and error if user attempts to create wallet with existing seed/xpub key.

Pull Request - Checklist

  • Initial Manual Tests Passed
  • Double check modified code and verify it with the feature/task requirements
  • Format code
  • Look for code duplication
  • Clear naming for variables and methods

JoeGruffins and others added 23 commits July 12, 2024 19:01
…ios and macos (cake-tech#1240)

* change cw_decred from package to plugin

* add libdcrwallet dependency and link library for android, ios and macos

* remove spvwallet, make some libdcrwallet fns async, light refactor

* libdcrwallet: use json payload returns

* use specific libwallet commit hash

* decred: fix Rename wallet.

---------

Co-authored-by: JoeGruff <[email protected]>
This allows a persistent peer to be unset, falling back to decred
seeders.
-  move hasRescan method to WalletBase and implement for decred

Signed-off-by: Philemon Ukane <[email protected]>
@ukane-philemon
Copy link
Author

TODO: Update build_decred.sh scripts for android, ios, and macos when decred/libwallet#8 has been merged and taged.

Copy link
Owner

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

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

If I cause it to error then try again it seems to multiply the amount of times I have to confirm on this screen:
image

Comment on lines 52 to 53
final w = await openWallet(walletName, walletPassword);
return credentials.pubkey == w.pubkey;
Copy link
Owner

Choose a reason for hiding this comment

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

Should we close the wallet afterwards?

Copy link
Author

Choose a reason for hiding this comment

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

We risk closing a currently used or previously loaded wallet. IMO, it doesn't hurt to have all decred wallets loaded(in libwallet).
By the time users need them in cake, it wouldn't be an issue.

Copy link
Author

@ukane-philemon ukane-philemon Aug 19, 2024

Choose a reason for hiding this comment

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

Or, better still, we could extend the implementation method to libwallet, which can be reused in other projects. That way we can close the wallets we know were not previously loaded in libwallet.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not really a fan of the map in the first place. I would like to close the wallet yeah. Also don't want to make any huge changes to libwallet when it seems to be working well atm. Let me think...

Copy link
Owner

Choose a reason for hiding this comment

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

Looking around, it doesn't look like they ever close wallets. So maybe it's fine...

Could you leave a message "TODO: Consider closing wallets."? And we will not worry about it for now.

@ukane-philemon
Copy link
Author

If I cause it to error then try again it seems to multiply the amount of times I have to confirm on this screen: image

I've noticed something wonky with that confirmation screen(on mac). If you click okay, it takes the user back to the previous screen but remains on the screen until user clicks outside the modal.

Let me know if you want me to look into it. The correct behaviour is to hide the modal once the use clicks ok.

@JoeGruffins
Copy link
Owner

Let me know if you want me to look into it. The correct behaviour is to hide the modal once the use clicks ok.

If it's their bug then don't worry about it.

Can you just add the close wallet TODO message for me? Then I think this is good. Still thinking about the libwallet changes but I think this pr is fine without them.

@JoeGruffins
Copy link
Owner

Thanks! Added to the main pr and testing pr.

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

Successfully merging this pull request may close these issues.

3 participants