Skip to content
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 LNURL payRequest #291

Closed
wants to merge 1 commit into from
Closed

Add LNURL payRequest #291

wants to merge 1 commit into from

Conversation

Dolu89
Copy link

@Dolu89 Dolu89 commented Sep 9, 2021

It aims to add a LNURL payRequest for each BlueWallet user (BlueWallet requires some updates to implement this)

I added a new "key" named publicid to not reveal users id which are used in the secret string when creating a wallet

@Overtorment Overtorment self-requested a review September 22, 2021 13:04
let userid = await this._redis.get('userid_for_' + publicId);

if (userid) {
this._userid = userid;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't you also set this._publicid?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, all the loadBy* methods should also fetch _publicid and store it in the _publicid.

@@ -131,6 +149,21 @@ export class User {
if (config.bitcoind) return this._bitcoindrpc.request('importaddress', [address, address, false]);
}

async getPublicId() {
if (!this._userid) return;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this._publicid exists, it should probably return early.

});
}

const satAmount = req.query.amount / 1000;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: Currently the largest channel is 10 BTC, so in millisatoshis, that would fit within Number.MAX_SAFE_INTEGER, but keep in mind that once you pass 90071.99 BTC this will break in weird ways silently.

I am sure other parts of LNDHub assume all integers will be fine too... so this is probably not a problem worth fixing right now.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this should enforce the minSendable / maxSendable you return above.

Return an error if they try to request an amount outside the parameters.

Copy link

@junderw junderw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comments

@@ -192,6 +205,71 @@ router.post('/addinvoice', postLimiter, async function (req, res) {
);
});

router.get('/lnurlp/:publicid/qr', postLimiter, async function (req, res) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
router.get('/lnurlp/:publicid/qr', postLimiter, async function (req, res) {
router.get('/lnurlp/:publicid/qr', postLimiter, async function (req, res) {

With this, Lightning addresses publicid@lndhub.io would be possible.

}

logger.log('/publicid', [req.id, 'userid: ' + u.getUserId(), 'invoice: ' + req.body.invoice]);
res.send(await u.getPublicId());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't you return json here?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, you might want to return the public id in the response on account creation.

@godSaysHODL
Copy link

godSaysHODL commented Feb 22, 2023

Can we make this feature happen? Would be the easiest way to get a lightning address on my node where I'm already using LNDhub 24/7

Will give 50 USD worth of sats to all five currrent participants when put into a release build, as a bounty. Obviously a way to figure out or set an accounts publicID in the clientside applications would be awesome too

@Overtorment @Dolu89 @junderw @kiwiidb @AaronDewes

@kiwiidb
Copy link

kiwiidb commented Feb 23, 2023

Can we make this feature happen? Would be the easiest way to get a lightning address on my node where I'm already using LNDhub 24/7

Will give 50 USD worth of sats to all five currrent participants when put into a release build, as a bounty. Obviously a way to figure out or set an accounts publicID in the clientside applications would be awesome too

@Overtorment @Dolu89 @junderw @kiwiidb @AaronDewes

I think it has been communicated this repo won't merge any new features anymore.

@iBobik
Copy link

iBobik commented Feb 23, 2023

Can we make this feature happen? Would be the easiest way to get a lightning address on my node where I'm already using LNDhub 24/7

Will give 50 USD worth of sats to all five currrent participants when put into a release build, as a bounty. Obviously a way to figure out or set an accounts publicID in the clientside applications would be awesome too

Wallet of Satoshi support this already:
IMG_9B6E56E725C0-1

Then you can use https://lnurl.fiatjaf.com/codec/ to encode it to the clickable url like this:

wateryjam05@walletofsatoshi.comlightning:lnurl1washgetj094xzmfsx4q8wctvd3jhgmmxwdshgmmndp5jucm0d5907lja

(You can send part of the bounty to me 😉)

@iBobik
Copy link

iBobik commented Feb 23, 2023

Also BTCPay server supports this by POS app in printable QR codes mode and LN addresses in store settings.

@godSaysHODL
Copy link

Can we make this feature happen? Would be the easiest way to get a lightning address on my node where I'm already using LNDhub 24/7
Will give 50 USD worth of sats to all five currrent participants when put into a release build, as a bounty. Obviously a way to figure out or set an accounts publicID in the clientside applications would be awesome too
@Overtorment @Dolu89 @junderw @kiwiidb @AaronDewes

I think it has been communicated this repo won't merge any new features anymore.

That's a true shame if true 😪

@Dolu89
Copy link
Author

Dolu89 commented Feb 23, 2023

Can we make this feature happen? Would be the easiest way to get a lightning address on my node where I'm already using LNDhub 24/7
Will give 50 USD worth of sats to all five currrent participants when put into a release build, as a bounty. Obviously a way to figure out or set an accounts publicID in the clientside applications would be awesome too
@Overtorment @Dolu89 @junderw @kiwiidb @AaronDewes

I think it has been communicated this repo won't merge any new features anymore.

That's a true shame if true 😪

It’s true
https://twitter.com/bluewalletio/status/1628733798438281216

I’m closing this PR

@Dolu89 Dolu89 closed this Feb 23, 2023
@godSaysHODL
Copy link

Can we make this feature happen? Would be the easiest way to get a lightning address on my node where I'm already using LNDhub 24/7
Will give 50 USD worth of sats to all five currrent participants when put into a release build, as a bounty. Obviously a way to figure out or set an accounts publicID in the clientside applications would be awesome too
@Overtorment @Dolu89 @junderw @kiwiidb @AaronDewes

I think it has been communicated this repo won't merge any new features anymore.

That's a true shame if true 😪

It’s true https://twitter.com/bluewalletio/status/1628733798438281216

I’m closing this PR

Them sunsetting the bluewallet node doesn't mean changes couldn't be merged for people who self host, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants