feat(ledger): balance#4052
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change introduces a complete customer credit balance retrieval feature across the v3 API, consisting of a new ledger service that computes balances from flat-fee and usage-based charges while respecting settlement modes, a corresponding HTTP handler, facade layer, and comprehensive integration with existing billing infrastructure through dependency injection. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as API Handler
participant Service as Balance Service
participant Ledger as Ledger Layer
participant ChargeService as Charge Service
participant UsageService as Usage Service
Client->>Handler: GetCustomerCreditBalance(customerId, filter)
Handler->>Service: GetBalance(ctx, customerId, routeFilter)
Service->>Ledger: GetBalance(FBO, routeFilter)
Ledger-->>Service: settled amount
Service->>ChargeService: ListCharges(customerId)
ChargeService-->>Service: [charges]
par Process Flat-Fee Charges
Service->>Service: NewImpact(charge, amount)
Note over Service: Calculate realized credits<br/>Check settlement mode
and Process Usage-Based Charges
Service->>UsageService: GetCurrentTotals(chargeId)
UsageService-->>Service: current totals
Service->>Service: NewImpact(charge, amount)
end
Service->>Service: CalculatePendingBalance(impacts)
Note over Service: Apply bounded/unbounded<br/>amounts to balance
Service-->>Handler: ledger.Balance{settled, pending}
Handler->>Handler: mapBalance(currency, balance)
Handler-->>Client: CreditBalance{Available, Pending}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
039238c to
13784da
Compare
| Charge: charge, | ||
| Customer: customerOverride, | ||
| FeatureMeter: featureMeter, | ||
| StoredAtOffset: clock.Now().Add(-usagebased.InternalCollectionPeriod), |
There was a problem hiding this comment.
Given this is ephemeral data we can just use clock.Now()
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/server/wire.go (1)
98-116:⚠️ Potential issue | 🟠 MajorGate the balance stack before DI runs.
Right now the flag only hides the route.
cmd/server/wire_gen.goalready buildscommon.NewCustomerBalanceService(...)on startup, so a deployment with credits turned off still pays the init cost—and can still hit provisioning-related startup failures before the flag is ever checked. I’d make this provider optional off the same flag so “disabled” really skips the balance stack.As per coding guidelines,
**/wire.go: Edit dependency injection wire providers in**/wire.gofiles, then runmake generateto regeneratewire_gen.gowith the Wire code generator.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/server/wire.go` around lines 98 - 116, The DI currently always includes the balance providers (common.CustomerBalance / common.NewCustomerBalanceService) via wire.Build so the balance stack is initialized even when credits are disabled; edit cmd/server/wire.go to gate inclusion of the balance provider set based on the same feature flag (e.g., create a provider func or provider set that returns either the balance providers or an empty provider set depending on the flag, and use that in wire.Build instead of unconditionally listing common.CustomerBalance/common.NewCustomerBalanceService), then run make generate to regenerate wire_gen.go.
🧹 Nitpick comments (3)
config.example.yaml (1)
77-78: Consider commenting this out or adding a note.Since the PR description mentions this is a "super-rough version" not intended for production use, having
credits.enabled: trueas the default in the example config might mislead folks. Maybe comment it out like other optional features, or add a note that additional setup (mentioned as coming in a separate PR) is required?-credits: - enabled: true +# credits: +# enabled: true # Requires additional provisioning - see documentation🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config.example.yaml` around lines 77 - 78, The example config currently enables credits by default (the credits: block with credits.enabled: true); change this to avoid suggesting production use by either commenting out the credits.enabled line (or the whole credits: block) like other optional features, or add a short inline note next to credits.enabled explaining this is a non-production default and that additional setup (as described in the incoming PR) is required before enabling; update the comments near the credits: section and ensure the YAML stays valid.openmeter/billing/charges/usagebased/service/currenttotals.go (1)
19-55: Avoid re-hydrating everything on each totals read.This path does a charge fetch, customer override fetch, and feature-meter resolution for every
GetCurrentTotalscall. In the new customer-balance flow that means several extra round-trips per usage-based charge on a single read request. Reusing the already-hydrated inputs, or adding a batch-oriented path, would keep this endpoint from scaling linearly with avoidable lookups.As per coding guidelines, performance should be a priority in critical code paths, and database operations should be vetted for performance bottlenecks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/service/currenttotals.go` around lines 19 - 55, The GetCurrentTotals path is re-hydrating charge, customer override and feature meters on every call (see s.adapter.GetByID, s.customerOverrideService.GetCustomerOverride, s.featureService.ResolveFeatureMeters and subsequent getRatingForUsage) causing extra round-trips; change GetCurrentTotals to accept or look up pre-hydrated inputs (e.g., add an overloaded/internal method GetCurrentTotalsWithResolved that takes a Charge object, resolved CustomerOverride and FeatureMeter) and update callers to use the pre-resolved batch path or implement a batched resolver that caches/returns multiple resolved items in one call to avoid per-charge DB/service calls. Ensure existing behavior is preserved by delegating from the original GetCurrentTotals to the new method when inputs are not already resolved.app/common/customerbalance.go (1)
133-184: Don't silently shrink a page when another charge type shows up.Anything outside flat-fee / usage-based just falls through and later gets skipped from
Items, whilePageandTotalCountstill describe the original search result. That makes this adapter quietly partial instead of obviously unsupported. I'd either constrain the search to supported types up front, or return an error on an unexpecteditem.Type.Based on learnings,
processorByType.creditPurchaseis intentionally left nil/empty because credit-purchase charges have their own separate lifecycle path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/common/customerbalance.go` around lines 133 - 184, The code currently switches over searchResult.Items and silently drops items with unexpected item.Type (only handles meta.ChargeTypeFlatFee and meta.ChargeTypeUsageBased), causing Page/TotalCount to mismatch the returned Items; modify the first loop that builds flatFeeIDs/usageBasedIDs (the switch over item.Type) to collect any unsupported types and immediately return a clear error (or validation error) listing those unexpected types instead of continuing, ensuring callers know the adapter doesn't support those charge types; update callers of this function or error handling as needed but keep the rest of the flow (flatFeeService.GetByIDs, usageBasedService.GetByIDs, chargesByID, assembling Items, and returning pagination.Result[charges.Charge]) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v3/handlers/customers/credits/get_balance.go`:
- Around line 115-122: mapBalance is returning ledger.Balance.Pending() into
api.CreditBalance.Pending but those semantics differ (ledger.Pending is live
delta, while the public Pending must be not-yet-consumable granted credits), so
stop exposing the misleading value: update the mapBalance(currency
currencyx.Code, balance ledger.Balance) function to not propagate
balance.Pending() into api.CreditBalance.Pending — instead set Pending to a safe
zero/neutral value (e.g., "0" or empty) or omit/populate the field with a
clearly safe placeholder until the customer balance service can provide the
correct granted-pending semantics; keep Currency and Available mapping the same
and add a TODO comment referencing api.CreditBalance and ledger.Balance.Pending
for future reconciliation.
In `@app/config/credits.go`:
- Around line 13-15: Change the default for the credits feature to be opt-in: in
ConfigureCredits, update the v.SetDefault call for "credits.enabled" to false so
the balance facade remains off by default (look for the ConfigureCredits
function and the v.SetDefault("credits.enabled", true) line and change the
boolean to false).
In `@openmeter/ledger/customerbalance/facade_test.go`:
- Around line 52-71: The test for facade.GetBalances is too loose: it only
checks that USD and EUR appear but would still pass if extra currencies are
returned; update the assertions after calling facade.GetBalances
(GetBalancesInput and balances) to assert the exact result size and/or exact set
of returned currencies (e.g., require.Equal for len(balances) and
require.ElementsMatch or equivalent to compare the currencies in balances to the
expected slice {"USD","EUR"}), and keep the existing per-currency balance checks
for USD and EUR.
In `@openmeter/ledger/customerbalance/facade.go`:
- Around line 78-111: The code currently falls back to the global currency
catalog when input.Currencies is empty, causing expensive full-catalog balance
scans; change the branch that builds codes (the block using
currency.Definitions(), dedupeCurrencies and routeFilter) so that when
input.Currencies.Codes is empty you either (A) require the caller to supply a
currency filter by returning a validation error, or (B) use a customer-scoped
discovery method on the service (e.g., add/ call
f.service.GetCustomerActiveCurrencies or
f.service.DiscoverCurrencies(customerID)) to retrieve only currencies the
customer has activity in, then dedupe/validate those codes and call
f.service.GetBalance only for that reduced set before constructing
BalanceByCurrency entries; update any callers/tests accordingly and keep the
existing validation loop (code.Validate()) and balance retrieval logic
(f.service.GetBalance, routeFilter) intact.
In `@openmeter/ledger/customerbalance/service.go`:
- Around line 151-154: The code silently treats failed charge conversions as
not-started because chargeHasStarted currently swallows
AsFlatFeeCharge/AsUsageBasedCharge errors; change chargeHasStarted to return
(bool, error) and update callers (notably Service.getChargePendingBalanceImpact
and the other callsite around lines 199-220) to check and propagate the error
instead of returning nil; specifically, modify chargeHasStarted to surface
conversion errors from AsFlatFeeCharge/AsUsageBasedCharge, then in
getChargePendingBalanceImpact and the other affected function(s) handle the
error (return it) or propagate it up so malformed/hydration failures are not
treated as harmless false negatives.
- Around line 112-149: The current getChargePendingBalanceImpacts implementation
materializes all non-final charges via pagination.CollectAll and then calls
getChargePendingBalanceImpact per charge, causing O(n) memory and many
per-charge service calls; change to process pages as a stream and aggregate
per-page to avoid full materialization: inside the pagination.NewPaginator
handler used by getChargePendingBalanceImpacts, for each returned page group
charges by type (flat vs usage) and for usage-based charges batch their IDs and
call a batched totals lookup (replace repeated GetCurrentTotals calls with a
bulk GetCurrentTotals/FetchCurrentTotalsByChargeIDs on ChargesService or a new
method), filter out irrelevant charges early, build Impacts per charge from the
batched results, and append to impacts incrementally; update
getChargePendingBalanceImpact usages to accept pre-fetched totals or convert it
into a helper that operates on provided batch results so you avoid per-charge
RPC/db calls and in-memory accumulation.
---
Outside diff comments:
In `@cmd/server/wire.go`:
- Around line 98-116: The DI currently always includes the balance providers
(common.CustomerBalance / common.NewCustomerBalanceService) via wire.Build so
the balance stack is initialized even when credits are disabled; edit
cmd/server/wire.go to gate inclusion of the balance provider set based on the
same feature flag (e.g., create a provider func or provider set that returns
either the balance providers or an empty provider set depending on the flag, and
use that in wire.Build instead of unconditionally listing
common.CustomerBalance/common.NewCustomerBalanceService), then run make generate
to regenerate wire_gen.go.
---
Nitpick comments:
In `@app/common/customerbalance.go`:
- Around line 133-184: The code currently switches over searchResult.Items and
silently drops items with unexpected item.Type (only handles
meta.ChargeTypeFlatFee and meta.ChargeTypeUsageBased), causing Page/TotalCount
to mismatch the returned Items; modify the first loop that builds
flatFeeIDs/usageBasedIDs (the switch over item.Type) to collect any unsupported
types and immediately return a clear error (or validation error) listing those
unexpected types instead of continuing, ensuring callers know the adapter
doesn't support those charge types; update callers of this function or error
handling as needed but keep the rest of the flow (flatFeeService.GetByIDs,
usageBasedService.GetByIDs, chargesByID, assembling Items, and returning
pagination.Result[charges.Charge]) unchanged.
In `@config.example.yaml`:
- Around line 77-78: The example config currently enables credits by default
(the credits: block with credits.enabled: true); change this to avoid suggesting
production use by either commenting out the credits.enabled line (or the whole
credits: block) like other optional features, or add a short inline note next to
credits.enabled explaining this is a non-production default and that additional
setup (as described in the incoming PR) is required before enabling; update the
comments near the credits: section and ensure the YAML stays valid.
In `@openmeter/billing/charges/usagebased/service/currenttotals.go`:
- Around line 19-55: The GetCurrentTotals path is re-hydrating charge, customer
override and feature meters on every call (see s.adapter.GetByID,
s.customerOverrideService.GetCustomerOverride,
s.featureService.ResolveFeatureMeters and subsequent getRatingForUsage) causing
extra round-trips; change GetCurrentTotals to accept or look up pre-hydrated
inputs (e.g., add an overloaded/internal method GetCurrentTotalsWithResolved
that takes a Charge object, resolved CustomerOverride and FeatureMeter) and
update callers to use the pre-resolved batch path or implement a batched
resolver that caches/returns multiple resolved items in one call to avoid
per-charge DB/service calls. Ensure existing behavior is preserved by delegating
from the original GetCurrentTotals to the new method when inputs are not already
resolved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b1fd3fb8-2e4d-4e9f-a95e-8dd53c8b96cd
📒 Files selected for processing (27)
api/v3/handlers/customers/credits/get_balance.goapi/v3/handlers/customers/credits/handler.goapi/v3/server/routes.goapi/v3/server/server.goapp/common/config.goapp/common/customerbalance.goapp/common/ledger.goapp/config/config.goapp/config/config_test.goapp/config/credits.gocmd/server/main.gocmd/server/wire.gocmd/server/wire_gen.goconfig.example.yamlopenmeter/billing/charges/charge.goopenmeter/billing/charges/usagebased/service.goopenmeter/billing/charges/usagebased/service/currenttotals.goopenmeter/ledger/customerbalance/calculation.goopenmeter/ledger/customerbalance/facade.goopenmeter/ledger/customerbalance/facade_test.goopenmeter/ledger/customerbalance/service.goopenmeter/ledger/customerbalance/service_test.goopenmeter/ledger/customerbalance/testenv_test.goopenmeter/ledger/testutils/deps.goopenmeter/ledger/testutils/integration.goopenmeter/server/router/router.goopenmeter/server/server.go
| func mapBalance(currency currencyx.Code, balance ledger.Balance) api.CreditBalance { | ||
| // Temporary mapping while the v3 credit-balance schema still predates the | ||
| // customerbalance service's settled/live-pending semantics. | ||
| return api.CreditBalance{ | ||
| Currency: api.BillingCurrencyCode(currency), | ||
| Available: balance.Settled().String(), | ||
| Pending: balance.Pending().String(), | ||
| } |
There was a problem hiding this comment.
Pending means something different in the public schema.
I get the temporary bridge comment, but balance.Pending() here is the live delta after outstanding charge impacts, while api.CreditBalance.Pending is documented as not-yet-consumable granted credits. That means this endpoint can return pending values with semantics clients won't expect, potentially even negative ones, so I don't think this field is safe to expose until the response shape catches up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/v3/handlers/customers/credits/get_balance.go` around lines 115 - 122,
mapBalance is returning ledger.Balance.Pending() into api.CreditBalance.Pending
but those semantics differ (ledger.Pending is live delta, while the public
Pending must be not-yet-consumable granted credits), so stop exposing the
misleading value: update the mapBalance(currency currencyx.Code, balance
ledger.Balance) function to not propagate balance.Pending() into
api.CreditBalance.Pending — instead set Pending to a safe zero/neutral value
(e.g., "0" or empty) or omit/populate the field with a clearly safe placeholder
until the customer balance service can provide the correct granted-pending
semantics; keep Currency and Available mapping the same and add a TODO comment
referencing api.CreditBalance and ledger.Balance.Pending for future
reconciliation.
| func (s *Service) getChargePendingBalanceImpacts(ctx context.Context, customerID customer.CustomerID, currency currencyx.Code) ([]Impact, error) { | ||
| items, err := pagination.CollectAll( | ||
| ctx, | ||
| pagination.NewPaginator(func(ctx context.Context, page pagination.Page) (pagination.Result[charges.Charge], error) { | ||
| return s.ChargesService.ListCharges(ctx, charges.ListChargesInput{ | ||
| Page: page, | ||
| Namespace: customerID.Namespace, | ||
| CustomerIDs: []string{customerID.ID}, | ||
| ChargeTypes: []meta.ChargeType{ | ||
| meta.ChargeTypeFlatFee, | ||
| meta.ChargeTypeUsageBased, | ||
| }, | ||
| StatusNotIn: []meta.ChargeStatus{meta.ChargeStatusFinal}, | ||
| Expands: meta.Expands{meta.ExpandRealizations}, | ||
| }) | ||
| }), | ||
| chargeListPageSize, | ||
| ) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("list charges: %w", err) | ||
| } | ||
|
|
||
| impacts := make([]Impact, 0, len(items)) | ||
| for _, charge := range items { | ||
| impact, err := s.getChargePendingBalanceImpact(ctx, charge, currency) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if impact == nil { | ||
| continue | ||
| } | ||
|
|
||
| impacts = append(impacts, *impact) | ||
| } | ||
|
|
||
| return impacts, nil | ||
| } |
There was a problem hiding this comment.
This read path is still pretty expensive at scale.
CollectAll pulls the full non-final charge set into memory, and every usage-based charge then triggers its own GetCurrentTotals call. For customers with a long tail of active charges, one balance lookup turns into an O(n) scan plus O(m) extra service/database calls on the hot request path. I'd strongly consider aggregating page-by-page and batching/filtering earlier instead of materializing everything first.
As per coding guidelines, performance should be a priority in critical code paths. Anything related to event ingestion, message processing, database operations (regardless of database) should be vetted for potential performance bottlenecks.
Also applies to: 179-196
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/ledger/customerbalance/service.go` around lines 112 - 149, The
current getChargePendingBalanceImpacts implementation materializes all non-final
charges via pagination.CollectAll and then calls getChargePendingBalanceImpact
per charge, causing O(n) memory and many per-charge service calls; change to
process pages as a stream and aggregate per-page to avoid full materialization:
inside the pagination.NewPaginator handler used by
getChargePendingBalanceImpacts, for each returned page group charges by type
(flat vs usage) and for usage-based charges batch their IDs and call a batched
totals lookup (replace repeated GetCurrentTotals calls with a bulk
GetCurrentTotals/FetchCurrentTotalsByChargeIDs on ChargesService or a new
method), filter out irrelevant charges early, build Impacts per charge from the
batched results, and append to impacts incrementally; update
getChargePendingBalanceImpact usages to accept pre-fetched totals or convert it
into a helper that operates on provided batch results so you avoid per-charge
RPC/db calls and in-memory accumulation.
| func (s *Service) getChargePendingBalanceImpact(ctx context.Context, charge charges.Charge, currency currencyx.Code) (*Impact, error) { | ||
| if !chargeHasStarted(charge) { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
Don't silently drop malformed charges here.
chargeHasStarted() turns AsFlatFeeCharge() / AsUsageBasedCharge() failures into false, so a hydration/mapping problem just disappears from the balance instead of surfacing as an error. That can under-report a customer's balance pretty quietly. I'd have chargeHasStarted return (bool, error) and bubble the conversion failure up.
💡 Possible fix
func (s *Service) getChargePendingBalanceImpact(ctx context.Context, charge charges.Charge, currency currencyx.Code) (*Impact, error) {
- if !chargeHasStarted(charge) {
+ started, err := chargeHasStarted(charge)
+ if err != nil {
+ return nil, err
+ }
+ if !started {
return nil, nil
}
switch charge.Type() {
case meta.ChargeTypeFlatFee:
@@
}
}
-func chargeHasStarted(charge charges.Charge) bool {
+func chargeHasStarted(charge charges.Charge) (bool, error) {
now := clock.Now()
switch charge.Type() {
case meta.ChargeTypeFlatFee:
flatFeeCharge, err := charge.AsFlatFeeCharge()
if err != nil {
- return false
+ return false, fmt.Errorf("map flat fee charge: %w", err)
}
- return !now.Before(flatFeeCharge.Intent.ServicePeriod.From)
+ return !now.Before(flatFeeCharge.Intent.ServicePeriod.From), nil
case meta.ChargeTypeUsageBased:
usageBasedCharge, err := charge.AsUsageBasedCharge()
if err != nil {
- return false
+ return false, fmt.Errorf("map usage based charge: %w", err)
}
- return !now.Before(usageBasedCharge.Intent.ServicePeriod.From)
+ return !now.Before(usageBasedCharge.Intent.ServicePeriod.From), nil
default:
- return false
+ return false, nil
}
}Also applies to: 199-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/ledger/customerbalance/service.go` around lines 151 - 154, The code
silently treats failed charge conversions as not-started because
chargeHasStarted currently swallows AsFlatFeeCharge/AsUsageBasedCharge errors;
change chargeHasStarted to return (bool, error) and update callers (notably
Service.getChargePendingBalanceImpact and the other callsite around lines
199-220) to check and propagate the error instead of returning nil;
specifically, modify chargeHasStarted to surface conversion errors from
AsFlatFeeCharge/AsUsageBasedCharge, then in getChargePendingBalanceImpact and
the other affected function(s) handle the error (return it) or propagate it up
so malformed/hydration failures are not treated as harmless false negatives.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
openmeter/ledger/customerbalance/testenv_test.go (1)
313-377: Code duplication with productioncustomerBalanceChargeStore.This implementation is nearly identical to
customerBalanceChargeStoreinapp/common/customerbalance.go. While some duplication in test infrastructure is acceptable, this is essentially the same logic repeated.Consider extracting the common hydration logic into a shared helper or having tests use the same implementation as production (if that's feasible with your DI setup). Not blocking, just something to keep in mind for maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/customerbalance/testenv_test.go` around lines 313 - 377, The test's chargeStore ListCharges duplicates production customerBalanceChargeStore logic; extract the common hydration logic into a shared helper (e.g., a function named hydrateCharges or chargesHydrator) that takes the search result, flatFeeService and usageBasedService and returns the paginated []charges.Charge, then update both chargeStore.ListCharges and customerBalanceChargeStore to call that helper (or, alternatively, have tests instantiate/reuse customerBalanceChargeStore via DI) so the lookup/ID-splitting, GetByIDs calls, and reassembly are implemented once.app/common/customerbalance.go (1)
115-186: Minor: TotalCount may be inconsistent if charges are dropped during hydration.The
TotalCountfromsearchResult(line 183) reflects the search results count, butitemsmay have fewer entries if any charges couldn't be hydrated (lines 173-176 silently skip missing ones). In practice this is unlikely since charges shouldn't disappear between search and hydration, but worth noting.Not blocking since:
- Per the context snippets,
GetByIDsonly returns existing entities (partial results by design)- This is an edge case that shouldn't happen in normal operation
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/common/customerbalance.go` around lines 115 - 186, Summary: TotalCount returned from searchResult may be larger than the number of hydrated items if some charges are missing; adjust TotalCount to account for any dropped items. Fix: in customerBalanceChargeStore.ListCharges, after building items, compute missing := len(searchResult.Items)-len(items) and if missing > 0 set adjustedTotal := searchResult.TotalCount - missing (ensure non-negative) and return pagination.Result with TotalCount: adjustedTotal; otherwise keep searchResult.TotalCount. Reference symbols: ListCharges, searchResult, items, TotalCount, GetByIDs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/common/customerbalance.go`:
- Around line 115-186: Summary: TotalCount returned from searchResult may be
larger than the number of hydrated items if some charges are missing; adjust
TotalCount to account for any dropped items. Fix: in
customerBalanceChargeStore.ListCharges, after building items, compute missing :=
len(searchResult.Items)-len(items) and if missing > 0 set adjustedTotal :=
searchResult.TotalCount - missing (ensure non-negative) and return
pagination.Result with TotalCount: adjustedTotal; otherwise keep
searchResult.TotalCount. Reference symbols: ListCharges, searchResult, items,
TotalCount, GetByIDs.
In `@openmeter/ledger/customerbalance/testenv_test.go`:
- Around line 313-377: The test's chargeStore ListCharges duplicates production
customerBalanceChargeStore logic; extract the common hydration logic into a
shared helper (e.g., a function named hydrateCharges or chargesHydrator) that
takes the search result, flatFeeService and usageBasedService and returns the
paginated []charges.Charge, then update both chargeStore.ListCharges and
customerBalanceChargeStore to call that helper (or, alternatively, have tests
instantiate/reuse customerBalanceChargeStore via DI) so the lookup/ID-splitting,
GetByIDs calls, and reassembly are implemented once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 042bfc38-a25f-4b64-8fc6-0abb3f97efcf
📒 Files selected for processing (7)
app/common/customerbalance.goapp/config/credits.goopenmeter/billing/charges/usagebased/service/currenttotals.goopenmeter/ledger/customerbalance/facade.goopenmeter/ledger/customerbalance/facade_test.goopenmeter/ledger/customerbalance/service.goopenmeter/ledger/customerbalance/testenv_test.go
✅ Files skipped from review due to trivial changes (2)
- app/config/credits.go
- openmeter/ledger/customerbalance/facade_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- openmeter/billing/charges/usagebased/service/currenttotals.go
f708d7f to
1056a8f
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
openmeter/ledger/customerbalance/service.go (2)
198-200:⚠️ Potential issue | 🟠 MajorBubble hydration errors instead of treating them as “not started.”
If
AsFlatFeeCharge()orAsUsageBasedCharge()fails, the charge just disappears from the balance calculation. That can quietly under-report a customer balance on malformed or mis-hydrated rows. I'd havechargeHasStartedreturn(bool, error)and propagate that back out ofgetChargePendingBalanceImpact.💡 Tightening diff
func (s *Service) getChargePendingBalanceImpact(ctx context.Context, charge charges.Charge, currency currencyx.Code) (*Impact, error) { - if !chargeHasStarted(charge) { + started, err := chargeHasStarted(charge) + if err != nil { + return nil, err + } + + if !started { return nil, nil } switch charge.Type() { @@ } } -func chargeHasStarted(charge charges.Charge) bool { +func chargeHasStarted(charge charges.Charge) (bool, error) { now := clock.Now() switch charge.Type() { case meta.ChargeTypeFlatFee: flatFeeCharge, err := charge.AsFlatFeeCharge() if err != nil { - return false + return false, fmt.Errorf("map flat fee charge: %w", err) } - return !now.Before(flatFeeCharge.Intent.ServicePeriod.From) + return !now.Before(flatFeeCharge.Intent.ServicePeriod.From), nil case meta.ChargeTypeUsageBased: usageBasedCharge, err := charge.AsUsageBasedCharge() if err != nil { - return false + return false, fmt.Errorf("map usage based charge: %w", err) } - return !now.Before(usageBasedCharge.Intent.ServicePeriod.From) + return !now.Before(usageBasedCharge.Intent.ServicePeriod.From), nil default: - return false + return false, nil } }Also applies to: 246-266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/customerbalance/service.go` around lines 198 - 200, The current approach swallows hydration errors by treating failed AsFlatFeeCharge()/AsUsageBasedCharge() calls as “not started”; change chargeHasStarted to return (bool, error) so hydration failures are surfaced, update all callers (notably getChargePendingBalanceImpact) to propagate and return the error rather than returning nil balance, and ensure getChargePendingBalanceImpact checks the error from chargeHasStarted and returns it; also apply the same change to the other affected block (lines around 246-266) so any failed bubble hydration surfaces as an error instead of silently dropping the charge.
159-176:⚠️ Potential issue | 🟠 MajorThis still does a full collect + per-charge totals fan-out.
pagination.CollectAllpulls the whole non-final charge set into memory, and every usage-based charge then does its ownGetCurrentTotalscall. On customers with a long tail of open charges, one balance read becomes an O(n) scan with N extra service/DB round-trips on the hot path. Streaming page-by-page and batching usage-based totals would keep this a lot flatter.As per coding guidelines, "Performance should be a priority in critical code paths. Anything related to event ingestion, message processing, database operations (regardless of database) should be vetted for potential performance bottlenecks."
Also applies to: 181-193, 236-243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/customerbalance/service.go` around lines 159 - 176, The current getChargePendingBalanceImpacts implementation uses pagination.CollectAll which loads all non-final charges into memory and then fans out per-charge TotalsService.GetCurrentTotals calls, causing O(n) memory and N extra round-trips; change it to stream page-by-page using pagination.NewPaginator directly (avoid CollectAll/chargeListPageSize collect), and for each page gather all usage-based charge IDs and call the totals/balances endpoint in a single batch per page (batching GetCurrentTotals or the equivalent TotalsService method) before building Impact entries; keep the call to ChargesService.ListCharges (same ListChargesInput) but eliminate per-charge GetCurrentTotals calls and instead perform batched totals lookups per page to flatten memory and round-trip complexity.
🧹 Nitpick comments (1)
openmeter/ledger/customerbalance/service_test.go (1)
17-149: Please add theinvoice_onlybalance case here.The new balance path now depends on
Charge.SettlementMode(), but this table only locks downcredit_onlyandcredit_then_invoice. A small case asserting thatinvoice_onlyleaves the customer credit balance unchanged would cover the third enum and make the behavior much harder to accidentally drift.As per coding guidelines, "
**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/customerbalance/service_test.go` around lines 17 - 149, Add an "invoice only" case to the TestGetBalance tests table: in the tests slice inside TestGetBalance add a case named like "flat fee invoice only" (or "invoice only") which calls env.bookFBOBalance(t, alpacadecimal.NewFromInt(100)) and then env.createFlatFeeCharge(t, alpacadecimal.NewFromInt(30), productcatalog.InvoiceOnlySettlementMode, env.sp()); set wantSettled to 100 and wantPending to 100 (i.e., leaving customer credit balance unchanged). This ensures TestGetBalance (and the tests slice) covers Charge.SettlementMode() == productcatalog.InvoiceOnlySettlementMode using the existing env.createFlatFeeCharge helper and assertions on balance.Settled()/balance.Pending().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/common/customerbalance.go`:
- Around line 171-185: The pagination currently returns searchResult.TotalCount
while silently skipping unresolved charges found via chargesByID lookup (in the
loop that builds items), causing a mismatch; update the return to set TotalCount
to the actual resolved count (len(items)) or, if you prefer to keep the original
total, add logging/tracking for unresolved IDs (the missing entries from
chargesByID) so callers know why Items length differs; locate the loop that
references searchResult.Items and chargesByID and the
pagination.Result[charges.Charge] return and either replace TotalCount with the
resolved items length or emit a concise log/report of unresolved item IDs before
returning.
In `@app/config/config.go`:
- Line 216: The ConfigureCredits(v) function is invoked twice (duplicate call)
which is redundant and confusing; remove the extra invocation so
ConfigureCredits(v) is only called once (keep the original/earlier call and
delete the duplicate later call), ensuring the configuration flow remains
identical—search for the ConfigureCredits(v) calls in this file and delete the
duplicated occurrence to leave a single call.
- Around line 147-149: The file calls c.Credits.Validate() twice with
inconsistent prefixes ("credits" at the first call and "credit" at the second),
which can produce duplicate/confusing errors; remove the duplicate call and keep
the first invocation that appends errors with errorsx.WithPrefix(err, "credits")
so only a single validation of c.Credits occurs (remove the later block that
calls c.Credits.Validate() and appends with the "credit" prefix).
In `@openmeter/billing/charges/charge.go`:
- Around line 135-152: The SettlementMode accessor currently returns whatever
persisted enum value it finds (from c.flatFee.Intent.SettlementMode or
c.usageBased.Intent.SettlementMode) without validating it, so invalid/unknown
enum values slip through; update Charge.SettlementMode to validate the returned
productcatalog.SettlementMode against the allowed set (e.g., the known enum
cases used by NewImpact) and return a non-nil error when the value is
unrecognized — locate the method SettlementMode on type Charge and add a
validation step after obtaining the mode from c.flatFee.Intent.SettlementMode
and c.usageBased.Intent.SettlementMode, returning a descriptive error if the
mode is not one of the supported productcatalog.SettlementMode values.
In `@openmeter/ledger/customerbalance/service.go`:
- Around line 113-120: The pending-path is dropping RouteFilter dimensions by
calling getChargePendingBalanceImpacts(ctx, customerID, filters.Currency) while
settled uses bookedBalance.Settled()/Pending() derived from the full filter; fix
by threading the same normalized RouteFilter through the pending calculation
(e.g., change getChargePendingBalanceImpacts to accept the full normalized
filter used by GetBalance and call it with that filter) or, alternatively,
validate/reject non-currency RouteFilter fields up front in GetBalance so you
only ever compute balances for a single supported dimension; update calls to
getChargePendingBalanceImpacts and balanceCalculator.CalculatePendingBalance to
use the consistent filter and adjust any signature/validation in
s.getChargePendingBalanceImpacts accordingly.
In `@openmeter/ledger/customerbalance/testenv_test.go`:
- Around line 362-367: The helper currently silently skips missing charges by
iterating searchResult.Items and continuing when a lookup in chargesByID fails;
change this to fail fast by returning an error when a searched ID is not present
(i.e., when charge, ok := chargesByID[item.ID.ID]; !ok). Update the helper's
signature to return (or propagate) an error instead of silently dropping items,
and adjust any callers to handle the error so tests fail loudly when the store
can't rehydrate a listed charge.
---
Duplicate comments:
In `@openmeter/ledger/customerbalance/service.go`:
- Around line 198-200: The current approach swallows hydration errors by
treating failed AsFlatFeeCharge()/AsUsageBasedCharge() calls as “not started”;
change chargeHasStarted to return (bool, error) so hydration failures are
surfaced, update all callers (notably getChargePendingBalanceImpact) to
propagate and return the error rather than returning nil balance, and ensure
getChargePendingBalanceImpact checks the error from chargeHasStarted and returns
it; also apply the same change to the other affected block (lines around
246-266) so any failed bubble hydration surfaces as an error instead of silently
dropping the charge.
- Around line 159-176: The current getChargePendingBalanceImpacts implementation
uses pagination.CollectAll which loads all non-final charges into memory and
then fans out per-charge TotalsService.GetCurrentTotals calls, causing O(n)
memory and N extra round-trips; change it to stream page-by-page using
pagination.NewPaginator directly (avoid CollectAll/chargeListPageSize collect),
and for each page gather all usage-based charge IDs and call the totals/balances
endpoint in a single batch per page (batching GetCurrentTotals or the equivalent
TotalsService method) before building Impact entries; keep the call to
ChargesService.ListCharges (same ListChargesInput) but eliminate per-charge
GetCurrentTotals calls and instead perform batched totals lookups per page to
flatten memory and round-trip complexity.
---
Nitpick comments:
In `@openmeter/ledger/customerbalance/service_test.go`:
- Around line 17-149: Add an "invoice only" case to the TestGetBalance tests
table: in the tests slice inside TestGetBalance add a case named like "flat fee
invoice only" (or "invoice only") which calls env.bookFBOBalance(t,
alpacadecimal.NewFromInt(100)) and then env.createFlatFeeCharge(t,
alpacadecimal.NewFromInt(30), productcatalog.InvoiceOnlySettlementMode,
env.sp()); set wantSettled to 100 and wantPending to 100 (i.e., leaving customer
credit balance unchanged). This ensures TestGetBalance (and the tests slice)
covers Charge.SettlementMode() == productcatalog.InvoiceOnlySettlementMode using
the existing env.createFlatFeeCharge helper and assertions on
balance.Settled()/balance.Pending().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 04f4132e-5362-4352-ba5c-6f383cdc6173
📒 Files selected for processing (26)
api/v3/handlers/customers/credits/get_balance.goapi/v3/handlers/customers/credits/handler.goapi/v3/server/routes.goapi/v3/server/server.goapp/common/config.goapp/common/customerbalance.goapp/common/ledger.goapp/config/config.goapp/config/config_test.gocmd/server/main.gocmd/server/wire.gocmd/server/wire_gen.goconfig.example.yamlopenmeter/billing/charges/charge.goopenmeter/billing/charges/usagebased/service.goopenmeter/billing/charges/usagebased/service/currenttotals.goopenmeter/ledger/customerbalance/calculation.goopenmeter/ledger/customerbalance/facade.goopenmeter/ledger/customerbalance/facade_test.goopenmeter/ledger/customerbalance/service.goopenmeter/ledger/customerbalance/service_test.goopenmeter/ledger/customerbalance/testenv_test.goopenmeter/ledger/testutils/deps.goopenmeter/ledger/testutils/integration.goopenmeter/server/router/router.goopenmeter/server/server.go
✅ Files skipped from review due to trivial changes (6)
- config.example.yaml
- app/config/config_test.go
- openmeter/ledger/customerbalance/facade_test.go
- openmeter/server/server.go
- app/common/ledger.go
- openmeter/ledger/customerbalance/facade.go
🚧 Files skipped from review as they are similar to previous changes (10)
- app/common/config.go
- cmd/server/wire.go
- cmd/server/main.go
- cmd/server/wire_gen.go
- api/v3/handlers/customers/credits/handler.go
- openmeter/billing/charges/usagebased/service/currenttotals.go
- openmeter/billing/charges/usagebased/service.go
- openmeter/ledger/testutils/deps.go
- openmeter/ledger/customerbalance/calculation.go
- openmeter/ledger/testutils/integration.go
| items := make([]charges.Charge, 0, len(searchResult.Items)) | ||
| for _, item := range searchResult.Items { | ||
| charge, ok := chargesByID[item.ID.ID] | ||
| if !ok { | ||
| continue | ||
| } | ||
|
|
||
| items = append(items, charge) | ||
| } | ||
|
|
||
| return pagination.Result[charges.Charge]{ | ||
| Page: searchResult.Page, | ||
| TotalCount: searchResult.TotalCount, | ||
| Items: items, | ||
| }, nil |
There was a problem hiding this comment.
Pagination TotalCount may not match actual Items count.
When charges can't be resolved (lines 174-176), they're silently skipped, but TotalCount (line 183) still reflects the original search count. This could confuse pagination clients - if the search returns 10 items but 2 can't be resolved, clients will see TotalCount: 10 with only 8 items.
A couple of options:
- Adjust
TotalCountto reflect actual resolved items - Log/track when charges can't be resolved (data consistency signal)
- If this mismatch is acceptable, add a comment explaining why
🤔 Suggested approach
If the mismatch is intentional, document it:
items := make([]charges.Charge, 0, len(searchResult.Items))
for _, item := range searchResult.Items {
charge, ok := chargesByID[item.ID.ID]
if !ok {
+ // Charge exists in search index but couldn't be resolved from service.
+ // This can happen during eventual consistency windows. TotalCount
+ // intentionally reflects the search index count, not resolved items.
continue
}
items = append(items, charge)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/common/customerbalance.go` around lines 171 - 185, The pagination
currently returns searchResult.TotalCount while silently skipping unresolved
charges found via chargesByID lookup (in the loop that builds items), causing a
mismatch; update the return to set TotalCount to the actual resolved count
(len(items)) or, if you prefer to keep the original total, add logging/tracking
for unresolved IDs (the missing entries from chargesByID) so callers know why
Items length differs; locate the loop that references searchResult.Items and
chargesByID and the pagination.Result[charges.Charge] return and either replace
TotalCount with the resolved items length or emit a concise log/report of
unresolved item IDs before returning.
| if err := c.Credits.Validate(); err != nil { | ||
| errs = append(errs, errorsx.WithPrefix(err, "credits")) | ||
| } |
There was a problem hiding this comment.
Duplicate Credits.Validate() calls with inconsistent prefixes.
c.Credits.Validate() is called twice - once at lines 147-149 with prefix "credits" and again at lines 171-173 with prefix "credit" (singular). While safe because Validate() is currently a no-op, this is confusing and will cause duplicate errors if validation logic is added later. Keep one call and remove the other.
🛠️ Suggested fix
Keep only the first call (lines 147-149) and remove the duplicate:
- if err := c.Credits.Validate(); err != nil {
- errs = append(errs, errorsx.WithPrefix(err, "credit"))
- }
-
return errors.Join(errs...)Also applies to: 171-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/config/config.go` around lines 147 - 149, The file calls
c.Credits.Validate() twice with inconsistent prefixes ("credits" at the first
call and "credit" at the second), which can produce duplicate/confusing errors;
remove the duplicate call and keep the first invocation that appends errors with
errorsx.WithPrefix(err, "credits") so only a single validation of c.Credits
occurs (remove the later block that calls c.Credits.Validate() and appends with
the "credit" prefix).
| ConfigureEvents(v) | ||
| ConfigureBalanceWorker(v) | ||
| ConfigureNotification(v) | ||
| ConfigureCredits(v) |
There was a problem hiding this comment.
Duplicate ConfigureCredits(v) calls.
ConfigureCredits(v) is called twice - at line 216 and line 225. While SetDefault is idempotent, having the call in two places is confusing for future maintainers. Remove one of them.
🛠️ Suggested fix
Remove the duplicate call at line 225:
ConfigureCustomer(v, "customer")
- ConfigureCredits(v)
}Also applies to: 225-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/config/config.go` at line 216, The ConfigureCredits(v) function is
invoked twice (duplicate call) which is redundant and confusing; remove the
extra invocation so ConfigureCredits(v) is only called once (keep the
original/earlier call and delete the duplicate later call), ensuring the
configuration flow remains identical—search for the ConfigureCredits(v) calls in
this file and delete the duplicated occurrence to leave a single call.
| func (c Charge) SettlementMode() (productcatalog.SettlementMode, error) { | ||
| switch c.t { | ||
| case meta.ChargeTypeFlatFee: | ||
| if c.flatFee == nil { | ||
| return "", fmt.Errorf("flat fee charge is nil") | ||
| } | ||
|
|
||
| return c.flatFee.Intent.SettlementMode, nil | ||
| case meta.ChargeTypeUsageBased: | ||
| if c.usageBased == nil { | ||
| return "", fmt.Errorf("usage based charge is nil") | ||
| } | ||
|
|
||
| return c.usageBased.Intent.SettlementMode, nil | ||
| default: | ||
| return "", fmt.Errorf("settlement mode is not supported for charge type %s", c.t) | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate the returned settlement mode here.
NewImpact now relies on this accessor to reject unsupported modes, but an invalid persisted enum currently comes back with nil error and then gets treated like a zero-impact charge downstream. That turns bad data into a silent balance undercount instead of a surfaced error.
💡 Small hardening diff
func (c Charge) SettlementMode() (productcatalog.SettlementMode, error) {
+ var settlementMode productcatalog.SettlementMode
+
switch c.t {
case meta.ChargeTypeFlatFee:
if c.flatFee == nil {
return "", fmt.Errorf("flat fee charge is nil")
}
- return c.flatFee.Intent.SettlementMode, nil
+ settlementMode = c.flatFee.Intent.SettlementMode
case meta.ChargeTypeUsageBased:
if c.usageBased == nil {
return "", fmt.Errorf("usage based charge is nil")
}
- return c.usageBased.Intent.SettlementMode, nil
+ settlementMode = c.usageBased.Intent.SettlementMode
default:
return "", fmt.Errorf("settlement mode is not supported for charge type %s", c.t)
}
+
+ if err := settlementMode.Validate(); err != nil {
+ return "", fmt.Errorf("invalid settlement mode for charge type %s: %w", c.t, err)
+ }
+
+ return settlementMode, nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/billing/charges/charge.go` around lines 135 - 152, The
SettlementMode accessor currently returns whatever persisted enum value it finds
(from c.flatFee.Intent.SettlementMode or c.usageBased.Intent.SettlementMode)
without validating it, so invalid/unknown enum values slip through; update
Charge.SettlementMode to validate the returned productcatalog.SettlementMode
against the allowed set (e.g., the known enum cases used by NewImpact) and
return a non-nil error when the value is unrecognized — locate the method
SettlementMode on type Charge and add a validation step after obtaining the mode
from c.flatFee.Intent.SettlementMode and c.usageBased.Intent.SettlementMode,
returning a descriptive error if the mode is not one of the supported
productcatalog.SettlementMode values.
| impacts, err := s.getChargePendingBalanceImpacts(ctx, customerID, filters.Currency) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("get charge pending balance impacts: %w", err) | ||
| } | ||
|
|
||
| return balance{ | ||
| settled: bookedBalance.Settled(), | ||
| pending: s.balanceCalculator.CalculatePendingBalance(bookedBalance.Pending(), impacts), |
There was a problem hiding this comment.
Don't drop the rest of ledger.RouteFilter on the pending path.
GetBalance asks the FBO account for a balance using the full filter, but pending impacts are recomputed with filters.Currency only. If a caller sets CreditPriority, TransactionAuthorizationStatus, or any future route dimension, Settled() and Pending() stop describing the same slice of the account. I'd either thread the normalized filter through this path or reject unsupported non-currency filters up front.
Also applies to: 159-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/ledger/customerbalance/service.go` around lines 113 - 120, The
pending-path is dropping RouteFilter dimensions by calling
getChargePendingBalanceImpacts(ctx, customerID, filters.Currency) while settled
uses bookedBalance.Settled()/Pending() derived from the full filter; fix by
threading the same normalized RouteFilter through the pending calculation (e.g.,
change getChargePendingBalanceImpacts to accept the full normalized filter used
by GetBalance and call it with that filter) or, alternatively, validate/reject
non-currency RouteFilter fields up front in GetBalance so you only ever compute
balances for a single supported dimension; update calls to
getChargePendingBalanceImpacts and balanceCalculator.CalculatePendingBalance to
use the consistent filter and adjust any signature/validation in
s.getChargePendingBalanceImpacts accordingly.
| items := make([]charges.Charge, 0, len(chargesByID)) | ||
| for _, item := range searchResult.Items { | ||
| charge, ok := chargesByID[item.ID.ID] | ||
| if !ok { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Fail fast if the test store can't rehydrate a listed charge.
Silently skipping a searched ID here means this helper can return fewer Items than the page metadata advertises, which can hide broken test wiring and leave the balance assertions falsely green. Returning an error on a missing ID would make these integration tests a lot more trustworthy.
As per coding guidelines, "**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/ledger/customerbalance/testenv_test.go` around lines 362 - 367, The
helper currently silently skips missing charges by iterating searchResult.Items
and continuing when a lookup in chargesByID fails; change this to fail fast by
returning an error when a searched ID is not present (i.e., when charge, ok :=
chargesByID[item.ID.ID]; !ok). Update the helper's signature to return (or
propagate) an error instead of silently dropping items, and adjust any callers
to handle the error so tests fail loudly when the store can't rehydrate a listed
charge.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
openmeter/ledger/customerbalance/calculation.go (2)
37-54: Consider adding a defensive comment for the ignored errors.The underscore pattern at lines 40 and 43 is currently safe because
NewImpactvalidates thatSettlementMode()succeeds (which implies the underlying charge is non-nil). But if the validation logic ever changes, these silent failures could cause incorrect balance calculations.A small comment noting why the error is safe to ignore would help future readers understand the invariant:
// AsFlatFeeCharge() cannot fail here because NewImpact validated SettlementMode(), // which already checked c.flatFee != nil for FlatFee charge types. charge, _ := i.AsFlatFeeCharge()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/customerbalance/calculation.go` around lines 37 - 54, In Impact.RealizedCredits(), the errors returned by AsFlatFeeCharge() and AsUsageBasedCharge() are ignored; add a brief defensive comment explaining why that is safe—referencing the invariant established by NewImpact and its validation of SettlementMode() (which guarantees the underlying charge is non-nil for the corresponding charge type) — e.g., next to the calls to AsFlatFeeCharge() and AsUsageBasedCharge() state that these functions cannot fail here because NewImpact validated SettlementMode() (and therefore c.flatFee / c.usageBased are present), so the underscore error can be safely ignored.
56-72: Silent zero return for InvoiceOnlySettlementMode.Just a heads-up: charges with
InvoiceOnlySettlementModewill have bothBoundedAmount()andUnboundedAmount()return zero, silently excluding them from pending balance calculations. If that's intentional (invoice-only charges don't affect credit balance), this is fine. If not, you might want to either filter them out earlier or add a comment clarifying this is expected behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/customerbalance/calculation.go` around lines 56 - 72, Both BoundedAmount() and UnboundedAmount() currently return zero for InvoiceOnlySettlementMode without explanation, which silently excludes invoice-only charges; update the functions Impact.BoundedAmount and Impact.UnboundedAmount (and/or SettlementMode() usage) to explicitly handle productcatalog.InvoiceOnlySettlementMode by either adding a clear comment above each function stating "Invoice-only charges intentionally do not affect credit balances" or, if the intended behavior is different, change the logic to filter invoice-only charges earlier or return the appropriate OutstandingAmount; reference Impact.BoundedAmount, Impact.UnboundedAmount and SettlementMode() when making the change so the intent is explicit in code.openmeter/ledger/customerbalance/service.go (1)
282-303: Double validation of service dependencies.The
validate()method re-checks the same service dependencies thatNew()already validated viaConfig.Validate(). This is defensive but slightly redundant sinceNew()is the only constructor and it guarantees these fields are set.If this is intentional (guarding against someone constructing
Service{}directly), consider making theServicestruct private or adding a comment explaining the defensive validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/customerbalance/service.go` around lines 282 - 303, The validate() method duplicates dependency checks already enforced by New() via Config.Validate(); remove the redundant nil-checks for s.AccountResolver, s.SubAccountService, s.ChargesService, and s.UsageBasedService from validate() and keep only the minimal runtime checks (e.g., s == nil and validation of customerID/filters) so callers relying on New() aren't re-validated twice; if you want to keep defensive checks instead, add a comment above validate() explaining the intentional redundancy or make the Service struct private (rename Service -> service) and only expose the New() constructor to prevent direct struct construction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openmeter/ledger/customerbalance/calculation.go`:
- Around line 37-54: In Impact.RealizedCredits(), the errors returned by
AsFlatFeeCharge() and AsUsageBasedCharge() are ignored; add a brief defensive
comment explaining why that is safe—referencing the invariant established by
NewImpact and its validation of SettlementMode() (which guarantees the
underlying charge is non-nil for the corresponding charge type) — e.g., next to
the calls to AsFlatFeeCharge() and AsUsageBasedCharge() state that these
functions cannot fail here because NewImpact validated SettlementMode() (and
therefore c.flatFee / c.usageBased are present), so the underscore error can be
safely ignored.
- Around line 56-72: Both BoundedAmount() and UnboundedAmount() currently return
zero for InvoiceOnlySettlementMode without explanation, which silently excludes
invoice-only charges; update the functions Impact.BoundedAmount and
Impact.UnboundedAmount (and/or SettlementMode() usage) to explicitly handle
productcatalog.InvoiceOnlySettlementMode by either adding a clear comment above
each function stating "Invoice-only charges intentionally do not affect credit
balances" or, if the intended behavior is different, change the logic to filter
invoice-only charges earlier or return the appropriate OutstandingAmount;
reference Impact.BoundedAmount, Impact.UnboundedAmount and SettlementMode() when
making the change so the intent is explicit in code.
In `@openmeter/ledger/customerbalance/service.go`:
- Around line 282-303: The validate() method duplicates dependency checks
already enforced by New() via Config.Validate(); remove the redundant nil-checks
for s.AccountResolver, s.SubAccountService, s.ChargesService, and
s.UsageBasedService from validate() and keep only the minimal runtime checks
(e.g., s == nil and validation of customerID/filters) so callers relying on
New() aren't re-validated twice; if you want to keep defensive checks instead,
add a comment above validate() explaining the intentional redundancy or make the
Service struct private (rename Service -> service) and only expose the New()
constructor to prevent direct struct construction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a266a14c-f646-43fe-8f3d-fe0b7768ec91
📒 Files selected for processing (26)
api/v3/handlers/customers/credits/get_balance.goapi/v3/handlers/customers/credits/handler.goapi/v3/server/routes.goapi/v3/server/server.goapp/common/config.goapp/common/customerbalance.goapp/common/ledger.goapp/config/config.goapp/config/config_test.gocmd/server/main.gocmd/server/wire.gocmd/server/wire_gen.goconfig.example.yamlopenmeter/billing/charges/charge.goopenmeter/billing/charges/usagebased/service.goopenmeter/billing/charges/usagebased/service/currenttotals.goopenmeter/ledger/customerbalance/calculation.goopenmeter/ledger/customerbalance/facade.goopenmeter/ledger/customerbalance/facade_test.goopenmeter/ledger/customerbalance/service.goopenmeter/ledger/customerbalance/service_test.goopenmeter/ledger/customerbalance/testenv_test.goopenmeter/ledger/testutils/deps.goopenmeter/ledger/testutils/integration.goopenmeter/server/router/router.goopenmeter/server/server.go
✅ Files skipped from review due to trivial changes (6)
- openmeter/server/server.go
- app/config/config_test.go
- openmeter/ledger/customerbalance/facade_test.go
- openmeter/ledger/testutils/integration.go
- cmd/server/wire.go
- openmeter/ledger/customerbalance/testenv_test.go
🚧 Files skipped from review as they are similar to previous changes (12)
- config.example.yaml
- app/common/config.go
- cmd/server/main.go
- app/common/ledger.go
- cmd/server/wire_gen.go
- openmeter/billing/charges/usagebased/service/currenttotals.go
- openmeter/billing/charges/usagebased/service.go
- openmeter/ledger/customerbalance/service_test.go
- openmeter/ledger/customerbalance/facade.go
- app/common/customerbalance.go
- app/config/config.go
- api/v3/server/routes.go
…dits config field Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ice provider Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
93d15ee to
0ed733e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app/config/config.go (1)
147-149:⚠️ Potential issue | 🟡 MinorKeep a single
Credits.Validate()call.
Configuration.Validate()now hitsc.Credits.Validate()twice with two different prefixes ("credits"and"credit"). It's harmless while the validator is empty, but it'll duplicate and muddy the error output as soon as credits validation gets real checks.Also applies to: 171-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/config/config.go` around lines 147 - 149, Configuration.Validate() currently calls c.Credits.Validate() twice with different prefixes, causing duplicated error output; remove the duplicate call so Credits.Validate() is invoked only once and standardize the prefix (use "credits" or "credit" consistently). Locate the two calls to c.Credits.Validate() inside Configuration.Validate(), keep one invocation and wrap its returned error with errorsx.WithPrefix using the chosen consistent prefix, and delete the other invocation and its associated append to errs to avoid duplicated errors.openmeter/billing/charges/charge.go (1)
135-152:⚠️ Potential issue | 🟠 MajorValidate the settlement mode before returning it.
customerbalance.NewImpact()treats a nil error fromSettlementMode()as "safe to use". If a charge carries an unknown settlement mode, this accessor returns it verbatim and the balance calculator later treats the charge as zero impact instead of surfacing bad data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/charge.go` around lines 135 - 152, The SettlementMode accessor (Charge.SettlementMode) must validate that the returned productcatalog.SettlementMode is one of the supported/known values before returning it; update Charge.SettlementMode to, after extracting Intent.SettlementMode for flatFee and usageBased, check the value against the canonical set (e.g., the package's allowed constants or a productcatalog.IsValidSettlementMode helper) and return an error if it's unknown/invalid so callers like customerbalance.NewImpact don't treat invalid modes as safe.app/common/customerbalance.go (1)
170-184:⚠️ Potential issue | 🟡 MinorDon't silently drop search hits here.
If a hit can't be reloaded from
GetByIDs, it gets skipped, butTotalCountstill reports the original search size. That makes the page internally inconsistent and hides whether the miss was expected or a data issue. Please either adjustTotalCountto the resolved slice or surface the skipped IDs explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/common/customerbalance.go` around lines 170 - 184, The code currently drops hits when a charge ID is missing (using chargesByID lookup) but still returns the original searchResult.TotalCount, causing inconsistent pages; modify the loop to collect any missing IDs (e.g., missingIDs slice) while building items and then set the returned TotalCount to len(items) (or alternatively include the missing IDs in the response if your pagination.Result supports it); also emit a warning/log with the missing IDs so data issues are visible. Update the block that builds items from searchResult.Items and the returned pagination.Result[charges.Charge] to use the adjusted TotalCount and log missing IDs.
🧹 Nitpick comments (2)
openmeter/ledger/customerbalance/service_test.go (1)
90-103: Please add the usage-based "already realized" case too.The idempotency check currently only locks down flat-fee charges, but
Impact.RealizedCredits()has a separate usage-based branch. A matching usage-based test would help keep that path from regressing. As per coding guidelines,**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/customerbalance/service_test.go` around lines 90 - 103, Add a parallel test case that exercises the usage-based idempotency branch in Impact.RealizedCredits(): after setting up the FBO balance (env.bookFBOBalance) create a usage-based charge (use the env helper analogous to createFlatFeeCharge, e.g. env.createUsageBasedCharge or env.createUsageCharge) with the same credit-only settlement mode, trigger whatever helper advances/realizes usage-based charges (the counterpart to env.advanceFlatFeeCharge, e.g. env.advanceUsageBasedCharge or env.recordUsageAndSettle), and assert wantSettled and wantPending equal the same values (70) so the "already realized" usage path is covered and cannot regress.api/v3/handlers/customers/credits/get_balance.go (1)
93-96: Consider extracting the zero-balance filtering condition for clarity.The condition on line 94 is doing a few things at once. It might be slightly easier to grok if the "no filter provided" check and the "is zero balance" check were more visually separated. That said, this is a minor readability nit and the current form works fine.
💡 Optional: Extract condition for readability
+ noFilterProvided := len(request.Currencies.Codes) == 0 + isZeroBalance := item.Balance.Settled().IsZero() && item.Balance.Pending().IsZero() for _, item := range balancesByCurrency { - if len(request.Currencies.Codes) == 0 && item.Balance.Settled().IsZero() && item.Balance.Pending().IsZero() { + if noFilterProvided && isZeroBalance { continue }Note:
noFilterProvidedcould be hoisted outside the loop, butisZeroBalancewould need to stay inside since it depends onitem.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/customers/credits/get_balance.go` around lines 93 - 96, The current loop over balancesByCurrency in the GetBalance handler contains a compound if that mixes the "no filter provided" check with the zero-balance check; refactor by introducing named booleans: hoist noFilterProvided = len(request.Currencies.Codes) == 0 outside the loop, compute isZeroBalance = item.Balance.Settled().IsZero() && item.Balance.Pending().IsZero() inside the loop, and replace the compound if with if noFilterProvided && isZeroBalance { continue } to improve readability while keeping existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/ledger/customerbalance/facade.go`:
- Around line 74-90: The currency discovery path only calls
f.service.getFBOCurrencies when input.Currencies.Codes is empty, so currencies
present only in pending charges (f.service.getChargePendingBalanceImpacts) can
be missed; update the discovery to union
dedupeCurrencies(input.Currencies.Codes) (or the empty set) with currencies
extracted from getChargePendingBalanceImpacts(ctx, input.CustomerID) and then
validate each code with code.Validate(); use the same dedupeCurrencies helper to
remove duplicates and ensure the resulting codes slice includes both FBO
currencies (getFBOCurrencies) and any currencies derived from pending charges
before returning or computing balances.
---
Duplicate comments:
In `@app/common/customerbalance.go`:
- Around line 170-184: The code currently drops hits when a charge ID is missing
(using chargesByID lookup) but still returns the original
searchResult.TotalCount, causing inconsistent pages; modify the loop to collect
any missing IDs (e.g., missingIDs slice) while building items and then set the
returned TotalCount to len(items) (or alternatively include the missing IDs in
the response if your pagination.Result supports it); also emit a warning/log
with the missing IDs so data issues are visible. Update the block that builds
items from searchResult.Items and the returned pagination.Result[charges.Charge]
to use the adjusted TotalCount and log missing IDs.
In `@app/config/config.go`:
- Around line 147-149: Configuration.Validate() currently calls
c.Credits.Validate() twice with different prefixes, causing duplicated error
output; remove the duplicate call so Credits.Validate() is invoked only once and
standardize the prefix (use "credits" or "credit" consistently). Locate the two
calls to c.Credits.Validate() inside Configuration.Validate(), keep one
invocation and wrap its returned error with errorsx.WithPrefix using the chosen
consistent prefix, and delete the other invocation and its associated append to
errs to avoid duplicated errors.
In `@openmeter/billing/charges/charge.go`:
- Around line 135-152: The SettlementMode accessor (Charge.SettlementMode) must
validate that the returned productcatalog.SettlementMode is one of the
supported/known values before returning it; update Charge.SettlementMode to,
after extracting Intent.SettlementMode for flatFee and usageBased, check the
value against the canonical set (e.g., the package's allowed constants or a
productcatalog.IsValidSettlementMode helper) and return an error if it's
unknown/invalid so callers like customerbalance.NewImpact don't treat invalid
modes as safe.
---
Nitpick comments:
In `@api/v3/handlers/customers/credits/get_balance.go`:
- Around line 93-96: The current loop over balancesByCurrency in the GetBalance
handler contains a compound if that mixes the "no filter provided" check with
the zero-balance check; refactor by introducing named booleans: hoist
noFilterProvided = len(request.Currencies.Codes) == 0 outside the loop, compute
isZeroBalance = item.Balance.Settled().IsZero() &&
item.Balance.Pending().IsZero() inside the loop, and replace the compound if
with if noFilterProvided && isZeroBalance { continue } to improve readability
while keeping existing behavior.
In `@openmeter/ledger/customerbalance/service_test.go`:
- Around line 90-103: Add a parallel test case that exercises the usage-based
idempotency branch in Impact.RealizedCredits(): after setting up the FBO balance
(env.bookFBOBalance) create a usage-based charge (use the env helper analogous
to createFlatFeeCharge, e.g. env.createUsageBasedCharge or
env.createUsageCharge) with the same credit-only settlement mode, trigger
whatever helper advances/realizes usage-based charges (the counterpart to
env.advanceFlatFeeCharge, e.g. env.advanceUsageBasedCharge or
env.recordUsageAndSettle), and assert wantSettled and wantPending equal the same
values (70) so the "already realized" usage path is covered and cannot regress.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c22d7b31-703a-401b-8067-6ac4db0f6639
📒 Files selected for processing (23)
api/v3/handlers/customers/credits/get_balance.goapi/v3/handlers/customers/credits/handler.goapi/v3/server/routes.goapi/v3/server/server.goapp/common/customerbalance.goapp/common/ledger.goapp/config/config.goapp/config/config_test.gocmd/server/main.gocmd/server/wire.gocmd/server/wire_gen.goconfig.example.yamlopenmeter/billing/charges/charge.goopenmeter/billing/charges/usagebased/service.goopenmeter/billing/charges/usagebased/service/currenttotals.goopenmeter/ledger/customerbalance/calculation.goopenmeter/ledger/customerbalance/facade.goopenmeter/ledger/customerbalance/facade_test.goopenmeter/ledger/customerbalance/service.goopenmeter/ledger/customerbalance/service_test.goopenmeter/ledger/customerbalance/testenv_test.goopenmeter/server/router/router.goopenmeter/server/server.go
✅ Files skipped from review due to trivial changes (6)
- openmeter/server/server.go
- app/common/ledger.go
- app/config/config_test.go
- openmeter/billing/charges/usagebased/service.go
- openmeter/ledger/customerbalance/testenv_test.go
- cmd/server/wire_gen.go
🚧 Files skipped from review as they are similar to previous changes (8)
- openmeter/ledger/customerbalance/facade_test.go
- api/v3/handlers/customers/credits/handler.go
- api/v3/server/routes.go
- config.example.yaml
- cmd/server/main.go
- cmd/server/wire.go
- openmeter/billing/charges/usagebased/service/currenttotals.go
- openmeter/ledger/customerbalance/service.go
| var codes []currencyx.Code | ||
| if len(input.Currencies.Codes) > 0 { | ||
| codes = dedupeCurrencies(input.Currencies.Codes) | ||
|
|
||
| for _, code := range codes { | ||
| if err := code.Validate(); err != nil { | ||
| return nil, fmt.Errorf("currency %q is not supported by ledger: %w", code, err) | ||
| } | ||
| } | ||
| } else { | ||
| var err error | ||
|
|
||
| codes, err = f.service.getFBOCurrencies(ctx, input.CustomerID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("get FBO currencies: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.go" | grep -E "(facade|customerbalance)" | head -20Repository: openmeterio/openmeter
Length of output: 389
🏁 Script executed:
cd openmeter && find . -type f -name "facade.go" | head -10Repository: openmeterio/openmeter
Length of output: 99
🏁 Script executed:
rg "getFBOCurrencies" --type go -B 2 -A 15Repository: openmeterio/openmeter
Length of output: 2762
🏁 Script executed:
rg "func.*GetBalance" --type go -B 2 -A 20Repository: openmeterio/openmeter
Length of output: 22656
🏁 Script executed:
rg "type Service" openmeter/ledger/customerbalance --type go -A 30 | head -50Repository: openmeterio/openmeter
Length of output: 2198
🏁 Script executed:
rg "func.*getFBOCurrencies" openmeter/ledger/customerbalance/service.go -A 30Repository: openmeterio/openmeter
Length of output: 1062
🏁 Script executed:
rg "func.*getChargePendingBalanceImpacts" openmeter/ledger/customerbalance/service.go -A 30Repository: openmeterio/openmeter
Length of output: 1100
🏁 Script executed:
cat -n openmeter/ledger/customerbalance/service.go | sed -n '1,200p'Repository: openmeterio/openmeter
Length of output: 7614
🏁 Script executed:
rg "createFlatFeeCharge" openmeter/ledger/customerbalance --type go -B 5 -A 15 | head -60Repository: openmeterio/openmeter
Length of output: 5034
🏁 Script executed:
rg "bookFBOBalance" openmeter/ledger/customerbalance --type go -B 3 -A 8Repository: openmeterio/openmeter
Length of output: 11542
🏁 Script executed:
rg "provisioning|provision" openmeter/ledger/customerbalance --type go -iRepository: openmeterio/openmeter
Length of output: 47
🏁 Script executed:
rg "pending.*charge|charge.*pending" openmeter/billing/charges --type go -l | head -5Repository: openmeterio/openmeter
Length of output: 108
🏁 Script executed:
cat -n openmeter/ledger/customerbalance/facade_test.go | sed -n '1,100p'Repository: openmeterio/openmeter
Length of output: 3900
🏁 Script executed:
rg "bookFBOBalance|createFlatFeeCharge" openmeter/ledger/customerbalance/testenv_test.go -B 2 -A 20 | head -80Repository: openmeterio/openmeter
Length of output: 2024
🏁 Script executed:
cat -n openmeter/ledger/customerbalance/testenv_test.go | sed -n '1,150p'Repository: openmeterio/openmeter
Length of output: 6093
🏁 Script executed:
rg "GetCustomerAccounts|FBOAccount" openmeter/ledger/account --type go -l | head -5Repository: openmeterio/openmeter
Length of output: 109
🏁 Script executed:
rg "ensure.*subaccount|create.*subaccount|provision" openmeter/billing/charges --type go -i | head -20Repository: openmeterio/openmeter
Length of output: 1627
🏁 Script executed:
cd openmeter && find . -path "*/ledger/account*" -name "*.go" -type f | grep -v test | head -10Repository: openmeterio/openmeter
Length of output: 374
🏁 Script executed:
rg "ListSubAccounts|GetBalance" openmeter/ledger/account/account.go -B 3 -A 15 | head -60Repository: openmeterio/openmeter
Length of output: 1367
🏁 Script executed:
cat -n openmeter/ledger/account/account_customer.go | sed -n '1,100p'Repository: openmeterio/openmeter
Length of output: 3944
🏁 Script executed:
rg "ensure.*fbo|fbo.*subaccount|route.*provision" openmeter/billing/charges --type go -i | head -20Repository: openmeterio/openmeter
Length of output: 47
🏁 Script executed:
cat -n openmeter/ledger/customerbalance/service.go | sed -n '195,250p'Repository: openmeterio/openmeter
Length of output: 2148
🏁 Script executed:
rg "EnsureSubAccount|GetSubAccountForRoute" openmeter/ledger/account --type go -B 3 -A 15 | head -80Repository: openmeterio/openmeter
Length of output: 6037
🏁 Script executed:
rg "GetBalance.*currency|route.*currency" openmeter/ledger/account/account.go --type go -B 2 -A 15 | head -50Repository: openmeterio/openmeter
Length of output: 47
🏁 Script executed:
rg "CreateCharge|flatfee.*Create" openmeter/billing/charges/flatfee/service --type go -lRepository: openmeterio/openmeter
Length of output: 116
🏁 Script executed:
cat -n openmeter/billing/charges/flatfee/service/create.go | sed -n '1,100p'Repository: openmeterio/openmeter
Length of output: 4038
🏁 Script executed:
rg "handler|Handler.*flatfee" openmeter/billing/charges/flatfee/service --type go -l | head -3Repository: openmeterio/openmeter
Length of output: 223
🏁 Script executed:
rg "getChargePendingBalanceImpact|getChargePendingBalanceImpacts" openmeter/ledger/customerbalance/service.go -B 2 -A 3Repository: openmeterio/openmeter
Length of output: 947
🏁 Script executed:
cat -n openmeter/ledger/customerbalance/service.go | sed -n '159,180p'Repository: openmeterio/openmeter
Length of output: 1033
🏁 Script executed:
rg "CreateCharges|Handler.*Create" openmeter/billing/charges/flatfee/adapter --type go -l | head -3Repository: openmeterio/openmeter
Length of output: 116
🏁 Script executed:
rg "ListCharges.*pending|StatusNotIn.*Final" openmeter/ledger/customerbalance/service.go -B 3 -A 10Repository: openmeterio/openmeter
Length of output: 400
🏁 Script executed:
cat -n openmeter/ledger/customerbalance/facade_test.go | sed -n '42,74p'Repository: openmeterio/openmeter
Length of output: 1552
🏁 Script executed:
rg "EnsureSubAccount|GetSubAccountForRoute" openmeter/billing/charges --type go -r | head -20Repository: openmeterio/openmeter
Length of output: 478
🏁 Script executed:
rg "handler|Handler" openmeter/billing/charges/flatfee/service/service.go -B 2 -A 5 | head -30Repository: openmeterio/openmeter
Length of output: 662
🏁 Script executed:
rg "EnsureSubAccount|GetSubAccountForRoute" openmeter/billing/charges --type go | head -20Repository: openmeterio/openmeter
Length of output: 47
🏁 Script executed:
rg "Handler.*Create|handler.*Create" openmeter/billing/charges/flatfee --type go -B 3 -A 10 | head -40Repository: openmeterio/openmeter
Length of output: 47
🏁 Script executed:
cat -n openmeter/ledger/account/account_customer.go | sed -n '85,120p'Repository: openmeterio/openmeter
Length of output: 1613
🏁 Script executed:
rg "func.*GetBalance" openmeter/ledger/account/account_customer.go -A 15Repository: openmeterio/openmeter
Length of output: 47
🏁 Script executed:
cat -n openmeter/ledger/account/account.go | sed -n '20,60p'Repository: openmeterio/openmeter
Length of output: 1425
When discovering currencies, also include any currencies from pending charges.
When Currencies.Codes is empty, the code discovers currencies only through getFBOCurrencies(), which queries for existing FBO subaccounts. However, getChargePendingBalanceImpacts() already fetches all pending charges and could derive currencies from those. If a customer has a pending charge in a currency where no FBO subaccount was provisioned yet, that currency won't appear in the discovered list, and its balance (including the pending impact) won't be included in the response.
Consider unioning the currencies from pending charges into the discovery logic, or confirm that your provisioning guarantees FBO subaccounts exist before charges can be created.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/ledger/customerbalance/facade.go` around lines 74 - 90, The
currency discovery path only calls f.service.getFBOCurrencies when
input.Currencies.Codes is empty, so currencies present only in pending charges
(f.service.getChargePendingBalanceImpacts) can be missed; update the discovery
to union dedupeCurrencies(input.Currencies.Codes) (or the empty set) with
currencies extracted from getChargePendingBalanceImpacts(ctx, input.CustomerID)
and then validate each code with code.Validate(); use the same dedupeCurrencies
helper to remove duplicates and ensure the resulting codes slice includes both
FBO currencies (getFBOCurrencies) and any currencies derived from pending
charges before returning or computing balances.
Overview
Does a super-rough version of a balance API, should not be used in production
Note that for this to work provisioning and other setup is still required, that's coming in a separate PR
Summary by CodeRabbit
Release Notes