feat: credit balances final#4559
Merged
Merged
Conversation
GAlexIHU
added
the
release-note/ignore
Contributor
|
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:
📝 WalkthroughWalkthroughThe PR updates credit balance and grant schemas, wires effective_at through credit purchase and grant flows, refactors ledger balance handling to return decimals directly, and expands customer balance computation, filtering, and tests. ChangesCredits and ledger routing updates
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR completes the credit balance refactor: renames
Confidence Score: 5/5The core balance calculation logic, the effectiveAt scheduling path, and the ledger booking time change are all correct and well-tested. All guard conditions in isPendingCreditGrantAt and canBecomeEffectiveLedgerCreditAt are verified by unit tests. The bookedAt = ServicePeriod.To change in issueCreditPurchaseGroup is covered by a dedicated chargeadapter test. The only new gaps found are a missing live assertion in the e2e helper and a silent no-match edge case for multi-feature internal callers — both non-blocking and already partially constrained by ValidateAsFeatureFilter. The listPendingGrantCandidateCharges path in service.go carries documented FIXME performance debt that should be revisited before this feature reaches high-volume customers. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+289
to
+310
| if err != nil { | ||
| return nil, fmt.Errorf("get pending grant amount: %w", err) | ||
| } | ||
|
|
||
| settled := bookedBalance.Add(advanceBalance) | ||
|
|
||
| return balance{ | ||
| settled: settled, | ||
| pending: s.balanceCalculator.CalculatePendingBalance(settled, impacts), | ||
| live: s.balanceCalculator.CalculateLiveBalance(settled, impacts), | ||
| pending: pending, | ||
| }, nil | ||
| } | ||
|
|
||
| func currentBalanceQuery(query ledger.BalanceQuery) ledger.BalanceQuery { | ||
| if query.After != nil || query.AsOf != nil { | ||
| return query | ||
| func (s *service) GetBalanceCurrencies(ctx context.Context, input GetBalanceCurrenciesInput) ([]currencyx.Code, error) { | ||
| if err := input.Validate(); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| asOf := clock.Now() | ||
| query.AsOf = &asOf | ||
| return query | ||
| fboCurrencies, err := s.getFBOCurrencies(ctx, input.CustomerID) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
Contributor
There was a problem hiding this comment.
Settled() is consumed
Every cursor-based GetBalance call (the path used by ListCreditTransactions to compute the running balance) still executes getPendingGrantAmount, which triggers the full listPendingGrantCandidateCharges expansion. The result is immediately thrown away — ListCreditTransactions calls runningBalance.Settled() on line 194 and discards Live() and Pending(). For customers with many credit-purchase charges this is an N-page × (charge-scan) overhead on every transaction-list request.
Comment on lines
33
to
56
| currency: Currencies.CurrencyCode; | ||
|
|
||
| /** | ||
| * Credits that have been granted but cannot yet be consumed. Includes grants | ||
| * awaiting payment clearance or with a future effective date. | ||
| * Credits available after applying currently live charge impacts. | ||
| */ | ||
| @visibility(Lifecycle.Read) | ||
| @example("200.00") | ||
| pending: Shared.Numeric; | ||
| live: Shared.Numeric; | ||
|
|
||
| /** | ||
| * Credits that can be consumed right now. Derived from cleared grants after | ||
| * applying eligibility and restriction rules. | ||
| * Credits that have been booked on the ledger as of the balance timestamp. | ||
| */ | ||
| @visibility(Lifecycle.Read) | ||
| @example("150.00") | ||
| available: Shared.Numeric; | ||
| settled: Shared.Numeric; | ||
|
|
||
| /** | ||
| * Credits that have been granted but are not yet written to the ledger, or are | ||
| * written to the ledger with a future booked time. | ||
| */ | ||
| @visibility(Lifecycle.Read) | ||
| @example("50.00") | ||
| pending: Shared.Numeric; | ||
| } |
Contributor
There was a problem hiding this comment.
available removed and pending semantics silently redefined
The old CreditBalance exposed available (consumable right now) and pending (awaiting payment / future effective). This PR removes available entirely and reuses the field name pending for a different concept (not-yet-ledger-booked grants). Any client currently reading available will get null/missing, and any client reading pending will get a value with entirely different meaning. If this API is consumed by external clients or SDKs, a versioned deprecation or at minimum an explicit changelog entry is needed before merging.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
openmeter/billing/creditgrant/service/service.go (1)
216-234: 📐 Maintainability & Code Quality | 🔵 TrivialThe instant period is correct—remove the stale TODO comment.
Good catch! The instant period (From == To == effectiveAt) is actually intentional. The
Intentstruct has a docstring that explains it directly: "When set, the credit purchase service period is pinned to this instant." Credit grants are point-in-time entities by design, and the test expectations confirm this behavior is deliberate.Since the Intent field's docstring already explains the semantics, the TODO comment on line 228 is redundant and confusing. Removing it would make the code clearer.
🤖 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/billing/creditgrant/service/service.go` around lines 216 - 234, The TODO comment on line 228 within the creditpurchase.Intent initialization is stale and should be removed. The instant period (where From, To, and FullServicePeriod are all set to the same effectiveAt value) is intentional and correct for credit grants as point-in-time entities, and the Intent struct's docstring already explains this behavior. Simply delete the comment "// TODO: replace with actual service period" to remove the confusion and make the code clearer.api/v3/handlers/customers/credits/feature_filter.go (1)
21-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider enhancing the error message for better UX.
The error "exists=true operator is not supported" is accurate, but users might not know what to do instead. You could optionally make it more helpful like:
"exists=true is not supported; use filter[feature_key][eq]=<key> to filter by feature, or omit the filter to include all features"That said, the current message is consistent with other operator rejections, so this is just a nice-to-have.
🤖 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 `@api/v3/handlers/customers/credits/feature_filter.go` around lines 21 - 27, The error message returned when the exists=true operator is encountered in the feature filter needs to be more user-friendly and provide guidance. In the condition where f.Exists is checked and returns an error (the branch returning customerbalance.AllFeatureFilter()), replace the current error message "exists=true operator is not supported" with a more helpful message that explains what users should do instead, such as suggesting they use filter[feature_key][eq]=<key> to filter by specific features or omit the filter entirely to include all features.test/credits/creditgrant_test.go (1)
191-237: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding defer clock.UnFreeze() after clock.SetTime().
The test freezes time on Line 202 with
clock.SetTime(now)but doesn't unfreeze it. As per coding guidelines, when freezing time in tests, pair it withdefer clock.UnFreeze()to prevent the frozen time from affecting later assertions or subtests.⏱️ Proposed fix
now := datetime.MustParseTimeInLocation(s.T(), "2026-04-17T11:23:00Z", time.UTC).AsTime() effectiveAt := datetime.MustParseTimeInLocation(s.T(), "2026-04-18T09:30:00Z", time.UTC).AsTime() clock.SetTime(now) + defer clock.UnFreeze() grant, err := s.CreditGrantService.Create(ctx, creditgrant.CreateInput{🤖 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 `@test/credits/creditgrant_test.go` around lines 191 - 237, The test function TestCreatePromotionalGrantUsesEffectiveAtForServicePeriodAndLedgerBookedAt freezes time with clock.SetTime(now) but does not unfreeze it, which can cause the frozen time to persist and affect subsequent tests. Add a defer statement immediately after the clock.SetTime(now) call to unfreeze the clock when the test completes, ensuring that the time freeze is scoped only to this test and does not interfere with other tests.Source: Coding guidelines
openmeter/ledger/customerbalance/service.go (2)
419-443: 🚀 Performance & Scalability | 🔵 TrivialAdd short-term guardrails around the pending-grant full scan path.
Nice callout in the FIXME. Since this runs on balance reads, consider adding lightweight guardrails now (latency/page-count metrics and a temporary max-page safety limit) so this path can’t silently become a tail-latency hotspot before the durable query shape lands.
As per path instructions
**/*.go: “Performance should be a priority in critical code paths… 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 419 - 443, The listPendingGrantCandidateCharges function performs a full scan without any performance guardrails, which could become a tail-latency hotspot since it runs on every balance read. Add lightweight guardrails to this function by implementing latency and page-count metrics to monitor performance, and introduce a temporary maximum page safety limit to prevent unbounded pagination scans. This will help catch performance degradation before the durable query shape is implemented.Source: Path instructions
553-556: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename pending-named impact helpers to live-named helpers.
These helpers now compute live balance impacts, but the
PendingBalanceImpactnames make the flow harder to follow.♻️ Suggested rename
- return getFlatFeeChargePendingBalanceImpact(charge, currency, featureFilter) + return getFlatFeeChargeLiveBalanceImpact(charge, currency, featureFilter) case meta.ChargeTypeUsageBased: - return s.getUsageBasedChargePendingBalanceImpact(ctx, charge, currency, featureFilter) + return s.getUsageBasedChargeLiveBalanceImpact(ctx, charge, currency, featureFilter) -func getFlatFeeChargePendingBalanceImpact(charge charges.Charge, currency currencyx.Code, featureFilter mo.Option[creditpurchase.FeatureFilters]) (*Impact, error) { +func getFlatFeeChargeLiveBalanceImpact(charge charges.Charge, currency currencyx.Code, featureFilter mo.Option[creditpurchase.FeatureFilters]) (*Impact, error) { -func (s *service) getUsageBasedChargePendingBalanceImpact(ctx context.Context, charge charges.Charge, currency currencyx.Code, featureFilter mo.Option[creditpurchase.FeatureFilters]) (*Impact, error) { +func (s *service) getUsageBasedChargeLiveBalanceImpact(ctx context.Context, charge charges.Charge, currency currencyx.Code, featureFilter mo.Option[creditpurchase.FeatureFilters]) (*Impact, error) {As per path instructions
**/*.go: “make readability and maintainability a priority.”Also applies to: 561-578
🤖 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 553 - 556, The function names getFlatFeeChargePendingBalanceImpact and getUsageBasedChargePendingBalanceImpact contain "PendingBalanceImpact" but now compute live balance impacts, making the code flow confusing. Rename these functions and all their usages throughout the file (including the range 561-578) to reflect that they compute live balance impacts instead of pending ones. Update all call sites where these functions are invoked to use the new names consistently.Source: Path instructions
🤖 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.
Inline comments:
In `@openmeter/ledger/customerbalance/expired_loader_test.go`:
- Around line 125-126: Add an immediate cleanup pairing for each
clock.FreezeTime call in the test file expired_loader_test.go. After each
clock.FreezeTime invocation (at lines 125-126, 320-321, and 600-601), add a
defer statement or t.Cleanup call in the same scope that invokes
clock.UnFreeze() to ensure frozen time does not leak across subsequent
assertions or subtests. This prevents test isolation issues where frozen time
from one test section affects another.
In `@openmeter/ledger/customerbalance/funded_loader.go`:
- Around line 35-40: The Load method has an N+1 query pattern where
balanceCursorForFundedActivity is called for each activity in the loop over
result.Items, causing a separate database round-trip per item. Move the balance
cursor resolution into the funded-activity query path itself so the cursors are
resolved as part of the initial query, or implement a batched group lookup by
IDs that resolves all cursors in a single operation instead of calling
balanceCursorForFundedActivity per activity. Ensure this is applied to all
locations in the Load method where this pattern occurs, including both the main
loop and the additional code sections mentioned in the review.
In `@openmeter/ledger/customerbalance/service_test.go`:
- Around line 278-279: Replace the boolean-based assertions using
`assert.True(t, balance.Settled().Equal(...))` and `assert.True(t,
balance.Live().Equal(...))` with the standard repo convention of using
`require.Equal(t, expectedValue, actual.InexactFloat64())`. Convert the
assertions to call `.InexactFloat64()` on the return values from `Settled()` and
`Live()` methods and use direct equality comparison with `require.Equal` instead
of the `.Equal()` method check wrapped in `assert.True`. Apply the same pattern
to all affected assertion lines noted in the comment.
---
Nitpick comments:
In `@api/v3/handlers/customers/credits/feature_filter.go`:
- Around line 21-27: The error message returned when the exists=true operator is
encountered in the feature filter needs to be more user-friendly and provide
guidance. In the condition where f.Exists is checked and returns an error (the
branch returning customerbalance.AllFeatureFilter()), replace the current error
message "exists=true operator is not supported" with a more helpful message that
explains what users should do instead, such as suggesting they use
filter[feature_key][eq]=<key> to filter by specific features or omit the filter
entirely to include all features.
In `@openmeter/billing/creditgrant/service/service.go`:
- Around line 216-234: The TODO comment on line 228 within the
creditpurchase.Intent initialization is stale and should be removed. The instant
period (where From, To, and FullServicePeriod are all set to the same
effectiveAt value) is intentional and correct for credit grants as point-in-time
entities, and the Intent struct's docstring already explains this behavior.
Simply delete the comment "// TODO: replace with actual service period" to
remove the confusion and make the code clearer.
In `@openmeter/ledger/customerbalance/service.go`:
- Around line 419-443: The listPendingGrantCandidateCharges function performs a
full scan without any performance guardrails, which could become a tail-latency
hotspot since it runs on every balance read. Add lightweight guardrails to this
function by implementing latency and page-count metrics to monitor performance,
and introduce a temporary maximum page safety limit to prevent unbounded
pagination scans. This will help catch performance degradation before the
durable query shape is implemented.
- Around line 553-556: The function names getFlatFeeChargePendingBalanceImpact
and getUsageBasedChargePendingBalanceImpact contain "PendingBalanceImpact" but
now compute live balance impacts, making the code flow confusing. Rename these
functions and all their usages throughout the file (including the range 561-578)
to reflect that they compute live balance impacts instead of pending ones.
Update all call sites where these functions are invoked to use the new names
consistently.
In `@test/credits/creditgrant_test.go`:
- Around line 191-237: The test function
TestCreatePromotionalGrantUsesEffectiveAtForServicePeriodAndLedgerBookedAt
freezes time with clock.SetTime(now) but does not unfreeze it, which can cause
the frozen time to persist and affect subsequent tests. Add a defer statement
immediately after the clock.SetTime(now) call to unfreeze the clock when the
test completes, ensuring that the time freeze is scoped only to this test and
does not interfere with other tests.
🪄 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: c9ac4572-810c-4dc9-a60f-92558af1f2ad
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (65)
api/spec/packages/aip-client-javascript/src/models/schemas.tsapi/spec/packages/aip-client-javascript/src/models/types.tsapi/spec/packages/aip/src/customers/credits/balance.tspapi/spec/packages/aip/src/customers/credits/grant.tspapi/spec/packages/aip/src/customers/credits/operations.tspapi/v3/api.gen.goapi/v3/filters/parse.goapi/v3/filters/parse_test.goapi/v3/handlers/customers/credits/convert.goapi/v3/handlers/customers/credits/feature_filter.goapi/v3/handlers/customers/credits/feature_filter_test.goapi/v3/handlers/customers/credits/get_balance.goapi/v3/handlers/customers/credits/list_transactions.goapi/v3/test/filters_test.goopenmeter/billing/charges/creditpurchase/adapter/funded_credit_activity.goopenmeter/billing/charges/creditpurchase/adapter/funded_credit_activity_test.goopenmeter/billing/charges/creditpurchase/charge.goopenmeter/billing/charges/creditpurchase/charge_test.goopenmeter/billing/charges/creditpurchase/featurefilters.goopenmeter/billing/charges/creditpurchase/funded_credit_activity.goopenmeter/billing/creditgrant/errors.goopenmeter/billing/creditgrant/service.goopenmeter/billing/creditgrant/service/service.goopenmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.goopenmeter/ledger/account/adapter/subaccount.goopenmeter/ledger/breakage/adapter/query.goopenmeter/ledger/breakage/adapter/query_test.goopenmeter/ledger/breakage/adapter/record.goopenmeter/ledger/breakage/adapter/record_test.goopenmeter/ledger/breakage/breakage_impacts.goopenmeter/ledger/breakage/service.goopenmeter/ledger/breakage/types.goopenmeter/ledger/chargeadapter/creditpurchase.goopenmeter/ledger/chargeadapter/creditpurchase_test.goopenmeter/ledger/chargeadapter/helpers.goopenmeter/ledger/collector/collection_fbo.goopenmeter/ledger/customerbalance/calculation.goopenmeter/ledger/customerbalance/expired_loader.goopenmeter/ledger/customerbalance/expired_loader_test.goopenmeter/ledger/customerbalance/facade.goopenmeter/ledger/customerbalance/facade_test.goopenmeter/ledger/customerbalance/feature_filter.goopenmeter/ledger/customerbalance/funded_loader.goopenmeter/ledger/customerbalance/ledger_loader.goopenmeter/ledger/customerbalance/loaders.goopenmeter/ledger/customerbalance/noop.goopenmeter/ledger/customerbalance/service.goopenmeter/ledger/customerbalance/service_test.goopenmeter/ledger/customerbalance/testenv_test.goopenmeter/ledger/customerbalance/transactions.goopenmeter/ledger/historical/adapter/ledger.goopenmeter/ledger/historical/adapter/ledger_test.goopenmeter/ledger/historical/adapter/sumentries_query.goopenmeter/ledger/historical/balance.goopenmeter/ledger/noop/noop.goopenmeter/ledger/primitives.goopenmeter/ledger/query.goopenmeter/ledger/routing.goopenmeter/ledger/routing_test.goopenmeter/ledger/testutils/integration.goopenmeter/ledger/transactions/collection.goopenmeter/ledger/validations_test.gotest/credits/base.gotest/credits/creditgrant_test.gotest/credits/sanity_test.go
💤 Files with no reviewable changes (2)
- openmeter/billing/creditgrant/errors.go
- openmeter/ledger/query.go
Comment on lines
35
to
+40
| items := make([]CreditTransaction, 0, len(result.Items)) | ||
| for _, activity := range result.Items { | ||
| balanceCursor, err := l.balanceCursorForFundedActivity(ctx, input.CustomerID.Namespace, activity) | ||
| if err != nil { | ||
| return creditTransactionLoaderResult{}, err | ||
| } |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Avoid per-activity ledger group fetches in this list path
Load() now resolves balanceCursor by calling GetTransactionGroup() once per funded activity. That creates an N+1 DB/read pattern on a hot listing path and can noticeably increase latency and backend load as page size or traffic grows.
Please move cursor resolution into the funded-activity query path (or add a batched group lookup by IDs) so each page stays near constant round-trips.
As per path instructions, "Performance should be a priority in critical code paths. Anything related to ... database operations ... should be vetted for potential performance bottlenecks."
Also applies to: 66-97
🤖 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/funded_loader.go` around lines 35 - 40, The
Load method has an N+1 query pattern where balanceCursorForFundedActivity is
called for each activity in the loop over result.Items, causing a separate
database round-trip per item. Move the balance cursor resolution into the
funded-activity query path itself so the cursors are resolved as part of the
initial query, or implement a batched group lookup by IDs that resolves all
cursors in a single operation instead of calling balanceCursorForFundedActivity
per activity. Ensure this is applied to all locations in the Load method where
this pattern occurs, including both the main loop and the additional code
sections mentioned in the review.
Source: Path instructions
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/ledger/customerbalance/expired_loader_test.go (1)
185-211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd brief given/when/then comments to these subtests.
These new scenarios are pretty dense, and a one-line intent comment at the top of each subtest body would make the feature-filter/breakage cases much easier to scan later. As per coding guidelines, "For service and lifecycle subtests in Go, start each subtest body with concise intent comments using given-when-then format when the scenario is non-trivial."
Also applies to: 383-398, 458-474
🤖 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/expired_loader_test.go` around lines 185 - 211, Add concise given-when-then intent comments at the start of each non-trivial service subtest body in expired_loader_test, including the subtests around the ListCreditTransactions cases and the other referenced subtest blocks. Update the t.Run closures to begin with a short comment that states the setup, action, and expected outcome so the feature-filter and breakage scenarios are easier to scan later. Keep the comments brief and consistent across the affected subtests, using the existing test names and ListCreditTransactions flow as the locator points.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/customerbalance/expired_loader_test.go`:
- Around line 185-211: Add concise given-when-then intent comments at the start
of each non-trivial service subtest body in expired_loader_test, including the
subtests around the ListCreditTransactions cases and the other referenced
subtest blocks. Update the t.Run closures to begin with a short comment that
states the setup, action, and expected outcome so the feature-filter and
breakage scenarios are easier to scan later. Keep the comments brief and
consistent across the affected subtests, using the existing test names and
ListCreditTransactions flow as the locator points.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46876848-d483-4bd4-9843-3530058335b9
📒 Files selected for processing (8)
openmeter/ledger/customerbalance/expired_loader_test.goopenmeter/ledger/customerbalance/facade.goopenmeter/ledger/customerbalance/funded_loader.goopenmeter/ledger/customerbalance/noop.goopenmeter/ledger/customerbalance/service.goopenmeter/ledger/customerbalance/service_test.goopenmeter/ledger/customerbalance/transactions.goopenmeter/ledger/customerbalance/transactions_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- openmeter/ledger/customerbalance/funded_loader.go
- openmeter/ledger/customerbalance/service_test.go
- openmeter/ledger/customerbalance/noop.go
- openmeter/ledger/customerbalance/transactions.go
- openmeter/ledger/customerbalance/service.go
tothandras
previously approved these changes
Jun 29, 2026
GAlexIHU
force-pushed
the
feat/credit-balances-final
branch
2 times, most recently
from
a5bd143 to
144ceee
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@test/credits/creditgrant_test.go`:
- Around line 203-205: The test case that calls clock.SetTime in the credit
grant suite leaves the global clock shifted for later tests. In the affected
test function around the now/effectiveAt setup, ensure the clock is restored
after the test completes by adding a deferred reset/cleanup in the same scope so
the mutation does not leak into other suite cases.
🪄 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: 70618b45-50bb-4517-bd11-e013ab6472e7
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (35)
api/spec/packages/aip-client-javascript/src/models/schemas.tsapi/spec/packages/aip-client-javascript/src/models/types.tsapi/spec/packages/aip/src/customers/credits/balance.tspapi/spec/packages/aip/src/customers/credits/grant.tspapi/v3/api.gen.goapi/v3/handlers/customers/credits/convert.goopenmeter/billing/charges/creditpurchase/charge.goopenmeter/billing/charges/creditpurchase/charge_test.goopenmeter/billing/creditgrant/service.goopenmeter/billing/creditgrant/service/service.goopenmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.goopenmeter/ledger/chargeadapter/creditpurchase.goopenmeter/ledger/chargeadapter/creditpurchase_test.goopenmeter/ledger/chargeadapter/helpers.goopenmeter/ledger/collector/collection_fbo.goopenmeter/ledger/customerbalance/calculation.goopenmeter/ledger/customerbalance/expired_loader_test.goopenmeter/ledger/customerbalance/facade.goopenmeter/ledger/customerbalance/facade_test.goopenmeter/ledger/customerbalance/funded_loader.goopenmeter/ledger/customerbalance/noop.goopenmeter/ledger/customerbalance/service.goopenmeter/ledger/customerbalance/service_test.goopenmeter/ledger/customerbalance/testenv_test.goopenmeter/ledger/customerbalance/transactions.goopenmeter/ledger/customerbalance/transactions_test.goopenmeter/ledger/historical/balance.goopenmeter/ledger/noop/noop.goopenmeter/ledger/primitives.goopenmeter/ledger/query.goopenmeter/ledger/testutils/integration.goopenmeter/ledger/transactions/collection.gotest/credits/base.gotest/credits/creditgrant_test.gotest/credits/sanity_test.go
💤 Files with no reviewable changes (1)
- openmeter/ledger/query.go
🚧 Files skipped from review as they are similar to previous changes (29)
- openmeter/ledger/chargeadapter/helpers.go
- openmeter/ledger/testutils/integration.go
- api/v3/handlers/customers/credits/convert.go
- api/spec/packages/aip/src/customers/credits/balance.tsp
- openmeter/billing/charges/creditpurchase/charge_test.go
- openmeter/ledger/collector/collection_fbo.go
- openmeter/ledger/transactions/collection.go
- openmeter/ledger/customerbalance/transactions_test.go
- openmeter/ledger/customerbalance/funded_loader.go
- api/spec/packages/aip-client-javascript/src/models/schemas.ts
- openmeter/billing/creditgrant/service.go
- openmeter/ledger/chargeadapter/creditpurchase_test.go
- openmeter/ledger/customerbalance/facade_test.go
- api/spec/packages/aip-client-javascript/src/models/types.ts
- test/credits/base.go
- openmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.go
- api/spec/packages/aip/src/customers/credits/grant.tsp
- openmeter/ledger/primitives.go
- openmeter/ledger/chargeadapter/creditpurchase.go
- openmeter/ledger/customerbalance/calculation.go
- openmeter/billing/charges/creditpurchase/charge.go
- openmeter/ledger/historical/balance.go
- openmeter/ledger/customerbalance/noop.go
- openmeter/billing/creditgrant/service/service.go
- openmeter/ledger/customerbalance/facade.go
- test/credits/sanity_test.go
- openmeter/ledger/noop/noop.go
- openmeter/ledger/customerbalance/service.go
- openmeter/ledger/customerbalance/service_test.go
Comment on lines
+203
to
+205
| now := datetime.MustParseTimeInLocation(s.T(), "2026-04-17T11:23:00Z", time.UTC).AsTime() | ||
| effectiveAt := datetime.MustParseTimeInLocation(s.T(), "2026-04-18T09:30:00Z", time.UTC).AsTime() | ||
| clock.SetTime(now) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reset the shifted clock after this test.
Line 205 calls clock.SetTime(now), but this new case never restores it. Since SetTime mutates global clock drift, later suite tests can inherit the shifted wall clock and become order-dependent.
🧹 Small fix
now := datetime.MustParseTimeInLocation(s.T(), "2026-04-17T11:23:00Z", time.UTC).AsTime()
effectiveAt := datetime.MustParseTimeInLocation(s.T(), "2026-04-18T09:30:00Z", time.UTC).AsTime()
clock.SetTime(now)
+ s.T().Cleanup(clock.ResetTime)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| now := datetime.MustParseTimeInLocation(s.T(), "2026-04-17T11:23:00Z", time.UTC).AsTime() | |
| effectiveAt := datetime.MustParseTimeInLocation(s.T(), "2026-04-18T09:30:00Z", time.UTC).AsTime() | |
| clock.SetTime(now) | |
| now := datetime.MustParseTimeInLocation(s.T(), "2026-04-17T11:23:00Z", time.UTC).AsTime() | |
| effectiveAt := datetime.MustParseTimeInLocation(s.T(), "2026-04-18T09:30:00Z", time.UTC).AsTime() | |
| clock.SetTime(now) | |
| s.T().Cleanup(clock.ResetTime) |
🤖 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 `@test/credits/creditgrant_test.go` around lines 203 - 205, The test case that
calls clock.SetTime in the credit grant suite leaves the global clock shifted
for later tests. In the affected test function around the now/effectiveAt setup,
ensure the clock is restored after the test completes by adding a deferred
reset/cleanup in the same scope so the mutation does not leak into other suite
cases.
tothandras
previously approved these changes
Jun 29, 2026
Comment on lines
+478
to
+500
| func isPendingCreditGrantAt(charge creditpurchase.Charge, asOf time.Time) bool { | ||
| if !canBecomeEffectiveLedgerCreditAt(charge, asOf) { | ||
| return false | ||
| } | ||
|
|
||
| if charge.Realizations.CreditGrantRealization == nil { | ||
| return true | ||
| } | ||
|
|
||
| return charge.Intent.ServicePeriod.To.After(asOf) | ||
| } | ||
|
|
||
| func canBecomeEffectiveLedgerCreditAt(charge creditpurchase.Charge, asOf time.Time) bool { | ||
| if charge.Status == creditpurchase.StatusDeleted || charge.IsDeletedAt(asOf) { | ||
| return false | ||
| } | ||
|
|
||
| if charge.Realizations.InvoiceSettlement != nil && charge.Realizations.InvoiceSettlement.IsDeletedAt(asOf) { | ||
| return false | ||
| } | ||
|
|
||
| if charge.Realizations.ExternalPaymentSettlement != nil && charge.Realizations.ExternalPaymentSettlement.IsDeletedAt(asOf) { | ||
| return false |
Contributor
There was a problem hiding this comment.
isPendingCreditGrantAt includes charges created after asOf in historical queries
listPendingGrantCandidateCharges fetches all credit-purchase charges for the customer with no creation-time filter. canBecomeEffectiveLedgerCreditAt only checks DeletedAt and Status, not CreatedAt. For a historical GetBalance(AsOf = T1) call, any charge created at T2 > T1 passes all guards — IsDeletedAt(T1) is false and status checks pass — so isPendingCreditGrantAt returns true for it. Charges that did not yet exist at the query point inflate the returned Pending() amount.
This does not affect live balance reads (AsOf == nil, resolved to clock.Now()), but any historical AsOf query can produce an incorrect pending total. Adding a guard like charge.CreatedAt.After(asOf) → skip in canBecomeEffectiveLedgerCreditAt would close this gap without conflicting with the documented bridge limitations.
Prompt To Fix With AI
This is a comment left during a code review.
Path: openmeter/ledger/customerbalance/service.go
Line: 478-500
Comment:
**`isPendingCreditGrantAt` includes charges created after `asOf` in historical queries**
`listPendingGrantCandidateCharges` fetches all credit-purchase charges for the customer with no creation-time filter. `canBecomeEffectiveLedgerCreditAt` only checks `DeletedAt` and `Status`, not `CreatedAt`. For a historical `GetBalance(AsOf = T1)` call, any charge created at `T2 > T1` passes all guards — `IsDeletedAt(T1)` is false and status checks pass — so `isPendingCreditGrantAt` returns `true` for it. Charges that did not yet exist at the query point inflate the returned `Pending()` amount.
This does not affect live balance reads (`AsOf == nil`, resolved to `clock.Now()`), but any historical `AsOf` query can produce an incorrect pending total. Adding a guard like `charge.CreatedAt.After(asOf)` → skip in `canBecomeEffectiveLedgerCreditAt` would close this gap without conflicting with the documented bridge limitations.
How can I resolve this? If you propose a fix, please make it concise.
GAlexIHU
force-pushed
the
feat/credit-balances-final
branch
from
144ceee to
9818aa0
Compare
Contributor
|
It's outside of this PR, but you could use func dedupeCurrencies(codes []currencyx.Code) []currencyx.Code {
seen := make(map[currencyx.Code]struct{}, len(codes))
out := make([]currencyx.Code, 0, len(codes))
for _, code := range codes {
if _, ok := seen[code]; ok {
continue
}
seen[code] = struct{}{}
out = append(out, code)
}
return out
} |
tothandras
approved these changes
Jun 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
available->settledpending->livepending= credit grants not yet effective in the ledgereffectiveAtby carrying it through creditpurchase service periods and ledger booking timeasOf >= service_period.toledger.Balanceinto a decimal aliasTesting
nix develop --impure .#ci -c env POSTGRES_HOST=127.0.0.1 go test -tags=dynamic ./openmeter/ledger/customerbalance -run 'Test(GetBalancePending|IsPendingCreditGrantAt|FacadeGetBalancesDiscoversPendingGrantCurrencies)'nix develop --impure .#ci -c go vet -tags=dynamic ./openmeter/ledger/customerbalancegit diff --checkNotes
The pending-grant lookup is intentionally a temporary bridge: it scans expanded creditpurchase charges on balance reads. The durable version should be an RTE/queryable scheduled-grant fact or index keyed for the balance path.
Summary by CodeRabbit