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

Implement Import TX to Broadcaster, Export TX to TransactionViewer, Import Hardware Wallet #1663

Merged
merged 21 commits into from Jul 4, 2019

Conversation

@nopara73
Copy link
Collaborator

nopara73 commented Jun 29, 2019

  • Implement import transaction to Transaction Broadcaster
  • Implement import hardware wallet to Load Hardware Wallet
  • Implement export transaction to Transaction View
  • Test if PSBT binary import is properly parsed and broadcasted
  • Test complete Coldcard workflow on Windows.
  • Test complete Coldcard workflow on Linux.
  • Test complete Coldcard workflow on OSX.

Resolves #1473
Resolves #1417

@nopara73 nopara73 requested a review from molnard Jun 29, 2019
@nopara73

This comment has been minimized.

Copy link
Collaborator Author

nopara73 commented Jun 29, 2019

@danwalmsley @molnard I cannot place a button properly to the TransactionViewer. Can you do it? It should look the same as in the TransactionBroadcaster.

<Button Command="{Binding ExportBinaryPsbtCommand}" Content="Export Binary PSBT" />

@nopara73 nopara73 added this to In Progress in v1.1.6 via automation Jun 29, 2019
@nopara73 nopara73 changed the title Implement Import Transaction to Broadcaster Implement Import TX to Broadcaster, Export TX to TransactionViewer, Import Hardware Wallet Jun 30, 2019
nopara73 added 4 commits Jul 1, 2019
@nopara73

This comment has been minimized.

Copy link
Collaborator Author

nopara73 commented Jul 1, 2019

This PR is ready. @MaxHillebrand Can you test it on Linux I suppose?

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

nopara73 commented Jul 1, 2019

It doesn't work. When trying to make CC to sign, it throws me failure, but the error is so fast I cannot read it.

@MaxHillebrand

This comment has been minimized.

Copy link
Collaborator

MaxHillebrand commented Jul 2, 2019

It doesn't work. When trying to make CC to sign, it throws me failure, but the error is so fast I cannot read it.

Not sure which exact error you have, but I think CC also shows an error for a split second when it is not the binary PSBT.
I'll test it later today.

nopara73 added 4 commits Jul 2, 2019
@nopara73

This comment has been minimized.

Copy link
Collaborator Author

nopara73 commented Jul 2, 2019

Update

The problem is that Coldcard exports the masterfingerprint in reverse byte order. In Wasabi we fix this with our import function to ensure backwards compatibility. As a consequence one cannot copypaste the coldcard skeleton to Wasabi.

@MaxHillebrand Can you test the complete coldcard SD card workflow on Linux?
@danwalmsley @molnard (Or anyone here.) Can you test the complete coldcard SD card workflow on a OSX?

@MaxHillebrand

This comment has been minimized.

Copy link
Collaborator

MaxHillebrand commented Jul 2, 2019

YES!!!

Oh wow, this works sooo beautifully! Seriously, the flow is very slick and intuitive!
I am way too excited for any calm analysis, haha :D

But there are a couple minor things...

  • The pop-up window with the file system is called dotnet, maybe change that to Wasabi or Wallet Import so the user is not confused what dotnet is...
  • When importing the wallet skeleton, then Wasabi changes the name of the wallet to Hardware Wallet 0, it might be good to take the actual file name. [And as a side-note, allow changing the wallet name in the GUI, as done for the receiving addresses? #1693]
  • It's important that the unsigned PSBT file has the .psbt ending - @nopara73, I think that was the issue that you ran into with the short error message. So maybe force this file type, if possible?
@danwalmsley

This comment has been minimized.

Copy link
Collaborator

danwalmsley commented Jul 2, 2019

@MaxHillebrand I can test this on OSX if you are available to guide me on what to do?

@kravens

This comment has been minimized.

Copy link

kravens commented Jul 2, 2019

I've also attempted to sign on ColdCard a PSBT generated from this branch WasabiWallet, this is the split second error:
image

  • update: no tx*-signed.psbt was generated on the ColdCard Mk2
@MaxHillebrand

This comment has been minimized.

Copy link
Collaborator

MaxHillebrand commented Jul 2, 2019

I have one design change:

Currently, in the Broadcast Transaction tab, the PSBT Json field shows the regular json of the psbt, but it has a bunch of brackets, and is not intuitively readable.

image

Can we parse the context and display it in a pretty wasabi theme? Maybe show which coin it is spending, to what address, with what fee, similar as with the regular sending window?

In general, the UX is pretty clunky right now and can probably be better integrated into the overall wasabi theme.

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

nopara73 commented Jul 3, 2019

@kravens Did you use Wasabi's import button?

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator

NicolasDorier commented Jul 3, 2019

@nopara73 coldcard fixed the reverse order in the latest firmware afaik.... you should asked them if they changed the json file, as your workaround might not work after.

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

nopara73 commented Jul 3, 2019

The pop-up window with the file system is called dotnet, maybe change that to Wasabi or Wallet Import so the user is not confused what dotnet is...

@lontivero @jmacato Can you give it a try? On Windows it's working properly, so it's a bunch of trial error work on Linux.

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator

NicolasDorier commented Jul 3, 2019

I take their fp from the config file in the following way.

result.AccountKeySettings[0].RootFingerprint = new HDFingerprint(jobj["ckcc_xfp"].Value<uint>());

I think it works with all versions.

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

nopara73 commented Jul 3, 2019

@NicolasDorier I'm not parsing the Electrum file. I'm parsing the Wasabi skeleton file they generate. Anyhow, update:

I talked to Rodolfo Novak and Peter D. Gray and they want to reverse the byte order in the Wasabi skeleton they generate. So the current firmware is going to be something that needs to be updated.

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

nopara73 commented Jul 3, 2019

The pop-up window with the file system is called dotnet, maybe change that to Wasabi or Wallet Import so the user is not confused what dotnet is...

@MaxHillebrand I added titles to the dialogs, I'm not sure how it looks like on Linux, so not sure I succeeded, can you check it? Anyhow, this is the best we can do.

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

nopara73 commented Jul 3, 2019

It's important that the unsigned PSBT file has the .psbt ending - @nopara73, I think that was the issue that you ran into with the short error message. So maybe force this file type, if possible?

@MaxHillebrand I enforce .psbt type now if extension is not specified explicitly by the user. However as I tested it it works on Windows even if I don't do that, except in an edge case when . is added to the end of file, maybe it doesn't work on other platforms, anyhow...

When i click Export Binary PSBT and only type in the popup testtransaction [without the .psbt ending] then it does not generate a readable file for cold card. The user must manually type in .psbt at the end of the file. Can we work around this?

So, this wasn't the case on Windows. It worked there.

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

nopara73 commented Jul 3, 2019

Say if I can start testing.

@molnard Go ahead!

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

nopara73 commented Jul 3, 2019

Just a heads up that version 2.1.1 of Coldcard (hopefully out later today) will change the Wasabi export file format:

new field: ColdCardFirmwareVersion with string like: 2.1.1

Thank you @peter-conalgo! I implemented the handling of it, so this PR is mergable now if the tests on OSX work, too.

@kravens

This comment has been minimized.

Copy link

kravens commented Jul 3, 2019

@nopara73 I’ve generated the psbt file in WasabiWallet, but dragged the new-wasabi.json file in the wallet dir manually from the SD card without import function

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

nopara73 commented Jul 3, 2019

@nopara73 I’ve generated the psbt file in WasabiWallet, but dragged the new-wasabi.json file in the wallet dir manually from the SD card without import function

Thank you @kravens, this explains it.

@MaxHillebrand

This comment has been minimized.

Copy link
Collaborator

MaxHillebrand commented Jul 3, 2019

Everything works flawlessly on Debian!

@MaxHillebrand

This comment has been minimized.

Copy link
Collaborator

MaxHillebrand commented Jul 3, 2019

ColdCard firmware v2.1.1 is released!
Thanks @peter-conalgo!!! <3
https://coldcardwallet.com/docs/upgrade

@kravens

This comment has been minimized.

Copy link

kravens commented Jul 3, 2019

Tried again with ColdCard firmware v2.1.1 and this WasabiWallet-psbtbinary, when testing the Import Coldcard button:
image

2019-07-04 01:18:14 ERROR TransactionBroadcasterViewModel: System.ArgumentException: Interface not registered

Failed to find proxy registration for IID: {ADD8BA80-002B-11D0-8F0F-00C04FD7D062}.
at Avalonia.Win32.Interop.UnmanagedMethods.IShellItem.GetDisplayName(UInt32 sigdnName, IntPtr& ppszName)
at Avalonia.Win32.SystemDialogImpl.GetAbsoluteFilePath(IShellItem shellItem) in D:\a\1\s\src\Windows\Avalonia.Win32\SystemDialogImpl.cs:line 154
at Avalonia.Win32.SystemDialogImpl.<>c__DisplayClass1_0.b__0() in D:\a\1\s\src\Windows\Avalonia.Win32\SystemDialogImpl.cs:line 92
at System.Threading.Tasks.Task`1.InnerInvoke()
at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location where exception was thrown ---
at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot)
--- End of stack trace from previous location where exception was thrown ---
at WalletWasabi.Gui.Tabs.WalletManager.LoadWalletViewModel.<>c__DisplayClass33_0.<<-ctor>b__12>d.MoveNext() in C:\Users\kevin\Desktop\Wasabi\WalletWasabi-psbtbinary\WalletWasabi.Gui\Tabs\WalletManager\LoadWalletViewModel.cs:line 109

p.s. I do have a pending W10 update... for .NET

@molnard

This comment has been minimized.

Copy link
Collaborator

molnard commented Jul 4, 2019

Have problems on Mac. As usual it freezes, maybe not depends on this PR but I noticed it here. Keep investigating.

nopara73 added 2 commits Jul 4, 2019
@nopara73

This comment has been minimized.

Copy link
Collaborator Author

nopara73 commented Jul 4, 2019

@molnard's Mac freezes are fixed. It wasn't related to this branch. @kravens's issue is worrysome, but we're investigating it.

@molnard

This comment has been minimized.

Copy link
Collaborator

molnard commented Jul 4, 2019

Mac freeze fixed: #1743
Tested the whole procedure on Mac and works fine. I could Import Wallet, Build PSBT, Sing and broadcast the tx. History and balance OK

@SovereignNode

This comment has been minimized.

Copy link

SovereignNode commented Jul 4, 2019

Can confirm this is working on Win10. Nice

Thanks all

Wasabi with CC

nopara73 added 2 commits Jul 4, 2019
@nopara73

This comment has been minimized.

Copy link
Collaborator Author

nopara73 commented Jul 4, 2019

@kravens we implemented a fallback mechanism in case we cannot use the OS's Save or Open file dialogs. But hopefully we'll be able to reproduce and fix your specific problem in the future.

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

nopara73 commented Jul 4, 2019

@MaxHillebrand You have to review the transaction in your coldcard, because that is where you are signing. If you are using coldcard, you cannot trust Wasabi with the transaction broadcaster.

@nopara73 nopara73 merged commit 30c2d61 into zkSNACKs:master Jul 4, 2019
4 checks passed
4 checks passed
CodeFactor No issues found.
Details
Wasabi.Linux #20190704.36 succeeded
Details
Wasabi.Osx #20190704.36 succeeded
Details
Wasabi.Windows #20190704.36 succeeded
Details
v1.1.6 automation moved this from In Progress to Done Jul 4, 2019
@nopara73 nopara73 deleted the nopara73:psbtbinary branch Jul 4, 2019
@kravens

This comment has been minimized.

Copy link

kravens commented Jul 6, 2019

@nopara73 I've triggered the fallback mechanism due to a system timeout:

2019-07-06 07:10:28 WARNING WalletManagerViewModel: System.TimeoutException: Timeout occured during the hwi operation. ---> System.Threading.Tasks.TaskCanceledException: A task was canceled.
   at System.Diagnostics.ProcessExtensions.WaitForExitAsync(Process process, CancellationToken cancellationToken) in C:\Users\kevin\Desktop\Wasabi\WalletWasabi\WalletWasabi\Extensions\ProcessExtensions.cs:line 16
   at WalletWasabi.Hwi.HwiProcessManager.SendCommandAsync(String command, CancellationToken cancellationToken, Boolean isMutexPriority) in C:\Users\kevin\Desktop\Wasabi\WalletWasabi\WalletWasabi\Hwi\HwiProcessManager.cs:line 183
   --- End of inner exception stack trace ---
   at WalletWasabi.Hwi.HwiProcessManager.SendCommandAsync(String command, CancellationToken cancellationToken, Boolean isMutexPriority) in C:\Users\kevin\Desktop\Wasabi\WalletWasabi\WalletWasabi\Hwi\HwiProcessManager.cs:line 207
   at WalletWasabi.Hwi.HwiProcessManager.EnumerateAsync() in C:\Users\kevin\Desktop\Wasabi\WalletWasabi\WalletWasabi\Hwi\HwiProcessManager.cs:line 128
   at WalletWasabi.Gui.Tabs.WalletManager.WalletManagerViewModel.RefreshHardwareWalletListAsync() in C:\Users\kevin\Desktop\Wasabi\WalletWasabi\WalletWasabi.Gui\Tabs\WalletManager\WalletManagerViewModel.cs:line 125

And successfully imported the Coldcard generated new-wasabi.json from the fallback menu!

2019-07-06 07:09:47 INFO LoadWalletViewModel: Creating new wallet file.
2019-07-06 07:10:28 INFO: Wallet loaded: Coldcard0.

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