feat: restructure plugin and options in listconfigs #3206

[m-schmoock]

Addresses #3206 and #3281

This will restructure the lightning-cli listconfigs output in a way that plugins are not duplicated and have their options within a substructure like this:

{
   "# version": "v0.7.3-141-gabdc4af",
   "lightning-dir": "/home/will/.lightning",
   "wallet": "sqlite3:///home/will/.lightning/lightningd.sqlite3",
   "plugins": [
      {
         "name": "autoclean",
         "path": "/home/will/projects/lightning.git/lightningd/../plugins/autoclean",
         "options": {
            "autocleaninvoice-cycle": null,
            "autocleaninvoice-expired-by": null
         }
      },
      {
         "name": "fundchannel",
         "path": "/home/will/projects/lightning.git/lightningd/../plugins/fundchannel"
      },
      {
         "name": "pay",
         "path": "/home/will/projects/lightning.git/lightningd/../plugins/pay"
      },
      {
         "name": "sendinvoiceless.py",
         "path": "/home/will/.lightning/plugins/sendinvoiceless/sendinvoiceless.py",
         "options": {
            "cltv-final": "10",
            "fee-base": "null",
            "fee-per-satoshi": "null"
         }
      },
      {
         "name": "rebalance.py",
         "path": "/home/will/.lightning/plugins/rebalance/rebalance.py",
         "options": {
            "cltv-final": "10"
         }
      },
      {
         "name": "summary.py",
         "path": "/home/will/.lightning/plugins/summary/summary.py",
         "options": {
            "summary-currency": "USD",
            "summary-currency-prefix": "USD $"
         }
      },
      {
         "name": "drain.py",
         "path": "/home/will/.lightning/plugins/drain/drain.py",
         "options": {
            "cltv-final": "10"
         }
      }
   ],
   "network": "testnet",
   "allow-deprecated-apis": false,
   "..." : "..."
}

[m-schmoock]

We could think about add a "name" feature to a plugin so we dont have to use the long command path-name as key...

[m-schmoock]

@cdecker how about adding a "name" to the plugin interface. For all the old plugins we guess the name by inspecting the commands paths lasts elements:

• if directory, take submost directory name
• if filename, take this name and remove extensions like '.py' and such

If we do this we can make this nicely without update all the plugins first...

[cdecker]

@cdecker how about adding a "name" to the plugin interface. For all the old plugins we guess the name by inspecting the commands paths lasts elements:

• if directory, take submost directory name
• if filename, take this name and remove extensions like '.py' and such

If we do this we can make this nicely without update all the plugins first...

I think this is a great idea, though we should also still display the path somewhere so we have a mapping between the file being executed and the (self-assigned) alias.

[m-schmoock]

We can address the dedicated plugin naming to a separate pull request...

[niftynei]

Awesome! Needs a Changelog-Deprecated note for changing how the JSON prints though :)

[m-schmoock]

I wonder if we can improve on the yet untyped/null values, what are those, date/times?:

            "autocleaninvoice-cycle": null,
            "autocleaninvoice-expired-by": null

From a code perspective autocleaninvoice-cycle is a u64, so should be possible to output this as json int, autocleaninvoice-expired-by should be a string, but the code that checks the type of that option does not return it as string. Seems like if (opt->value->as_str) and if (opt->value->as_int) does not work here.

Update: seems like those values are C NULL values, however for other python plugins add_option('some_name', None, 'some description') gives you a string "null", the latter one is the problem, for example sendinvoiceless reports an unset 'integer' value as "fee-base": "null", which is wrong in three ways:

• it actually holds a non-null runtime value
• the type should be integer
• if it would really be null it should not be reported as a string containing the 4 letters "null".

Open Questions:

• How do we handle python plugin parameters more correctly that are None or non-string types?
• Can the C code reflect the current (runtime) value of a plugins options? It seems to me that the current way only outputs the plugins default values ("fee-per-satoshi": "null" should no be null, as the value is set in plugins init() method.)
• Currently some plugins (i.e. mine and @gallizoltan ) are using values that should be taken from clightningd directly instead of redefining a runtime copy of them :D

We could/should address these problems in another PR...

[darosior]

ACK 82ef5f5

I have some nits (ah and the commit message is wrong (_cmd)) about the code, but the interface is much cleaner that way !
I think too that this "null" fields should be fixed as part of another PR.

I wonder if we can improve on the yet untyped/null values, what are those, date/times?:

I think having null values is OK. Don't forget that's Javascript Object Notation so the typing is ... You know.... :D

How do we handle python plugin parameters more correctly that are None or non-string types?

By setting them to null Pylightning side ?..

Can the C code reflect the current (runtime) value of a plugins options? It seems to me that the current way only outputs the plugins default values ("fee-per-satoshi": "null" should no be null, as the value is set in plugins init() method.)

Shouldn't be set as a default instead of at init() ? Or even add a method to set it, but I don't think a startup option is meant to be set at runtime.

[m-schmoock]

Okay, lets use the basename including extension for now until plugin can set their name properly.
Parameter type binding and naming can be addressed later as we all seem to agree.

[darosior]

ACK 02059ee

[darosior]

@m-schmoock Regarding

How do we handle python plugin parameters more correctly that are None or non-string types?

By setting them to null Pylightning side ?..

I gave a quick look to pyln and they seem to actually be set to null :

lightning/contrib/pyln-client/pyln/client/plugin.py

Lines 416 to 419 in 6defc69

	def _write_locked(self, obj):
	# ensure_ascii turns UTF-8 into \uXXXX so we need to suppress that,
	# then utf8 ourselves.
	s = bytes(json.dumps(obj, cls=LightningRpc.LightningJSONEncoder, ensure_ascii=False) + "\n\n", encoding='utf-8')

...

Seems to have been addressed.