-
Notifications
You must be signed in to change notification settings - Fork 152
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
Fix lnproxy support #586
Fix lnproxy support #586
Conversation
- Use POST instead of GET, so create and send a body parameter - Path is /spec/ instead of /api/, and list of relays from lnproxy will contain /spec already, so path parameter for ApiClient.post() is an empty string
doing this so that the “scripts” subfolder in .github/workflows can be added
Locale strings not added yet
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 so soo top! 🚀 Thank you very much for investing the time into getting this done.
I will find the time before tomorrow to run the UI and comment on it.
const fs = require('fs'); | ||
|
||
let rawRelays = JSON.parse(fs.readFileSync('./lnproxy_relays.json')); | ||
let formattedRelays = []; |
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.
There is a chance the /lnproxy/...relays.json
does not see much action (or stops being maintained altogether). In such case, we might start adding ourselves new relays into ./frontend/static/lnproxies.json
. This script will remove the new proxies we add. Maybe it's best to join existing and incoming proxies instead of starting blank []
.
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.
OK. I updated the script so that only new proxies that appear in /lnproxy/...relays.json
will be added to the RoboSats list. We can easily make a more sophisticated merge in the future if needed.
.gitignore
Outdated
### VirtualEnv template | ||
# Virtualenv | ||
# http://iamzed.com/2009/05/07/a-primer-on-virtualenv/ | ||
[Bb]in |
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.
Awesome. On a recent PR I also moved a couple scripts into /scripts
and deleted these lines from .gitignore
. Maybe that creates a small merge conflict, not a problem though.
clearnetCount++; | ||
} | ||
|
||
let relayName = `LNProxy ${relayType}${relayType === "TOR" ? torCount : ''}${relayType === "I2P" ? i2pCount : ''}${relayType === "Clearnet" ? clearnetCount : ''}`; |
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.
What about using the first 6 characters of the domain name ? e.g.:
let relayName = ${relayType}${relayType === "TOR" ? torCount : ''}${relayType === "I2P" ? i2pCount : ''}${relayType === "Clearnet" ? clearnetCount : ''} ${url.split('/')[2].substring(0,6)}
So it would read: TOR1 w3sqmn I think something in this line will make the dropdown menu more transparent for the user (rather than just a number, as the user does not know what is behind the number)
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.
Yeah, this could go in many ways, there's probably more you could inspect on a relay other than just the name. For example status - some kind of ping to show whether the host is up (currently one of the tor lnproxy relays is down).
But first six is a good start :)
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.
We will just need to keep an eye on the relay list - it's OK for now but something like "https://www.paidrelayproxiesfromthefuture.com" would render as "Clearnet1 www.pa"
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.
It's looking great. Was just able to test it over TOR / mainnet. The filtered list showed me only the Clearnet1
server, not sure where the issue was, maybe the missing dependency settings.host
.
I was able to get a request through, but the response was "Internal Error", I think though that everything is working fine with those three fixes I commented.
- Change hook deps from settings.network to settings - routing_msat param updates for lnproxy API
Thanks! I've made the changes. Busy trying to figure out how I can also test these changes over TOR/mainnet. I've got the mainnet onion wired up, just figuring out how to expose my running npm as a hidden service. |
Updated and tested on mainnet |
Tested successfully over Clearnet and TOR. The proxied payment made it through on the first attempt, completing the whole order process 🚀 I did not get to text the GH workflow, but we will see how that works :D Thank you once again for this great work! @shyfire131 Please, accept a small tip from our devfund of 200K Sats. Submit an invoice with a +24h expiration time and from a proxy nodeid (...lnproxy!?). |
lnbc2m1pjxv7wepp5uw0e9ggdjjc6rzutcy8gxrq73qp2scqn8mq4evnmr6k2xzk6y4nsdqqcqpjsp5w2vygz377ty752x96nwgywq7k970tggduszatlqs7nmkdazrx0lq9q7sqqqqqqqqqqqqqqqqqqqsqqqqqysgqmqz9gxqyjw5qrzjqwryaup9lh50kkranzgcdnn2fgvx390wgj5jd07rwr3vxeje0glcllczrm9kqyzs5sqqqqlgqqqqqeqqjqz45lq0j8d5jdslhrcj4gcz25zl05964fpn0m0krmmgdt876sf5kr0ve83yydylpzzrarf637vxqfh576pppwact74uya9q87j9df63sqr7ngjv |
|
b65a75953e044a64e42a1a3101e0edb60f241dfd89d963e90b48678091609552 |
Received, thanks! (apologies, I had deleted my comment with the invoice because it had a 24 HR expiry and not > 24 hours as you had commented) |
What does this PR do?
Fixes #560
This PR introduces a GitHub workflow. The workflow fetches the LnProxy relay list which is now stored in a standalone JSON file: https://github.com/lnproxy/lnproxy-webui2/blob/e2b8cf00393e0e2d1b71a346d24e6b7ac3ad95eb/assets/relays.json
The LnProxy API calls have also been updated to reflect the new structure used in that project.
Lastly this PR refactors the error handling slightly, since Tor-only relays seem to be a thing
Checklist before merging
pip install pre-commit
, thenpre-commit install
. Pre-commit installs git hooks that automatically check the codebase. If pre-commit fails when you commit your changes, please fix the problems it points out.cd frontend/static/locales; python collect_phrases.py
to collect them for translation.Note: The output of the
collect_phrases.py
script was a bit of a mess, so I didn't commit the changes