fix(ledger): honor feature-restricted balance sources - #4609
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds feature-key filtering to breakage-plan FBO collection and refactors live balance calculation to consume impacts against feature-matched sub-account sources. ChangesFeature-key eligibility filtering for FBO and live balance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR fixes balance handling for feature-restricted ledger credits. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "fix(ledger): order customer balance impo..." | Re-trigger Greptile |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
openmeter/ledger/collector/collection_fbo.go (1)
207-208: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
slices.Containsfor this new membership check.Tiny cleanup: this is a plain slice lookup, so the standard helper fits better than
lo.Contains. If you touch it, it’d be nice to align the nearby identical check too. As per coding guidelines, “Prefer standard libraryslicesandmapshelpers for common collection operations.”♻️ Proposed direction
- if len(route.Features) > 0 && !lo.Contains(route.Features, featureKey) { + if len(route.Features) > 0 && !slices.Contains(route.Features, featureKey) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/ledger/collector/collection_fbo.go` around lines 207 - 208, The feature membership check in collection_fbo.go currently uses lo.Contains for a plain slice lookup; switch this and the nearby identical route.Features check in the same collection logic to the standard library slices.Contains helper. Update the relevant condition(s) in the collector flow so the membership tests are consistent with the coding guideline and no longer rely on lo.Contains.Source: Coding guidelines
openmeter/ledger/customerbalance/service.go (1)
336-345: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftWatch the new N+1 balance-query path.
This now calls
GetSubAccountBalanceonce per matching sub-account duringGetBalance. For customers with many feature/priority routes, that can turn a balance read into many DB-backed balance reads; a batched “balances by sub-account/route” query would keep the new allocability behavior without the round-trip growth. As per path instructions, “Anything related to event ingestion, message processing, database operations should be vetted for potential performance bottlenecks.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/ledger/customerbalance/service.go` around lines 336 - 345, The new GetBalance loop introduces an N+1 database pattern by calling GetSubAccountBalance once per matching sub-account in the subAccounts iteration. Update the GetBalance flow in customerbalance/service.go to batch these lookups through a single balances-by-sub-account/route query in BalanceQuerier, then map results back to each subAccount while preserving the existing routeFilter and allocability behavior. Use the GetBalance and GetSubAccountBalance paths to locate the change and replace the per-item DB round trips with one batched fetch.Source: Path instructions
openmeter/ledger/collector/collection_fbo_test.go (1)
143-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repo’s decimal assertion style here.
These expected values are simple integers, so
require.Equal(..., Amount.InexactFloat64())matches the test guideline and keeps failures a bit cleaner. As per coding guidelines, “When assertingalpacadecimal.Decimalequality in tests, preferrequire.Equal(t, expectedFloat64, actual.InexactFloat64())and simplefloat64(...)literals when precision allows.”♻️ Proposed test assertion cleanup
- require.True(t, alpacadecimal.NewFromInt(30).Equal(sources[0].Amount), "restricted source amount: %s", sources[0].Amount) + require.Equal(t, float64(30), sources[0].Amount.InexactFloat64(), "restricted source amount") require.Empty(t, sources[1].Address.Route().Route().Features) - require.True(t, alpacadecimal.NewFromInt(10).Equal(sources[1].Amount), "unrestricted source amount: %s", sources[1].Amount) + require.Equal(t, float64(10), sources[1].Amount.InexactFloat64(), "unrestricted source amount") @@ - require.True(t, alpacadecimal.NewFromInt(10).Equal(unattributedSources[0].Amount), "unrestricted source amount: %s", unattributedSources[0].Amount) + require.Equal(t, float64(10), unattributedSources[0].Amount.InexactFloat64(), "unrestricted source amount")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/ledger/collector/collection_fbo_test.go` around lines 143 - 158, The test assertions in collectCustomerFBOForFeatureForTest are using alpacadecimal equality directly for simple integer expectations, but the repo prefers decimal assertions via float64 comparisons in tests. Update the checks on the Amount values in collection_fbo_test.go to use require.Equal with expected float64 literals against Amount.InexactFloat64(), including the sources[0], sources[1], and unattributedSources[0] assertions, while keeping the surrounding feature/length checks unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@openmeter/ledger/collector/collection_fbo_test.go`:
- Around line 143-158: The test assertions in
collectCustomerFBOForFeatureForTest are using alpacadecimal equality directly
for simple integer expectations, but the repo prefers decimal assertions via
float64 comparisons in tests. Update the checks on the Amount values in
collection_fbo_test.go to use require.Equal with expected float64 literals
against Amount.InexactFloat64(), including the sources[0], sources[1], and
unattributedSources[0] assertions, while keeping the surrounding feature/length
checks unchanged.
In `@openmeter/ledger/collector/collection_fbo.go`:
- Around line 207-208: The feature membership check in collection_fbo.go
currently uses lo.Contains for a plain slice lookup; switch this and the nearby
identical route.Features check in the same collection logic to the standard
library slices.Contains helper. Update the relevant condition(s) in the
collector flow so the membership tests are consistent with the coding guideline
and no longer rely on lo.Contains.
In `@openmeter/ledger/customerbalance/service.go`:
- Around line 336-345: The new GetBalance loop introduces an N+1 database
pattern by calling GetSubAccountBalance once per matching sub-account in the
subAccounts iteration. Update the GetBalance flow in customerbalance/service.go
to batch these lookups through a single balances-by-sub-account/route query in
BalanceQuerier, then map results back to each subAccount while preserving the
existing routeFilter and allocability behavior. Use the GetBalance and
GetSubAccountBalance paths to locate the change and replace the per-item DB
round trips with one batched fetch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e09e6d43-2d06-4033-ae47-0838f1da0938
📒 Files selected for processing (5)
openmeter/ledger/collector/collection_fbo.goopenmeter/ledger/collector/collection_fbo_test.goopenmeter/ledger/customerbalance/calculation.goopenmeter/ledger/customerbalance/service.goopenmeter/ledger/customerbalance/service_test.go
Summary
Tests
Summary by CodeRabbit