request-invoice #90
request-invoice #90
Conversation
|
Unrelated Travis error, I think #91 fixes that... Could you add a basic test just to make sure we don't break your plugin's commands upstream ? |
|
You need to make your plugin executable... Did you test locally ? |
Codecov Report
@@ Coverage Diff @@
## master #90 +/- ##
==========================================
+ Coverage 52.46% 52.65% +0.18%
==========================================
Files 17 18 +1
Lines 1984 2053 +69
==========================================
+ Hits 1041 1081 +40
- Misses 943 972 +29
Continue to review full report at Codecov.
|
README.md
Outdated
| [js-api]: https://github.com/darosior/clightningjs | ||
| [monitor]: https://github.com/renepickhardt/plugins/tree/master/monitor | ||
| [reckless]: https://github.com/darosior/reckless | ||
| [request-invoice]: https://github.com/Alex-CodeLab/plugins/tree/master/request-invoice |
There was a problem hiding this comment.
This should probably link to the local directory, since the code is here as well 😉
| [request-invoice]: https://github.com/Alex-CodeLab/plugins/tree/master/request-invoice | |
| [request-invoice]: https://github.com/lightningd/plugins/tree/master/request-invoice |
request-invoice/requestinvoice.py
Outdated
| limiter = Limiter( | ||
| app, | ||
| key_func=get_remote_address, | ||
| default_limits=["200 per day", "50 per hour"] |
There was a problem hiding this comment.
According to the flask-limiter docs this is per IP address, correct?
request-invoice/requestinvoice.py
Outdated
| def getinvoice(amount, description): | ||
| global plugin | ||
| label = "ln-getinvoice-{}".format(random()) | ||
| invoice = plugin.rpc.invoice(int(amount)*1000, label, description) |
There was a problem hiding this comment.
If you skip this multiplication you could potentially use things like 1msat or 0.01btc which might be a nicer interface.
We could go one step further and add fiat denominated amounts, and use an exchange rate to convert that into something that c-lightning understands.
request-invoice/requestinvoice.py
Outdated
| @limiter.limit("20 per minute") | ||
| def getinvoice(amount, description): | ||
| global plugin | ||
| label = "ln-getinvoice-{}".format(random()) |
There was a problem hiding this comment.
Using random here might cause collisions. Maybe we can use the plugin startup time and a monotonic counter? Or if you want to stick with random labels you could use the uuid module.
request-invoice/requestinvoice.py
Outdated
|
|
||
|
|
||
| # change this | ||
| secret= 'caba27ba-45c7-4495-aa53-fd6a5866fbd8' |
There was a problem hiding this comment.
Better grab this from an environment variable, or make it a command line option. Otherwise changing the secret means having to change the source code.
request-invoice/requestinvoice.py
Outdated
| # if command unknown make start our default command | ||
| if command not in commands: | ||
| command = "start" |
There was a problem hiding this comment.
This kind of clobbering the command if unknown may be strange. For example if I call invoiceserver STOP 1234 it'd start a new one, because the subcommand is not in the list. Better to return an error here.
request-invoice/requestinvoice.py
Outdated
| try: | ||
| port = int(port) | ||
| except: | ||
| port = int(plugin.options['invoice-web-port']['value']) |
There was a problem hiding this comment.
This is also rather unexpected, why not just return an error message telling the user that this is not a valid port number?
request-invoice/requestinvoice.py
Outdated
| if port in jobs: | ||
| return False, "server already running" |
There was a problem hiding this comment.
This check is duplicated in the invoiceserver RPC method.
request-invoice/requestinvoice.py
Outdated
|
|
||
| if command == "stop": | ||
| if stop_server(port): | ||
| return "stopped server on port{}".format(port) |
There was a problem hiding this comment.
| return "stopped server on port{}".format(port) | |
| return "stopped server on port {}".format(port) |
| full_path = "python3 " + plugin_path | ||
| check_output(["pkill","-f", full_path]) |
There was a problem hiding this comment.
Yep, that's going to be a problem. Hence my suggestion of using threading.Thread() instead of multiprocessing.Process().
Codecov Report
@@ Coverage Diff @@
## master #90 +/- ##
==========================================
+ Coverage 57.32% 57.34% +0.01%
==========================================
Files 22 23 +1
Lines 2580 2649 +69
==========================================
+ Hits 1479 1519 +40
- Misses 1101 1130 +29
Continue to review full report at Codecov.
|
|
care to explain? |
|
My bad, I misinterpreted this as being a stale PR, and closed it prematurely. I'll review and merge if appropriate. |
|
Hi @Alex-CodeLab , what's the state of this ? |
|
@hi Daroior |
3 participants
This plugin starts a minimal, rate limited HTTP Server and returns a invoice.