Refactor Payment PR 2#9826
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 (
|
f670374 to
e13b29c
Compare
e13b29c to
26afc39
Compare
39ae57b to
bfdb4c1
Compare
bfdb4c1 to
6e866e2
Compare
We rename the file to payment_kv_store to highlight that this is the kv implementation of the payment db backend.
6e866e2 to
f90ba60
Compare
f90ba60 to
ba1907d
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).
ba1907d to
c52c3c9
Compare
c52c3c9 to
d5cb7de
Compare
We also addd this new db on the server level to use it in the following commit to do all the payment related queries of the rpcserver. We add a new payment db instance on the server level. Which we will you for the payment related queries in a following commit.
We move all kv particular code which was in the payments.go file to the kv related file. Code which will be backend independant will remain in the payments.go file although only the kv backend is currently supported.
All interactions related to the payment db are now part of the kvPaymentDB struct.
d5cb7de to
2b3b27f
Compare
yyforyongyu
left a comment
There was a problem hiding this comment.
LGTM🌹 I think for future PRs we can merge them to a side branch - you can also consider creating branches in upstream so this PR will be based on top of the branch #9825, thus the diff here will be 4 commits instead of 8.
|
|
||
| // KVPaymentsDB is the database that stores all payment related | ||
| // information. | ||
| KVPaymentsDB *channeldb.KVPaymentsDB |
There was a problem hiding this comment.
nit: can just name it PaymentsDB given it will be an interface supporting multiple types of DBs.
ellemouton
left a comment
There was a problem hiding this comment.
lgtm!
Agreed with YY re pushing upstream branches so that the diff just shows the commits in question.
But not fully agreed on the side-branch comment as imo, there is lots of value in getting these diffs into master so that the CI for all open PRs is exercising these changes (rather than just the side branch PR and the PRs pointing to it). That way we can be more confident about the changes at release time. I think that's less scary than the all-or-nothing approach of merging in the side-branch once it is huge/feature complete. Obvs open to debate :)
For refactoring PRs, sure. For feature-based PRs, I'm not sure how to ship them to master when the feature is not complete tho, given they are built in smaller PRs. I'm also not sure why it gives more confidence that way - maybe it's because it's merged, and everyone else would rebase the PR, and let it fail in other PRs' CIs? |
Builds on top of #9825, the first 4 commits are from the PR 1.
This Refactor PR does separates out the paymentDB currently just for the kv backend and unifies all payment related db queries under this unified umbrella. Before we would have a PaymentControl struct for payment lifecycle related data and then queries which fetched payments or deleted payments were implemented directly on the kvdb pointer receiver. This is now changed.
In the next PR we are going to introduce the new payments package, separate DB agnostic stuff more clearly.