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

Plugins that add command-line commands #1814

Open
doc-hex opened this issue May 26, 2016 · 19 comments
Open

Plugins that add command-line commands #1814

doc-hex opened this issue May 26, 2016 · 19 comments

Comments

@doc-hex
Copy link
Contributor

doc-hex commented May 26, 2016

I'd like to make a plugin that defines a new command-line command. At this point, that doesn't seem possible, and existing plugins are only adding support for hardware wallets so they can interact with standard command-line commands.

Challenges include:

  • plugins are not initialized before lib.commands.get_parser is called, so they are not listed in known_commands when the command-line parser is constructed.
  • not clear if we are allowed to use lib.commands.command decorator outside of that file.
  • there is an unknown command: error message hacked into URI handling for the GUI: electrum:310
  • when the command arguments are parsed, we do not yet know if starting the GUI or running in command-line mode
  • the configuration is not fully-defined while parsing arguments, so not clear if a plugin is enabled.

My proposed changes (which I would make and submit as a PR for further review):

step 1:

  • define a custom argparse.Action() handler for url argument so it's only set if starts with bitcoin:
  • remove/replace the "unknown command" error message associated with URI
  • do not start the GUI if unparsed command line remains; print error instead (ie. unknown command)
  • AFAIK this will preserve the existing behaviour of entering the GUI on: electrum bitcoin:1foo

step 2A:

  • add a new "type" of plugin that is used only to define command line stuff: perhaps argparser
  • like qt, cmdline are now, plugins may define plugins/FOO/argparser.py and it gets hooked into the argument processing stuff, based on the available_for value defined in plugins/FOO/__init__.py
  • plugins of the new type would be loaded very early before the command line is parsed
  • a new base class would be used to expose an interface (wouldn't use BasePlugin)
  • they can define new commands and existing optparser code would detect them in knowncommands

OR a different approach: step 2B:

  • new plugin type, perhaps called early
  • this type of plugin gets loaded very early, before the command-line parser is setup
  • add some new hooks for updating the arg parser (maybe)
  • new plugin type could do almost anything, but I would use it just to register new commands into known_commands

From my point of view, approach 2A is better, but 2B is more general purpose and flexible. The only real difference is that 2A is "more objected oriented".

What do you think of these changes? Other ideas?

@doc-hex
Copy link
Contributor Author

doc-hex commented May 27, 2016

Just had a much better idea, call it Option 3:

  • new global command-line option that enables a (cmdline type) plugin by name: --plugin foobar
  • the plugins selected get loaded as early as possible
  • they can change the command line parsing of following options (hopefully, not sure if possible)
  • no need to wonder if the plugin is enabled or not, because it's explicit
  • probably don't need any new hooks
  • no need for a new plugin type

@ecdsa
Copy link
Member

ecdsa commented Jun 16, 2016

why not add a hook in the command line parser?

@thecockatiel
Copy link
Contributor

this feature would be nice

currently if we want to add a command, it means forking Electrum is the only way to do so

@ecdsa
Copy link
Member

ecdsa commented Oct 23, 2024

It is not possible to add commands to Electrum from a plugin, because we use argparse to parse commands.py.
However, we could add a generic hook that sends command to plugins.
Try this and let me know if it works for your use case:

index 6d713fb25..69296d9fc 100644
--- a/electrum/commands.py
+++ b/electrum/commands.py
@@ -1410,6 +1410,12 @@ class Commands:
             "source": self.daemon.fx.exchange.name(),
         }
 
+    @command('')
+    async def to_plugins(self, *args, **kwargs):
+        """send a command to plugin"""
+        response = run_hook('to_plugins', *args, **kwargs)
+        return response
+
 
 def eval_bool(x: str) -> bool:
     if x == 'false': return False

Note: one restriction is that run_hook is not async

@thecockatiel
Copy link
Contributor

what if we need network command('n') or other dependencies ?

@ecdsa
Copy link
Member

ecdsa commented Oct 23, 2024

it makes sense to add wallet. not sure about network.
network is singleton, you should be able to figure out the existence from your plugin code.

@thecockatiel
Copy link
Contributor

thecockatiel commented Oct 23, 2024

hmm,
other than that it means that we cant add direct commands to electrum, like electrum do_something, it would be electrum to_plugin do_something ? if so, it makes it hard to create a replacement for some other existing cli using electrum

@thecockatiel
Copy link
Contributor

but wait, doesn't importing and using command from commands.py register new commands for us? then the only thing remaining would be to import the plugin's file before trying to execute the command ?

@ecdsa
Copy link
Member

ecdsa commented Oct 23, 2024

other than that it means that we cant add direct commands to electrum, like electrum do_something, it would be electrum to_plugin do_something ? if so, it makes it hard to create a replacement for some other existing cli using electrum

what is your use case? do you want to replace an existing command?

@thecockatiel
Copy link
Contributor

no, my use case is to bring bisq client's functionality to electrum as a plugin

@ecdsa
Copy link
Member

ecdsa commented Oct 23, 2024

but wait, doesn't importing and using command from commands.py register new commands for us? then the only thing remaining would be to import the plugin's file before trying to execute the command ?

the issue is, argparse does not work like that. when you run a command from the CLI, you are running a client that sends a request to the electrum daemon. The client needs to import commands.py in order to figure out the syntax of commands. However, the client cannot load plugins.

@thecockatiel
Copy link
Contributor

thecockatiel commented Oct 23, 2024

hmm, can we change it to make it possible? I am willing to work on it if it means maintaining the project i want to work on will become easier

@ecdsa
Copy link
Member

ecdsa commented Oct 23, 2024

well, what is it exactly that you need?

@thecockatiel
Copy link
Contributor

to be able to register commands without a prefix like to_plugins
so I can directly call electrum my_command, so it can become a drop in replacement for some other projects

@SomberNight
Copy link
Member

It is not possible to add commands to Electrum from a plugin, because we use argparse to parse commands.py. However, we could add a generic hook that sends command to plugins. Try this and let me know if it works for your use case:

index 6d713fb25..69296d9fc 100644
--- a/electrum/commands.py
+++ b/electrum/commands.py
@@ -1410,6 +1410,12 @@ class Commands:
             "source": self.daemon.fx.exchange.name(),
         }
 
+    @command('')
+    async def to_plugins(self, *args, **kwargs):
+        """send a command to plugin"""
+        response = run_hook('to_plugins', *args, **kwargs)
+        return response
+
 
 def eval_bool(x: str) -> bool:
     if x == 'false': return False

This is not really workable.

  • as already pointed out above, the requirements in the @command() decorator cannot be customised
  • it seems difficult to pass arguments to the hook. What you suggest does not work. argparse complains about unrecognized arguments.
  • not critical, but ideally the help command should also work for these commands

Note: one restriction is that run_hook is not async

That's actually a non-issue. It is true we currently don't have any examples of async functions being decorated with @hook but it is fully workable.

To illustrate most of my points above, I have a working example implementation of your suggestion below -- it is quite ugly :)

patch
diff --git a/electrum/commands.py b/electrum/commands.py
index 6d713fb25b..a01e8d7a0e 100644
--- a/electrum/commands.py
+++ b/electrum/commands.py
@@ -1137,6 +1137,17 @@ class Commands:
             "confirmations": wallet.adb.get_tx_height(txid).conf,
         }
 
+    @command('')
+    async def to_plugins(self, cmdname, kwargs=None):
+        """send a command to plugin"""
+        if kwargs is None:
+            kwargs = {}
+        hook_result = run_hook(cmdname, **kwargs)
+        if asyncio.iscoroutine(hook_result):
+            return await hook_result
+        else:
+            return hook_result
+
     @command('')
     async def help(self):
         # for the python console
@@ -1491,6 +1502,7 @@ command_options = {
     'from_ccy':    (None, "Currency to convert from"),
     'to_ccy':      (None, "Currency to convert to"),
     'public':      (None, 'Channel will be announced'),
+    'kwargs':      (None, "custom kwargs to pass to command"),
 }
 
 
@@ -1516,6 +1528,7 @@ arg_types = {
     'encrypt_file': eval_bool,
     'rbf': eval_bool,
     'timeout': float,
+    'kwargs': ast.literal_eval,
 }
 
 config_variables = {
@@ -1555,7 +1568,6 @@ argparse.ArgumentParser.set_default_subparser = set_default_subparser
 
 
 # workaround https://bugs.python.org/issue23058
-# see https://github.com/nickstenning/honcho/pull/121
 
 def subparser_call(self, parser, namespace, values, option_string=None):
     from argparse import ArgumentError, SUPPRESS, _UNRECOGNIZED_ARGS_ATTR
diff --git a/electrum/plugin.py b/electrum/plugin.py
index eb7a8f49d5..9a4039d557 100644
--- a/electrum/plugin.py
+++ b/electrum/plugin.py
@@ -365,13 +365,13 @@ def hook(func):
     return func
 
 
-def run_hook(name, *args):
+def run_hook(name, *args, **kwargs):
     results = []
     f_list = hooks.get(name, [])
     for p, f in f_list:
         if p.is_enabled():
             try:
-                r = f(*args)
+                r = f(*args, **kwargs)
             except Exception:
                 _logger.exception(f"Plugin error. plugin: {p}, hook: {name}")
                 r = False
diff --git a/electrum/plugins/labels/labels.py b/electrum/plugins/labels/labels.py
index 20e9946e93..1a1711586f 100644
--- a/electrum/plugins/labels/labels.py
+++ b/electrum/plugins/labels/labels.py
@@ -127,6 +127,15 @@ class LabelsPlugin(BasePlugin):
                                      'externalId': encoded_key})
         await self.do_post("/labels", bundle)
 
+    @hook
+    async def pull_labels_async(self, **kwargs):
+        await asyncio.sleep(2)
+        return f"done stuff 1. called with: {kwargs=}"
+
+    @hook
+    def pull_labels_sync(self, **kwargs):
+        return f"done stuff 2. called with: {kwargs=}"
+
     async def pull_thread(self, wallet: 'Abstract_Wallet', force: bool):
         wallet_data = self.wallets.get(wallet, None)
         if not wallet_data:
$ ./run_electrum -o --testnet to_plugins pull_labels_sync --kwargs='{"hey": 1}'
done stuff 2. called with: kwargs={'hey': 1}

$ ./run_electrum -o --testnet to_plugins pull_labels_async --kwargs='{"hey": 1}'
done stuff 1. called with: kwargs={'hey': 1}

@thecockatiel
Copy link
Contributor

However, the client cannot load plugins.

Can't we make it a special case and load cmdline.py or another new special file from plugins inside the Commands init or anywhere suitable? I can't see why @command() decorator cannot be used in other files

@SomberNight
Copy link
Member

However, the client cannot load plugins.

Can't we make it a special case and load cmdline.py or another new special file from plugins inside the Commands init or anywhere suitable? I can't see why @command() decorator cannot be used in other files

We could have a special keyword e.g. "plugin"/"plugincmd" (or maybe even the name of specific plugins), and if that keyword is the first argument in sys.argv, then we call init_plugins early, before argparse gets to run (so near the beginning of main()).
Then plugin code would get a chance to run and have @command() decorators executed, before argparse runs.

And then an example command invocation would be: $ ./run_electrum plugincmd upgrade_coldcard_firmware.


Note that we probably don't want to unconditionally load the plugins before argparse (hence this plugincmd idea) as that would make CLI invocations even slower. We already have a problem with our regtest functional tests that one reason they are slow is because they call a lot of commands and commands have a lot of overhead. Every time you run a command, it imports half of the codebase. Just try $ time ./run_electrum -o version_info, it takes 1+ second. Loading the plugins unconditionally would make it even slower. (but a proper fix would be some kind of larger redesign of commands -- probably having separate entry point scripts for e.g. starting the gui or running a command).

@thecockatiel
Copy link
Contributor

like rewriting it so each command lives as a file with the same name as command in a directory, if not found try each plugin's commands directory for a file with the same name?

@thecockatiel
Copy link
Contributor

thecockatiel commented Oct 27, 2024

okay I have changed my approach so I don't exactly need it to be directly after run_electrum,
./run_electrum plugincmd upgrade_coldcard_firmware sounds good enough for me

edit: I didn't need this all along. I decided to do it as a standalone project, later to be used as a lib inside the plugin, so yeah, not needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants