From 9d4148ce683223ec286d14c3d5bc737cc7ba66ed Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Mon, 5 Aug 2019 02:19:48 +0200 Subject: [PATCH] pylightning: Warn users of plugins that may break due to extra args We recently noticed that the way we unpack the call arguments for hooks and notifications in pylightning breaks pretty quickly once you start changing the hook and notification params. If you add params they will not get mapped correctly causing the plugin to error out. This can be fixed by adding a `VAR_KEYWORD` argument to the calbacks, i.e., by adding a single `**kwargs` argument at the end of the signature. This commit adds a check that such a catch-all argument exists, and emits a warning if it doesn't. It also fixes up the plugins that we ship ourselves. Signed-off-by: Christian Decker --- contrib/plugins/helloworld.py | 10 ++++---- contrib/pylightning/lightning/plugin.py | 26 +++++++++++++++++++++ tests/plugins/dblog.py | 2 +- tests/plugins/fail_htlcs.py | 2 +- tests/plugins/hold_htlcs.py | 2 +- tests/plugins/hold_invoice.py | 2 +- tests/plugins/pretend_badlog.py | 2 +- tests/plugins/reject.py | 2 +- tests/plugins/reject_odd_funding_amounts.py | 2 +- tests/plugins/reject_some_invoices.py | 2 +- tests/plugins/shortcircuit.py | 2 +- 11 files changed, 40 insertions(+), 14 deletions(-) diff --git a/contrib/plugins/helloworld.py b/contrib/plugins/helloworld.py index e7987329c..d74b96627 100755 --- a/contrib/plugins/helloworld.py +++ b/contrib/plugins/helloworld.py @@ -20,22 +20,22 @@ def hello(plugin, name="world"): @plugin.init() -def init(options, configuration, plugin): +def init(options, configuration, plugin, **kwargs): plugin.log("Plugin helloworld.py initialized") @plugin.subscribe("connect") -def on_connect(plugin, id, address): +def on_connect(plugin, id, address, **kwargs): plugin.log("Received connect event for peer {}".format(id)) @plugin.subscribe("disconnect") -def on_disconnect(plugin, id): +def on_disconnect(plugin, id, **kwargs): plugin.log("Received disconnect event for peer {}".format(id)) @plugin.subscribe("invoice_payment") -def on_payment(plugin, invoice_payment): +def on_payment(plugin, invoice_payment, **kwargs): plugin.log("Received invoice_payment event for label {}, preimage {}," " and amount of {}".format(invoice_payment.get("label"), invoice_payment.get("preimage"), @@ -43,7 +43,7 @@ def on_payment(plugin, invoice_payment): @plugin.hook("htlc_accepted") -def on_htlc_accepted(onion, htlc, plugin): +def on_htlc_accepted(onion, htlc, plugin, **kwargs): plugin.log('on_htlc_accepted called') time.sleep(20) return {'result': 'continue'} diff --git a/contrib/pylightning/lightning/plugin.py b/contrib/pylightning/lightning/plugin.py index 038f9d256..ab2c288a9 100644 --- a/contrib/pylightning/lightning/plugin.py +++ b/contrib/pylightning/lightning/plugin.py @@ -183,6 +183,19 @@ class Plugin(object): raise ValueError( "Topic {} already has a handler".format(topic) ) + + # Make sure the notification callback has a **kwargs argument so that it + # doesn't break if we add more arguments to the call later on. Issue a + # warning if it does not. + s = inspect.signature(func) + kinds = [p.kind for p in s.parameters.values()] + if inspect.Parameter.VAR_KEYWORD not in kinds: + self.log( + "Notification handler {} for notification {} does not have a " + "variable keyword argument. It is strongly suggested to add " + "`**kwargs` as last parameter to hook and notification " + "handlers.".format(func.__name__, topic), level="warn") + self.subscriptions[topic] = func def subscribe(self, topic): @@ -251,6 +264,19 @@ class Plugin(object): raise ValueError( "Method {} was already registered".format(name, self.methods[name]) ) + + # Make sure the hook callback has a **kwargs argument so that it + # doesn't break if we add more arguments to the call later on. Issue a + # warning if it does not. + s = inspect.signature(func) + kinds = [p.kind for p in s.parameters.values()] + if inspect.Parameter.VAR_KEYWORD not in kinds: + self.log( + "Hook handler {} for hook {} does not have a variable keyword " + "argument. It is strongly suggested to add `**kwargs` as last " + "parameter to hook and notification handlers.".format( + func.__name__, name), level="warn") + method = Method(name, func, MethodType.HOOK) method.background = background self.methods[name] = method diff --git a/tests/plugins/dblog.py b/tests/plugins/dblog.py index 1cf601075..2b985958a 100755 --- a/tests/plugins/dblog.py +++ b/tests/plugins/dblog.py @@ -24,7 +24,7 @@ def init(configuration, options, plugin): @plugin.hook('db_write') -def db_write(plugin, writes): +def db_write(plugin, writes, **kwargs): if not plugin.initted: plugin.log("deferring {} commands".format(len(writes))) plugin.sqlite_pre_init_cmds += writes diff --git a/tests/plugins/fail_htlcs.py b/tests/plugins/fail_htlcs.py index b925dff54..67cde7ce1 100755 --- a/tests/plugins/fail_htlcs.py +++ b/tests/plugins/fail_htlcs.py @@ -6,7 +6,7 @@ plugin = Plugin() @plugin.hook("htlc_accepted") -def on_htlc_accepted(htlc, onion, plugin): +def on_htlc_accepted(onion, plugin, **kwargs): plugin.log("Failing htlc on purpose") plugin.log("onion: %r" % (onion)) return {"result": "fail", "failure_code": 16399} diff --git a/tests/plugins/hold_htlcs.py b/tests/plugins/hold_htlcs.py index 3e4a1008a..0533b1b2a 100755 --- a/tests/plugins/hold_htlcs.py +++ b/tests/plugins/hold_htlcs.py @@ -17,7 +17,7 @@ plugin = Plugin() @plugin.hook("htlc_accepted") -def on_htlc_accepted(htlc, onion, plugin): +def on_htlc_accepted(htlc, onion, plugin, **kwargs): # Stash the onion so the test can check it fname = os.path.join(tempfile.mkdtemp(), "onion.json") with open(fname, 'w') as f: diff --git a/tests/plugins/hold_invoice.py b/tests/plugins/hold_invoice.py index ec11d18bf..ebdcf418c 100755 --- a/tests/plugins/hold_invoice.py +++ b/tests/plugins/hold_invoice.py @@ -9,7 +9,7 @@ plugin = Plugin() @plugin.hook('invoice_payment') -def on_payment(payment, plugin): +def on_payment(payment, plugin, **kwargs): time.sleep(float(plugin.get_option('holdtime'))) return {} diff --git a/tests/plugins/pretend_badlog.py b/tests/plugins/pretend_badlog.py index 854ec6c3c..23c7ec877 100755 --- a/tests/plugins/pretend_badlog.py +++ b/tests/plugins/pretend_badlog.py @@ -12,7 +12,7 @@ def init(configuration, options, plugin): @plugin.subscribe("warning") -def notify_warning(plugin, warning): +def notify_warning(plugin, warning, **kwargs): plugin.log("Received warning") plugin.log("level: {}".format(warning['level'])) plugin.log("time: {}".format(warning['time'])) diff --git a/tests/plugins/reject.py b/tests/plugins/reject.py index 64f9c4ff4..da39519ee 100755 --- a/tests/plugins/reject.py +++ b/tests/plugins/reject.py @@ -13,7 +13,7 @@ plugin = Plugin() @plugin.hook('peer_connected') -def on_connected(peer, plugin): +def on_connected(peer, plugin, **kwargs): if peer['id'] in plugin.reject_ids: print("{} is in reject list, disconnecting".format(peer['id'])) return {'result': 'disconnect', 'error_message': 'You are in reject list'} diff --git a/tests/plugins/reject_odd_funding_amounts.py b/tests/plugins/reject_odd_funding_amounts.py index 76c295da2..6dc5dc310 100755 --- a/tests/plugins/reject_odd_funding_amounts.py +++ b/tests/plugins/reject_odd_funding_amounts.py @@ -10,7 +10,7 @@ plugin = Plugin() @plugin.hook('openchannel') -def on_openchannel(openchannel, plugin): +def on_openchannel(openchannel, plugin, **kwargs): print("{} VARS".format(len(openchannel.keys()))) for k in sorted(openchannel.keys()): print("{}={}".format(k, openchannel[k])) diff --git a/tests/plugins/reject_some_invoices.py b/tests/plugins/reject_some_invoices.py index 99ac6103e..c6f8822e9 100755 --- a/tests/plugins/reject_some_invoices.py +++ b/tests/plugins/reject_some_invoices.py @@ -10,7 +10,7 @@ plugin = Plugin() @plugin.hook('invoice_payment') -def on_payment(payment, plugin): +def on_payment(payment, plugin, **kwargs): print("label={}".format(payment['label'])) print("msat={}".format(payment['msat'])) print("preimage={}".format(payment['preimage'])) diff --git a/tests/plugins/shortcircuit.py b/tests/plugins/shortcircuit.py index 3696c721f..8403b68ec 100755 --- a/tests/plugins/shortcircuit.py +++ b/tests/plugins/shortcircuit.py @@ -6,7 +6,7 @@ plugin = Plugin() @plugin.hook("htlc_accepted") -def on_htlc_accepted(onion, htlc, plugin): +def on_htlc_accepted(onion, htlc, plugin, **kwargs): return {"result": "resolve", "payment_key": "00" * 32}