feat(accounts,pools): compute balances inside postgres #605
Conversation
WalkthroughReplaces per-account balance retrieval with a bulk storage API Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Service as Service Layer
participant Storage as Storage Layer
participant DB as Database
rect rgb(220,240,220)
Note over Service,Storage: Before — per-account retrieval
Client->>Service: PoolsBalances(poolID)
loop per pool account
Service->>Storage: BalancesGetLatest(accountID)
Storage->>DB: Query balances for account
DB-->>Storage: rows
Storage-->>Service: []*Balance
Service->>Service: Aggregate by asset
end
Service-->>Client: Aggregated balances
end
rect rgb(240,220,220)
Note over Service,Storage: After — bulk multi-account retrieval
Client->>Service: PoolsBalances(poolID)
Service->>Storage: BalancesGetFromAccountIDs(accountIDs, at?)
Storage->>DB: Single bulk query (accountIDs, optional timestamp)
DB-->>Storage: rows
Storage->>Storage: Group by asset, sum Amount, collect RelatedAccounts
Storage-->>Service: []AggregatedBalance
Service-->>Client: Aggregated balances
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2024-12-12T12:45:27.164ZApplied to files:
📚 Learning: 2024-12-17T09:09:17.827ZApplied to files:
📚 Learning: 2025-11-06T15:50:27.524ZApplied to files:
🧬 Code graph analysis (1)internal/storage/balances.go (1)
🔇 Additional comments (1)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #605 +/- ##
==========================================
- Coverage 63.18% 63.12% -0.06%
==========================================
Files 787 787
Lines 35597 35554 -43
==========================================
- Hits 22491 22443 -48
- Misses 11621 11625 +4
- Partials 1485 1486 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
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 (8)
internal/api/services/pools_balances.go(1 hunks)internal/api/services/pools_balances_at.go(2 hunks)internal/api/services/pools_balances_at_test.go(3 hunks)internal/api/services/pools_balances_test.go(2 hunks)internal/storage/balances.go(2 hunks)internal/storage/balances_test.go(9 hunks)internal/storage/storage.go(1 hunks)internal/storage/storage_generated.go(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-12-12T12:45:27.164Z
Learnt from: laouji
Repo: formancehq/payments PR: 193
File: internal/connectors/plugins/public/dummypay/client/client.go:104-131
Timestamp: 2024-12-12T12:45:27.164Z
Learning: The `Balance` struct in `internal/connectors/plugins/public/dummypay/client/client.go` is only used in tests, so changing `AmountInMinors` from `int64` to `*big.Int` is not necessary.
Applied to files:
internal/api/services/pools_balances.gointernal/api/services/pools_balances_at.gointernal/storage/balances.gointernal/storage/balances_test.gointernal/api/services/pools_balances_at_test.gointernal/api/services/pools_balances_test.go
📚 Learning: 2025-05-15T16:35:29.660Z
Learnt from: fguery
Repo: formancehq/payments PR: 427
File: internal/connectors/plugins/public/qonto/balances.go:20-55
Timestamp: 2025-05-15T16:35:29.660Z
Learning: The Qonto connector plugin has unit tests for fetchNextBalances in internal/connectors/plugins/public/qonto/balances_test.go, covering success cases, missing FromPayload, invalid JSON handling, and other error conditions.
Applied to files:
internal/storage/balances_test.gointernal/api/services/pools_balances_at_test.gointernal/api/services/pools_balances_test.go
📚 Learning: 2025-11-06T15:50:27.524Z
Learnt from: paul-nicolas
Repo: formancehq/payments PR: 580
File: internal/connectors/engine/workflow/send_events.go:69-75
Timestamp: 2025-11-06T15:50:27.524Z
Learning: In the payments codebase, the BankAccount model (internal/models/bank_accounts.go) does not have a ConnectorID field. Bank accounts are global entities not scoped to individual connectors, so when sending bank account events via SendEventsRequest, the ConnectorID field should be nil/omitted.
Applied to files:
internal/storage/balances_test.go
🧬 Code graph analysis (5)
internal/storage/storage.go (1)
internal/models/balances.go (1)
AggregatedBalance(155-159)
internal/storage/balances.go (1)
internal/models/balances.go (2)
AggregatedBalance(155-159)Balance(54-69)
internal/storage/storage_generated.go (1)
internal/models/balances.go (1)
AggregatedBalance(155-159)
internal/storage/balances_test.go (1)
internal/models/balances.go (1)
Balance(54-69)
internal/api/services/pools_balances_at_test.go (1)
internal/models/balances.go (1)
AggregatedBalance(155-159)
🔇 Additional comments (11)
internal/api/services/pools_balances_test.go (2)
30-55: LGTM! Test data properly updated to use AggregatedBalance.The test data structure correctly reflects the new
AggregatedBalancemodel withRelatedAccountsandAmountfields, properly testing the aggregation behavior.
100-100: Mock expectation correctly updated for new API.The mock correctly expects
BalancesGetFromAccountIDswithnilfor theatparameter, matching the latest balance retrieval use case.internal/api/services/pools_balances_at.go (1)
34-39: Clean refactor delegating aggregation to storage layer.The implementation correctly uses
pointer.For(at)to pass the timestamp and delegates balance aggregation to the storage layer. This simplifies the service layer code.internal/api/services/pools_balances.go (1)
27-32: LGTM! Consistent with PoolsBalancesAt implementation.The implementation correctly uses
nilfor theatparameter to fetch latest balances, maintaining consistency withPoolsBalancesAtwhich usespointer.For(at).internal/storage/balances_test.go (2)
70-104: Good test data for aggregation scenarios.The
defaultBalancesDuplicates()helper properly sets up test data with multiple balance entries per account/asset combination, enabling proper testing of the aggregation logic.
946-997: Test coverage for multi-account batch queries looks good.The test properly validates:
- Batch querying multiple account IDs
- Aggregation returning one balance per currency
RelatedAccountspopulated correctly- Latest balance amount retrieved after updates
internal/storage/storage.go (1)
33-33: Interface migration complete and verified.All usages of
BalancesGetAtandBalancesGetLatesthave been updated to use the newBalancesGetFromAccountIDsmethod. The old methods have been properly removed from the Storage interface, and the consolidation into a single method with an optional*time.Timeparameter is clean and idiomatic.Note: Test function names (
TestBalancesGetAt,TestBalancesGetLatestFromAccountIDs) still reference the old method naming but correctly call the new interface method.internal/storage/storage_generated.go (1)
162-175: Generated mock correctly reflects the new interface.The mock implementation properly captures the new
BalancesGetFromAccountIDssignature with[]models.AccountIDand optional*time.Timeparameters, returning[]models.AggregatedBalance.internal/api/services/pools_balances_at_test.go (2)
31-56: Test data correctly adapted to newAggregatedBalancetype.The test fixtures properly use the new structure with
RelatedAccounts,Asset, andAmountfields, matching theAggregatedBalancemodel definition.
102-102: Mock expectation correctly updated for new API.Using
pointer.For(at)properly wraps the timestamp for the optional*time.Timeparameter.internal/storage/balances.go (1)
290-342: New aggregated balance retrieval looks well-structured.The query approach using a CTE with
DISTINCT ONto get the latest balance per account/asset, followed by aggregation witharray_aggandSUM, is efficient and correctly consolidates balances across multiple accounts. The optional time filtering logic correctly identifies balances that were active at the specified point in time.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
internal/storage/balances.go (3)
7-7: Remove unusedfmtimport after removing debug statements.The
fmtimport is only used for the debug print that should be removed.- "fmt"
324-324: Remove debugfmt.Printlnstatement before merging.Debug output left in production code will pollute logs and potentially leak query/balance information.
- fmt.Println(balanceAssets) -
328-331:MustAccountIDFromStringcan panic on invalid data.If the database contains malformed account IDs, this will cause a runtime panic. Use the non-panicking variant with error handling.
for _, balanceAsset := range balanceAssets { - relatedAccounts := make([]models.AccountID, len(balanceAsset.AccountIDs)) - for i, accountID := range balanceAsset.AccountIDs { - relatedAccounts[i] = models.MustAccountIDFromString(accountID) + relatedAccounts := make([]models.AccountID, 0, len(balanceAsset.AccountIDs)) + for _, accountID := range balanceAsset.AccountIDs { + parsed, err := models.AccountIDFromString(accountID) + if err != nil { + return nil, e("failed to parse account ID", err) + } + relatedAccounts = append(relatedAccounts, parsed) }
📜 Review details
Configuration used: CodeRabbit UI
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/storage/balances.go(2 hunks)internal/storage/balances_test.go(8 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-12-12T12:45:27.164Z
Learnt from: laouji
Repo: formancehq/payments PR: 193
File: internal/connectors/plugins/public/dummypay/client/client.go:104-131
Timestamp: 2024-12-12T12:45:27.164Z
Learning: The `Balance` struct in `internal/connectors/plugins/public/dummypay/client/client.go` is only used in tests, so changing `AmountInMinors` from `int64` to `*big.Int` is not necessary.
Applied to files:
internal/storage/balances.gointernal/storage/balances_test.go
📚 Learning: 2024-12-17T09:09:17.827Z
Learnt from: laouji
Repo: formancehq/payments PR: 207
File: internal/models/bank_accounts.go:37-37
Timestamp: 2024-12-17T09:09:17.827Z
Learning: In the codebase, calling `String()` on ID types may involve JSON marshaling under the hood and may have potential to panic; therefore, suggesting to use `String()` for optimization may not be beneficial.
Applied to files:
internal/storage/balances.go
📚 Learning: 2025-11-06T15:50:27.524Z
Learnt from: paul-nicolas
Repo: formancehq/payments PR: 580
File: internal/connectors/engine/workflow/send_events.go:69-75
Timestamp: 2025-11-06T15:50:27.524Z
Learning: In the payments codebase, the BankAccount model (internal/models/bank_accounts.go) does not have a ConnectorID field. Bank accounts are global entities not scoped to individual connectors, so when sending bank account events via SendEventsRequest, the ConnectorID field should be nil/omitted.
Applied to files:
internal/storage/balances.gointernal/storage/balances_test.go
📚 Learning: 2025-05-15T16:35:29.660Z
Learnt from: fguery
Repo: formancehq/payments PR: 427
File: internal/connectors/plugins/public/qonto/balances.go:20-55
Timestamp: 2025-05-15T16:35:29.660Z
Learning: The Qonto connector plugin has unit tests for fetchNextBalances in internal/connectors/plugins/public/qonto/balances_test.go, covering success cases, missing FromPayload, invalid JSON handling, and other error conditions.
Applied to files:
internal/storage/balances_test.go
🧬 Code graph analysis (2)
internal/storage/balances.go (1)
internal/models/balances.go (2)
AggregatedBalance(155-159)Balance(54-69)
internal/storage/balances_test.go (1)
internal/models/balances.go (1)
Balance(54-69)
🔇 Additional comments (4)
internal/storage/balances_test.go (4)
69-103: Good addition of test data for multi-account aggregation scenarios.The
defaultBalancesDuplicates()function provides a clear test fixture with cross-account and cross-asset balances to exercise the aggregation logic. The overlapping USD/2 balances for the same account at different times will properly test the "latest balance per account per asset" behavior.
877-942: Tests properly adapted to new API.The point-in-time balance retrieval tests correctly use
BalancesGetFromAccountIDswith a time pointer and validate the expected behavior for empty results, single balances, and multiple assets.
945-995: Comprehensive test coverage for the new bulk retrieval API.The test properly exercises:
- Multi-account aggregation with
niltime for latest balances- Verification of aggregated
AmountandRelatedAccountsfields- Behavior after inserting new balances
997-1035: Good rollback test coverage.The test properly validates that when a foreign key violation occurs, the transaction rolls back and neither balances nor outbox events are persisted.
paul-nicolas
force-pushed
the
feat/compute-balances-postgres
branch
from
789e2f4 to
aa6bafc
Compare
No description provided.