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 BIP21 bitcoin payment URI support to sendpayment.py #560

Merged
merged 1 commit into from
Apr 26, 2020
Merged

Add BIP21 bitcoin payment URI support to sendpayment.py #560

merged 1 commit into from
Apr 26, 2020

Conversation

kristapsk
Copy link
Member

In future could and should be improved to add this also to Qt GUI, but didn't want to do that now, need to figure out proper UX there.

This is needed for #557.

@kristapsk kristapsk requested a review from AdamISZ April 23, 2020 14:18
@chris-belcher
Copy link
Contributor

chris-belcher commented Apr 23, 2020

Simple neat code. utACK.

I played around with the regex in bip21.py to make sure it really does work.

In terms of UX for bip21 in the GUI, note that the Electrum version in master branch only gives out bip21 URIs for paying into its own wallet. It must have some scheme for allowing the user to input both (address, amount) and URI.

@kristapsk
Copy link
Member Author

Added also checks and tests for non-empty address part.

docs/USAGE.md Outdated Show resolved Hide resolved
scripts/sendpayment.py Outdated Show resolved Hide resolved
@AdamISZ
Copy link
Member

AdamISZ commented Apr 25, 2020

@kristapsk although I've added several comments, I don't think there is anything major, this code seems very clear.
Thanks.

I guess you will add support for the ?pj= parameter in a separate commit.

@kristapsk
Copy link
Member Author

I guess you will add support for the ?pj= parameter in a separate commit.

Yes, this is meant to add only BIP21 support, BIP79-style PayJoin should be a separate commit.

@AdamISZ
Copy link
Member

AdamISZ commented Apr 26, 2020

Yes thanks, I think you've addressed what was needed (especially the arguments logic). Nice thing about this is, it's not breaking anything that already exists, so utACK, and go ahead and merge.

@kristapsk kristapsk merged commit 2d7b0a0 into JoinMarket-Org:master Apr 26, 2020
@kristapsk kristapsk deleted the bip21 branch April 26, 2020 22:03
@kristapsk
Copy link
Member Author

About potential GUI UX - both Electrum and Wasabi Wallet allows pasting BIP21 URI in address field, which is then automatically parsed and all related fields filled, e.g. amount, value in address field is automatically converted to address.

See wasabi docs: sending payjoin step-by-step.

@AdamISZ
Copy link
Member

AdamISZ commented May 2, 2020

Yes. Thank you. I will try to knock together a first draft of a script in the next couple of days (I hope); if you're around on IRC we can work on these things a bit together. The script will give us the material to make the Qt part. I had the same thought about the BIP21 aspect since I remember it from Electrum.

@kristapsk
Copy link
Member Author

I will try to implement generic BIP21 support in GUI first, then payjoin can be added on top of that afterwards.

@AdamISZ
Copy link
Member

AdamISZ commented May 2, 2020

Oh yes of course, I see; good idea!

@kristapsk kristapsk mentioned this pull request May 2, 2020
@AdamISZ
Copy link
Member

AdamISZ commented May 9, 2020

An update on the status of this project:

  • python-bitcointx backend for jmbitcoin + bip78 and snicker. #536 now has full sendpayment support for payjoin as in btcpayserver form (tested on mainnet)

  • The above means that we will support these BIP79++ payjoins in the project in the next major release (0.7.0 is currently what we plan, with a minor release of 0.6.3 coming first to bed in all existing improvements in master. And 0.7.0 will require Py3.6 as discussed).

  • A few technical things hold up the finalization of python-bitcointx backend for jmbitcoin + bip78 and snicker. #536 's merge readiness. Best guess is 2-4 weeks.

  • There have been quite a lot of discussions involving myself, Dorier, Hillebrand, Kukks, and several others about some small details of how the protocol does (and should) work, an outcome of this is a probable new BIP incoming from Dorier: BIP: Add payjoin proposal NicolasDorier/bips#3 (so if that gets number-assigned it will no longer be "BIP79++" but BIP whatever).

kristapsk added a commit that referenced this pull request May 9, 2020
a539345 Implement BIP21 in GUI (Kristaps Kaupe)

Pull request description:

  Allows pasting BIP21 URI in address field for single sends. See #560 (comment). Also added stripping address field input of extra whitespace at beginning / end. <del>Change with `raise ValueError` in `decode_bip21_uri()` is needed because `amount_str` can be invalid and that raised `decimal.InvalidOperation` on `amount_to_sat()` call.</del><ins>(UPDATE - that one is already merged in with #568)</ins>

Top commit has no ACKs.

Tree-SHA512: 079ca866cc0cded1624531ad6397fcdeeb7ebed7c6eab778e8640561387a3c08e39be2dc137df785f3aae2ad1b4a56b636d1f304ef854da27cf651293d78266f
@kristapsk
Copy link
Member Author

@AdamISZ What I want to do before 0.7.0 is payjoin support in GUI, but want to get back to #530 first, at it has some useful refactorizations for simple send form that will be useful. Also need to look how Wasabi has implemented this from UX perspective.

@AdamISZ
Copy link
Member

AdamISZ commented May 14, 2020

What I want to do before 0.7.0 is payjoin support in GUI

FWIW I disagree, with a large change like this it would I think be absolutely fine, even preferable, for the new function to be command line only, so only some (more tech savvy) users would initially try it out. I don't think adding to GUI is hard, but let's have it be in the minor release after.

@kristapsk
Copy link
Member Author

Ok, that's a valid argument.

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