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

Mega-merge 1: Test invoice creation with description hash (WIP) #812

Merged
merged 7 commits into from Aug 1, 2022

Conversation

callebtc
Copy link
Collaborator

@callebtc callebtc commented Jul 30, 2022

The Mega-merge

This is the first PR of the Mega-merge series. The ultimate goal of the Mega-merge series it to use updated CLN and LND libraries and fix the use of description_hash for all wallets (which ultimately enables LNURL).

The Mega-merge series is separated into following parts in order to avoid total chaos and make review easier. All successive PRs have their predecessor merged into them already which is why the number of changed files is deceivingly large for the later PRs (they should go down as we merge one by one).

Dependency graph:

                      ┌───┐
Tests    Refactor   ┌─┤MM3│ CLN
┌────┐     ┌───┐    │ └───┘
│MM1 ├─────►MM2├────►
└────┘     └───┘    │ ┌───┐
                    └─┤MM4│ LND
                      └───┘

Mega-merge 1: Test invoice creation with description_hash (PR #812)

The purpose of this (present) PR is to add tests for invoice creation via the endpoint /api/v1/payments with description_hash. The tests are expected to fail but should succeed after the successive fixes in following merges.

Mega-merge 2: Refactor description_hash handling (PR #814)

This PR refactors how description_hash in handled in LNbits. Since most wallets (LND, Eclair, web-wallets) expect the hash of the description and others (CLN and FakeWallet/bolt11) expect the description itself, the process of hashing needs to be refactored to the very end of the pipeline into the wallets itself. Before, LNbits was hashing the description in all LNURL operations before calling create_invoice. After this PR, LNbits will pass the description and the wallet decides to hash it or not

Mega-merge 3: Update CLN to use description_hash (PR #792)

This PR updates the CLN library and enables the native use of description_hash. This PR is the reason why the previous two PR's exist. They are separated so that the previous parts can be merged before the lib is updated.

Mega-merge 4: Reenable LND GRPC and update invoice status checking (PR #745)

This PR is not directly related to the previous ones and could be merged separately. Here we updated the RPC stub to the latest version and use TrackPaymentV2 to check whether an outgoing payment has succeeded or not.

To be continued...

The end of this series (which I will tackle some other day after we merged all this) will be a new payment checking flow in which the payment will be created in the backend asynchronously and a task is kicked off that regularly checks the status of the payment. In theory, this should get rid of stuck payments. A precondition for this would be that all payments are written to the LNbits database with their payment_hash as the checking_id and not some post-hoc label that we only know when the payment is sent to the backend. More thoughts on this are written down in the notes of PR #745.

@codecov-commenter
Copy link

codecov-commenter commented Jul 30, 2022

Codecov Report

Merging #812 (123ea6c) into main (f24d593) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #812      +/-   ##
==========================================
+ Coverage   41.45%   41.53%   +0.07%     
==========================================
  Files         215      215              
  Lines       11271    11271              
==========================================
+ Hits         4672     4681       +9     
+ Misses       6599     6590       -9     
Impacted Files Coverage Δ
lnbits/bolt11.py 78.57% <100.00%> (+2.38%) ⬆️
lnbits/core/views/api.py 41.04% <0.00%> (+0.65%) ⬆️
lnbits/wallets/fake.py 76.08% <0.00%> (+4.34%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@callebtc callebtc changed the title WIP: test invoice creation with description hash Mega-merge 1: test invoice creation with description hash (WIP) Jul 31, 2022
@callebtc callebtc changed the title Mega-merge 1: test invoice creation with description hash (WIP) Mega-merge 1: Test invoice creation with description hash (WIP) Jul 31, 2022
@callebtc callebtc added the testing Code testing label Jul 31, 2022
@callebtc callebtc merged commit c88e6b0 into main Aug 1, 2022
@callebtc callebtc deleted the tests/description_hash_invoice branch August 1, 2022 14:12
@john-zaprite
Copy link

Does this solve #511?

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

Successfully merging this pull request may close these issues.

None yet

3 participants