-
-
Notifications
You must be signed in to change notification settings - Fork 121
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
Add CLN send via CLNRest #1981
base: master
Are you sure you want to change the base?
Add CLN send via CLNRest #1981
Conversation
export * from '@/wallets/cln' | ||
|
||
export async function testSendPayment ({ socket, runePay }, { logger, signal }) { | ||
// TODO: check rune |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing TODO.
We usually do "integration tests" here that involve the network unlike the formik/yup validation that triggers no network requests.
So for example, you could try to use the rune in some way that wouldn't work if the rune didn't exist but since we don't actually want to pay something (this was planned in #1341 but I got blocked by an issue), you can't actually use the pay
method. Maybe it helps to look at testSendPayment
from existing wallets.
{ | ||
name: 'runePay', | ||
label: 'pay rune', | ||
type: 'text', | ||
placeholder: 'S34KtUW-6gqS_hD_9cc_PNhfF-NinZyBOCgr1aIrark9NCZtZXRob2Q9aW52b2ljZQ==', | ||
hint: 'must allow method=pay', | ||
clear: true, | ||
clientOnly: true, | ||
optional: 'for sending', | ||
requiredWithout: 'rune', | ||
validate: string().matches(B64_URL_REGEX, { message: 'invalid rune' }) | ||
.test({ | ||
name: 'runePay', | ||
test: (v, context) => { | ||
if (!v) return true | ||
const decoded = decodeRune(v) | ||
if (!decoded) return context.createError({ message: 'invalid rune' }) | ||
return true | ||
} | ||
}) | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add help
like we have for the invoice only rune.
test: (v, context) => { | ||
if (!v) return true | ||
const decoded = decodeRune(v) | ||
if (!decoded) return context.createError({ message: 'invalid rune' }) | ||
return true | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't test that the rune can actually pay.
} | ||
|
||
export async function sendPayment (bolt11, { socket, runePay }, { logger, signal }) { | ||
const url = `https://${socket}/v1/pay` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't hardcode the protocol since we allow HTTP over Tor.
This also breaks testing in local dev so I couldn't test this.
So this will definitely require testing with a real CLNRest server.
We have a CLN node in our docker-compose setup that you should be able to test with.
Description
Fix #1967
Adds client.js to wallets/cln
Screenshots
Additional Context
I tested the /v1/pay call against the demo server cln-regtest-demo.blockstream.com with their demo rune.
First created an invoice, then paid that invoice.
The demo calls showed the syntax for parameters and result.
Checklist
Are your changes backwards compatible? Please answer below:
If the new field runePay isn't filled, there should be no regressions. Couldn't test configuring a server-only CLN wallet, though.
On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:
1
Ran sndev with COMPOSE_PROFILES=minimal,payments, configured cln wallet for sending, zapped a post to make a payment. It failed due to the demo credentials, of course.
So this will definitely require testing with a real CLNRest server.
For frontend changes: Tested on mobile, light and dark mode? Please answer below:
Desktop only.
Did you introduce any new environment variables? If so, call them out explicitly here:
No.