Page MenuHomePhabricator

[electrum] support parsing multi-output URIs
ClosedPublic

Authored by PiRK on Jan 23 2024, 15:54.

Details

Reviewers
bytesofman
Group Reviewers
Restricted Project
Commits
rABC0c949827812c: [electrum] support parsing multi-output URIs
Summary

This adds support for multi-outputs URIs in the send tab.
For now, the receive tab is still limited to generating regular single-output URIs.

Depends on D15385

Test Plan

Paste the multi-output URI examples from the added test and from the bip21-ecash-additions.md document into the payto field, and check that the transactions are created as expected.

Test also other examples from the test_bip21 test suite in the GUI, especially the exotic ones such as ecash:?op_return_raw=04deadbeef (takes one input randomly and creates an op_return output and a change output) and ecash:?r=http://domain.tld/page?h%3D2a8628fc2fbe (this one will not work because the bip70 url is invalid, but it should still try to resolve it and give a meaningful error message).

python test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
arcpatch-D15250_2
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 26933
Build 53432: Build Diffelectrum-tests
Build 53431: arc lint + arc unit

Event Timeline

PiRK requested review of this revision.Jan 23 2024, 15:54
PiRK planned changes to this revision.Jan 23 2024, 15:54
PiRK edited the summary of this revision. (Show Details)

One of the unit tests is not broken for wrong reasons.

make the test_parameter_pollution succeed for a good reason, add a test for inconsistent number of "amount" vs "addr"

bytesofman added a subscriber: bytesofman.

not able to follow test plan without rebase

image.png (179×449 px, 14 KB)

This revision now requires changes to proceed.Jan 23 2024, 20:54

That's odd, I thought I rebased this on the parent commit just before landing it. Will rebase again.

PiRK planned changes to this revision.Jan 23 2024, 21:05

needs a bugfix for the empty address

couple of issues

  1. Paste ecash:qrh3ethkfms79tlcw7m736t38hp9kg5f7gycxeymme?amount=100&op_return_raw=0401020304&addr=qz252dlyuzfqk7k35f57csamlgxc23ahz5accatyk9&amount=300&addr=qzrseeup3rhehuaf9e6nr3sgm6t5eegufuuht750at&amount=200 into Send

Everything generates fine

Delete it -- OP_RETURN input remains. no blocker but probably not the behavior we want here.

  1. Paste ecash:qrh3ethkfms79tlcw7m736t38hp9kg5f7gycxeymme?amount=40.00&label=test&amount=30.00 into send screen

Get multiple error notifications. I actually cannot click them all closed so that I can delete the bad string, because they keep popping up.

image.png (522×882 px, 68 KB)

PiRK edited the test plan for this revision. (Show Details)

fix the empty address case ecash:?op_return_raw=04deadbeef (if len(addresses) == 0, we cannot access addresses[0]). Add a test to the test plan and the unit tests

PiRK planned changes to this revision.EditedJan 23 2024, 21:23

I actually didn't think of interactively testing the ones that are supposed to fail. Good catch.

automatically clear the "pay to" field when the URI is malformed, otherwise it will keep raising the same error as the text is changed while the user attempts to correct it.
I don't expect people to manually type payment URIs, they will be copied and pasted, so this should not cause any loss of data.

Delete it -- OP_RETURN input remains. no blocker but probably not the behavior we want here.

I'm trying to think of a solution, but that issue is not simple. Maybe the payment URI should be its own separate widget (that could be hidden and only appear when the user pastes a non-trivial URI into the "pay to" field), that when filled causes the other ones to become slaves to the URI widget. But are users still allowed to manually change anything in the other fields to adjust the transaction, or do they become read-only as long as the URI field is not cleared?

Definitely not going to happen in this diff, and it would probably be good to have the opinion of a professional UI expert one of these days.

In D15250#342968, @PiRK wrote:

Delete it -- OP_RETURN input remains. no blocker but probably not the behavior we want here.

I'm trying to think of a solution, but that issue is not simple. Maybe the payment URI should be its own separate widget (that could be hidden and only appear when the user pastes a non-trivial URI into the "pay to" field), that when filled causes the other ones to become slaves to the URI widget. But are users still allowed to manually change anything in the other fields to adjust the transaction, or do they become read-only as long as the URI field is not cleared?

Definitely not going to happen in this diff, and it would probably be good to have the opinion of a professional UI expert one of these days.

In Cashtab this is handled by

  • If there is a bip21 URI with the amount param set, then the amount input is set by the bip21 uri and user modification of the amount field is disabled
  • If the amount param is removed from the bip21 URI string (or the whole bip21 string is removed), amount param is cleared and ready to accept input
  • If there is a bip21 URI with no amount param, then the amount field is available for user input
  • If there is a bip21 URI witht he op_return_raw param, then the cashtab message input is not rendered
  • If there is a bip21 URI with no op_return_raw param, then the cashtab msg input is available for user input
  • If `op_return_raw_ is removed from the bip21 URI string (or the whole bip21 string is removed), cashtab msg input is available for user input

the whole thing is pretty complicated, and this is in React which is optimized for implementing and testing this sort of UI issue.

It's probably not a big deal to leave OP_RETURN populated if some input there was received, as the field is still present and visible to the user.

In D15250#342822, @PiRK wrote:

I actually didn't think of interactively testing the ones that are supposed to fail. Good catch.

This issue appears to exist in electrum before this diff, at least I get the same thing when I run ecash:qrh3ethkfms79tlcw7m736t38hp9kg5f7gycxeymme?amount=40.00&label=test&amount=30.00 through 5.2.11

especially the exotic ones such as ecash:?op_return_raw=04deadbeef

Spec does seem to be ambiguous about whether or not the initital address param is required. In Cashtab, the address param is de facto required, e.g.

image.png (419×486 px, 21 KB)

You can't send anything until you add the address param, and in this config it can only be added in the URI string.

Entering this in Electrum-ABC works but it's pretty confusing to the user, as the input itself simply disappears after adding the params to the tx:

image.png (212×835 px, 19 KB)

In this case, we probably want the input to persist so we know what happened and why. I'm not sure on easiest solution to this. Probably just require bip21 strings to have an address.

This revision now requires changes to proceed.Jan 24 2024, 14:02
In D15250#342965, @PiRK wrote:

automatically clear the "pay to" field when the URI is malformed, otherwise it will keep raising the same error as the text is changed while the user attempts to correct it.
I don't expect people to manually type payment URIs, they will be copied and pasted, so this should not cause any loss of data.

Is it possible to handle this without clearing the input? It's pretty difficult to troubleshoot a bad URI string with this behavior. The error msg appears, just says it's invalid, then clicking ok resets everything.

It is better than the current behavior and possibly acceptable for electrum which is more power-user focused (again this issue exists in the prod version already and I don't think we've ever had a complaint about it). We don't want to re-engineer the whole UI for something that isn't having a real world impact.

If we can keep the bad bip21 string in place without throwing constant error msgs without a major refactor, though, it is probably worth it.

especially the exotic ones such as ecash:?op_return_raw=04deadbeef

Spec does seem to be ambiguous about whether or not the initital address param is required. In Cashtab, the address param is de facto required, e.g.

image.png (419×486 px, 21 KB)

You can't send anything until you add the address param, and in this config it can only be added in the URI string.

Entering this in Electrum-ABC works but it's pretty confusing to the user, as the input itself simply disappears after adding the params to the tx:

image.png (212×835 px, 19 KB)

In this case, we probably want the input to persist so we know what happened and why. I'm not sure on easiest solution to this. Probably just require bip21 strings to have an address.

Not having any address seems to address some use cases, though.
The obvious one is ecash:?r=... : BIP72 payment URI that is not BIP21 backward compatible. BIP72 explicitly states that the address and amount can be ignored in the presence of a payment request url.

The other one is wanting someone to send some OP_RETURN data tx on chain without other XEC outputs. Not sure if this is a real-world need, but it was part of the rationale for https://github.com/Electron-Cash/Electron-Cash/pull/1127
I don't think we need to add this to the BIP21 spec unless someone starts relying on it for a use case, but I also don't see a good reason not to support it in Electrum ABC.

If we can keep the bad bip21 string in place without throwing constant error msgs without a major refactor, though, it is probably worth it.

I need to think this through. The obvious solution is to not throw an error message as a popup, but we still need to raise the reason for the failure to parse the URI to the user in some other way (some kind of status bar or a unique non-modal error message)

.

rebase on D15385, use a ShowPopupLabel (temporary popup, doesn't take the focus and does not block the editing of the Pay To field) instead of a blocking popup window.

This is a minimalist change on the UI side. More things could be improved wrt to URI handling, as discussed in the review, but that would require a much deeper refactoring of the current code in the send tab. So I'll wait for some more feedback before deciding if a task is warranted.

To summarize the issues that could (should?) be fixed: currently there is a single line field to paste either a destination address, a CSV formatted address, amount... multi-output text, or a payment URI. When a valid URI is pasted, or opened via a link, or passed on the command line, it is parsed and the various fields corresponding to the parameters are filled, and the URI disappears (replaced by the destination address).
It would be better if there was a specific widget for URIs, and if the other fields became read-only when in URI mode, so that the URI would be preserved and so that users could not easily or accidentaly change any of the parameters.

This revision is now accepted and ready to land.Feb 5 2024, 21:47
This revision was automatically updated to reflect the committed changes.