invoice+payment: fix timestamp precision for queries #8432
Conversation
WalkthroughThe update focuses on enhancing date handling and precision in payments and invoices within the system. It shifts from using Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between 0d37621 and cd5bd20b133d5ab5eecb5aa79f9d99b8451c892b.Files selected for processing (7)
- channeldb/invoices.go (2 hunks)
- channeldb/payments.go (3 hunks)
- docs/release-notes/release-notes-0.18.0.md (1 hunks)
- invoices/interface.go (2 hunks)
- itest/list_on_test.go (1 hunks)
- itest/lnd_payment_test.go (3 hunks)
- rpcserver.go (2 hunks)
Files skipped from review due to trivial changes (1)
- itest/list_on_test.go
Additional comments: 17
invoices/interface.go (2)
- 128-130: The transition from
time.Timetoint64forCreationDateStartcorrects the timestamp precision issue. Ensure all related queries and logic are updated to handleint64Unix timestamp values.- 134-134: The transition from
time.Timetoint64forCreationDateEndaligns with the system-wide standardization of timestamp precision. Verify that all dependent functionalities are adapted to work with Unix timestamp in seconds.docs/release-notes/release-notes-0.18.0.md (1)
- 76-78: The addition to the release notes accurately reflects the changes made to address the timestamp precision issue. This documentation will help users understand the improvements in querying payments and invoices.
itest/lnd_payment_test.go (2)
- 169-169: The removal of code related to fetching the number of invoices and directly creating an invoice for testing simplifies the test setup. This change makes the test more focused on the functionality being tested.
- 205-293: The addition of tests for timestamp filters in
ListPaymentsandListInvoicesfunctions is crucial for validating the changes made to timestamp handling. Ensure these tests cover edge cases, such as dates exactly on the boundary of the filter range.channeldb/payments.go (3)
- 560-572: The conversion of
CreationTimeto Unix seconds (createTime := payment.Info.CreationTime.Unix()) is correct and aligns with the PR's objective to standardize timestamp precision. However, ensure that all instances whereCreationTimeis used throughout the codebase are updated to reflect this change in data type and logic.Verification successful
The script output indicates that
CreationTimeis handled consistently across the codebase, with conversions to Unix seconds where appropriate, and in alignment with the PR's objective for standardized timestamp precision. The instances found, including in tests and protobuf definitions, do not conflict with the PR's changes but rather support a coherent approach to timestamp handling. Therefore, the initial review comment concern appears to be addressed adequately within the provided context.* 515-515: The initialization of `resp PaymentsResponse` is correct and follows Go's idiomatic way of declaring variables. This ensures that the variable is ready to be used within the function scope and is reset in case of an error, as seen in the deferred function call. * 515-515: Consider evaluating the performance impact of the new timestamp filtering logic in `QueryPayments`, especially for large datasets. The current implementation may be sufficient, but it's important to verify that the changes do not significantly degrade query performance.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for instances where CreationTime is used but not converted to Unix seconds. rg --type go 'CreationTime[^.Unix()]'Length of output: 1256
channeldb/invoices.go (7)
- 537-555: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [500-552]
The logic within
QueryInvoicesfor filtering invoices based on creation time uses direct comparisons withcreateTime(Unix timestamp in seconds). This approach is consistent and efficient for the purpose of filtering invoices within a specified date range. The removal ofstartDateSetandendDateSetin favor of directly using Unix timestamps simplifies the logic and improves readability.
- 537-555: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [500-552]
In the
QueryInvoicesmethod, the pagination and filtering logic is well-implemented. The use of a paginator to handle both forward and reverse iteration through invoice records is a good practice, ensuring that the code can efficiently handle large datasets. Additionally, the final step of reversing the slice of invoices when queried in reverse order ensures that the returned list is always in the expected order, enhancing the usability of this method.
- 537-555: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [500-552]
The conversion of
CreationDateto Unix timestamp (createTime := invoice.CreationDate.Unix()) for comparison againstq.CreationDateStartandq.CreationDateEndis a straightforward and effective way to filter invoices by creation date. This approach, combined with the removal ofstartDateSetandendDateSet, streamlines the date handling logic in theQueryInvoicesmethod.
- 537-555: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [500-552]
The method
QueryInvoicescorrectly handles the edge cases whereCreationDateEndis not set (q.CreationDateEnd != 0) by ensuring that invoices are only skipped if they were created after the specified end date. This logic ensures that all invoices up to and including the end date are included in the results when an end date is specified, which aligns with the expected behavior for date range filtering.
- 537-555: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [500-552]
The changes made to the
QueryInvoicesmethod, including the removal ofstartDateSetandendDateSetin favor of direct timestamp comparisons, are logically sound and improve the clarity of the code. The method's handling of pagination and invoice filtering based on creation dates is correctly implemented, ensuring accurate and efficient retrieval of invoice records. The approach taken here aligns well with best practices for database querying and data handling.
- 537-555: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [500-552]
The implementation of the
DeleteCanceledInvoicesandDeleteInvoicemethods demonstrates careful consideration of data integrity and consistency. The methods include checks to ensure that only the intended invoices are deleted and that all related data, including AMP invoice data and index entries, are also removed. The use of transactions ensures that these operations are atomic, preventing partial updates that could lead to data inconsistency. The error handling within these methods is appropriate, with errors being returned for further handling, ensuring robustness in the face of potential issues during the deletion process.
- 537-555: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [500-552]
The overall structure and organization of the code in
channeldb/invoices.goare well-designed, with clear separation of concerns and logical grouping of related functionality. The code is well-documented, with comments explaining the purpose and logic of the main methods and functions, enhancing readability and maintainability. The handling of AMP invoices, while complex, is implemented in a way that is logical and consistent with the rest of the codebase. The use of transactions and careful error handling further contributes to the robustness of the invoice management functionality provided by this file.rpcserver.go (2)
- 5774-5775: The conversion of
req.CreationDateStartandreq.CreationDateEndfrom their original types toint64directly in the struct initialization assumes that the incoming types are compatible with a direct conversion toint64. This could potentially lead to runtime errors if the types do not match or if the conversion is not safe. It's important to ensure thatreq.CreationDateStartandreq.CreationDateEndare indeed in a format that can be safely converted toint64without loss of information or precision.- 6611-6612: Similar to the previous comment, the direct conversion of
req.CreationDateStartandreq.CreationDateEndtoint64here assumes compatibility and safety of the conversion. Verify that the types of these fields are indeed intended for direct conversion toint64and that this conversion does not introduce any potential issues such as loss of precision or runtime errors.
cd5bd20 to
7adc38e
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between 0d37621 and 7adc38e0da2183ed529ac19b97412979c9837369.Files selected for processing (9)
- channeldb/invoice_test.go (10 hunks)
- channeldb/invoices.go (2 hunks)
- channeldb/payments.go (3 hunks)
- channeldb/payments_test.go (3 hunks)
- docs/release-notes/release-notes-0.18.0.md (1 hunks)
- invoices/interface.go (2 hunks)
- itest/list_on_test.go (1 hunks)
- itest/lnd_payment_test.go (3 hunks)
- rpcserver.go (2 hunks)
Files skipped from review as they are similar to previous changes (6)
- channeldb/invoices.go
- channeldb/payments.go
- invoices/interface.go
- itest/list_on_test.go
- itest/lnd_payment_test.go
- rpcserver.go
Additional comments: 15
docs/release-notes/release-notes-0.18.0.md (1)
- 76-78: The description of the fix for the timestamp precision issue is clear and concise.
channeldb/payments_test.go (3)
- 427-427: The change to use direct integer values for
CreationDateStartin test cases aligns with the PR's objective to standardize timestamp precision. This simplifies the test setup and ensures consistency with the new timestamp handling logic.- 441-441: Using direct integer values for
CreationDateStartin this test case is consistent with the changes made across the system for handling timestamps. It ensures that the tests are aligned with the new logic for timestamp precision.- 454-455: The introduction of direct integer values for both
CreationDateStartandCreationDateEndin this test case is a good practice. It directly tests the functionality with the new timestamp precision handling, ensuring that the system behaves as expected when filtering payments within a specific time range.channeldb/invoice_test.go (11)
- 1410-1410: The
CreationDateEndfield is being set directly with an integer value. This change aligns with the PR's objective to standardize timestamp precision by using Unix timestamps in seconds. However, ensure that all related logic and tests have been updated to accommodate this change in data type and representation.- 1418-1418: The
CreationDateStartfield is being set directly with an integer value. This change aligns with the PR's objective to standardize timestamp precision by using Unix timestamps in seconds. However, ensure that all related logic and tests have been updated to accommodate this change in data type and representation.- 1427-1427: The
CreationDateEndfield is being set directly with an integer value for pending invoices. This change aligns with the PR's objective to standardize timestamp precision by using Unix timestamps in seconds. However, ensure that all related logic and tests have been updated to accommodate this change in data type and representation.- 1436-1436: The
CreationDateStartfield is being set directly with an integer value for pending invoices. This change aligns with the PR's objective to standardize timestamp precision by using Unix timestamps in seconds. However, ensure that all related logic and tests have been updated to accommodate this change in data type and representation.- 1445-1445: The
CreationDateEndfield is being set directly with an integer value for invoices with an offset. This change aligns with the PR's objective to standardize timestamp precision by using Unix timestamps in seconds. However, ensure that all related logic and tests have been updated to accommodate this change in data type and representation.- 1458-1458: The
CreationDateStartfield is being set directly with an integer value for pending invoices in reversed order. This change aligns with the PR's objective to standardize timestamp precision by using Unix timestamps in seconds. However, ensure that all related logic and tests have been updated to accommodate this change in data type and representation.- 1468-1469: The
CreationDateStartandCreationDateEndfields are being set directly with integer values. This change aligns with the PR's objective to standardize timestamp precision by using Unix timestamps in seconds. However, ensure that all related logic and tests have been updated to accommodate this change in data type and representation.- 1478-1479: The
CreationDateStartandCreationDateEndfields are being set directly with integer values for pending invoices. This change aligns with the PR's objective to standardize timestamp precision by using Unix timestamps in seconds. However, ensure that all related logic and tests have been updated to accommodate this change in data type and representation.- 1489-1490: The
CreationDateStartandCreationDateEndfields are being set directly with integer values in reverse order. This change aligns with the PR's objective to standardize timestamp precision by using Unix timestamps in seconds. However, ensure that all related logic and tests have been updated to accommodate this change in data type and representation.- 1501-1502: The
CreationDateStartandCreationDateEndfields are being set directly with integer values for pending invoices in reverse order. This change aligns with the PR's objective to standardize timestamp precision by using Unix timestamps in seconds. However, ensure that all related logic and tests have been updated to accommodate this change in data type and representation.- 1511-1512: The
CreationDateStartandCreationDateEndfields are being set directly with integer values where the start date is greater than the end date. This change aligns with the PR's objective to standardize timestamp precision by using Unix timestamps in seconds. However, ensure that all related logic and tests have been updated to accommodate this change in data type and representation.
7adc38e to
32de26b
Compare
32de26b to
9e065c2
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between 0d37621 and 9e065c257de9cd0a4095c98895a2806099929ba6.Files selected for processing (9)
- channeldb/invoice_test.go (10 hunks)
- channeldb/invoices.go (2 hunks)
- channeldb/payments.go (3 hunks)
- channeldb/payments_test.go (4 hunks)
- docs/release-notes/release-notes-0.18.0.md (1 hunks)
- invoices/interface.go (2 hunks)
- itest/list_on_test.go (1 hunks)
- itest/lnd_payment_test.go (3 hunks)
- rpcserver.go (2 hunks)
Files skipped from review as they are similar to previous changes (9)
- channeldb/invoice_test.go
- channeldb/invoices.go
- channeldb/payments.go
- channeldb/payments_test.go
- docs/release-notes/release-notes-0.18.0.md
- invoices/interface.go
- itest/list_on_test.go
- itest/lnd_payment_test.go
- rpcserver.go
9e065c2 to
ac7c203
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between 5fe99f0 and ac7c2036846a1bb878dff6bcd1f60b18963b2317.Files selected for processing (9)
- channeldb/invoice_test.go (10 hunks)
- channeldb/invoices.go (2 hunks)
- channeldb/payments.go (3 hunks)
- channeldb/payments_test.go (4 hunks)
- docs/release-notes/release-notes-0.18.0.md (1 hunks)
- invoices/interface.go (2 hunks)
- itest/list_on_test.go (1 hunks)
- itest/lnd_payment_test.go (3 hunks)
- rpcserver.go (2 hunks)
Files skipped from review as they are similar to previous changes (8)
- channeldb/invoice_test.go
- channeldb/invoices.go
- channeldb/payments.go
- channeldb/payments_test.go
- invoices/interface.go
- itest/list_on_test.go
- itest/lnd_payment_test.go
- rpcserver.go
Additional comments: 1
docs/release-notes/release-notes-0.18.0.md (1)
- 81-83: The description of the fix for the timestamp precision issue is clear and concise, accurately reflecting the changes made in the PR. It's important for release notes to communicate the essence of the fix to the users effectively, and this entry does so by specifying the affected functionality (querying payments and invoices) and the nature of the fix (addressing a timestamp precision issue with start and end date filters).
This commit fixes the timestamp precision to always compare the dates using unix seconds for invoices.
This commit fixes the timestamp precision to always compare the dates using unix seconds for payments.
ac7c203 to
4da9582
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (9)
- channeldb/invoice_test.go (10 hunks)
- channeldb/invoices.go (2 hunks)
- channeldb/payments.go (3 hunks)
- channeldb/payments_test.go (4 hunks)
- docs/release-notes/release-notes-0.18.0.md (1 hunks)
- invoices/interface.go (2 hunks)
- itest/list_on_test.go (1 hunks)
- itest/lnd_payment_test.go (3 hunks)
- rpcserver.go (2 hunks)
Files skipped from review as they are similar to previous changes (9)
- channeldb/invoice_test.go
- channeldb/invoices.go
- channeldb/payments.go
- channeldb/payments_test.go
- docs/release-notes/release-notes-0.18.0.md
- invoices/interface.go
- itest/list_on_test.go
- itest/lnd_payment_test.go
- rpcserver.go
Reported by a user, that the
ListPaymentsquery cannot properly give back the results based on the timestamp filters. Turns out there's a precision issue, as the creation dates are saved in nanoseconds, but the filters use seconds, which may sometimes cause unexpected records being filtered. For instance, a user creates a payment at time1706285122.897131000s, and tries to filter it usingCreationEndDate = 1706285122s, but the payment is saved using nanoseconds, so it's greater than theCreationEndDateby0.897131000s.To see how the precision issue occurs, run the itest
list_paymentsin the first commit.This PR fixes it by making sure the comparison always use Unix timestamp in seconds.
Summary by CodeRabbit
Summary by CodeRabbit
time.Timewithint64for date representations.