accounts: add account payments subcommand#1316
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a new account payment history feature for the Accounts subserver. By adding a new gRPC endpoint and a corresponding CLI command, users can now efficiently query and paginate through the payment history of their off-chain accounts. The changes include necessary database store updates, permission configurations, and updated protobuf definitions to support this new functionality. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 introduces the AccountPayments gRPC endpoint and a corresponding litcli accounts payments subcommand to retrieve the detailed payment history of an off-chain account. The implementation includes backend store updates for both BoltDB and SQL, concurrent payment tracking from LND, and pagination/filtering options. Feedback on the changes highlights two main issues: first, the reversed ordering is implemented as reverse lexicographical rather than the documented reverse chronological order; second, the cleanup function returned by RawClientWithMacAuth is ignored, which could lead to resource leaks.
| } | ||
|
|
||
| // Fetch the detailed payments concurrently from LND. | ||
| rawCtx, _, client := s.service.routerClient.RawClientWithMacAuth(ctx) |
There was a problem hiding this comment.
The cleanup/cancel function returned by RawClientWithMacAuth is ignored. Ignoring this function can lead to a context or resource leak. It should be captured and deferred.
\trawCtx, cleanup, client := s.service.routerClient.RawClientWithMacAuth(ctx)\n\tdefer cleanup()There was a problem hiding this comment.
RawClientWithMacAuth doesn't return a cleanup function, the second return value is a time.Duration representing the macaroon timeout. Reference
81b3e1f to
69da0b5
Compare
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
Thanks a lot again for the contribution @Cyberguru1 🔥🙏! I've added a few comments below as an initial review.
23fc956 to
fe130cf
Compare
Thanks again for taking the time to review the PR. I’ve addressed the comments you pointed out and pushed the updates. Regarding the failing CI, I investigated the issue and tried reproducing it locally multiple times, but the tests pass on my end. From the logs, it seems the failure happens while starting the Bob node, where the Looking forward to a follow-up review. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the litcli accounts payments subcommand and the corresponding AccountPayments gRPC endpoint to retrieve the detailed, paginated, and sorted off-chain payment history of a single account. The implementation spans store interface updates, Bolt and SQL backend support, concurrent payment tracking from LND, and comprehensive integration tests. Feedback on the changes highlights a potential integer overflow when converting req.IndexOffset to int32, fragile error handling where a single payment tracking failure aborts the entire RPC request, and the use of SELECT * in SQL queries which could break row scanning during future schema migrations.
| args = append(args, arg.Status.Int16) | ||
| } | ||
|
|
||
| query := "SELECT * FROM account_payments" |
There was a problem hiding this comment.
Using SELECT * is an anti-pattern when manually scanning rows into a struct. If the database schema is updated in the future (e.g., by adding new columns to the account_payments table), rows.Scan will fail because the number of columns returned won't match the number of destination arguments. Explicitly listing the columns makes the query robust against future schema migrations.
| query := "SELECT * FROM account_payments" | |
| query := "SELECT account_id, hash, status, full_amount_msat FROM account_payments" |
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
Thanks a lot for the updates @Cyberguru1! This is looking much better 🎉. I'm leaving a few more comments, but I think this PR is almost there now 🔥.
eba60ad to
efded06
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the litcli accounts payments subcommand and the corresponding AccountPayments gRPC endpoint to fetch paginated, sorted off-chain payment histories for accounts. Feedback highlights a potential resource leak from an ignored cleanup function in rpcserver.go, a potential slice index overflow panic in the KV store pagination logic, and a risk of test flakiness in litd_accounts_test.go due to reusing a timed-out context for cleanup.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| errs []error | ||
| ) | ||
|
|
||
| rawCtx, _, client := s.service.routerClient.RawClientWithMacAuth(ctx) |
There was a problem hiding this comment.
The second return value of RawClientWithMacAuth is a cleanup/cancel function. Ignoring it with _ can lead to resource leaks (such as unreleased context resources or metadata). It should be deferred to ensure proper cleanup.
| rawCtx, _, client := s.service.routerClient.RawClientWithMacAuth(ctx) | |
| rawCtx, cleanup, client := s.service.routerClient.RawClientWithMacAuth(ctx) | |
| defer cleanup() |
| end := offset + limit | ||
| if limit <= 0 || end > total { | ||
| end = total | ||
| } |
There was a problem hiding this comment.
If offset + limit overflows int32, end becomes negative, which will cause a panic when slicing matchedPayments[offset:end]. Although total is practically limited by memory and slice size, we can make this calculation completely overflow-safe by avoiding the addition when limit is positive.
| end := offset + limit | |
| if limit <= 0 || end > total { | |
| end = total | |
| } | |
| end := total | |
| if limit > 0 && total-offset > limit { | |
| end = offset + limit | |
| } |
| _, err = charlie.SettleInvoice(testCtx, &invoicesrpc.SettleInvoiceMsg{ | ||
| Preimage: preimage[:], | ||
| }) |
There was a problem hiding this comment.
Using testCtx (which has a timeout of defaultTimeout and is used for the entire duration of the test) for the final cleanup step SettleInvoice is risky. If the test runs slowly on CI, testCtx might already be expired by the time we reach the end of the test, causing the cleanup to fail and flaking the test. It is safer to use a fresh context with its own short timeout for this cleanup step.
| _, err = charlie.SettleInvoice(testCtx, &invoicesrpc.SettleInvoiceMsg{ | |
| Preimage: preimage[:], | |
| }) | |
| settleCtx, settleCancel := context.WithTimeout(context.Background(), 5*time.Second) | |
| defer settleCancel() | |
| _, err = charlie.SettleInvoice(settleCtx, &invoicesrpc.SettleInvoiceMsg{ | |
| Preimage: preimage[:], | |
| }) |
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
Generally this looks good now! I realized though that the reverse flag doesn't really add any value in litd, and I've left a comment regarding that below. Really sorry for not catching this earlier!
If you agree with my reasoning, I'll also request a review from @bitromortac once that's been addressed, as this PR is generally in a great state now!
efded06 to
38fff82
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the litcli accounts payments subcommand and the corresponding AccountPayments gRPC endpoint to retrieve the detailed, paginated off-chain payment history of an account. The changes span the database stores (both BoltDB and SQL backends), the RPC server, CLI commands, and integration tests. The review feedback focuses on defensive programming and backend consistency, suggesting that negative offsets be handled safely and that limit/offset behaviors be normalized between the BoltDB and SQL implementations to prevent runtime panics or database errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Apply pagination limits and offsets. | ||
| total := int32(len(matchedPayments)) | ||
| if offset >= total { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
Defensive programming: Ensure that offset is non-negative before using it for slicing matchedPayments. If a negative offset is passed to ListAccountPayments, it will cause a runtime panic due to out-of-bounds slice indexing.
| // Apply pagination limits and offsets. | |
| total := int32(len(matchedPayments)) | |
| if offset >= total { | |
| return nil, nil | |
| } | |
| if offset < 0 { | |
| offset = 0 | |
| } | |
| total := int32(len(matchedPayments)) | |
| if offset >= total { | |
| return nil, nil | |
| } |
References
- Ensure that invalid inputs or states are safely handled in all cases.
| dbPayments, err = db.AccountPaymentsPaginated( | ||
| ctx, sqlc.AccountPaymentsPaginatedParams{ | ||
| AccountID: id, | ||
| Limit: limit, | ||
| Offset: offset, | ||
| }, |
There was a problem hiding this comment.
Defensive programming and backend consistency: Ensure that offset is non-negative and limit is handled correctly when it is less than or equal to 0. In BoltStore, a limit <= 0 returns all remaining rows, whereas in SQL, LIMIT 0 returns zero rows and a negative limit results in a database error in Postgres. Normalizing these values ensures consistent behavior across both database backends.
if offset < 0 {
offset = 0
}
if limit <= 0 {
limit = 2147483647
}
var dbPayments []sqlc.AccountPayment
dbPayments, err = db.AccountPaymentsPaginated(
ctx, sqlc.AccountPaymentsPaginatedParams{
AccountID: id,
Limit: limit,
Offset: offset,
},
)References
- Ensure that invalid inputs or states are safely handled in all cases, and maintain behavioral consistency across different database backends.
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
Nice looks great, thanks a lot for the fixes 🔥🎉!
LGTM! I'll test this thoroughly tomorrow, but since the itest is quite thorough, I don't think I'll have any additional feedback after the testing round. Meanwhile, I'm requesting a review from @bitromortac.
|
@bitromortac: review reminder |
There was a problem hiding this comment.
Nice addition, the account payment is a useful endpoint. I left a few inline notes: one about how we handle a failed lookup and the rest are small polish and doc things.
One structural thought on the commits: it'd be nice to introduce the unit tests in the same commit as the code they exercise, so each commit stays self-contained.
Thanks for putting this together!
38fff82 to
1e5964c
Compare
@bitromortac Thanks for the review! 🙏 Addressed the inline notes and folded the test commit into the code commit it exercises, so history's self-contained now. Looking forward to the next review round! 🚀 |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new AccountPayments gRPC endpoint and a corresponding litcli accounts payments subcommand to retrieve the paginated and sorted off-chain payment history of a single account. This is supported by database updates (both Bolt and SQL backends) and concurrent queries to LND's TrackPaymentV2 API. Feedback on the implementation highlights several critical issues: a classic Go goroutine data race where the loop variable entry is captured by reference, and an overly strict error handling strategy where a single failed payment track aborts the entire RPC call. Additionally, defensive checks should be added to prevent runtime panics from negative offsets in the KV store, and concurrent tracking logic can be simplified by removing unused error slices and handling context cancellation more cleanly.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
1e5964c to
a70f430
Compare
bitromortac
left a comment
bitromortac
left a comment
There was a problem hiding this comment.
Thank you for the changes. Could you look into the thread #1316 (comment) about skipping pagination if we fail for a single query?
a70f430 to
50ab59b
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds the litcli accounts payments subcommand and the corresponding AccountPayments gRPC endpoint to retrieve paginated off-chain payment history for accounts, supported by both Bolt and SQL database backends. The review feedback highlights a critical issue in the pagination logic where offsets are calculated using successfully fetched LND payments instead of the store's retrieved entries, which can lead to duplicate results or stuck pagination. Additionally, the reviewer recommends adding defensive checks to prevent negative offsets in both database stores and updating the integration tests accordingly.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| var firstIndexOffset, lastIndexOffset uint64 | ||
| if len(finalPayments) > 0 { | ||
| firstIndexOffset = offset | ||
| lastIndexOffset = offset + uint64(len(finalPayments)) | ||
| } |
There was a problem hiding this comment.
The pagination offsets firstIndexOffset and lastIndexOffset should be based on the number of elements retrieved from the store (paymentsFromStore), rather than the number of successfully fetched payments from LND (finalPayments).
If any payments are skipped (e.g., because they are not found in LND), basing the offsets on finalPayments causes two issues:
- Duplicate/Overlapping Results: The next query's offset will overlap with the current page, causing already-returned payments to be fetched and returned again.
- Stuck Pagination: If an entire page of payments is missing in LND,
lastIndexOffsetwill be0, making it impossible for the client to paginate past the dead zone to fetch subsequent valid payments.
Using paymentsFromStore ensures that pagination correctly and deterministically advances through the store's index space.
| var firstIndexOffset, lastIndexOffset uint64 | |
| if len(finalPayments) > 0 { | |
| firstIndexOffset = offset | |
| lastIndexOffset = offset + uint64(len(finalPayments)) | |
| } | |
| var firstIndexOffset, lastIndexOffset uint64 | |
| if len(paymentsFromStore) > 0 { | |
| firstIndexOffset = offset | |
| lastIndexOffset = offset + uint64(len(paymentsFromStore)) | |
| } |
| require.EqualValues(t.t, 3, paymentsResp.TotalNumPayments) | ||
| require.EqualValues(t.t, 0, paymentsResp.FirstIndexOffset) | ||
| require.EqualValues(t.t, 2, paymentsResp.LastIndexOffset) |
There was a problem hiding this comment.
Update the test assertion to expect 3 instead of 2 for LastIndexOffset to align with the pagination offset fix (which bases offsets on the store's index space rather than the successfully fetched LND payments).
| require.EqualValues(t.t, 3, paymentsResp.TotalNumPayments) | |
| require.EqualValues(t.t, 0, paymentsResp.FirstIndexOffset) | |
| require.EqualValues(t.t, 2, paymentsResp.LastIndexOffset) | |
| require.EqualValues(t.t, 3, paymentsResp.TotalNumPayments) | |
| require.EqualValues(t.t, 0, paymentsResp.FirstIndexOffset) | |
| require.EqualValues(t.t, 3, paymentsResp.LastIndexOffset) |
| func (s *BoltStore) ListAccountPayments(ctx context.Context, id AccountID, | ||
| offset, limit int32) ([]*AccountPaymentEntry, error) { |
There was a problem hiding this comment.
Add a defensive check to ensure that offset is non-negative. If a negative offset is passed to ListAccountPayments, it could cause a runtime panic due to out-of-bounds slice indexing.
func (s *BoltStore) ListAccountPayments(ctx context.Context, id AccountID,
offset, limit int32) ([]*AccountPaymentEntry, error) {
if offset < 0 {
return nil, fmt.Errorf("offset cannot be negative")
}| func (s *SQLStore) ListAccountPayments(ctx context.Context, alias AccountID, | ||
| offset, limit int32) ([]*AccountPaymentEntry, error) { |
There was a problem hiding this comment.
Add a defensive check to ensure that offset is non-negative. If a negative offset is passed to ListAccountPayments, it could cause database execution errors or unexpected behavior.
func (s *SQLStore) ListAccountPayments(ctx context.Context, alias AccountID,
offset, limit int32) ([]*AccountPaymentEntry, error) {
if offset < 0 {
return nil, fmt.Errorf("offset cannot be negative")
}
36e894e to
7259911
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the litcli accounts payments subcommand and the corresponding AccountPayments gRPC endpoint to retrieve paginated off-chain payment history for a single account. It adds database store support for both Bolt and SQL backends, along with comprehensive integration tests. The review feedback focuses on optimizing the concurrent payment tracking in the RPC server during context cancellation or timeouts. Specifically, it is recommended to return early when the context is cancelled to avoid redundant error logging and lock contention, and to cleanly propagate the context error after all goroutines have finished.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
7259911 to
9bda27d
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds the AccountPayments gRPC endpoint and corresponding litcli accounts payments subcommand to retrieve the off-chain payment history of an account. The changes include store implementations for both BoltDB and SQL backends supporting pagination and counting, along with comprehensive integration and unit tests. The review feedback highlights opportunities to improve error logging for concurrent operations, resolve an inconsistency in negative limit handling between store backends, and explicitly list columns instead of using SELECT * in SQL queries.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if len(errs) > 0 { | ||
| return nil, fmt.Errorf( | ||
| "failed to fetch payment details: %w", errs[0], | ||
| ) | ||
| } |
There was a problem hiding this comment.
When multiple concurrent operations fail, returning only the first error from a non-deterministically ordered slice can make debugging difficult. It would be more informative to log all the errors that occurred.
| if len(errs) > 0 { | |
| return nil, fmt.Errorf( | |
| "failed to fetch payment details: %w", errs[0], | |
| ) | |
| } | |
| if len(errs) > 0 { | |
| log.Errorf("Encountered %d errors fetching payment details: %v", len(errs), errs) | |
| return nil, fmt.Errorf( | |
| "failed to fetch payment details: %w", errs[0], | |
| ) | |
| } |
| if limit < 0 || end > total { | ||
| end = total | ||
| } |
There was a problem hiding this comment.
This implementation handles a negative limit by effectively treating it as 'no limit' (fetching all records from the offset). However, the SQL implementation in store_sql.go passes the limit directly to the database, which will likely result in an error for a negative value. This creates an inconsistency between the two store implementations.
While the RPC layer currently ensures the limit is non-negative, it would be best to make the store implementations consistent. I'd suggest either documenting this behavior in the Store interface and replicating it in the SQL store, or adding a check for a negative limit here and in the SQL store to return an error, as it's an invalid argument.
| FROM account_payments; | ||
|
|
||
| -- name: AccountPaymentsPaginated :many | ||
| SELECT * |
There was a problem hiding this comment.
For better maintainability and to avoid accidentally fetching more data than needed if the table schema changes, it's a good practice to explicitly list the columns instead of using SELECT *.
SELECT account_id, hash, status, full_amount_msat
bitromortac
left a comment
bitromortac
left a comment
There was a problem hiding this comment.
LGTM, thanks for the updates and additional test coverage 🎉. Due to a merge of another PR we need another rebase unfortunately 🙏.
Define the AccountPayments RPC endpoint and its request/response messages in lit-accounts.proto. This endpoint allows querying paginated payment history for a specific account. Also update the frontend JS/TS proto sanitization script to map the imported lnd.proto file to its flat directory path, fixing app build.
Add SQL queries to select account payments with pagination (limit and offset), and a query to count the total payments for a given account.
Add sqlc queries to select account payment hashes from the database. Implement the ListAccountPayments method in the SQLStore, allowing retrieval of stored payment hashes for SQLite and Postgres backends. Also define the AccountPaymentEntry helper struct in accounts/interface.go to wrap payment hashes and details.
Implement ListAccountPayments in the Bolt-based kvdb account store. The retrieved account payments are sorted in ascending lexicographical order of their payment hash.
Introduce the ListAccountPayments and CountAccountPayments methods to the accounts Store interface. ListAccountPayments enables retrieval of a paginated list of payment entries associated with a given account ID, supporting offset and limit. CountAccountPayments returns the total number of payments associated with the account.
Implement the AccountPayments handler on the Accounts RPC server. The handler resolves accounts by ID or label, loads its payment hashes, applies pagination constraints, and fetches complete payment details concurrently from LND's TrackPaymentV2. It also returns pagination metadata including the total count of payments.
Introduce the 'payments' subcommand under 'litcli accounts' to query account payment history. Supports parameters for offset, page size limits, and counting of total payments.
Register the /litrpc.Accounts/AccountPayments RPC endpoint in the RequiredPermissions map, requiring read access on the account entity.
Add integration tests inside itest/litd_accounts_test.go to verify retrieval of account payments against a running LND node, validating correct responses for ID/label lookup, offsets, pagination limits, and counting of total payments.
Add release notes for the new accounts payments CLI subcommand and associated RPC endpoint to the v0.17.1 release notes document.
9bda27d to
c6d81a8
Compare
@bitromortac Thanks! 🙏 I've rebased the branch and resolved the merge conflicts. |
4 participants
closes #935
This PR implements the off-chain account payments history subcommand (
litcli accounts payments) and its backend support in the Accounts gRPC subserver.It adds database store methods for BoltStore and SQLStore to fetch account payment hashes in insertion order, exposes the
AccountPaymentsRPC endpoint on the RPC server with support for paginated limits, offsets, reversing order, and concurrently fetching payment details from LND, and updates the release documentation.