feat(api/v2): Implement /transactions/sum endpoint with account-based transaction aggregation #1071
Conversation
… transaction aggregation
WalkthroughAdds a new v2 GET /transactions/sum endpoint and related types, implements store SQL to compute per-asset transaction sums (optional time range and asset filter), extends controller/store/mocks with sum APIs and tracing/metrics, updates views/rendering, and adds unit tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant R as Router (/transactions/sum)
participant H as Handler getTransactionsSum
participant LC as LedgerController
participant S as Store (SQL)
C->>R: GET /transactions/sum?account=acct(&asset=ASSET)(&start_time&end_time)
R->>H: invoke handler
H->>H: validate params, parse times
H->>LC: GetTransactionsSumWithTimeRange(ctx, account, start, end)
alt controller delegates to store
LC->>S: TransactionsSumWithTimeRange(ledger, account, start, end)
S-->>LC: []TransactionsSum{Asset, Sum(string)}
end
LC-->>H: []TransactionsSum
H->>H: convert sums -> big.Int, apply asset filter
H-->>C: 200 OK with JSON array of sumResponse
alt errors
H-->>C: 400 (bad params) / 500 (internal)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
internal/api/v2/controllers_transactions_sum.go (2)
29-31: Consider extracting duplicate map initialization.The pattern of checking if a key exists and initializing with
big.NewInt(0)is duplicated for both source and destination cases.You could extract this into a helper to reduce duplication:
func processPostings(account string, txs *bunpaginate.Cursor[ledger.Transaction], assetFilter string) []sumResponse { // Calculate sums per asset assetSums := make(map[string]*big.Int) + + ensureAsset := func(asset string) { + if _, ok := assetSums[asset]; !ok { + assetSums[asset] = big.NewInt(0) + } + } for _, tx := range txs.Data { for _, posting := range tx.Postings { if posting.Source == account { // Debit from the account (negative amount) - if _, ok := assetSums[posting.Asset]; !ok { - assetSums[posting.Asset] = big.NewInt(0) - } + ensureAsset(posting.Asset) assetSums[posting.Asset] = new(big.Int).Sub(assetSums[posting.Asset], posting.Amount) } else if posting.Destination == account { // Credit to the account (positive amount) - if _, ok := assetSums[posting.Asset]; !ok { - assetSums[posting.Asset] = big.NewInt(0) - } + ensureAsset(posting.Asset) assetSums[posting.Asset] = new(big.Int).Add(assetSums[posting.Asset], posting.Amount) } } }Also applies to: 35-37
103-103: Clarify the comment.The comment states "The test expects a single response object in an array" but the endpoint actually returns an array of
sumResponseobjects (potentially multiple items, one per asset).Consider updating the comment to be more accurate:
- // The test expects a single response object in an array + // Return an array of sumResponse objects (one per asset) if len(response) == 0 { // If no postings match, return an empty array api.Ok(w, []sumResponse{}) return } api.Ok(w, response)internal/api/v2/controllers_transactions_sum_test.go (2)
19-103: Consider expanding test coverage.The current tests cover the basic happy path and missing parameter validation, which is good. However, consider adding tests for:
- Multiple assets in the response
- Account with no matching transactions (empty result)
- Using the
assetfilter parameter- Pagination scenarios (though this relates to the pagination issue flagged separately)
- Large transaction amounts that test big.Int handling
Example additional test case for multiple assets:
t.Run("success with multiple assets", func(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) mockLedgerController := NewLedgerController(ctrl) desc := bunpaginate.OrderDesc order := bunpaginate.Order(desc) expectedQuery := storagecommon.InitialPaginatedQuery[any]{ PageSize: 15, Column: "timestamp", Order: &order, Options: storagecommon.ResourceQuery[any]{ Expand: []string{}, PIT: nil, OOT: nil, Builder: nil, Opts: nil, }, } mockLedgerController.EXPECT(). ListTransactions(gomock.Any(), matchPaginatedQuery(expectedQuery)). Return(&bunpaginate.Cursor[ledger.Transaction]{ Data: []ledger.Transaction{ ledger.NewTransaction().WithPostings( ledger.NewPosting("world", "expenses:salary", "USD", big.NewInt(1000)), ), ledger.NewTransaction().WithPostings( ledger.NewPosting("world", "expenses:salary", "EUR", big.NewInt(500)), ), }, }, nil) server := newTestServer(t, mockLedgerController) req, err := http.NewRequest(http.MethodGet, "/transactions/sum?account=expenses:salary&pageSize=15", nil) require.NoError(t, err) rr := httptest.NewRecorder() server.ServeHTTP(rr, req) require.Equal(t, http.StatusOK, rr.Code) var responseWrapper struct { Data []sumResponse `json:"data"` } err = json.Unmarshal(rr.Body.Bytes(), &responseWrapper) require.NoError(t, err) require.Len(t, responseWrapper.Data, 2) // Verify both USD and EUR are present with correct amounts })
122-129: Consider improving the gomock matcher for better debugging.The
matchPaginatedQuerymatcher returns an empty string in the formatter, which makes debugging test failures harder.Consider returning a more descriptive string:
func matchPaginatedQuery(expected storagecommon.InitialPaginatedQuery[any]) gomock.Matcher { return gomock.GotFormatterAdapter( gomock.GotFormatterFunc(func(actual interface{}) string { - return "" + if query, ok := actual.(storagecommon.InitialPaginatedQuery[any]); ok { + return fmt.Sprintf("InitialPaginatedQuery{PageSize: %d, Column: %s, Order: %v}", + query.PageSize, query.Column, query.Order) + } + return fmt.Sprintf("%#v", actual) }), gomock.Eq(expected), ) }Don't forget to add the
fmtimport if you implement this change.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
internal/api/v2/controllers_transactions_sum.go(1 hunks)internal/api/v2/controllers_transactions_sum_test.go(1 hunks)internal/api/v2/routes.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/api/v2/controllers_transactions_sum.go (4)
internal/posting.go (1)
Postings(35-35)internal/api/common/errors.go (3)
ErrValidation(19-19)InternalServerError(68-73)HandleCommonPaginationErrors(57-66)internal/storage/common/pagination.go (1)
InitialPaginatedQuery(9-14)internal/storage/common/resource.go (1)
ResourceQuery(408-414)
internal/api/v2/controllers_transactions_sum_test.go (4)
internal/storage/common/pagination.go (1)
InitialPaginatedQuery(9-14)internal/storage/common/resource.go (1)
ResourceQuery(408-414)internal/transaction.go (1)
NewTransaction(227-231)internal/posting.go (1)
NewPosting(26-33)
🔇 Additional comments (2)
internal/api/v2/routes.go (1)
105-105: LGTM!The route is correctly placed within the
/transactionsroute group and follows the existing naming conventions.internal/api/v2/controllers_transactions_sum.go (1)
85-85: No action needed:getExpandis defined in internal/api/v2/common.go (line 67).
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
internal/api/v2/controllers_transactions_sum.go(1 hunks)internal/api/v2/controllers_transactions_sum_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/api/v2/controllers_transactions_sum.go (4)
internal/posting.go (1)
Postings(35-35)internal/api/common/errors.go (3)
ErrValidation(19-19)InternalServerError(68-73)HandleCommonPaginationErrors(57-66)internal/storage/common/pagination.go (1)
InitialPaginatedQuery(9-14)internal/storage/common/resource.go (1)
ResourceQuery(408-414)
internal/api/v2/controllers_transactions_sum_test.go (5)
internal/storage/common/pagination.go (1)
InitialPaginatedQuery(9-14)internal/storage/common/resource.go (1)
ResourceQuery(408-414)internal/transaction.go (1)
NewTransaction(227-231)internal/posting.go (1)
NewPosting(26-33)internal/api/v2/routes.go (1)
NewRouter(20-121)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/api/v2/controllers_transactions_sum.go (1)
127-148: Critical: Pagination cursor never applied—loop fetches first page repeatedly.The loop calls
getPaginatedTransactions(w, r, ledgerInstance, defaultPageSize)with the same original requestron every iteration. SincegetPaginatedTransactionsusesstoragecommon.Extract(r, ...)to read the cursor fromr.URL.Query(), andrnever changes, the cursor is always empty. This causesExtractto use thedefaulterevery time, fetching the first page repeatedly instead of advancing through pages. The computed sums will be incorrect (only reflecting the first page, possibly counted multiple times).Based on the signature of
bunpaginate.Cursorand the fact thatcursor.Nextis a string (checked at line 145), you need to either:
- Pass the cursor token explicitly instead of relying on Extract reading from the request
- Manually construct the paginated query for subsequent iterations using the cursor token
Here's a suggested fix using approach #2:
func getTransactionsSum(w http.ResponseWriter, r *http.Request) { // Get account from query parameters account := r.URL.Query().Get("account") if account == "" { api.BadRequest(w, common.ErrValidation, errors.New("account parameter is required")) return } // Get asset filter if provided assetFilter := r.URL.Query().Get("asset") // Get transactions ledgerInstance := common.LedgerFromContext(r.Context()) if ledgerInstance == nil { api.InternalServerError(w, r, errors.New("ledger not found in context")) return } // Use a reasonable default page size const defaultPageSize = 100 assetSums := make(map[string]*big.Int) - var cursor *bunpaginate.Cursor[ledger.Transaction] - var ok bool + + // Prepare initial query + order := bunpaginate.Order(bunpaginate.OrderDesc) + var currentCursor string + maxIterations := 10000 // Defensive limit + iteration := 0 // Process all pages of transactions for { - cursor, ok = getPaginatedTransactions(w, r, ledgerInstance, defaultPageSize) - if !ok { - return // Error already handled + iteration++ + if iteration > maxIterations { + api.InternalServerError(w, r, errors.New("pagination limit exceeded")) + return } + + var rq storagecommon.PaginatedQuery[any] + var err error + + if currentCursor == "" { + // First page - use Extract from request + rq, err = storagecommon.Extract[any](r, func() (*storagecommon.InitialPaginatedQuery[any], error) { + return &storagecommon.InitialPaginatedQuery[any]{ + PageSize: defaultPageSize, + Column: "timestamp", + Order: &order, + Options: storagecommon.ResourceQuery[any]{ + Expand: getExpand(r), + }, + }, nil + }) + } else { + // Subsequent pages - unmarshal the cursor + rq, err = storagecommon.UnmarshalCursor[any](currentCursor) + } + + if err != nil { + api.BadRequest(w, common.ErrValidation, err) + return + } + + cursor, err := ledgerInstance.ListTransactions(r.Context(), rq) + if err != nil { + common.HandleCommonPaginationErrors(w, r, err) + return + } // Process the current page of transactions pageSums := processPostings(account, cursor, "") // Don't filter by asset yet // Accumulate sums for each asset for _, ps := range pageSums { if _, exists := assetSums[ps.Asset]; !exists { assetSums[ps.Asset] = big.NewInt(0) } assetSums[ps.Asset] = new(big.Int).Add(assetSums[ps.Asset], ps.Sum) } // If there are no more pages, break the loop if !cursor.HasMore || cursor.Next == "" { break } + + // Defensive check: ensure cursor is advancing + if cursor.Next == currentCursor { + api.InternalServerError(w, r, errors.New("pagination cursor not advancing")) + return + } + + currentCursor = cursor.Next } // Prepare the response response := make([]sumResponse, 0, len(assetSums)) for asset, sum := range assetSums { // Skip if this asset doesn't match the filter (if any) if assetFilter != "" && assetFilter != asset { continue } response = append(response, sumResponse{ Account: account, Asset: asset, Sum: sum, }) } // If we have an asset filter but no matching asset, return empty array if assetFilter != "" && len(response) == 0 { api.Ok(w, []sumResponse{}) return } // Return the response api.Ok(w, response) }Note: You'll need to verify if
storagecommon.UnmarshalCursoris the correct function name in your codebase (it's referenced in the Extract function but not shown in the snippets). Adjust the function name and import if needed.
🧹 Nitpick comments (1)
internal/api/v2/controllers_transactions_sum.go (1)
16-20: Consider unexporting sumResponse if not used externally.The
sumResponsetype is exported (capitalized) but appears to be used only within this package for the API response. Unless it's intentionally exposed for external use or testing, consider renaming it tosumResponse(lowercase) to limit its scope and follow the principle of least exposure.Apply this diff if the type doesn't need to be public:
-type sumResponse struct { +type sumResponse struct { Account string `json:"account"` Asset string `json:"asset"` Sum *big.Int `json:"sum"` }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
internal/api/v2/controllers_transactions_sum.go(1 hunks)internal/api/v2/controllers_transactions_sum_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/api/v2/controllers_transactions_sum_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/api/v2/controllers_transactions_sum.go (4)
internal/storage/common/cursor.go (1)
Extract(14-28)internal/storage/common/pagination.go (1)
InitialPaginatedQuery(9-14)internal/storage/common/resource.go (1)
ResourceQuery(408-414)internal/api/common/errors.go (3)
ErrValidation(19-19)HandleCommonPaginationErrors(57-66)InternalServerError(68-73)
🔇 Additional comments (3)
internal/api/v2/controllers_transactions_sum.go (3)
22-67: LGTM!The double-entry bookkeeping logic is correct: debits (source) subtract from the balance and credits (destination) add to it. The use of
big.Intfor financial calculations and the careful handling of immutability withnew(big.Int).Sub/Addare appropriate. Asset filtering logic is also correct.
69-100: LGTM!The function correctly uses
storagecommon.Extractto handle pagination, including cursor tokens from the request, and properly handles errors. The single-page fetch logic is sound.
151-162: LGTM!The final response preparation correctly filters by asset when a filter is provided, and properly handles the case where no assets match the filter by returning an empty array (lines 165-168).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/api/v2/controllers_transactions_sum.go (2)
190-209: Add defensive safeguards against infinite pagination loops.As noted in previous reviews, the pagination loop lacks protective measures that could prevent infinite loops or excessive resource consumption. Add:
- Maximum iteration limit: Cap the number of pages to prevent runaway loops
- Cursor advancement check: Verify that the cursor changes between iterations
Add these defensive checks:
const defaultPageSize = 100 + const maxIterations = 10000 // Adjust based on expected maximum pages assetSums := make(map[string]*big.Int) // Create a new request with the same context to avoid modifying the original req := r.Clone(r.Context()) + iteration := 0 + previousCursor := "" + for { + iteration++ + if iteration > maxIterations { + api.InternalServerError(w, r, errors.New("pagination limit exceeded")) + return + } + cursor, ok := getPaginatedTransactions(w, req, ledgerInstance, defaultPageSize) if !ok { return // Error already handled } pageSums := processPostings(account, cursor, "") for _, ps := range pageSums { if _, exists := assetSums[ps.Asset]; !exists { assetSums[ps.Asset] = big.NewInt(0) } assetSums[ps.Asset] = new(big.Int).Add(assetSums[ps.Asset], ps.Sum) } if !cursor.HasMore || cursor.Next == "" { break } + // Verify cursor is advancing + if cursor.Next == previousCursor { + api.InternalServerError(w, r, errors.New("pagination cursor not advancing")) + return + } + previousCursor = cursor.Next }These safeguards protect against edge cases and make debugging easier if pagination issues arise.
87-163: Critical: Cursor not propagated between pagination iterations.The cursor propagation is broken due to variable shadowing. Line 87 creates a local clone
req := r.Clone(r.Context())that shadows the parameterr. When the cursor is set on this local clone (line 162), the changes are lost because:
- The local
reqvariable is never used again after line 162- On the next iteration in
getTransactionsSum, the same unmodifiedr(now calledreqin the caller) is passed againstoragecommon.Extractnever sees the cursor parameter and always uses the initial queryThis causes the pagination loop to repeatedly fetch the same first page, leading to incorrect sums (or potentially an infinite loop).
Fix the cursor propagation by removing the local clone and modifying the parameter directly:
func getPaginatedTransactions( w http.ResponseWriter, r *http.Request, ledgerInstance ledgercontroller.Controller, pageSize uint64, ) (*bunpaginate.Cursor[ledger.Transaction], bool) { order := bunpaginate.Order(bunpaginate.OrderDesc) // Extract query parameters queryParams := r.URL.Query() startTime := queryParams.Get("start_time") endTime := queryParams.Get("end_time") - // Create a new request with the same context to avoid modifying the original - req := r.Clone(r.Context()) - - rq, err := storagecommon.Extract[any](req, func() (*storagecommon.InitialPaginatedQuery[any], error) { + rq, err := storagecommon.Extract[any](r, func() (*storagecommon.InitialPaginatedQuery[any], error) { // Create a slice to hold query conditions var conditions []query.Builder // Add date range filters if provided if startTime != "" { t, err := time.Parse(time.RFC3339, startTime) if err != nil { return nil, fmt.Errorf("invalid start_time format: %w", err) } conditions = append(conditions, query.Gte("timestamp", t)) } if endTime != "" { t, err := time.Parse(time.RFC3339, endTime) if err != nil { return nil, fmt.Errorf("invalid end_time format: %w", err) } conditions = append(conditions, query.Lte("timestamp", t)) } // For test compatibility, if no conditions, don't set the Builder var builder query.Builder if len(conditions) > 0 { builder = query.And(conditions...) } queryOptions := storagecommon.ResourceQuery[any]{ - Expand: getExpand(req), + Expand: getExpand(r), Builder: builder, } return &storagecommon.InitialPaginatedQuery[any]{ PageSize: pageSize, Column: "timestamp", Order: &order, Options: queryOptions, }, nil }) if err != nil { api.BadRequest(w, common.ErrValidation, err) return nil, false } txs, err := ledgerInstance.ListTransactions(r.Context(), rq) if err != nil { common.HandleCommonPaginationErrors(w, r, err) return nil, false } if txs.HasMore && txs.Next != "" { - q := req.URL.Query() + q := r.URL.Query() q.Set("cursor", txs.Next) - req.URL.RawQuery = q.Encode() + r.URL.RawQuery = q.Encode() } return txs, true }This ensures that the cursor is set on the request that will be passed to the next iteration, allowing
storagecommon.Extractto properly detect and unmarshal the cursor.
🧹 Nitpick comments (1)
internal/api/v2/controllers_transactions_sum.go (1)
27-67: Consider simplifying asset filtering logic.The
processPostingsfunction supports an optionalassetFilter, but it's always called with an empty string (line 196), and the actual filtering happens later ingetTransactionsSum(lines 213-215). This creates redundant iteration over the results.Consider either:
- Pass
assetFilterdirectly toprocessPostingson line 196 to filter during aggregation- Remove the
assetFilterparameter fromprocessPostingsentirely and keep filtering ingetTransactionsSumOption 1 would be more efficient as it filters during aggregation rather than after.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
internal/api/v2/controllers_transactions_sum.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/api/v2/controllers_transactions_sum.go (5)
internal/posting.go (1)
Postings(35-35)internal/storage/common/cursor.go (1)
Extract(14-28)internal/storage/common/pagination.go (1)
InitialPaginatedQuery(9-14)internal/storage/common/resource.go (1)
ResourceQuery(408-414)internal/api/common/errors.go (3)
ErrValidation(19-19)HandleCommonPaginationErrors(57-66)InternalServerError(68-73)
🔇 Additional comments (2)
internal/api/v2/controllers_transactions_sum.go (2)
21-25: LGTM: Well-structured response type.The
sumResponsestruct appropriately uses*big.Intfor arbitrary-precision arithmetic, which is essential for financial calculations.
211-228: LGTM: Response construction handles edge cases correctly.The response construction properly handles the asset filter and correctly returns an empty array when no matching assets are found.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/api/v2/controllers_transactions_sum.go (2)
168-187: Add defensive safeguards against infinite loops.The pagination loop lacks protections against infinite loops, which could occur if the cursor implementation has bugs or doesn't advance properly. As noted in previous reviews, add:
- Maximum iteration limit: Cap the number of pages
- Cursor advancement check: Verify that the cursor value changes between iterations
Based on learnings
Apply this diff:
+ const maxIterations = 10000 // Adjust based on expected transaction volume + iteration := 0 + var previousCursor string + for { + iteration++ + if iteration > maxIterations { + api.InternalServerError(w, r, errors.New("pagination limit exceeded")) + return + } + cursor, ok := getPaginatedTransactions(w, req, ledgerInstance, defaultPageSize) if !ok { return // Error already handled } pageSums := processPostings(account, cursor, "") for _, ps := range pageSums { if _, exists := assetSums[ps.Asset]; !exists { assetSums[ps.Asset] = big.NewInt(0) } assetSums[ps.Asset] = new(big.Int).Add(assetSums[ps.Asset], ps.Sum) } if !cursor.HasMore || cursor.Next == "" { break } + + // Verify cursor is advancing + if cursor.Next == previousCursor { + api.InternalServerError(w, r, errors.New("pagination cursor not advancing")) + return + } + previousCursor = cursor.Next }
82-83: Critical: Local clone discards cursor, breaking pagination.Line 83 creates a new local clone of the request, which means any cursor modifications made at lines 138-140 are lost when the function returns. The outer
reqin the calling loop (line 169) is never updated with the cursor, so pagination never advances—each iteration re-fetches the same first page.Remove the local clone and use the passed-in request directly, since the caller already clones it:
- // Create a new request with the same context to avoid modifying the original - req := r.Clone(r.Context()) - - rq, err := storagecommon.Extract[any](req, func() (*storagecommon.InitialPaginatedQuery[any], error) { + rq, err := storagecommon.Extract[any](r, func() (*storagecommon.InitialPaginatedQuery[any], error) {Then update all subsequent uses of
reqtor:queryOptions := storagecommon.ResourceQuery[any]{ - Expand: getExpand(req), + Expand: getExpand(r), Builder: builder, }if txs.HasMore && txs.Next != "" { - q := req.URL.Query() + q := r.URL.Query() q.Set("cursor", txs.Next) - req.URL.RawQuery = q.Encode() + r.URL.RawQuery = q.Encode() }
🧹 Nitpick comments (1)
internal/api/v2/controllers_transactions_sum.go (1)
25-65: Consider early asset filtering for efficiency.The function computes sums for all assets and then filters at the end (lines 44-53). Additionally, the asset filter is applied again in
getTransactionsSum(lines 191-193). While this works correctly, if an asset filter is provided, filtering postings early could reduce memory usage and improve performance, especially with many assets.Example optimization:
func processPostings(account string, txs *bunpaginate.Cursor[ledger.Transaction], assetFilter string) []sumResponse { assetSums := make(map[string]*big.Int) for _, tx := range txs.Data { for _, posting := range tx.Postings { // Skip postings that don't match the filter if assetFilter != "" && posting.Asset != assetFilter { continue } if posting.Source == account { if _, ok := assetSums[posting.Asset]; !ok { assetSums[posting.Asset] = big.NewInt(0) } assetSums[posting.Asset] = new(big.Int).Sub(assetSums[posting.Asset], posting.Amount) } else if posting.Destination == account { if _, ok := assetSums[posting.Asset]; !ok { assetSums[posting.Asset] = big.NewInt(0) } assetSums[posting.Asset] = new(big.Int).Add(assetSums[posting.Asset], posting.Amount) } } } response := make([]sumResponse, 0, len(assetSums)) for asset, amount := range assetSums { response = append(response, sumResponse{ Account: account, Asset: asset, Sum: amount, }) } return response }Then update line 174 to pass the asset filter and remove lines 191-193 in
getTransactionsSum.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
internal/api/v2/controllers_transactions_sum.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/api/v2/controllers_transactions_sum.go (4)
internal/storage/common/cursor.go (1)
Extract(14-28)internal/storage/common/pagination.go (1)
InitialPaginatedQuery(9-14)internal/storage/common/resource.go (1)
ResourceQuery(408-414)internal/api/common/errors.go (2)
ErrValidation(19-19)HandleCommonPaginationErrors(57-66)
🔇 Additional comments (2)
internal/api/v2/controllers_transactions_sum.go (2)
19-23: LGTM!The
sumResponsestruct is well-defined with appropriate field types, including*big.Intfor handling large financial amounts.
189-207: LGTM on response building logic.The response construction and asset filter application are implemented correctly. The handling of empty results (lines 201-204) ensures proper behavior when no transactions match the filter.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/api/v2/controllers_transactions_sum_test.go (2)
79-80: Remove debug logging.Debug logging in tests should be removed or commented out before merging to keep test output clean.
Apply this diff:
- // Print response body for debugging - t.Logf("Response body: %s", body)
119-285: Reduce code duplication in pagination setup.The test sets up 10 nearly identical page expectations with significant duplication. Consider refactoring using a loop.
Apply this diff to reduce duplication:
- // First page (transactions 0-99) - expectedQuery1 := storagecommon.InitialPaginatedQuery[any]{ - PageSize: 100, - Column: "timestamp", - Order: &order, - Options: storagecommon.ResourceQuery[any]{ - Expand: []string{}, - }, - } - mockLedgerController.EXPECT(). - ListTransactions(gomock.Any(), matchPaginatedQuery(expectedQuery1)). - Return(&bunpaginate.Cursor[ledger.Transaction]{ - Data: transactions[:100], - HasMore: true, - Next: "page2", - }, nil) - - // Second page (transactions 100-199) - expectedQuery2 := storagecommon.InitialPaginatedQuery[any]{ - PageSize: 100, - Column: "timestamp", - Order: &order, - Options: storagecommon.ResourceQuery[any]{ - Expand: []string{}, - }, - } - mockLedgerController.EXPECT(). - ListTransactions(gomock.Any(), matchPaginatedQuery(expectedQuery2)). - Return(&bunpaginate.Cursor[ledger.Transaction]{ - Data: transactions[100:200], - HasMore: true, - Next: "page3", - }, nil) - - // Third page (transactions 200-299) - expectedQuery3 := storagecommon.InitialPaginatedQuery[any]{ - PageSize: 100, - Column: "timestamp", - Order: &order, - Options: storagecommon.ResourceQuery[any]{ - Expand: []string{}, - }, - } - mockLedgerController.EXPECT(). - ListTransactions(gomock.Any(), matchPaginatedQuery(expectedQuery3)). - Return(&bunpaginate.Cursor[ledger.Transaction]{ - Data: transactions[200:300], - HasMore: true, - Next: "page4", - }, nil) - - // Fourth page (transactions 300-399) - expectedQuery4 := storagecommon.InitialPaginatedQuery[any]{ - PageSize: 100, - Column: "timestamp", - Order: &order, - Options: storagecommon.ResourceQuery[any]{ - Expand: []string{}, - }, - } - mockLedgerController.EXPECT(). - ListTransactions(gomock.Any(), matchPaginatedQuery(expectedQuery4)). - Return(&bunpaginate.Cursor[ledger.Transaction]{ - Data: transactions[300:400], - HasMore: true, - Next: "page5", - }, nil) - - // Fifth page (transactions 400-499) - expectedQuery5 := storagecommon.InitialPaginatedQuery[any]{ - PageSize: 100, - Column: "timestamp", - Order: &order, - Options: storagecommon.ResourceQuery[any]{ - Expand: []string{}, - }, - } - mockLedgerController.EXPECT(). - ListTransactions(gomock.Any(), matchPaginatedQuery(expectedQuery5)). - Return(&bunpaginate.Cursor[ledger.Transaction]{ - Data: transactions[400:500], - HasMore: true, - Next: "page6", - }, nil) - - // Sixth page (transactions 500-599) - expectedQuery6 := storagecommon.InitialPaginatedQuery[any]{ - PageSize: 100, - Column: "timestamp", - Order: &order, - Options: storagecommon.ResourceQuery[any]{ - Expand: []string{}, - }, - } - mockLedgerController.EXPECT(). - ListTransactions(gomock.Any(), matchPaginatedQuery(expectedQuery6)). - Return(&bunpaginate.Cursor[ledger.Transaction]{ - Data: transactions[500:600], - HasMore: true, - Next: "page7", - }, nil) - - // Seventh page (transactions 600-699) - expectedQuery7 := storagecommon.InitialPaginatedQuery[any]{ - PageSize: 100, - Column: "timestamp", - Order: &order, - Options: storagecommon.ResourceQuery[any]{ - Expand: []string{}, - }, - } - mockLedgerController.EXPECT(). - ListTransactions(gomock.Any(), matchPaginatedQuery(expectedQuery7)). - Return(&bunpaginate.Cursor[ledger.Transaction]{ - Data: transactions[600:700], - HasMore: true, - Next: "page8", - }, nil) - - // Eighth page (transactions 700-799) - expectedQuery8 := storagecommon.InitialPaginatedQuery[any]{ - PageSize: 100, - Column: "timestamp", - Order: &order, - Options: storagecommon.ResourceQuery[any]{ - Expand: []string{}, - }, - } - mockLedgerController.EXPECT(). - ListTransactions(gomock.Any(), matchPaginatedQuery(expectedQuery8)). - Return(&bunpaginate.Cursor[ledger.Transaction]{ - Data: transactions[700:800], - HasMore: true, - Next: "page9", - }, nil) - - // Ninth page (transactions 800-899) - expectedQuery9 := storagecommon.InitialPaginatedQuery[any]{ - PageSize: 100, - Column: "timestamp", - Order: &order, - Options: storagecommon.ResourceQuery[any]{ - Expand: []string{}, - }, - } - mockLedgerController.EXPECT(). - ListTransactions(gomock.Any(), matchPaginatedQuery(expectedQuery9)). - Return(&bunpaginate.Cursor[ledger.Transaction]{ - Data: transactions[800:900], - HasMore: true, - Next: "page10", - }, nil) - - // Tenth page (transactions 900-999) - last page - expectedQuery10 := storagecommon.InitialPaginatedQuery[any]{ - PageSize: 100, - Column: "timestamp", - Order: &order, - Options: storagecommon.ResourceQuery[any]{ - Expand: []string{}, - }, - } - mockLedgerController.EXPECT(). - ListTransactions(gomock.Any(), matchPaginatedQuery(expectedQuery10)). - Return(&bunpaginate.Cursor[ledger.Transaction]{ - Data: transactions[900:], - HasMore: false, - }, nil) + // Set up expected query template + expectedQuery := storagecommon.InitialPaginatedQuery[any]{ + PageSize: 100, + Column: "timestamp", + Order: &order, + Options: storagecommon.ResourceQuery[any]{ + Expand: []string{}, + }, + } + + // Mock all 10 pages + pageConfigs := []struct { + startIdx int + endIdx int + hasMore bool + next string + }{ + {0, 100, true, "page2"}, + {100, 200, true, "page3"}, + {200, 300, true, "page4"}, + {300, 400, true, "page5"}, + {400, 500, true, "page6"}, + {500, 600, true, "page7"}, + {600, 700, true, "page8"}, + {700, 800, true, "page9"}, + {800, 900, true, "page10"}, + {900, 1000, false, ""}, + } + + for _, cfg := range pageConfigs { + mockLedgerController.EXPECT(). + ListTransactions(gomock.Any(), matchPaginatedQuery(expectedQuery)). + Return(&bunpaginate.Cursor[ledger.Transaction]{ + Data: transactions[cfg.startIdx:cfg.endIdx], + HasMore: cfg.hasMore, + Next: cfg.next, + }, nil) + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
internal/api/v2/controllers_transactions_sum_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/api/v2/controllers_transactions_sum_test.go (4)
internal/storage/common/pagination.go (2)
InitialPaginatedQuery(9-14)PaginatedQuery(25-28)internal/storage/common/resource.go (1)
ResourceQuery(408-414)internal/transaction.go (1)
NewTransaction(227-231)internal/posting.go (1)
NewPosting(26-33)
🔇 Additional comments (5)
internal/api/v2/controllers_transactions_sum_test.go (5)
314-327: LGTM!Error handling test is correctly implemented with appropriate nil controller and response verification.
414-427: LGTM!The test server setup correctly uses chi router patterns and context injection for the mock controller.
429-448: LGTM!The custom gomock matcher is properly implemented with appropriate use of
reflect.DeepEqualfor struct comparison.
450-564: LGTM!Comprehensive testing of the pagination helper function with proper verification of query parameters using
DoAndReturnand appropriate error propagation testing.
119-285: Cursor-based pagination is implemented correctly.getPaginatedTransactionsupdates thecursorURL parameter whentxs.HasMoreandtxs.Nextare set, andgetTransactionsSumloops accordingly until no more pages remain.
|
Hey 🔴 Critical Issues
💡 Solution: SQL Implementation Use SQL aggregation instead (like /aggregate/balances). Reference: internal/storage/ledger/resource_aggregated_balances.go SQL query (using moves table): Files to create/modify:
🤔 Architectural Question Do we really need a dedicated endpoint for this? I'm not entirely convinced we need /transactions/sum. Maybe we should consider evolving existing filters/endpoints instead (e.g., add aggregation capabilities to existing This needs more thought and discussion before we commit to this API design. |
Thanks for the review! |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
docker-compose.ymlis excluded by!**/*.yml
📒 Files selected for processing (12)
internal/api/v2/controllers_transactions_sum.go(1 hunks)internal/api/v2/controllers_transactions_sum_test.go(1 hunks)internal/api/v2/mocks_ledger_controller_test.go(3 hunks)internal/api/v2/routes.go(2 hunks)internal/api/v2/views.go(10 hunks)internal/controller/ledger/controller.go(3 hunks)internal/controller/ledger/controller_default.go(2 hunks)internal/controller/ledger/controller_with_traces.go(4 hunks)internal/controller/ledger/store.go(2 hunks)internal/controller/ledger/store_generated_test.go(2 hunks)internal/storage/ledger/resource_transactions_sum.go(1 hunks)internal/storage/ledger/store.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/api/v2/routes.go
🧰 Additional context used
🧬 Code graph analysis (10)
internal/storage/ledger/resource_transactions_sum.go (2)
internal/controller/ledger/store.go (1)
Store(28-64)internal/storage/ledger/store.go (1)
Store(22-44)
internal/api/v2/controllers_transactions_sum.go (2)
internal/api/common/errors.go (2)
ErrValidation(19-19)InternalServerError(68-73)internal/storage/ledger/resource_transactions_sum.go (1)
TransactionsSum(10-13)
internal/api/v2/mocks_ledger_controller_test.go (2)
internal/api/v1/mocks_ledger_controller_test.go (2)
LedgerController(26-30)LedgerControllerMockRecorder(33-35)internal/storage/ledger/resource_transactions_sum.go (1)
TransactionsSum(10-13)
internal/api/v2/controllers_transactions_sum_test.go (4)
internal/api/v2/mocks_ledger_controller_test.go (2)
NewLedgerController(39-43)LedgerController(27-31)internal/storage/ledger/resource_transactions_sum.go (1)
TransactionsSum(10-13)internal/api/v2/routes.go (1)
NewRouter(21-122)internal/storage/common/pagination.go (1)
InitialPaginatedQuery(9-14)
internal/controller/ledger/controller_with_traces.go (3)
internal/storage/ledger/resource_transactions_sum.go (1)
TransactionsSum(10-13)internal/tracing/tracing.go (1)
TraceWithMetric(100-128)internal/controller/ledger/controller.go (1)
Controller(21-85)
internal/controller/ledger/store.go (1)
internal/storage/ledger/resource_transactions_sum.go (1)
TransactionsSum(10-13)
internal/controller/ledger/store_generated_test.go (1)
internal/storage/ledger/resource_transactions_sum.go (1)
TransactionsSum(10-13)
internal/controller/ledger/controller.go (1)
internal/storage/ledger/resource_transactions_sum.go (1)
TransactionsSum(10-13)
internal/controller/ledger/controller_default.go (2)
internal/controller/system/controller.go (1)
DefaultController(55-71)internal/storage/ledger/resource_transactions_sum.go (1)
TransactionsSum(10-13)
internal/api/v2/views.go (3)
internal/transaction.go (2)
Transaction(36-50)Transaction(52-59)internal/volumes.go (3)
PostCommitVolumes(118-118)Volumes(12-15)Volumes(40-43)internal/storage/ledger/resource_transactions_sum.go (1)
TransactionsSum(10-13)
🔇 Additional comments (5)
internal/controller/ledger/controller_with_traces.go (5)
8-8: LGTM!The time import is correctly added to support the
*time.Timeparameters inGetTransactionsSumWithTimeRange.
47-47: LGTM!The histogram field follows the established naming convention and is correctly placed with other histogram fields.
153-156: LGTM!The histogram initialization follows the consistent panic-on-error pattern used throughout this constructor.
476-498: LGTM!Both methods correctly wrap the underlying controller calls with
TraceWithMetric, following the established pattern used by other methods in this file.Note: Both methods share
getTransactionsSumHistogram, which is reasonable since they measure the same logical operation (fetching transaction sums) with different parameters. If you later need to distinguish between these calls in metrics, you could add separate histograms.
500-529: LGTM!The refactored
LockLedgermethod now correctly follows the same pattern asBeginTX(lines 161-188):
- Declares variables in outer scope
- Calls
TraceWithMetricand assigns wrapped controller toctrl- Inside the closure, calls
underlying.LockLedgerto capturedbandclosein outer scope- Returns the wrapped controller along with the captured
dbandclosefunctionsThe variable reuse (where
ctrlis first assigned the underlying controller at line 514, then reassigned the wrapped controller at line 507) follows the established pattern and is correct.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/api/v2/controllers_transactions_sum.go (1)
59-79: Consider pushing asset filter to the SQL layer for efficiency.The asset filter is currently applied in-memory after fetching all per-asset sums (lines 63-65). While this works correctly, passing
assetFilterdown to the store layer would allow the SQL query to filter assets via aWHERE asset = ?clause, reducing both data transfer and processing overhead—especially beneficial for accounts with many distinct assets.Example changes:
In
controllers_transactions_sum.go:func getTransactionsSum(w http.ResponseWriter, r *http.Request) { account := r.URL.Query().Get("account") if account == "" { api.BadRequest(w, common.ErrValidation, errors.New("account parameter is required")) return } assetFilter := r.URL.Query().Get("asset") // Parse time filters startTime, err := getDate(r, "start_time") if err != nil { api.BadRequest(w, common.ErrValidation, fmt.Errorf("invalid start_time: %w", err)) return } endTime, err := getDate(r, "end_time") if err != nil { api.BadRequest(w, common.ErrValidation, fmt.Errorf("invalid end_time: %w", err)) return } ledgerInstance := common.LedgerFromContext(r.Context()) if ledgerInstance == nil { api.InternalServerError(w, r, errors.New("ledger not found in request context")) return } var transactionsSum []ledgerstore.TransactionsSum - if startTime == nil && endTime == nil { - transactionsSum, err = ledgerInstance.GetTransactionsSum(r.Context(), account) - } else { - transactionsSum, err = ledgerInstance.GetTransactionsSumWithTimeRange(r.Context(), account, startTime, endTime) - } + if startTime == nil && endTime == nil { + transactionsSum, err = ledgerInstance.GetTransactionsSum(r.Context(), account, assetFilter) + } else { + transactionsSum, err = ledgerInstance.GetTransactionsSumWithTimeRange(r.Context(), account, assetFilter, startTime, endTime) + } if err != nil { api.InternalServerError(w, r, err) return } // Convert to response format response := make([]sumResponse, 0, len(transactionsSum)) for _, ts := range transactionsSum { - // Apply asset filter if provided - if assetFilter != "" && assetFilter != ts.Asset { - continue - } - // Parse the sum from string to big.Int exactly sum := new(big.Int) if _, ok := sum.SetString(ts.Sum, 10); !ok { api.InternalServerError(w, r, fmt.Errorf("invalid sum format: %s", ts.Sum)) return } response = append(response, sumResponse{ Account: account, Asset: ts.Asset, Sum: sum, }) } api.Ok(w, response) }Then update the store methods in
resource_transactions_sum.goto accept and use theassetparameter in the WHERE clause (see corresponding comment on that file).internal/storage/ledger/resource_transactions_sum.go (1)
19-31: Consider adding asset filter parameter for SQL-level filtering.To complement the suggested refactor in the API handler, this method could accept an optional
assetparameter and add it to the WHERE clause when provided. This would allow the database to filter by asset, reducing data transfer and processing overhead.Example implementation:
-func (s *Store) TransactionsSumWithTimeRange(ctx context.Context, ledger string, account string, startTime, endTime *time.Time) ([]TransactionsSum, error) { +func (s *Store) TransactionsSumWithTimeRange(ctx context.Context, ledger string, account string, asset string, startTime, endTime *time.Time) ([]TransactionsSum, error) { whereClause := "ledger = ? AND accounts_address = ?" args := []any{ledger, account} + if asset != "" { + whereClause += " AND asset = ?" + args = append(args, asset) + } + if startTime != nil { whereClause += " AND effective_date >= ?" args = append(args, startTime) } if endTime != nil { whereClause += " AND effective_date <= ?" args = append(args, endTime) } query := fmt.Sprintf("SELECT asset, SUM(CASE WHEN is_source THEN -amount::numeric ELSE amount::numeric END)::text as sum FROM %s WHERE %s GROUP BY asset", s.GetPrefixedRelationName("moves"), whereClause) rows, err := s.db.QueryContext(ctx, query, args...) if err != nil { return nil, err } defer rows.Close() var results []TransactionsSum for rows.Next() { var asset string var sum string if err := rows.Scan(&asset, &sum); err != nil { return nil, err } results = append(results, TransactionsSum{ Asset: asset, Sum: sum, }) } if err := rows.Err(); err != nil { return nil, err } return results, nil }Also update the
TransactionsSumwrapper method:-func (s *Store) TransactionsSum(ctx context.Context, ledger string, account string) ([]TransactionsSum, error) { - return s.TransactionsSumWithTimeRange(ctx, ledger, account, nil, nil) +func (s *Store) TransactionsSum(ctx context.Context, ledger string, account string, asset string) ([]TransactionsSum, error) { + return s.TransactionsSumWithTimeRange(ctx, ledger, account, asset, nil, nil) }Note: This refactor would require updating all call sites, including controller methods and their corresponding interface definitions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
internal/api/v2/controllers_transactions_sum.go(1 hunks)internal/storage/ledger/resource_transactions_sum.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/storage/ledger/resource_transactions_sum.go (2)
internal/controller/ledger/store.go (1)
Store(28-64)internal/storage/ledger/store.go (1)
Store(22-44)
internal/api/v2/controllers_transactions_sum.go (2)
internal/api/common/errors.go (2)
ErrValidation(19-19)InternalServerError(68-73)internal/storage/ledger/resource_transactions_sum.go (1)
TransactionsSum(10-13)
🔇 Additional comments (5)
internal/api/v2/controllers_transactions_sum.go (2)
20-82: Excellent refactor—SQL aggregation resolves previous pagination concerns.The switch from in-memory pagination to SQL-level aggregation (via
GetTransactionsSum/GetTransactionsSumWithTimeRange) addresses the critical pagination bugs flagged in earlier reviews. The handler now correctly computes sums across all matching transactions without the risk of incomplete results or infinite loops.
68-72: Sum parsing error handling is correct.The strict parsing of sum strings to
big.Intwith error handling ensures invalid formats are caught and reported appropriately as 500 Internal Server Error.internal/storage/ledger/resource_transactions_sum.go (3)
19-59: SQL aggregation implementation is efficient and correct.The use of SQL
GROUP BYwithSUM(CASE WHEN ...)properly aggregates transaction amounts at the database level, avoiding the memory and performance issues of the previous in-memory approach. The ledger filter is now correctly applied (line 20), resolving the compile error flagged in past reviews.
35-39: Proper resource cleanup for database query.The
defer rows.Close()ensures the result set is properly closed even if an error occurs during scanning, preventing resource leaks.
33-33: No action needed:effective_dateis the intended date column. Themovestable defines botheffective_dateandinsertion_date(no generictimestampfield), so default filtering oneffective_dateis correct.
|
i'll close it, i need more context to understand the pattern here :) |
2 participants
Description:
This PR introduces the
/transactions/sumendpoint to the v2 API, which calculates and returns the sum of transactions for a specified account, grouped by asset. The implementation includes:The endpoint follows the API's standard response format with data wrapping and adheres to the project's coding standards.