Refactor Payment PR 4#9847
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 (
|
ziggie1984
force-pushed
the
refactor-payments-code-04
branch
2 times, most recently
from
2538b0e to
2880b9a
Compare
ziggie1984
force-pushed
the
refactor-payments-code-04
branch
2 times, most recently
from
7e1853b to
10ef9e5
Compare
ziggie1984
force-pushed
the
refactor-payments-code-04
branch
2 times, most recently
from
c30e5fa to
3680d42
Compare
ziggie1984
force-pushed
the
refactor-payments-code-04
branch
from
3680d42 to
184a58b
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant refactoring that moves payment-related logic from the channeldb package to a new payments package. The changes are largely consistent with this goal. However, I've identified a few critical issues related to incorrect function calls between packages, which will likely cause compilation errors. Specifically, the new paymentsdb package attempts to call unexported functions from channeldb and also uses the channeldb codec for types that are now handled by the paymentsdb codec. Additionally, there's a minor formatting issue in an error message that could be improved for conciseness.
ziggie1984
force-pushed
the
refactor-payments-code-04
branch
from
4d3e78e to
1b88e4b
Compare
|
The above unit test fail because we haven't yet introduced the native sql backend code for payments which will be done in PR 6 onward. |
ziggie1984
force-pushed
the
refactor-payments-code-04
branch
4 times, most recently
from
6bbf37b to
ac2726d
Compare
yyforyongyu
left a comment
yyforyongyu
left a comment
There was a problem hiding this comment.
Looking good - think we just need to fix the CI and one more question re db initialization and removal.
| invoiceBucket, | ||
| payAddrIndexBucket, | ||
| setIDIndexBucket, | ||
| paymentsIndexBucket, |
There was a problem hiding this comment.
does it mean we no longer init the payments db in initChannelDB or remove it in Wipe?
There was a problem hiding this comment.
exactly we have a separate init method now in the kv store => initKVStore
There was a problem hiding this comment.
And the wipe method was only used in tests, which did not affect the payment related data and I don't think we currently need a wipe test on the paymentsdb package side, because the test seems not really necessary imo.
| type UnknownElementType = channeldb.UnknownElementType | ||
|
|
||
| // ReadElement deserializes a single element from the provided io.Reader. | ||
| func ReadElement(r io.Reader, element interface{}) error { |
There was a problem hiding this comment.
So this is new method right?
There was a problem hiding this comment.
yes, we need it because we move the paymentIndexType definition into the payment pkg, and don't want to import the paymentsdb into the channeldb to create a circular dependancy.
ziggie1984
force-pushed
the
refactor-payments-code-04
branch
from
ac2726d to
feda7ed
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a large refactoring that moves payment-related logic from the channeldb package into a new payments/db package. This is a significant architectural improvement that increases modularity and separation of concerns. The changes are mostly mechanical, involving moving files, updating import paths, and renaming some functions for better clarity.
My review focuses on ensuring the refactoring is clean and doesn't introduce new issues. I've identified a case of code duplication, a potential issue in a test file related to concurrent execution, and a line length violation. Overall, the refactoring is well-executed and a positive change for the codebase.
| // deserializeTime deserializes time as unix nanoseconds. | ||
| func deserializeTime(r io.Reader) (time.Time, error) { | ||
| var scratch [8]byte | ||
| if _, err := io.ReadFull(r, scratch[:]); err != nil { | ||
| return time.Time{}, err | ||
| } | ||
|
|
||
| // Convert to time.Time. Interpret unix nano time zero as a zero | ||
| // time.Time value. | ||
| unixNano := byteOrder.Uint64(scratch[:]) | ||
| if unixNano == 0 { | ||
| return time.Time{}, nil | ||
| } | ||
|
|
||
| return time.Unix(0, int64(unixNano)), nil | ||
| } | ||
|
|
||
| // serializeTime serializes time as unix nanoseconds. | ||
| func serializeTime(w io.Writer, t time.Time) error { | ||
| var scratch [8]byte | ||
|
|
||
| // Convert to unix nano seconds, but only if time is non-zero. Calling | ||
| // UnixNano() on a zero time yields an undefined result. | ||
| var unixNano int64 | ||
| if !t.IsZero() { | ||
| unixNano = t.UnixNano() | ||
| } | ||
|
|
||
| byteOrder.PutUint64(scratch[:], uint64(unixNano)) | ||
| _, err := w.Write(scratch[:]) | ||
|
|
||
| return err | ||
| } |
There was a problem hiding this comment.
The functions deserializeTime and serializeTime are added here, but they also exist in the new payments/db/codec.go file. This appears to be code duplication introduced during the refactoring. Since these functions are related to payment serialization and are used within the new payments package, they should likely only reside in payments/db/codec.go. Please remove the duplicated functions from this file.
|
|
||
| case channeldb.FailureReasonPaymentDetails: | ||
| case paymentsdb.FailureReasonPaymentDetails: | ||
| return lnrpc.PaymentFailureReason_FAILURE_REASON_INCORRECT_PAYMENT_DETAILS, nil |
There was a problem hiding this comment.
This line exceeds the 80-character limit specified in the style guide.1 Please refactor it for better readability and to adhere to the coding standards.
| return lnrpc.PaymentFailureReason_FAILURE_REASON_INCORRECT_PAYMENT_DETAILS, nil | |
| reason := lnrpc.PaymentFailureReason_FAILURE_REASON_INCORRECT_PAYMENT_DETAILS | |
| return reason, nil |
Style Guide References
Footnotes
| } | ||
|
|
||
| for _, test := range tests { | ||
| test := test |
There was a problem hiding this comment.
The line test := test has been removed. This line was likely there to prevent a race condition by creating a new variable for the closure in the subtest. If the Go version used by this project is older than 1.22, removing this line can lead to tests failing intermittently because the test variable will be captured by reference and may change before the subtest goroutine executes. Please restore this line to ensure test correctness across different Go versions.
We export some methods related to the pagination logic be the kv store implemenation of the payment data will live in another package.
This method is not only used by the payment logic so we need to move it to a generalized place because in the following commits we move payment related code into its own package.
ziggie1984
force-pushed
the
refactor-payments-code-04
branch
2 times, most recently
from
19145a9 to
b4419dd
Compare
This commit moves most of the code into its own package. It is the smallest code move possible without moving import cycles and keeping the changes to the code base as small as possible during refactor.
We also rename the struct since it is now in its own package there is no need to prefix it.
ziggie1984
force-pushed
the
refactor-payments-code-04
branch
from
b4419dd to
2b856f0
Compare
3 participants
Depends on
#9844
This moves all data payment related data into the payment package.
The first commit is quite huge because otherwise function more changes would have been needed. Notice that we still maintain the dependence of the
channeldbin the payment package. See also the codec.go file where we enhance the kv related parsing methods by the payment index.