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

Bitcoin URL support #1949

Merged
merged 21 commits into from Jul 25, 2019

Conversation

@benthecarman
Copy link
Collaborator

commented Jul 18, 2019

Closes #732
Works similar to when you have an address on your clipboard. If you have a bitcoin url on your clipboard and click the address field, it will auto fill the address, label, and amount using the url's respective fields. The message field is currently ignored, unsure if we should concatenate this with the label, display it elsewhere, or simply continue ignoring it.

@lontivero

This comment has been minimized.

Copy link
Collaborator

commented Jul 19, 2019

Concept ACK. Same concern about messing with non-attached controls as note by @molnard

@MaxHillebrand

This comment has been minimized.

Copy link
Collaborator

commented Jul 19, 2019

Concept ACK, very good, thanks!

Would like to test, but where can I get a Bitcoin url? 👀

@benthecarman

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 19, 2019

Would like to test, but where can I get a Bitcoin url?

Here is the BIP, you can read the general format here

@MaxHillebrand

This comment has been minimized.

Copy link
Collaborator

commented Jul 19, 2019

I get Invalid Address for bitcoin:175tWpb8K1S7NmH4Zx6rewF9WQrcZv245W?amount=20.3&label=Luke-Jr [but I also get the error for the legacy address without the url]
But I also get Invalid Address for bitcoin:tb1qmqdf39a4sg2c4swvtk6ph25r68gnz963jdhu33?amount=20.3

So the issue is regardless segwit, amount, label, or even just bitcoin:<address>

On Debian 9 with up to date branch.

@benthecarman

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 19, 2019

It takes into account that you are using the correct network, is that the problem? You shouldn't need to paste it in, you should just click the address box and it auto-fills everything

@MaxHillebrand

This comment has been minimized.

Copy link
Collaborator

commented Jul 19, 2019

Well, it doesn't work with the testnet segwit either.
There's a little qubes-only hickup of the automatic paste, when the address is copied in another qube, and then transferred with Shift+Ctrl+C to the Wasabi qube.

@benthecarman

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 19, 2019

Does the address auto-pasting in master work for you?

@MaxHillebrand

This comment has been minimized.

Copy link
Collaborator

commented Jul 19, 2019

Oh, magic, it works upon restart - all good now!

that copy and paste hickup is specific for qubesOS, it's a non-issue, nothing's broken, just one extra click.

@benthecarman

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 19, 2019

Refactored, ready for re-review

@MaxHillebrand

This comment has been minimized.

Copy link
Collaborator

commented Jul 20, 2019

testing on qubes 4.0 in a debian 9 qube, latest commit in branch.

I'm trying to paste bitcoin:175tWpb8K1S7NmH4Zx6rewF9WQrcZv245W?amount=20.3&label=Luke-Jr

  • When I have it in the clipboard of the wasabi qube, then it does not paste when clicking the address box, the logs show PasteAddressOrUrlCommand.
  • When I have something in the clipboard, it won't allow me to put the cursor into the address box.
  • When I have nothing in the clipboard, then I can get the cursor in the address box, and when I start typing, then copy the url, delete what I've typed, and then paste the url, then it is displayed in the address box with Invalid Address error.

I'm not sure, but don't think that this is related to me running qubes, so please someone review on bare metal debian as well.

@benthecarman benthecarman force-pushed the benthecarman:bitcoin_url_support branch from 1c541e3 to b09ffee Jul 20, 2019

@benthecarman

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 20, 2019

Removed some debug code I accidentally left in

@MaxHillebrand

This comment has been minimized.

Copy link
Collaborator

commented Jul 21, 2019

tested again after this last push - still, I get PasteAddressOrUrlCommand in the terminal, yet nothing is registered in the GUI.

@molnard
Copy link
Collaborator

left a comment

@benthecarman you took a very practical approach but unfortunately it is not good. Now many features are missing. For example I cannot edit, Autocopy missing, etc.
I would revert the last commit.

  • Let PasteAddressOnClickBehavior.cs be.
  • Add an optional command parameter to PasteAddressOnClickBehavior which is fired after an address or an URL is pasted. Do not remove the state machine from PasteAddressOnClickBehavior!
  • Move every action which involve external controls to the viewmodel under the commandExecuted event.
@MaxHillebrand

This comment has been minimized.

Copy link
Collaborator

commented Jul 22, 2019

Well @molnard, it might be an issue to allow changes of amount when url is used, because the merchant might expect to get exactly that amount.

But because the label is stored locally, it should be changeable.

@benthecarman benthecarman force-pushed the benthecarman:bitcoin_url_support branch from b09ffee to 1dc689c Jul 22, 2019

@benthecarman

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 22, 2019

Addressed @molnard comments, I believe the implementation is now much better

nopara73 added 3 commits Jul 23, 2019
@nopara73
Copy link
Collaborator

left a comment

The code is good, but it doesn't work if autocopy is turned off. (Also concept ACK.)

@benthecarman

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 23, 2019

Address auto paste didn't work when autocopy was off before, do you want that not to affect the auto pasting for both?

@lontivero

This comment has been minimized.

Copy link
Collaborator

commented Jul 23, 2019

It is okay as it is. This PR does not change the current behavior and it is backward compatible. IMO the problem is in the settings page where it says "Autococpy on Receive and History wallet tabs". That setting enable/disable autocopy everywhere, what it is okay to me.

Update: I've tested it and it works well.

@benthecarman

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 23, 2019

That setting enable/disable autocopy everywhere

I could make a change in this PR, or in a separate PR, to change the wording of it to be something like

Automatic copy and pasting of bitcoin addresses, urls, and transaction ids

@MaxHillebrand

This comment has been minimized.

Copy link
Collaborator

commented Jul 24, 2019

Tested on Qubes 4 in Debian 9, current branch without any conflicts.

I still cannot get it to work... I have the url in the wasabi qube clipboard [with ctrl+shift+c], but when I click inside the address field then it only puts the cursor there, no automatic pasting. When I then manually paste [ctrl + v] the url, I get the "invalid address" error.

I do not know if this is because of qubes and the clipboard separating, or if this is a general bug...
But I don't think so, because the click-paste works for regular addresses...

Regardless, I think if the url get's copied in the addressfield, it should be able to detect that and set everything up.

@molnard

This comment has been minimized.

Copy link
Collaborator

commented Jul 24, 2019

What is the difference between Label and Message in the url - and how are the two handled in this PR?

We are not handling the message field.

@MaxHillebrand

This comment has been minimized.

Copy link
Collaborator

commented Jul 24, 2019

Aaaah, I found my mistake - quite stupid, Wasabi Wallet was on testnet, but the url has a mainnet legacy address.
It works when I use a testnet native segwit address in the url.
ACK.

But I can still change the amount after the url is pasted. I hear that this is an issue for merchants in regards to over & underpayments. But I like that this gives more user choice to, maybe send max amount without change...
So I'm not sure if we should leave it like it is now, or prohibit changing the amount.

molnard added 4 commits Jul 24, 2019
@lontivero
Copy link
Collaborator

left a comment

This version has a bug when some invalid inputs are entered. After getting a NRE Wasabi stop responding and I have to restart it.

2019-07-24 13:59:46 CRITICAL Program: System.NullReferenceException: Object reference not set to an instance of an object.
   at WalletWasabi.Helpers.AddressStringParser.TryParseBitcoinUrl(String text, Network expectedNetwork, BitcoinUrlBuilder& url) in /home/lontivero/GitHub/WalletWasabi/WalletWasabi/Helpers/AddressStringParser.cs:line 48
   at WalletWasabi.Gui.Behaviors.PasteAddressOnClickBehavior.TryParse(String text, BitcoinUrlBuilder& result) in /home/lontivero/GitHub/WalletWasabi/WalletWasabi.Gui/Behaviors/PasteAddressOnClickBehavior.cs:line 79
   at WalletWasabi.Gui.Behaviors.PasteAddressOnClickBehavior.<OnAttached>b__15_2(PointerEventArgs pointerEnter) in /home/lontivero/GitHub/WalletWasabi/WalletWasabi.Gui/Behaviors/PasteAddressOnClickBehavior.cs:line 191
   at Avalonia.Threading.JobRunner.RunJobs(Nullable`1 priority) in D:\a\1\s\src\Avalonia.Base\Threading\JobRunner.cs:line 40
   at Avalonia.X11.X11PlatformThreading.HandleX11(CancellationToken cancellationToken)
   at Avalonia.X11.X11PlatformThreading.RunLoop(CancellationToken cancellationToken) in D:\a\1\s\src\Avalonia.X11\X11PlatformThreading.cs:line 200
   at Avalonia.Threading.Dispatcher.MainLoop(CancellationToken cancellationToken) in D:\a\1\s\src\Avalonia.Base\Threading\Dispatcher.cs:line 65
   at Avalonia.Application.Run(Window mainWindow) in D:\a\1\s\src\Avalonia.Controls\Application.cs:line 237
   at WalletWasabi.Gui.Program.Main(String[] args) in /home/lontivero/GitHub/WalletWasabi/WalletWasabi.Gui/Program.cs:line 43
@lontivero

This comment has been minimized.

Copy link
Collaborator

commented Jul 24, 2019

It seems to work well. I've been thinking that this could introduce some vector of attack. Below you can see an attempt to take money from you and, if well it is an obvious one, playing with escape characters, zero-width characters and other techniques these kind of attempts could achieve a better level.

bitcoin:bc1qwyxske6rr0nkmmz66uauu92fv5ygghvr65ypvn?%20amount=0.010&label=whatever%20texto&description=Disclaimer,%20the%20&amount=%20%20%20%200.10&in%20the%20is%20%the%20real%20one
@MaxHillebrand

This comment has been minimized.

Copy link
Collaborator

commented Jul 24, 2019

Tried again, works very well now!

Well, one minor thing is that, when I past the url in the address field, then for one second it shows Invalid Address and then it pastes the label and amount afterwards.

So, even the manual pasting works now, but there is this odd GUI bug.

molnard added 4 commits Jul 25, 2019
@molnard

This comment has been minimized.

Copy link
Collaborator

commented Jul 25, 2019

It seems to work well. I've been thinking that this could introduce some vector of attack. Below you can see an attempt to take money from you and, if well it is an obvious one, playing with escape characters, zero-width characters and other techniques these kind of attempts could achieve a better level.

bitcoin:bc1qwyxske6rr0nkmmz66uauu92fv5ygghvr65ypvn?%20amount=0.010&label=whatever%20texto&description=Disclaimer,%20the%20&amount=%20%20%20%200.10&in%20the%20is%20%the%20real%20one

Good point. I could implement some detection of that but I won't. The user can see the result in the amount field before sending the money so he can check the if the parsing was correct or not. IMO a regular user will look the wasabi textboxes and do not try to validate the URL by looking it like Neo from the matrix - so the user won't be confused by comparing the URL and he result - he will just check the result.

At this case 0.1 will be sent.

@molnard

This comment has been minimized.

Copy link
Collaborator

commented Jul 25, 2019

Tried again, works very well now!

Well, one minor thing is that, when I past the url in the address field, then for one second it shows Invalid Address and then it pastes the label and amount afterwards.

So, even the manual pasting works now, but there is this odd GUI bug.

Good catch, you have been served Sir!

molnard added 5 commits Jul 25, 2019

@nopara73 nopara73 merged commit dedd6ed into zkSNACKs:master Jul 25, 2019

4 checks passed

CodeFactor No issues found.
Details
Wasabi.Linux #20190725.5 succeeded
Details
Wasabi.Osx #20190725.5 succeeded
Details
Wasabi.Windows #20190725.6 succeeded
Details
@MaxHillebrand

This comment has been minimized.

Copy link
Collaborator

commented Jul 25, 2019

Great work!!
Thanks all ❤️

@benthecarman benthecarman deleted the benthecarman:bitcoin_url_support branch Jul 25, 2019

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