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

minor fixes #221

Merged
merged 12 commits into from
Jul 14, 2020
Merged

minor fixes #221

merged 12 commits into from
Jul 14, 2020

Conversation

stepansnigirev
Copy link
Collaborator

@stepansnigirev stepansnigirev commented Jul 13, 2020

Fixes:

  • If device is set to "Other" it should still have process_psbt function
  • mnemonic is undefined error in adding keys for a device
  • show proper error messages when combine failed due to server error
  • allow pasting / scanning / uploading final raw transactions
  • extra amount check in new tx screen (0.2 sat is a wrong amount)
  • generic device

@stepansnigirev stepansnigirev changed the title other device should not break stuff minor fixes Jul 14, 2020
@@ -11,6 +11,10 @@ def __init__(self, name, alias, device_type, keys, fullpath, manager):
self.fullpath = fullpath
self.manager = manager

def create_psbts(self, base64_psbt, wallet):
Copy link
Contributor

Choose a reason for hiding this comment

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

Works :) But maybe if no method is available, we should show the PSBT/ a button to copy it and tell the user to use the paste transaction button to do the signing? Right now it just brings a popup with no options. You can just take the copy PSBT functionality from the Raw PSBT section in the transaction details.

Copy link
Collaborator Author

@stepansnigirev stepansnigirev Jul 14, 2020

Choose a reason for hiding this comment

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

That's the next thing I was thinking about. I thought about adding a "Generic wallet" option that would include QR code with base64 psbt, copy-text and download file - all with the same base64 psbt. Especially taking into account that "Other" option is not available anymore in the import device screen - we show an error there if the device type is not selected.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds even better :)

Copy link
Contributor

@ben-kaufman ben-kaufman left a comment

Choose a reason for hiding this comment

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

I think I tested most/ all cases, works great :) I guess the Generic device can be either here or in its own PR if you want to keep this to bug fixes.

Co-authored-by: benk10 <ben.kaufman10@gmail.com>
@ben-kaufman
Copy link
Contributor

Oh, just 2 suggestions for changes which feel too small for another PR:

  • Show the Pacman loading animation on creating a hot wallet device, since it too does some lengthy communication with Bitcoin Core.
  • Increment the fee steps to 0.5 instead of 0.25 in the send screen, so it doesn't start too small.

@stepansnigirev
Copy link
Collaborator Author

I think I tested most/ all cases, works great :) I guess the Generic device can be either here or in its own PR if you want to keep this to bug fixes.

Working on it already :)

Copy link
Contributor

@ben-kaufman ben-kaufman left a comment

Choose a reason for hiding this comment

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

Works great!

@stepansnigirev stepansnigirev merged commit ef65bf2 into cryptoadvance:master Jul 14, 2020
@stepansnigirev stepansnigirev deleted the fix-other-device branch July 18, 2020 10:09
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.

2 participants