[electrum] fix BIP 21 URI handling
ClosedPublic
Actions

Authored by PiRK on Aug 17 2023, 09:36.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABCada8d4e87792: [electrum] fix BIP 21 URI handling

Summary

Commit https://github.com/Bitcoin-ABC/ElectrumABC/pull/31/commits/00cb0b018f6edbc6a4e6f2d22ddbd509e5c8681e broke the URI handling.

When the application is called with a URI as a first positional argument, it should start a GUI with the send tab pre-filled with the correct information. Unfortunately argparse does not provide a good way to default to a subparser when the provided command line argument is not one of the explicitely specified command. The previous fix was hacky because it relied on monkey-patching private variables in argparse, which could break in any new python release. So reverting the commit is not a good idea IMO.

Manually insert the gui command before the URI when the first argument is a URI.
Note that this is incompatible with using any global argument, but the scope of this diff is to fix mimetype association only, and in this case the URI will be the only argument.

More advanced use cases require to properly specify the gui command on the command line: ./electrum-abc --dir /path/to/data gui <URI>

Depends on D14382

Test Plan

./test_runner.py

./electrum-abc "ecash:qz4eqgk9fqs3uxxt2xwx0jpfj704e2t6rs7357tn7e?amount=1337.06&label=Donation to toto&op_return=Thx for running this service!"

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 24781
Build 49154: Build Diff	electrum-tests
Build 49153: arc lint + arc unit

Event Timeline

PiRK created this revision.Aug 17 2023, 09:36

Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 17 2023, 09:36

PiRK requested review of this revision.Aug 17 2023, 09:36

Harbormaster completed remote builds in B24780: Diff 41839.Aug 17 2023, 09:37

not sure this will always work (global options can be before cmd, so the URI could be after sys.argv[1])

It fixes the main issue of URI mimetype associations no longer working (in that case the application is called with the URI as the single arg), but ideally it would be nice to make it work in the general case with any valid command line.

properly handle cases where global options are specified before the URI

Harbormaster completed remote builds in B24781: Diff 41840.Aug 17 2023, 13:13

bytesofman added a subscriber: bytesofman.Aug 17 2023, 17:59

bytesofman added inline comments.

electrum/electrumabc/commands.py
1585	If there are more than one of these arguments -- does this need to be inserted again at a later index? Or do we know they would all be in order and at the end?

I want to try another (possibly less messy) approach: it seems that argparse allows both subparsers and positional args, so maybe it is possible to just add a url argument in addition to the gui and daemon subparsers.

electrum/electrumabc/commands.py
1585	There should be only one.

The alternative I imagined does not work. But this new solution is messy because of the global vars keeping track of global options. I think I will revert this diff to its initial solution (https://reviews.bitcoinabc.org/D14383?id=41839). This is enough to fix the actual problem that this diff is trying to solve: let the OS associate "ecash:..." URI with the application. In this case there will always be only a single argument.

revert this diff to fixing only the simple use case of the application being called with the URI as the first argument (no other global option), which should be enough to fix the mimetype association use-case.

Harbormaster completed remote builds in B24791: Diff 41851.Aug 18 2023, 12:33

OK after digging into the issue I don't like this solution but we have limited options: https://stackoverflow.com/questions/8668519/python-argparse-positional-arguments-and-sub-commands

Can you please extend the comment to explain that there is no way to use argparse for this ? It's in the summary but it will be lost quickly otherwise

This revision is now accepted and ready to land.Aug 18 2023, 15:35

Closed by commit rABCada8d4e87792: [electrum] fix BIP 21 URI handling (authored by PiRK). · Explain WhyAug 20 2023, 16:30

This revision was automatically updated to reflect the committed changes.

PiRK added a commit: rABCada8d4e87792: [electrum] fix BIP 21 URI handling.