Page MenuHomePhabricator

[electrum] fix BIP 21 URI handling
ClosedPublic

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

Event Timeline

PiRK requested review of this revision.Aug 17 2023, 09:36
PiRK planned changes to this revision.EditedAug 17 2023, 10:11

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.

PiRK edited the summary of this revision. (Show Details)

properly handle cases where global options are specified before the URI

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?

PiRK planned changes to this revision.Aug 18 2023, 11:05

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.

PiRK edited the summary of this revision. (Show Details)

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.

Fabien added a subscriber: Fabien.

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
This revision was automatically updated to reflect the committed changes.