#824 Core Lightning Auto Withdraw #865
Conversation
dillon-co
requested review from
huumn and
ekzyis
and removed request for
huumn
There was a problem hiding this comment.
Before this gets merged, we will need to verify that the rune does not give us spending permissions before sending the rune to the server. We did that for LND autowithdraw and we'll want to do that here too.
Also please be extra sure you've tested this end to end before asking for review again. By end to end I mean the user inputs the values in the GUI and it withdrawals as expected. There have been a few things in this PR that would prevent it from working end to end and in order to ship it it'll need to work.
| </Info> | ||
| </div> | ||
| } | ||
| name='macaroon' |
There was a problem hiding this comment.
This won't work with this field named incorrectly.
| <h2 className='pb-2'>Core Lightning</h2> | ||
| <h6 className='text-muted text-center pb-3'>autowithdraw to your Core Lightning node</h6> | ||
| <h6 className='text-muted text-center pb-3'> You must have CLNRest working on your node. <a href='https://docs.corelightning.org/docs/rest\n\n'>More info here.</a></h6> |
worker/autowithdraw.js
Outdated
|
|
||
| const invoice = await fetch(`${socket}/v1/invoice`, options) | ||
|
|
||
| return await createWithdrawal(null, { invoice: invoice.payment_hash, maxFee }, { me, models, autoWithdraw: true }) |
There was a problem hiding this comment.
haven't looked at all the code but you can't pay an invoice hash
| } | ||
| }, | ||
| { settings, data }, { me, models }) | ||
| }, |
There was a problem hiding this comment.
As I mentioned in the prior review, we do not want credentials that can spend from a stacker's node to be sent to the server (even if we don't store them).
This check should be done on the client like we do with LND and be part of validation.
There was a problem hiding this comment.
Fixed and moved it to the validator. I'm also using the demo node url for the rune check to decouple the socket and rune from the validation
There was a problem hiding this comment.
Should we be sending runes to blockstream for all of our users? Or is this a placeholder while you work on something else?
There was a problem hiding this comment.
Originally it was a placeholder to make sure it worked so I didn't have to spin up my own core lightning node again. I had to do that because Polar was giving me grief trying to get the rest api working. But I kind of feel like it would be good to decouple the 2. I just pulled that URL from their rest api example page here. But idk what do you think? Also I am working on the statistics page at the moment. I've been doing these 2 at the same time
There was a problem hiding this comment.
There's this library but it hasn't been touched in a couple years and the health score looks pretty low
There was a problem hiding this comment.
But idk what do you think?
As a user I'd be pissed the application I'm using is sending credentials for my money to other people/companies. Wouldn't you?
There's this library but it hasn't been touched in a couple years and the health score looks pretty low
The healthscore is low for non-security reasons. It's a really simple package with a single function calling a few utility functions that easy to audit.
…always up because if the socket is invalid the test will fail
…e-lightning-auto-withdraw
|
Just leaving myself some TODOs:
|
closes #824
Definitely more involved than I thought it would be lol. Kind of difficult to test because your core lightning node needs the rest plugin working, but I couldn't figure that out with polar and had to build a core lightning node from scratch.