-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Refactor Payment PR 1 #9825
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
Refactor Payment PR 1 #9825
Conversation
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
fb6f2c6
to
3635b27
Compare
3635b27
to
cedd931
Compare
6c85629
to
da88254
Compare
da88254
to
3a02bcd
Compare
@yyforyongyu: review reminder |
d357969
to
d8cb28e
Compare
We rename the file to payment_kv_store to highlight that this is the kv implementation of the payment db backend.
e22c3bd
to
a2705a0
Compare
In the following commits we will gradually unify the current payment db operations into an interface to later down the road support both backends (sql+kv).
a2705a0
to
67dd72d
Compare
67dd72d
to
1f2efd2
Compare
1f2efd2
to
84b2a94
Compare
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.
Pretty clean and straightforward! Can you confirm the itest failure is pre-existing? Guess it's related to #9912.
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 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.
ah, actually, quick question: should we not instead rename the test file s.t. it doesnt include 'kv' in the name? cause wont we want to run the unit tests against all backend types depending on build flags (like is done in graph/db/graph_test.go)?
yes we will run them against the interface backend, but this will be part of a future commit. So we will move all tests which can run against all backends into a separate file in the paymentsDB package which will be introduced later. |
@ziggie1984 - cool, sgtm 👍 |
This PR is the start of a refactor series to finally support both payment backends out of the box for LND.
This PR starts with unifying the PaymentDB code which is currently splitted across multiple files and introducing a new KVPaymentDB struct at the server level. Apart from the second commit which adds a new DB to the server level (but does not use it yet) all of the changes are only refactors so nothing changed.