[electrum] remove support for signed payment requests in URIs
ClosedPublic
Actions

Authored by PiRK on Jan 20 2024, 09:55.

Details

Reviewers

bytesofman

Group Reviewers

Restricted Project

Commits

rABCd6b851ad3542: [electrum] remove support for signed payment requests in URIs

Summary

This was added in 2015, and I'm not aware of any use case for it today. This feature makes it hard to decouple BIP21 payments URIs from BIP70 payment requests, so it makes it harder to extend BIP21 with multiple destination and amounts.

BIP72 states that:

when Bitcoin wallet software that supports this BIP receives a bitcoin: URI with a request parameter, it should ignore the bitcoin address/amount/label/message in the URI and instead fetch a PaymentRequest message and then follow the payment protocol, as described in BIP 70.

After this diff, the "sig", "name", "exp", "memo" and "time" field are still used for internal dict representation of payment requests, but they should no longer affect payment URIs.

See following commits for the introduction of the parameters that this diff removes:
https://github.com/spesmilo/electrum/commit/9c30ad3dd5fb10832579e858c3cfdded06444dad
https://github.com/spesmilo/electrum/commit/e77f0c9
https://github.com/spesmilo/electrum/commit/0cef063ee231cc9459dff6829dc33ffc2ed499e2
https://github.com/spesmilo/electrum/commit/a6c65b8997c2407502edbe5cc9d818722e1db2cd

Test Plan

python test_runner.py

Test that a regular BIP72 payment URI still works:

Generate a payment request: curl -L -H 'Content-Type: application/json' -d '{"outputs":[{"address":"1NLcNpAaBBMekgBZk7NxwdxwtSUTfTV8Aq","amount":560}],"currency":"ecash"}' https://pay.badger.cash/create_invoice
Run ./electrum-abc
In the send tab, paste ecash:?r=https://pay.badger.cash/i/JFPTG (replace the URL with the one returned by the above curl command
Send

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

PiRK created this revision.Jan 20 2024, 09:55

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 20 2024, 09:55

PiRK requested review of this revision.Jan 20 2024, 09:55

PiRK edited the test plan for this revision. (Show Details)Jan 20 2024, 09:57

Harbormaster completed remote builds in B26510: Diff 44388.Jan 20 2024, 09:58

Seems like there is mixed logic in electrum for parsing this stuff. I don't think it's likely we will use these features for anything ecash related soon, but someone might.

Is it impractical to split it up, something like "if address input is bip 70, process as bip70, else if address input is bip21, process as bip21" ?

Even if we do simplify the approach by losing some of these params, seems like the function remains overloaded if there is not a clear distinction between handleBip21input and handleBip70input

This revision now requires changes to proceed.Jan 20 2024, 13:56

In D15221#341986, @bytesofman wrote:

Is it impractical to split it up, something like "if address input is bip 70, process as bip70, else if address input is bip21, process as bip21" ?

Well, my goal here is to take a first step in splitting it up. Removing these unused params would make it trivial to detect if a payment URI is BIP70/72 (presence of a &r=...) or BIP21, and more importantly would remove any need to parse any of the BIP21 keywords present in the URI in the BIP70 case.

Besides making this splitting easier, my other problem with those extra params is that they are not documented anywhere, not tested, and I couldn't even find an issue or a pull request mentioning them, so I cannot start to guess how to use them. This old Electrum code is as usual pretty hard to follow, so I have a hard time getting a clear picture of which BIP21 params are re-used to encode that payment request.

bytesofman accepted this revision.Jan 20 2024, 16:27

This revision is now accepted and ready to land.Jan 20 2024, 16:27

Closed by commit rABCd6b851ad3542: [electrum] remove support for signed payment requests in URIs (authored by PiRK). · Explain WhyJan 22 2024, 08:55

This revision was automatically updated to reflect the committed changes.

PiRK added a commit: rABCd6b851ad3542: [electrum] remove support for signed payment requests in URIs.