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

always lowercase payreq #149

Merged
merged 1 commit into from
Apr 9, 2022
Merged

always lowercase payreq #149

merged 1 commit into from
Apr 9, 2022

Conversation

kiwiidb
Copy link
Contributor

@kiwiidb kiwiidb commented Apr 8, 2022

When testing out Alby and Bluewallet on regtest, I noticed that Bluewallet puts in the invoice uppercase when calling payinvoice. This caused internal Alby payments to fail, as Alby could not find the invoice in the db.

@kiwiidb kiwiidb requested a review from bumi April 8, 2022 09:12
@bumi
Copy link
Contributor

bumi commented Apr 9, 2022

Can you explain that? why is it relevant that we pass in the invoice in lowercase to the DecodePayReq call?
if we always need it lowercase shouldn't we do it then here:

func (svc *LndhubService) DecodePaymentRequest(ctx context.Context, bolt11 string) (*lnrpc.PayReq, error) {
?

@kiwiidb
Copy link
Contributor Author

kiwiidb commented Apr 9, 2022

This variable is not only passed into the DecodePayReq call, but also below, in the AddOutgoingInvoice call. This causes a mismatch between lowercase and an uppercase payment request strings in the database.

@kiwiidb kiwiidb merged commit b3ffca4 into main Apr 9, 2022
@kiwiidb kiwiidb deleted the fix/lowercase-invoice branch April 9, 2022 07:49
@bumi
Copy link
Contributor

bumi commented Apr 10, 2022

ok, but then we should do it there. Making sure that we write lowercase in the DB and also use lowecase when we try too look it up.
Maybe AddOutgoingInvoice will be used somewhere else then we have the same problem again.

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.

2 participants