Conversation
GAlexIHU
added
the
release-note/ignore
Contributor
📝 WalkthroughWalkthroughThis PR significantly expands OpenMeter's ledger system by introducing cost-basis routing dimensions, a new customer-accrued account type, a comprehensive routing-rule validation framework, and two ledger charge adapters (flat-fee and credit-purchase handlers). It redesigns sub-account and account service composition to resolve parent accounts eagerly, migrates test infrastructure from Wire-based dependency injection to manual composition in testutils, and adds supporting transaction templates for accrual and earnings flows. Changes
Sequence DiagramsequenceDiagram
participant Event as Charge Event<br/>(e.g., FlatFee)
participant Handler as Charge Adapter<br/>(FlatFeeHandler)
participant Resolver as Transaction<br/>Resolver
participant AcctSvc as Account<br/>Service
participant SubAcctSvc as SubAccount<br/>Service
participant Ledger as Historical<br/>Ledger
participant DB as Database
Event->>Handler: OnAssignedToInvoice(charge)
Handler->>Resolver: ResolveTransactions(templates)
Resolver->>Resolver: Validate templates
Resolver->>AcctSvc: GetCustomerAccounts(namespace, customerID)
AcctSvc-->>Resolver: CustomerAccounts
Resolver->>SubAcctSvc: EnsureSubAccount(for accrued)
SubAcctSvc->>AcctSvc: GetAccountByID(accountID)
AcctSvc-->>SubAcctSvc: Parent Account
SubAcctSvc-->>Resolver: SubAccount (with parent)
Resolver-->>Handler: TransactionInputs[]
Handler->>Ledger: CommitGroup(GroupInput{entries, ...})
Ledger->>Ledger: ValidateTransactionInput(with RoutingValidator)
Ledger->>DB: Write transactions & entries
DB-->>Ledger: GroupID
Ledger-->>Handler: TransactionGroup
Handler-->>Event: GroupReference + Realization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes The diff introduces substantial new functionality across multiple interrelated domains (account types, routing rules, charge adapters, transaction templates, test infrastructure), with dense validation logic and schema changes. While the changes are modular and follow consistent patterns, they require careful review of error-handling semantics, parent-account resolution eagerness, routing-rule compositions, and integration-test wiring—particularly the migration away from Wire-based injection. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
GAlexIHU
force-pushed
the
feat/ledger-charges-rebased
branch
from
5f96c9a to
3f07693
Compare
GAlexIHU
added
release-note/feature
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/ledger/historical/ledger.go (1)
150-162:⚠️ Potential issue | 🟠 MajorInclude
AccountTypeCustomerAccruedin the account locking logic.The switch statement only locks
CustomerFBOandCustomerReceivable, but the routing rules showCustomerAccruedcan also be affected by transaction inputs—it receives transfers from both FBO and Receivable, and sends to Earnings. Since all three can be involved in the same transaction batch andAsCustomerAccount()already supports converting it to a lockable customer account,AccountTypeCustomerAccruedshould be added to the locking case to protect against concurrent writes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/historical/ledger.go` around lines 150 - 162, The switch over acc.Type() in the accounts loop currently locks only ledger.AccountTypeCustomerFBO and ledger.AccountTypeCustomerReceivable; add ledger.AccountTypeCustomerAccrued to that case so Accrued accounts are also converted via acc.AsCustomerAccount() and locked with cSvc.Lock(ctx). Update the case handling in the loop that references accounts, acc.Type(), AsCustomerAccount(), and cSvc.Lock(ctx) to include AccountTypeCustomerAccrued alongside the other customer types.
🧹 Nitpick comments (13)
AGENTS.md (1)
128-132: Small consistency tweak: keep examples Makefile-first where possible.The new Nix guidance is helpful. On Lines 128-132, consider preferring
makewrappers in examples for common tasks, so this section stays aligned with the repo convention.Suggested wording update
-nix develop --impure .#ci -c gofmt -w openmeter/ledger/historical/entry.go +nix develop --impure .#ci -c make fmt nix develop --impure .#ci -c make lint-go -nix develop --impure .#ci -c sh -lc 'POSTGRES_HOST=127.0.0.1 go test -tags=dynamic ./openmeter/ledger/historical/...' +nix develop --impure .#ci -c make testBased on learnings: “Use the Makefile for all common tasks (running tests, generating code, linting, etc.) rather than running commands directly.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 128 - 132, Replace the direct nix commands in the example (the gofmt, lint-go and POSTGRES_HOST go test invocations) with the repository's Makefile wrappers so examples stay Makefile-first; update the three shown commands to call the corresponding make targets (e.g., make fmt or make gofmt, make lint-go, and make test with POSTGRES_HOST set) and ensure you call them via the same nix develop invocation if needed so semantics remain identical to the originals.pkg/timeutil/openperiod_test.go (1)
14-30: Good start—please tighten the Validate assertions a bit.On Lines 21-28, asserting only “error exists” can let wrong failure reasons slip through. It’d be safer to also assert the expected message, and add open-ended valid cases (
From=nilorTo=nil) for completeness.Suggested test hardening
t.Run("Validate", func(t *testing.T) { valid := OpenPeriod{From: &before, To: &after} if err := valid.Validate(); err != nil { t.Fatalf("expected valid period, got %v", err) } invalid := OpenPeriod{From: &after, To: &before} - if err := invalid.Validate(); err == nil { - t.Fatal("expected invalid period error") + if err := invalid.Validate(); err == nil || err.Error() != "from must be before to" { + t.Fatalf("expected %q, got %v", "from must be before to", err) } touching := OpenPeriod{From: &now, To: &now} - if err := touching.Validate(); err == nil { - t.Fatal("expected touching period error") + if err := touching.Validate(); err == nil || err.Error() != "from must be before to" { + t.Fatalf("expected %q, got %v", "from must be before to", err) } + + openStart := OpenPeriod{To: &after} + if err := openStart.Validate(); err != nil { + t.Fatalf("expected open-start period, got %v", err) + } + + openEnd := OpenPeriod{From: &before} + if err := openEnd.Validate(); err != nil { + t.Fatalf("expected open-end period, got %v", err) + } })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 `@pkg/timeutil/openperiod_test.go` around lines 14 - 30, The tests for OpenPeriod.Validate are too weak: when expecting an error (for invalid and touching periods) assert the error message/content to ensure the failure reason is correct, and add additional cases where From==nil and To==nil to verify those open-ended periods are treated as valid; update the test block around the OpenPeriod instances (variables valid, invalid, touching) to check err.Error() contains the expected substring from Validate and add two new t.Run subcases for OpenPeriod{From:nil, To:&after} and OpenPeriod{From:&before, To:nil} asserting no error.openmeter/ledger/transactions/fx.go (1)
34-37: Tiny consistency tweak: use the normalizedcostBasiseverywhere.Since you already normalize/validate into
costBasis, using it insourceAmountkeeps the path fully consistent and future-proof.Suggested diff
- sourceAmount := t.TargetAmount.Mul(t.CostBasis) + sourceAmount := t.TargetAmount.Mul(costBasis)Also applies to: 83-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/transactions/fx.go` around lines 34 - 37, The code validates/normalizes t.CostBasis into the local variable costBasis but then still reads t.CostBasis later; change usages to use the normalized costBasis (e.g., when computing sourceAmount and the other occurrence around the fx.go block that references cost basis) so the function consistently uses the validated value (update references of t.CostBasis to costBasis in the sourceAmount calculation and the other spot mentioned).openmeter/ledger/testutils/deps.go (1)
14-38: Nice test utility for dependency wiring!This consolidates ledger test setup into one place, making tests cleaner. The error handling for locker creation is appropriate.
A brief doc comment on
InitDepsexplaining its purpose would help future maintainers:📝 Optional doc comment
+// InitDeps constructs a fully-wired ledger dependency stack for integration tests. +// It returns services needed for account provisioning, resolution, and historical ledger operations. func InitDeps(db *entdb.Client, logger *slog.Logger) (Deps, error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/testutils/deps.go` around lines 14 - 38, Add a short doc comment above the InitDeps function describing that InitDeps wires up test dependencies (creates repo, locker, services and returns a Deps struct used in ledger tests) and mention its return values and error behavior; update the Deps type comment to state it groups AccountService, ResolversService, and HistoricalLedger for tests, and keep the note that InitDeps may return an error when locker creation fails. Use the symbols Deps and InitDeps to locate where to add the comments.openmeter/ledger/account/service/service.go (1)
97-119: The FIXME is justified - this is an N+1 query pattern.The caching by
accountsByIDhelps when sub-accounts share parents, but you're still making oneGetAccountByIDcall per unique parent account. For large result sets, this could become a performance bottleneck.Consider batching the parent account fetches:
🔧 Suggested approach
// Collect unique parent account IDs first parentIDs := make(map[string]struct{}) for _, subAccountData := range subAccountDatas { parentIDs[subAccountData.AccountID] = struct{}{} } // Batch fetch all parent accounts (assuming a ListAccountsByIDs method exists or can be added) accountsByID, err := s.batchGetAccounts(ctx, namespace, parentIDs) if err != nil { return nil, err } // Then map sub-accountsThis could be a follow-up improvement if
ListSubAccountsis called with large result sets.Would you like me to open an issue to track optimizing this N+1 query pattern?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/account/service/service.go` around lines 97 - 119, The loop over subAccountDatas in service.go creates an N+1 query by calling s.GetAccountByID for each unique parent; instead, collect unique parent IDs from subAccountDatas, implement or call a batch loader (e.g., s.BatchGetAccounts or ListAccountsByIDs) to fetch all parent accounts in one call and build accountsByID from that result, then iterate subAccountDatas and call account.NewSubAccountFromData with the cached acc from accountsByID; update error handling to return any batch fetch errors and keep the existing mapping/append logic for subAccounts.openmeter/ledger/resolvers/account.go (1)
121-154: Consistent pattern for account fetching - consider a helper if this grows.The fetch → convert → error-handle pattern is repeated three times for FBO, Receivable, and Accrued accounts. It's readable as-is, but if more account types are added, extracting a helper function could reduce duplication.
Something like:
func (s *AccountResolver) fetchAndConvert[T any](ctx context.Context, ns, id string, convert func(*ledgeraccount.Account) (T, error)) (T, error)Not critical for now - just a thought for future maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/resolvers/account.go` around lines 121 - 154, The repeated fetch→convert→error pattern for FBO, Receivable and Accrued accounts should be extracted into a small helper to reduce duplication: add a generic helper on the resolver (e.g. func (s *AccountResolver) fetchAndConvert[T any](ctx context.Context, ns, id string, convert func(*ledgeraccount.Account) (T, error)) (T, error)) that calls s.AccountService.GetAccountByID, returns a wrapped error on GetAccountByID failure, invokes the provided convert function (e.g. AsCustomerFBOAccount, AsCustomerReceivableAccount, AsCustomerAccruedAccount) and returns its error/result; then replace the three identical blocks that call GetAccountByID and AsCustomer* with calls to fetchAndConvert for each account type (FBOAccount, ReceivableAccount, AccruedAccount).openmeter/ledger/routingrules/routingrules_test.go (1)
82-98: Helper looks good, tiny note on parallel safety.The timestamp-based
RouteIDgeneration should be fine for sequential tests. If these ever run witht.Parallel(), consider using a UUID or atomic counter instead to avoid any theoretical collision.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/routingrules/routingrules_test.go` around lines 82 - 98, The addressForRoute helper builds a RouteID using a timestamp which can collide under t.Parallel(); update it to generate a unique RouteID deterministically (e.g., use a UUID or a package-scoped atomic counter) when calling ledgeraccount.NewAddressFromData in addressForRoute so tests are safe to run in parallel — replace the time-based "route-"+subAccountID+"-"+time.Now().UTC().Format(...) construction with a UUID or incremented counter-based string to guarantee uniqueness.openmeter/ledger/account/subaccount.go (1)
75-86: Defensive nil check is good, but consider the design.The nil check for
s.accountat runtime is safe, but it means callers can construct aSubAccountwithout a parent and only discover the issue when callingGetBalance(). Ifaccountis always required for meaningful use, consider validating it in the constructor.That said, there may be legitimate use cases where a sub-account is constructed without a parent (read-only scenarios?), so this might be intentional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/account/subaccount.go` around lines 75 - 86, The GetBalance method defensively checks s.account but callers can still create a SubAccount without a parent and only fail at runtime; add a constructor like NewSubAccount(data SubAccountData, account ledger.Account) (*SubAccount, error) that returns an error when account is nil (or return (*SubAccount, nil) only when account is provided), enforce that callers use NewSubAccount instead of constructing SubAccount literals, and update call sites to use this constructor; if nil-parent instances are intentionally allowed, instead document this behavior on the SubAccount type and consider adding a helper NewReadOnlySubAccount(...) to make the intent explicit.openmeter/ledger/account/address.go (1)
72-93: Consider simplifying the equality check.The
Equalmethod manually compares multiple fields. Since you're comparing routes, you might want to consider whetherSubAccountRouteshould implement its ownEqualmethod to consolidate this logic. That said, this works fine as-is.💡 Optional refactor idea
If
SubAccountRoutehad anEqualmethod, you could simplify:func (a *Address) Equal(other ledger.PostingAddress) bool { return a.SubAccountID() == other.SubAccountID() && a.AccountType() == other.AccountType() && a.Route().Equal(other.Route()) }This would centralize route comparison logic. Just a thought for future maintainability!
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/account/address.go` around lines 72 - 93, The Equal method on Address is manually comparing Route fields; refactor by adding an Equal method to the SubAccountRoute type (e.g., SubAccountRoute.Equal(other SubAccountRoute) bool) that encapsulates the routing comparisons, then update Address.Equal (method on Address that accepts ledger.PostingAddress) to return a simple conjunction: compare SubAccountID(), AccountType(), and call Route().Equal(other.Route()); this centralizes route comparison and simplifies Address.Equal.openmeter/ledger/transactions/testenv_test.go (1)
62-83: fundPriority helper could benefit from CostBasis support.The helper currently doesn't pass
CostBasisto the templates. If you need to test cost basis scenarios with funded priorities, you might want to add an optional parameter or a separate helper. For now,nilis a reasonable default if most tests don't need it.💡 Optional: Add CostBasis parameter if needed
If tests need to fund with a specific cost basis:
-func (e *transactionsTestEnv) fundPriority(t *testing.T, priority int, amount int64) ledger.SubAccount { +func (e *transactionsTestEnv) fundPriority(t *testing.T, priority int, amount int64, costBasis *alpacadecimal.Decimal) ledger.SubAccount { t.Helper() subAccount := e.FBOSubAccount(t, priority) e.resolveAndCommit( t, IssueCustomerReceivableTemplate{ At: e.Now(), Amount: alpacadecimal.NewFromInt(amount), Currency: e.Currency, + CostBasis: costBasis, CreditPriority: &priority, }, FundCustomerReceivableTemplate{ At: e.Now(), Amount: alpacadecimal.NewFromInt(amount), Currency: e.Currency, + CostBasis: costBasis, }, ) return subAccount }Or keep the current signature and add
fundPriorityWithCostBasisif needed later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/transactions/testenv_test.go` around lines 62 - 83, The helper fundPriority currently doesn't set CostBasis on the IssueCustomerReceivableTemplate and FundCustomerReceivableTemplate; update the helper to accept an optional costBasis parameter (e.g., costBasis *alpacadecimal.CostBasis or interface used in templates) or create a new fundPriorityWithCostBasis helper, and pass that costBasis (or nil by default) into both IssueCustomerReceivableTemplate and FundCustomerReceivableTemplate when calling e.resolveAndCommit so tests can exercise cost-basis scenarios; locate function fundPriority and the two templates IssueCustomerReceivableTemplate and FundCustomerReceivableTemplate to apply this change.openmeter/ledger/routing.go (1)
297-314: Clarify the purpose of the string round-trip.The
NewFromString(v.String())pattern at line 308 looks like it's normalizing the decimal representation (e.g.,0.70→0.7). A quick comment would help future readers understand this is intentional for canonical routing key generation rather than a no-op.💬 Suggested comment
func normalizeOptionalCostBasis(v *alpacadecimal.Decimal) (*alpacadecimal.Decimal, error) { if v == nil { return nil, nil } if v.IsNegative() { return nil, ErrCostBasisInvalid.WithAttrs(models.Attributes{ "cost_basis": v.String(), }) } + // Round-trip through string to get canonical representation (e.g., "0.70" → "0.7") + // so semantically equal values produce identical routing keys. normalized, err := alpacadecimal.NewFromString(v.String()) if err != nil { return nil, fmt.Errorf("normalize cost basis: %w", err) } return &normalized, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/routing.go` around lines 297 - 314, Add a short explanatory comment inside normalizeOptionalCostBasis explaining that the alpacadecimal.NewFromString(v.String()) round‑trip is intentional to canonicalize decimal formatting (e.g., "0.70" → "0.7") for deterministic routing key generation; place the comment near the NewFromString call and keep it concise so future readers know this is normalization rather than a no‑op (no code changes to logic or error handling needed).openmeter/ledger/testutils/integration.go (1)
194-201: Consider exposing pending balance too.
SumBalanceonly returnsSettled(). If tests ever need to assert on pending balances, you might want aSumPendingBalancehelper or have this return the fullQuerySummedResult. Not blocking, just a thought for future test coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/testutils/integration.go` around lines 194 - 201, SumBalance currently returns only the settled amount by calling sum.Settled(); to expose pending balances, either add a new helper e.g. IntegrationEnv.SumPendingBalance that returns sum.Pending() (and mirrors SumBalance's error handling), or change SumBalance to return the full ledger.QuerySummedResult (the variable sum) so callers can inspect both Settled() and Pending(); update callers/tests to use the new helper or the returned QuerySummedResult and keep the same error handling using require.NoError(t, err).openmeter/ledger/chargeadapter/creditpurchase.go (1)
196-198: Consider using a sentinel error for "not implemented".Returning a raw
fmt.Errorfworks, but you've got all these nice structured errors inerrors.go. Might be worth adding anErrNotImplementedor similar so callers can programmatically detect this case if needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/chargeadapter/creditpurchase.go` around lines 196 - 198, Replace the ad-hoc fmt.Errorf in OnCreditPurchasePaymentSettled with a package-level sentinel error so callers can detect the "not implemented" case programmatically: add a exported ErrNotImplemented = errors.New("not implemented") to the errors.go file (or reuse an existing sentinel there), then return that sentinel from creditPurchaseHandler.OnCreditPurchasePaymentSettled instead of fmt.Errorf; keep the ledgertransaction.GroupReference{} return value and do not wrap the sentinel so callers can compare the error directly.
🤖 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/chargeadapter/creditpurchase.go`:
- Around line 145-194: The OnCreditPurchasePaymentAuthorized handler is missing
the zero-amount early-return used by the other handlers; add a guard that checks
charge.Intent.CreditAmount.IsZero() at the top of
OnCreditPurchasePaymentAuthorized and return an empty
ledgertransaction.GroupReference,nil when true (or, if omission is intentional,
insert a brief comment in OnCreditPurchasePaymentAuthorized explaining why
processing must continue for zero amounts) so behavior is consistent with
OnPromotionalCreditPurchase and OnCreditPurchaseInitiated.
In `@openmeter/ledger/chargeadapter/flatfee.go`:
- Around line 204-227: The receivable funding (receivableReplenishment) can
exceed what's actually on the ledger because only recognitionAmount is capped to
accruedBalance; update receivableReplenishment after computing recognitionAmount
so it is at most the remaining accrued balance (accruedBalance minus
recognitionAmount) before appending FundCustomerReceivableTemplate. Locate
settledBalanceForSubAccount/accruedBalance and the recognitionAmount calculation
and then constrain receivableReplenishment = min(receivableReplenishment,
accruedBalance.Sub(recognitionAmount)) (or equivalent) so recognition +
receivable funding never exceed accruedBalance.
In `@openmeter/ledger/ledger_fx_test.go`:
- Around line 103-106: The local variable `deps` declared in the
transactions.ResolverDependencies literal shadows the outer `deps`, causing
deps.AccountService to reference the uninitialized inner variable; rename the
inner variable (e.g., `resolverDeps`) or construct the ResolverDependencies
using resolversSvc for AccountService and the outer deps.AccountService for
SubAccountService (matching the pattern used in testenv_test.go) so the
SubAccountService correctly references the outer deps.AccountService.
In `@openmeter/ledger/query.go`:
- Around line 71-77: The call to p.Filters.Route.Normalize() currently discards
the returned normalized RouteFilter, causing redundant normalization later;
change it to capture the result (e.g., normalizedRoute, err :=
p.Filters.Route.Normalize()), on error return ErrLedgerQueryInvalid as before,
and on success assign the normalized value back to p.Filters.Route (or use
normalizedRoute where needed) so downstream adapters like sumentries_query.go
can rely on the already-normalized route.
In `@openmeter/ledger/transactions/collection.go`:
- Around line 53-91: The current loop calls settledBalanceForSubAccount for
every subAccount, causing an N+1 DB round-trip; instead first sort subAccounts
by route.CreditPriority (falling back to ledger.DefaultCustomerFBOPriority and
subAccount.Address().SubAccountID() tie-breaker using the existing sort logic),
then either (preferred) call a batched service method (e.g., add/use
SubAccountService.ListSettledBalances(ctx, []SubAccountID) to return balances
for all IDs) and map those into fboSource, or (simpler) compute balances lazily
after sorting and stop calling settledBalanceForSubAccount once the remaining
target amount is fully covered; update code references:
SubAccountService.ListSubAccounts, settledBalanceForSubAccount, fboSource, and
the sort.Slice comparator to implement this change.
In `@openmeter/ledger/transactions/customer_test.go`:
- Around line 12-29: Add a new test case inside
TestIssueCustomerReceivableTemplate that issues a receivable with a non-zero
CostBasis on the IssueCustomerReceivableTemplate input, then run the full
round-trip (issue → fund → cover) via env.resolveAndCommit for the corresponding
Fund/ Cover messages; after each step assert balances using env.SumBalance(t,
env.FBOSubAccount(t, priority)) and env.SumBalance(t,
env.ReceivableSubAccount(t)) and additionally assert the balance for the routed
subaccount that includes the CostBasis dimension (i.e. the same routed
subaccount used by your system for cost-basis routing) to ensure the CostBasis
value survives the issue/fund/cover flow. Use the existing symbols
IssueCustomerReceivableTemplate, resolveAndCommit, FBOSubAccount and
ReceivableSubAccount to locate where to add the new case.
---
Outside diff comments:
In `@openmeter/ledger/historical/ledger.go`:
- Around line 150-162: The switch over acc.Type() in the accounts loop currently
locks only ledger.AccountTypeCustomerFBO and
ledger.AccountTypeCustomerReceivable; add ledger.AccountTypeCustomerAccrued to
that case so Accrued accounts are also converted via acc.AsCustomerAccount() and
locked with cSvc.Lock(ctx). Update the case handling in the loop that references
accounts, acc.Type(), AsCustomerAccount(), and cSvc.Lock(ctx) to include
AccountTypeCustomerAccrued alongside the other customer types.
---
Nitpick comments:
In `@AGENTS.md`:
- Around line 128-132: Replace the direct nix commands in the example (the
gofmt, lint-go and POSTGRES_HOST go test invocations) with the repository's
Makefile wrappers so examples stay Makefile-first; update the three shown
commands to call the corresponding make targets (e.g., make fmt or make gofmt,
make lint-go, and make test with POSTGRES_HOST set) and ensure you call them via
the same nix develop invocation if needed so semantics remain identical to the
originals.
In `@openmeter/ledger/account/address.go`:
- Around line 72-93: The Equal method on Address is manually comparing Route
fields; refactor by adding an Equal method to the SubAccountRoute type (e.g.,
SubAccountRoute.Equal(other SubAccountRoute) bool) that encapsulates the routing
comparisons, then update Address.Equal (method on Address that accepts
ledger.PostingAddress) to return a simple conjunction: compare SubAccountID(),
AccountType(), and call Route().Equal(other.Route()); this centralizes route
comparison and simplifies Address.Equal.
In `@openmeter/ledger/account/service/service.go`:
- Around line 97-119: The loop over subAccountDatas in service.go creates an N+1
query by calling s.GetAccountByID for each unique parent; instead, collect
unique parent IDs from subAccountDatas, implement or call a batch loader (e.g.,
s.BatchGetAccounts or ListAccountsByIDs) to fetch all parent accounts in one
call and build accountsByID from that result, then iterate subAccountDatas and
call account.NewSubAccountFromData with the cached acc from accountsByID; update
error handling to return any batch fetch errors and keep the existing
mapping/append logic for subAccounts.
In `@openmeter/ledger/account/subaccount.go`:
- Around line 75-86: The GetBalance method defensively checks s.account but
callers can still create a SubAccount without a parent and only fail at runtime;
add a constructor like NewSubAccount(data SubAccountData, account
ledger.Account) (*SubAccount, error) that returns an error when account is nil
(or return (*SubAccount, nil) only when account is provided), enforce that
callers use NewSubAccount instead of constructing SubAccount literals, and
update call sites to use this constructor; if nil-parent instances are
intentionally allowed, instead document this behavior on the SubAccount type and
consider adding a helper NewReadOnlySubAccount(...) to make the intent explicit.
In `@openmeter/ledger/chargeadapter/creditpurchase.go`:
- Around line 196-198: Replace the ad-hoc fmt.Errorf in
OnCreditPurchasePaymentSettled with a package-level sentinel error so callers
can detect the "not implemented" case programmatically: add a exported
ErrNotImplemented = errors.New("not implemented") to the errors.go file (or
reuse an existing sentinel there), then return that sentinel from
creditPurchaseHandler.OnCreditPurchasePaymentSettled instead of fmt.Errorf; keep
the ledgertransaction.GroupReference{} return value and do not wrap the sentinel
so callers can compare the error directly.
In `@openmeter/ledger/resolvers/account.go`:
- Around line 121-154: The repeated fetch→convert→error pattern for FBO,
Receivable and Accrued accounts should be extracted into a small helper to
reduce duplication: add a generic helper on the resolver (e.g. func (s
*AccountResolver) fetchAndConvert[T any](ctx context.Context, ns, id string,
convert func(*ledgeraccount.Account) (T, error)) (T, error)) that calls
s.AccountService.GetAccountByID, returns a wrapped error on GetAccountByID
failure, invokes the provided convert function (e.g. AsCustomerFBOAccount,
AsCustomerReceivableAccount, AsCustomerAccruedAccount) and returns its
error/result; then replace the three identical blocks that call GetAccountByID
and AsCustomer* with calls to fetchAndConvert for each account type (FBOAccount,
ReceivableAccount, AccruedAccount).
In `@openmeter/ledger/routing.go`:
- Around line 297-314: Add a short explanatory comment inside
normalizeOptionalCostBasis explaining that the
alpacadecimal.NewFromString(v.String()) round‑trip is intentional to
canonicalize decimal formatting (e.g., "0.70" → "0.7") for deterministic routing
key generation; place the comment near the NewFromString call and keep it
concise so future readers know this is normalization rather than a no‑op (no
code changes to logic or error handling needed).
In `@openmeter/ledger/routingrules/routingrules_test.go`:
- Around line 82-98: The addressForRoute helper builds a RouteID using a
timestamp which can collide under t.Parallel(); update it to generate a unique
RouteID deterministically (e.g., use a UUID or a package-scoped atomic counter)
when calling ledgeraccount.NewAddressFromData in addressForRoute so tests are
safe to run in parallel — replace the time-based
"route-"+subAccountID+"-"+time.Now().UTC().Format(...) construction with a UUID
or incremented counter-based string to guarantee uniqueness.
In `@openmeter/ledger/testutils/deps.go`:
- Around line 14-38: Add a short doc comment above the InitDeps function
describing that InitDeps wires up test dependencies (creates repo, locker,
services and returns a Deps struct used in ledger tests) and mention its return
values and error behavior; update the Deps type comment to state it groups
AccountService, ResolversService, and HistoricalLedger for tests, and keep the
note that InitDeps may return an error when locker creation fails. Use the
symbols Deps and InitDeps to locate where to add the comments.
In `@openmeter/ledger/testutils/integration.go`:
- Around line 194-201: SumBalance currently returns only the settled amount by
calling sum.Settled(); to expose pending balances, either add a new helper e.g.
IntegrationEnv.SumPendingBalance that returns sum.Pending() (and mirrors
SumBalance's error handling), or change SumBalance to return the full
ledger.QuerySummedResult (the variable sum) so callers can inspect both
Settled() and Pending(); update callers/tests to use the new helper or the
returned QuerySummedResult and keep the same error handling using
require.NoError(t, err).
In `@openmeter/ledger/transactions/fx.go`:
- Around line 34-37: The code validates/normalizes t.CostBasis into the local
variable costBasis but then still reads t.CostBasis later; change usages to use
the normalized costBasis (e.g., when computing sourceAmount and the other
occurrence around the fx.go block that references cost basis) so the function
consistently uses the validated value (update references of t.CostBasis to
costBasis in the sourceAmount calculation and the other spot mentioned).
In `@openmeter/ledger/transactions/testenv_test.go`:
- Around line 62-83: The helper fundPriority currently doesn't set CostBasis on
the IssueCustomerReceivableTemplate and FundCustomerReceivableTemplate; update
the helper to accept an optional costBasis parameter (e.g., costBasis
*alpacadecimal.CostBasis or interface used in templates) or create a new
fundPriorityWithCostBasis helper, and pass that costBasis (or nil by default)
into both IssueCustomerReceivableTemplate and FundCustomerReceivableTemplate
when calling e.resolveAndCommit so tests can exercise cost-basis scenarios;
locate function fundPriority and the two templates
IssueCustomerReceivableTemplate and FundCustomerReceivableTemplate to apply this
change.
In `@pkg/timeutil/openperiod_test.go`:
- Around line 14-30: The tests for OpenPeriod.Validate are too weak: when
expecting an error (for invalid and touching periods) assert the error
message/content to ensure the failure reason is correct, and add additional
cases where From==nil and To==nil to verify those open-ended periods are treated
as valid; update the test block around the OpenPeriod instances (variables
valid, invalid, touching) to check err.Error() contains the expected substring
from Validate and add two new t.Run subcases for OpenPeriod{From:nil, To:&after}
and OpenPeriod{From:&before, To:nil} asserting no error.
🪄 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: 0b75aa24-d950-4573-b138-f0dcf4ca8757
⛔ Files ignored due to path filters (8)
openmeter/ent/db/ledgersubaccountroute.gois excluded by!**/ent/db/**openmeter/ent/db/ledgersubaccountroute/ledgersubaccountroute.gois excluded by!**/ent/db/**openmeter/ent/db/ledgersubaccountroute/where.gois excluded by!**/ent/db/**openmeter/ent/db/ledgersubaccountroute_create.gois excluded by!**/ent/db/**openmeter/ent/db/ledgersubaccountroute_update.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (59)
.agents/skills/ledger/SKILL.md.agents/skills/rebase/SKILL.mdAGENTS.mdapp/common/ledger.goopenmeter/ent/schema/ledger_account.goopenmeter/ledger/account.goopenmeter/ledger/account/account.goopenmeter/ledger/account/account_business.goopenmeter/ledger/account/account_customer.goopenmeter/ledger/account/adapter/repo_test.goopenmeter/ledger/account/adapter/subaccount.goopenmeter/ledger/account/address.goopenmeter/ledger/account/service/service.goopenmeter/ledger/account/subaccount.goopenmeter/ledger/accounts.goopenmeter/ledger/annotations.goopenmeter/ledger/chargeadapter/creditpurchase.goopenmeter/ledger/chargeadapter/creditpurchase_test.goopenmeter/ledger/chargeadapter/flatfee.goopenmeter/ledger/chargeadapter/flatfee_test.goopenmeter/ledger/errors.goopenmeter/ledger/historical/adapter/ledger.goopenmeter/ledger/historical/adapter/ledger_test.goopenmeter/ledger/historical/adapter/sumentries_query.goopenmeter/ledger/historical/entry.goopenmeter/ledger/historical/ledger.goopenmeter/ledger/historical/transaction.goopenmeter/ledger/ledger_fx_test.goopenmeter/ledger/ledger_test.goopenmeter/ledger/primitives.goopenmeter/ledger/query.goopenmeter/ledger/resolvers/account.goopenmeter/ledger/routing.goopenmeter/ledger/routing_test.goopenmeter/ledger/routing_validator.goopenmeter/ledger/routingrules/defaults.goopenmeter/ledger/routingrules/routingrules.goopenmeter/ledger/routingrules/routingrules_test.goopenmeter/ledger/routingrules/view.goopenmeter/ledger/testutil/generate.goopenmeter/ledger/testutil/wire.goopenmeter/ledger/testutil/wire_gen.goopenmeter/ledger/testutils/deps.goopenmeter/ledger/testutils/integration.goopenmeter/ledger/transactions/accrual.goopenmeter/ledger/transactions/accrual_test.goopenmeter/ledger/transactions/collection.goopenmeter/ledger/transactions/customer.goopenmeter/ledger/transactions/customer_test.goopenmeter/ledger/transactions/earnings.goopenmeter/ledger/transactions/earnings_test.goopenmeter/ledger/transactions/fx.goopenmeter/ledger/transactions/resolve.goopenmeter/ledger/transactions/testenv_test.goopenmeter/ledger/validations.gopkg/timeutil/openperiod.gopkg/timeutil/openperiod_test.gotools/migrate/migrations/20260320161000_ledger-route-cost-basis.down.sqltools/migrate/migrations/20260320161000_ledger-route-cost-basis.up.sql
💤 Files with no reviewable changes (3)
- openmeter/ledger/testutil/generate.go
- openmeter/ledger/testutil/wire.go
- openmeter/ledger/testutil/wire_gen.go
Comment on lines
+145
to
+194
| func (h *creditPurchaseHandler) OnCreditPurchasePaymentAuthorized(ctx context.Context, charge chargecreditpurchase.Charge) (ledgertransaction.GroupReference, error) { | ||
| if err := charge.Validate(); err != nil { | ||
| return ledgertransaction.GroupReference{}, err | ||
| } | ||
|
|
||
| externalSettlement, err := charge.Intent.Settlement.AsExternalSettlement() | ||
| if err != nil { | ||
| return ledgertransaction.GroupReference{}, fmt.Errorf("external settlement: %w", err) | ||
| } | ||
|
|
||
| customerID := customer.CustomerID{ | ||
| Namespace: charge.Namespace, | ||
| ID: charge.Intent.CustomerID, | ||
| } | ||
| annotations := ledger.ChargeAnnotations(models.NamespacedID{ | ||
| Namespace: charge.Namespace, | ||
| ID: charge.ID, | ||
| }) | ||
|
|
||
| inputs, err := transactions.ResolveTransactions( | ||
| ctx, | ||
| h.resolverDependencies(), | ||
| transactions.ResolutionScope{ | ||
| CustomerID: customerID, | ||
| Namespace: charge.Namespace, | ||
| }, | ||
| transactions.FundCustomerReceivableTemplate{ | ||
| At: charge.CreatedAt, | ||
| Amount: charge.Intent.CreditAmount, | ||
| Currency: charge.Intent.Currency, | ||
| CostBasis: &externalSettlement.CostBasis, | ||
| }, | ||
| ) | ||
| if err != nil { | ||
| return ledgertransaction.GroupReference{}, fmt.Errorf("resolve transactions: %w", err) | ||
| } | ||
|
|
||
| transactionGroup, err := h.ledger.CommitGroup(ctx, transactions.GroupInputs( | ||
| charge.Namespace, | ||
| annotations, | ||
| inputs..., | ||
| )) | ||
| if err != nil { | ||
| return ledgertransaction.GroupReference{}, fmt.Errorf("commit ledger transaction group: %w", err) | ||
| } | ||
|
|
||
| return ledgertransaction.GroupReference{ | ||
| TransactionGroupID: transactionGroup.ID().ID, | ||
| }, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Missing zero-amount early return in OnCreditPurchasePaymentAuthorized.
The other two handlers (OnPromotionalCreditPurchase and OnCreditPurchaseInitiated) have an early return when charge.Intent.CreditAmount.IsZero(), but this one doesn't. If that's intentional (perhaps authorization should always proceed regardless of amount), it might be worth a quick comment explaining why. Otherwise, consider adding the check for consistency.
💡 Suggested fix if the check should be present
func (h *creditPurchaseHandler) OnCreditPurchasePaymentAuthorized(ctx context.Context, charge chargecreditpurchase.Charge) (ledgertransaction.GroupReference, error) {
if err := charge.Validate(); err != nil {
return ledgertransaction.GroupReference{}, err
}
+
+ if charge.Intent.CreditAmount.IsZero() {
+ return ledgertransaction.GroupReference{}, nil
+ }
externalSettlement, err := charge.Intent.Settlement.AsExternalSettlement()📝 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
| func (h *creditPurchaseHandler) OnCreditPurchasePaymentAuthorized(ctx context.Context, charge chargecreditpurchase.Charge) (ledgertransaction.GroupReference, error) { | |
| if err := charge.Validate(); err != nil { | |
| return ledgertransaction.GroupReference{}, err | |
| } | |
| externalSettlement, err := charge.Intent.Settlement.AsExternalSettlement() | |
| if err != nil { | |
| return ledgertransaction.GroupReference{}, fmt.Errorf("external settlement: %w", err) | |
| } | |
| customerID := customer.CustomerID{ | |
| Namespace: charge.Namespace, | |
| ID: charge.Intent.CustomerID, | |
| } | |
| annotations := ledger.ChargeAnnotations(models.NamespacedID{ | |
| Namespace: charge.Namespace, | |
| ID: charge.ID, | |
| }) | |
| inputs, err := transactions.ResolveTransactions( | |
| ctx, | |
| h.resolverDependencies(), | |
| transactions.ResolutionScope{ | |
| CustomerID: customerID, | |
| Namespace: charge.Namespace, | |
| }, | |
| transactions.FundCustomerReceivableTemplate{ | |
| At: charge.CreatedAt, | |
| Amount: charge.Intent.CreditAmount, | |
| Currency: charge.Intent.Currency, | |
| CostBasis: &externalSettlement.CostBasis, | |
| }, | |
| ) | |
| if err != nil { | |
| return ledgertransaction.GroupReference{}, fmt.Errorf("resolve transactions: %w", err) | |
| } | |
| transactionGroup, err := h.ledger.CommitGroup(ctx, transactions.GroupInputs( | |
| charge.Namespace, | |
| annotations, | |
| inputs..., | |
| )) | |
| if err != nil { | |
| return ledgertransaction.GroupReference{}, fmt.Errorf("commit ledger transaction group: %w", err) | |
| } | |
| return ledgertransaction.GroupReference{ | |
| TransactionGroupID: transactionGroup.ID().ID, | |
| }, nil | |
| } | |
| func (h *creditPurchaseHandler) OnCreditPurchasePaymentAuthorized(ctx context.Context, charge chargecreditpurchase.Charge) (ledgertransaction.GroupReference, error) { | |
| if err := charge.Validate(); err != nil { | |
| return ledgertransaction.GroupReference{}, err | |
| } | |
| if charge.Intent.CreditAmount.IsZero() { | |
| return ledgertransaction.GroupReference{}, nil | |
| } | |
| externalSettlement, err := charge.Intent.Settlement.AsExternalSettlement() | |
| if err != nil { | |
| return ledgertransaction.GroupReference{}, fmt.Errorf("external settlement: %w", err) | |
| } | |
| customerID := customer.CustomerID{ | |
| Namespace: charge.Namespace, | |
| ID: charge.Intent.CustomerID, | |
| } | |
| annotations := ledger.ChargeAnnotations(models.NamespacedID{ | |
| Namespace: charge.Namespace, | |
| ID: charge.ID, | |
| }) | |
| inputs, err := transactions.ResolveTransactions( | |
| ctx, | |
| h.resolverDependencies(), | |
| transactions.ResolutionScope{ | |
| CustomerID: customerID, | |
| Namespace: charge.Namespace, | |
| }, | |
| transactions.FundCustomerReceivableTemplate{ | |
| At: charge.CreatedAt, | |
| Amount: charge.Intent.CreditAmount, | |
| Currency: charge.Intent.Currency, | |
| CostBasis: &externalSettlement.CostBasis, | |
| }, | |
| ) | |
| if err != nil { | |
| return ledgertransaction.GroupReference{}, fmt.Errorf("resolve transactions: %w", err) | |
| } | |
| transactionGroup, err := h.ledger.CommitGroup(ctx, transactions.GroupInputs( | |
| charge.Namespace, | |
| annotations, | |
| inputs..., | |
| )) | |
| if err != nil { | |
| return ledgertransaction.GroupReference{}, fmt.Errorf("commit ledger transaction group: %w", err) | |
| } | |
| return ledgertransaction.GroupReference{ | |
| TransactionGroupID: transactionGroup.ID().ID, | |
| }, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/ledger/chargeadapter/creditpurchase.go` around lines 145 - 194, The
OnCreditPurchasePaymentAuthorized handler is missing the zero-amount
early-return used by the other handlers; add a guard that checks
charge.Intent.CreditAmount.IsZero() at the top of
OnCreditPurchasePaymentAuthorized and return an empty
ledgertransaction.GroupReference,nil when true (or, if omission is intentional,
insert a brief comment in OnCreditPurchasePaymentAuthorized explaining why
processing must continue for zero amounts) so behavior is consistent with
OnPromotionalCreditPurchase and OnCreditPurchaseInitiated.
Comment on lines
+204
to
+227
| accruedBalance, err := settledBalanceForSubAccount(ctx, accruedSubAccount) | ||
| if err != nil { | ||
| return ledgertransaction.GroupReference{}, err | ||
| } | ||
|
|
||
| recognitionAmount := totalRecognition | ||
| if recognitionAmount.GreaterThan(accruedBalance) { | ||
| recognitionAmount = accruedBalance | ||
| } | ||
|
|
||
| var templates []transactions.Resolver | ||
| if receivableReplenishment.IsPositive() { | ||
| templates = append(templates, transactions.FundCustomerReceivableTemplate{ | ||
| At: charge.Intent.InvoiceAt, | ||
| Amount: receivableReplenishment, | ||
| Currency: charge.Intent.Currency, | ||
| }) | ||
| } | ||
| if recognitionAmount.IsPositive() { | ||
| templates = append(templates, transactions.RecognizeEarningsFromAccruedTemplate{ | ||
| At: charge.Intent.InvoiceAt, | ||
| Amount: recognitionAmount, | ||
| Currency: charge.Intent.Currency, | ||
| }) |
Contributor
There was a problem hiding this comment.
Cap receivable funding to what’s actually outstanding on the ledger.
recognitionAmount is capped to accruedBalance, but receivableReplenishment still uses the full charge.State.AccruedUsage amount. If ledger-accrued is lower than the charge state, this books extra wash → receivable and can leave receivable positive instead of just clearing the outstanding invoice-backed balance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/ledger/chargeadapter/flatfee.go` around lines 204 - 227, The
receivable funding (receivableReplenishment) can exceed what's actually on the
ledger because only recognitionAmount is capped to accruedBalance; update
receivableReplenishment after computing recognitionAmount so it is at most the
remaining accrued balance (accruedBalance minus recognitionAmount) before
appending FundCustomerReceivableTemplate. Locate
settledBalanceForSubAccount/accruedBalance and the recognitionAmount calculation
and then constrain receivableReplenishment = min(receivableReplenishment,
accruedBalance.Sub(recognitionAmount)) (or equivalent) so recognition +
receivable funding never exceed accruedBalance.
Comment on lines
+71
to
+77
| if _, err := p.Filters.Route.Normalize(); err != nil { | ||
| return ErrLedgerQueryInvalid.WithAttrs(models.Attributes{ | ||
| "reason": "route_invalid", | ||
| "route": p.Filters.Route, | ||
| "error": err, | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how Route.Normalize() is used elsewhere and if callers expect pre-normalized routes
rg -n "Filters\.Route" --type go -C3Repository: openmeterio/openmeter
Length of output: 1610
🏁 Script executed:
# Find the Route type and Normalize method definition
rg -n "func.*Route.*Normalize|type Route" --type go -A 5Repository: openmeterio/openmeter
Length of output: 3325
🏁 Script executed:
# Check all references to Route.Normalize in the codebase
rg -n "\.Normalize\(\)" --type go | grep -i routeRepository: openmeterio/openmeter
Length of output: 763
🏁 Script executed:
# Check if there are any other validations or uses of Filters.Route in query.go
cat -n openmeter/ledger/query.go | head -100Repository: openmeterio/openmeter
Length of output: 3154
🏁 Script executed:
# Get the full Normalize method implementations to understand what they do
sed -n '160,177p' openmeter/ledger/routing.goRepository: openmeterio/openmeter
Length of output: 467
🏁 Script executed:
# Check if Normalize mutates the receiver or returns a new value
sed -n '178,190p' openmeter/ledger/routing.goRepository: openmeterio/openmeter
Length of output: 386
Store the normalized route to avoid redundant normalization in adapters
Good catch! The Normalize() method returns a new RouteFilter with normalized values (sorted features, canonicalized cost basis), but line 71 discards it. Every other place in the codebase that calls Normalize() captures and uses the result—this is the exception that stands out.
Because the normalized value is thrown away, downstream adapters (like sumentries_query.go) have to call Normalize() again. You should store it:
Fix
- if _, err := p.Filters.Route.Normalize(); err != nil {
+ normalizedRoute, err := p.Filters.Route.Normalize()
+ if err != nil {
return ErrLedgerQueryInvalid.WithAttrs(models.Attributes{
"reason": "route_invalid",
"route": p.Filters.Route,
"error": err,
})
}
+ p.Filters.Route = normalizedRoute📝 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
| if _, err := p.Filters.Route.Normalize(); err != nil { | |
| return ErrLedgerQueryInvalid.WithAttrs(models.Attributes{ | |
| "reason": "route_invalid", | |
| "route": p.Filters.Route, | |
| "error": err, | |
| }) | |
| } | |
| normalizedRoute, err := p.Filters.Route.Normalize() | |
| if err != nil { | |
| return ErrLedgerQueryInvalid.WithAttrs(models.Attributes{ | |
| "reason": "route_invalid", | |
| "route": p.Filters.Route, | |
| "error": err, | |
| }) | |
| } | |
| p.Filters.Route = normalizedRoute |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/ledger/query.go` around lines 71 - 77, The call to
p.Filters.Route.Normalize() currently discards the returned normalized
RouteFilter, causing redundant normalization later; change it to capture the
result (e.g., normalizedRoute, err := p.Filters.Route.Normalize()), on error
return ErrLedgerQueryInvalid as before, and on success assign the normalized
value back to p.Filters.Route (or use normalizedRoute where needed) so
downstream adapters like sumentries_query.go can rely on the already-normalized
route.
Comment on lines
+53
to
+91
| subAccounts, err := deps.SubAccountService.ListSubAccounts(ctx, ledgeraccount.ListSubAccountsInput{ | ||
| Namespace: fboAccountWithID.ID().Namespace, | ||
| AccountID: fboAccountWithID.ID().ID, | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("list sub-accounts: %w", err) | ||
| } | ||
|
|
||
| sources := make([]fboSource, 0, len(subAccounts)) | ||
| for _, subAccount := range subAccounts { | ||
| route := subAccount.Route() | ||
| if route.Currency != currency { | ||
| continue | ||
| } | ||
|
|
||
| balance, err := settledBalanceForSubAccount(ctx, subAccount) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| priority := ledger.DefaultCustomerFBOPriority | ||
| if route.CreditPriority != nil { | ||
| priority = *route.CreditPriority | ||
| } | ||
|
|
||
| sources = append(sources, fboSource{ | ||
| subAccount: subAccount, | ||
| priority: priority, | ||
| balance: balance, | ||
| }) | ||
| } | ||
|
|
||
| sort.Slice(sources, func(i, j int) bool { | ||
| if sources[i].priority == sources[j].priority { | ||
| return sources[i].subAccount.Address().SubAccountID() < sources[j].subAccount.Address().SubAccountID() | ||
| } | ||
|
|
||
| return sources[i].priority < sources[j].priority | ||
| }) |
Contributor
There was a problem hiding this comment.
This turns prioritized collection into an N+1 balance scan.
We load every FBO subaccount and then call GetBalance once per bucket before we even know whether the target is already covered. With priority and cost_basis fan-out, that’s one extra DB round-trip per subaccount on an invoice-assignment path. I’d batch settled-balance lookup, or at least sort by priority first and stop querying once the remaining target hits zero.
As per coding guidelines, "**/*.go: 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."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/ledger/transactions/collection.go` around lines 53 - 91, The
current loop calls settledBalanceForSubAccount for every subAccount, causing an
N+1 DB round-trip; instead first sort subAccounts by route.CreditPriority
(falling back to ledger.DefaultCustomerFBOPriority and
subAccount.Address().SubAccountID() tie-breaker using the existing sort logic),
then either (preferred) call a batched service method (e.g., add/use
SubAccountService.ListSettledBalances(ctx, []SubAccountID) to return balances
for all IDs) and map those into fboSource, or (simpler) compute balances lazily
after sorting and stop calling settledBalanceForSubAccount once the remaining
target amount is fully covered; update code references:
SubAccountService.ListSubAccounts, settledBalanceForSubAccount, fboSource, and
the sort.Slice comparator to implement this change.
Comment on lines
+12
to
+29
| func TestIssueCustomerReceivableTemplate(t *testing.T) { | ||
| env := newTransactionsTestEnv(t) | ||
|
|
||
| priority := 7 | ||
| inputs := env.resolveAndCommit( | ||
| t, | ||
| IssueCustomerReceivableTemplate{ | ||
| At: env.Now(), | ||
| Amount: alpacadecimal.NewFromInt(50), | ||
| Currency: env.Currency, | ||
| CreditPriority: &priority, | ||
| }, | ||
| ) | ||
| require.Len(t, inputs, 1) | ||
|
|
||
| require.True(t, env.SumBalance(t, env.FBOSubAccount(t, priority)).Equal(alpacadecimal.NewFromInt(50))) | ||
| require.True(t, env.SumBalance(t, env.ReceivableSubAccount(t)).Equal(alpacadecimal.NewFromInt(-50))) | ||
| } |
Contributor
There was a problem hiding this comment.
Add one non-zero CostBasis round-trip case here.
These tests cover default and explicit priority nicely, but the new CostBasis route dimension never gets exercised. A regression that drops CostBasis on issue/fund/cover would still pass this file, so I’d add one scenario that runs the full flow against a non-zero cost basis and asserts the balances on that routed subaccount.
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."
Also applies to: 31-46, 48-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/ledger/transactions/customer_test.go` around lines 12 - 29, Add a
new test case inside TestIssueCustomerReceivableTemplate that issues a
receivable with a non-zero CostBasis on the IssueCustomerReceivableTemplate
input, then run the full round-trip (issue → fund → cover) via
env.resolveAndCommit for the corresponding Fund/ Cover messages; after each step
assert balances using env.SumBalance(t, env.FBOSubAccount(t, priority)) and
env.SumBalance(t, env.ReceivableSubAccount(t)) and additionally assert the
balance for the routed subaccount that includes the CostBasis dimension (i.e.
the same routed subaccount used by your system for cost-basis routing) to ensure
the CostBasis value survives the issue/fund/cover flow. Use the existing symbols
IssueCustomerReceivableTemplate, resolveAndCommit, FBOSubAccount and
ReceivableSubAccount to locate where to add the new case.
turip
reviewed
Mar 23, 2026
| return nil, fmt.Errorf("failed to list sub-accounts: %w", err) | ||
| } | ||
|
|
||
| // FIXME: this will be a problem |
Member
There was a problem hiding this comment.
please elaborate ;)
I mean given that the input only allows specifying a single AccountID why do we need account mapping here?
Contributor
Author
There was a problem hiding this comment.
i'll remove the comment, this is just a relic of old times
| At time.Time | ||
| Amount alpacadecimal.Decimal | ||
| Currency currencyx.Code | ||
| } |
Member
There was a problem hiding this comment.
Please add a validator for these and ensure that those are valid before we are resolving it. I know that the routing rules are there, but here we can have more meaningful messages for easier troubleshooting.
Contributor
Author
There was a problem hiding this comment.
Agree, in general we can validate these as "meaningful inputs" just lets keep in mind that with getting rid of dimensions, the principle is we're treating these fields as "opaque" (fields that will later translate to RouteFilters, like currency)
| At: input.Charge.Intent.InvoiceAt, | ||
| Amount: input.PreTaxTotalAmount, | ||
| Currency: input.Charge.Intent.Currency, | ||
| }, |
Member
There was a problem hiding this comment.
I think at this point any credits consumed/allocated should be also booked as earnings (considering the cost basis of course). It's fine if it's done at a later stage.
Comment on lines
+171
to
+176
| // The receivable portion needs wash → receivable replenishment. | ||
| receivableReplenishment := alpacadecimal.NewFromInt(0) | ||
| if charge.State.AccruedUsage != nil { | ||
| receivableReplenishment = charge.State.AccruedUsage.Totals.Total | ||
| totalRecognition = totalRecognition.Add(charge.State.AccruedUsage.Totals.Total) | ||
| } |
Member
There was a problem hiding this comment.
As far as I remember as per the Revenue Recognition design docs, use cases example 2 the accrued usage should be recognized as revenue regardless of payment status.
Comment on lines
+166
to
+169
| totalRecognition := alpacadecimal.NewFromInt(0) | ||
| for _, cr := range charge.State.CreditRealizations { | ||
| totalRecognition = totalRecognition.Add(cr.Amount) | ||
| } |
Member
There was a problem hiding this comment.
As per the recognition docs example 3 states that non-negative credits (sorry for unprofessional wording), should be recognized as far as the customer's ballance.
Contributor
Author
There was a problem hiding this comment.
for this & other revrecog related comments:
- some mismatch is due to negative balance not being handled properly as of yet
- i'd suggest these behaviors are fixed & corrected with the integration tests as thats the cleanest point we can assert it
- i suggest we write a sort of reverse-collection flow for credit funding that can take care of potential revrecog in that case
openmeter/ledger/routing.go
Outdated
Comment on lines
+308
to
+311
| normalized, err := alpacadecimal.NewFromString(v.String()) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("normalize cost basis: %w", err) | ||
| } |
Member
There was a problem hiding this comment.
We should just use the alpacadecimal.Equal operator for equality check, as if the decimal doesn't fit into the fixed precision representation then the bigint representations will not match
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
openmeter/ledger/transactions/resolve_test.go (1)
33-50: Good foundation, but consider expanding test coverage.The test nicely verifies the happy path where
Validate()is called. To make it more comprehensive, you might want to add a few more scenarios:
- Validation failure path: When
Validate()returns an error,ResolveTransactionsshould return early without callingresolve().- Multiple templates: Verify that
Validate()is called for each template (and stops on first failure).- Ensure
resolve()is only called after successful validation: Trackresolve()calls in the spy to confirm the ordering.These would help catch regressions if someone accidentally reorders the validation logic.
💡 Example of additional test cases
func TestResolveTransactions_validationFailure(t *testing.T) { t.Parallel() spy := &spyCustomerTemplate{ validateErr: errors.New("validation failed"), } _, err := ResolveTransactions( t.Context(), ResolverDependencies{}, ResolutionScope{ CustomerID: customer.CustomerID{ Namespace: "ns", ID: "cust", }, }, spy, ) require.Error(t, err) require.Equal(t, 1, spy.validateCalls) require.Equal(t, 0, spy.resolveCalls, "resolve should not be called when Validate fails") }This would require extending the spy:
type spyCustomerTemplate struct { validateCalls int + resolveCalls int + validateErr error } func (s *spyCustomerTemplate) Validate() error { s.validateCalls++ - return nil + return s.validateErr } func (s *spyCustomerTemplate) resolve(context.Context, customer.CustomerID, ResolverDependencies) (ledger.TransactionInput, error) { + s.resolveCalls++ return nil, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/transactions/resolve_test.go` around lines 33 - 50, Add tests covering validation failure, multiple templates, and ordering so ResolveTransactions correctly returns on Validate error and only calls resolve after successful validation: extend the spyCustomerTemplate to record validateCalls, resolveCalls and accept a validateErr field, then add a test where spy.validateErr != nil asserting ResolveTransactions returns an error, validateCalls==1 and resolveCalls==0; add a test with multiple templates (e.g., two spies) asserting Validate is called for each until a failure and that resolve is invoked only for templates whose Validate succeeded; reference ResolveTransactions, spyCustomerTemplate, Validate and resolve when locating and updating/adding these tests.openmeter/ledger/transactions/customer.go (1)
25-32: Could we reuseValidateTransactionAmounthere too?This is the only template in this group still open-coding the amount checks, so its validation errors will drift from the sentinel/attrs shape the other templates now return. Reusing the shared helper would keep this path aligned.
♻️ Small cleanup
func (t IssueCustomerReceivableTemplate) Validate() error { - if t.Amount.IsNegative() { - return fmt.Errorf("amount must be positive") - } - - if t.Amount.IsZero() { - return fmt.Errorf("amount must be non-zero") - } + if err := ledger.ValidateTransactionAmount(t.Amount); err != nil { + return fmt.Errorf("amount: %w", err) + } if t.At.IsZero() { return fmt.Errorf("at is required") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/transactions/customer.go` around lines 25 - 32, The IssueCustomerReceivableTemplate.Validate method currently duplicates amount checks; replace the manual IsNegative/IsZero checks with a call to the shared helper ValidateTransactionAmount(t.Amount) so this template returns the same sentinel/attrs-shaped errors as the other templates (i.e., remove the explicit fmt.Errorf checks and return the helper's error directly inside IssueCustomerReceivableTemplate.Validate).
🤖 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/routing.go`:
- Around line 163-187: Route.Normalize only sorts Features and doesn't
canonicalize optional string fields (e.g., TaxCode) to match routing-key
semantics (where ptr("") should collapse to nil via optionalStringValue), and
RouteFilter.Normalize incorrectly casts a partial filter to Route causing
Currency to be treated as required; fix by extending Route.Normalize to collapse
empty optional string fields using the same optionalStringValue logic used for
routing keys (apply to TaxCode and any other optional strings) and change
RouteFilter.Normalize to normalize only present filter fields (do not cast the
whole RouteFilter to Route) — instead, build a normalized RouteFilter by copying
fields from f, normalizing Features with SortedFeatures, collapsing optional
strings via optionalStringValue, and leaving absent fields nil so partial
filters remain valid.
In `@openmeter/ledger/transactions/accrual.go`:
- Around line 17-84: The resolve path for TransferCustomerFBOToAccruedTemplate
is losing the cost_basis dimension—update resolve
(TransferCustomerFBOToAccruedTemplate.resolve) and the collection helper
(collectFromPrioritizedCustomerFBO) to filter/return collections by both
Currency and CostBasis, propagate cost_basis into each collection result, and
include cost_basis when selecting the accrued sub-account
(ledger.CustomerAccruedRouteParams) and when building EntryInput entries so
debits/credits are applied to the correct {Currency, CostBasis} buckets; ensure
any signatures/types for collectFromPrioritizedCustomerFBO and the collection
struct are changed accordingly so cost_basis flows through the whole transfer.
---
Nitpick comments:
In `@openmeter/ledger/transactions/customer.go`:
- Around line 25-32: The IssueCustomerReceivableTemplate.Validate method
currently duplicates amount checks; replace the manual IsNegative/IsZero checks
with a call to the shared helper ValidateTransactionAmount(t.Amount) so this
template returns the same sentinel/attrs-shaped errors as the other templates
(i.e., remove the explicit fmt.Errorf checks and return the helper's error
directly inside IssueCustomerReceivableTemplate.Validate).
In `@openmeter/ledger/transactions/resolve_test.go`:
- Around line 33-50: Add tests covering validation failure, multiple templates,
and ordering so ResolveTransactions correctly returns on Validate error and only
calls resolve after successful validation: extend the spyCustomerTemplate to
record validateCalls, resolveCalls and accept a validateErr field, then add a
test where spy.validateErr != nil asserting ResolveTransactions returns an
error, validateCalls==1 and resolveCalls==0; add a test with multiple templates
(e.g., two spies) asserting Validate is called for each until a failure and that
resolve is invoked only for templates whose Validate succeeded; reference
ResolveTransactions, spyCustomerTemplate, Validate and resolve when locating and
updating/adding these 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: a0dcd2a8-7283-4f2e-a64c-60dbeb889725
📒 Files selected for processing (9)
openmeter/ledger/account/service/service.goopenmeter/ledger/errors.goopenmeter/ledger/routing.goopenmeter/ledger/transactions/accrual.goopenmeter/ledger/transactions/customer.goopenmeter/ledger/transactions/earnings.goopenmeter/ledger/transactions/fx.goopenmeter/ledger/transactions/resolve.goopenmeter/ledger/transactions/resolve_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- openmeter/ledger/transactions/fx.go
- openmeter/ledger/transactions/resolve.go
- openmeter/ledger/errors.go
Comment on lines
+163
to
+187
| // Normalize canonicalizes route values so semantically equivalent routes share | ||
| // the same stored literals and routing keys. | ||
| func (r Route) Normalize() (Route, error) { | ||
| if err := r.Validate(); err != nil { | ||
| return Route{}, err | ||
| } | ||
|
|
||
| normalized := r | ||
| normalized.Features = SortedFeatures(r.Features) | ||
|
|
||
| return normalized, nil | ||
| } | ||
|
|
||
| // Normalize canonicalizes route filter values before querying. | ||
| func (f RouteFilter) Normalize() (RouteFilter, error) { | ||
| if f.Currency == "" && f.TaxCode == nil && len(f.Features) == 0 && f.CostBasis == nil && f.CreditPriority == nil { | ||
| return f, nil | ||
| } | ||
|
|
||
| normalized, err := Route(f).Normalize() | ||
| if err != nil { | ||
| return RouteFilter{}, err | ||
| } | ||
|
|
||
| return RouteFilter(normalized), nil |
Contributor
There was a problem hiding this comment.
The new normalization path is out of sync with the routing-key semantics.
Route.Normalize() only sorts features, so TaxCode: ptr("") survives even though optionalStringValue() collapses it to the same routing-key value as nil. On top of that, RouteFilter.Normalize() casts the filter to a full Route, which makes Currency effectively required and rejects otherwise-valid partial filters like {CostBasis: ...} or {CreditPriority: ...}. That breaks both canonical storage and partial route queries.
💡 Suggested fix
func (r Route) Normalize() (Route, error) {
if err := r.Validate(); err != nil {
return Route{}, err
}
normalized := r
normalized.Features = SortedFeatures(r.Features)
+ if normalized.TaxCode != nil && *normalized.TaxCode == "" {
+ normalized.TaxCode = nil
+ }
return normalized, nil
}
// Normalize canonicalizes route filter values before querying.
func (f RouteFilter) Normalize() (RouteFilter, error) {
- if f.Currency == "" && f.TaxCode == nil && len(f.Features) == 0 && f.CostBasis == nil && f.CreditPriority == nil {
- return f, nil
- }
-
- normalized, err := Route(f).Normalize()
- if err != nil {
- return RouteFilter{}, err
- }
-
- return RouteFilter(normalized), nil
+ normalized := f
+ normalized.Features = SortedFeatures(f.Features)
+ if normalized.TaxCode != nil && *normalized.TaxCode == "" {
+ normalized.TaxCode = nil
+ }
+ if normalized.Currency != "" {
+ if err := ValidateCurrency(normalized.Currency); err != nil {
+ return RouteFilter{}, err
+ }
+ }
+ if normalized.CreditPriority != nil {
+ if err := ValidateCreditPriority(*normalized.CreditPriority); err != nil {
+ return RouteFilter{}, err
+ }
+ }
+ if normalized.CostBasis != nil {
+ if err := ValidateCostBasis(*normalized.CostBasis); err != nil {
+ return RouteFilter{}, err
+ }
+ }
+
+ return normalized, nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/ledger/routing.go` around lines 163 - 187, Route.Normalize only
sorts Features and doesn't canonicalize optional string fields (e.g., TaxCode)
to match routing-key semantics (where ptr("") should collapse to nil via
optionalStringValue), and RouteFilter.Normalize incorrectly casts a partial
filter to Route causing Currency to be treated as required; fix by extending
Route.Normalize to collapse empty optional string fields using the same
optionalStringValue logic used for routing keys (apply to TaxCode and any other
optional strings) and change RouteFilter.Normalize to normalize only present
filter fields (do not cast the whole RouteFilter to Route) — instead, build a
normalized RouteFilter by copying fields from f, normalizing Features with
SortedFeatures, collapsing optional strings via optionalStringValue, and leaving
absent fields nil so partial filters remain valid.
Comment on lines
+17
to
+84
| type TransferCustomerFBOToAccruedTemplate struct { | ||
| At time.Time | ||
| Amount alpacadecimal.Decimal | ||
| Currency currencyx.Code | ||
| } | ||
|
|
||
| func (t TransferCustomerFBOToAccruedTemplate) Validate() error { | ||
| if t.At.IsZero() { | ||
| return fmt.Errorf("at is required") | ||
| } | ||
|
|
||
| if err := ledger.ValidateTransactionAmount(t.Amount); err != nil { | ||
| return fmt.Errorf("amount: %w", err) | ||
| } | ||
|
|
||
| if err := ledger.ValidateCurrency(t.Currency); err != nil { | ||
| return fmt.Errorf("currency: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (t TransferCustomerFBOToAccruedTemplate) typeGuard() guard { | ||
| return true | ||
| } | ||
|
|
||
| var _ CustomerTransactionTemplate = (TransferCustomerFBOToAccruedTemplate{}) | ||
|
|
||
| func (t TransferCustomerFBOToAccruedTemplate) resolve(ctx context.Context, customerID customer.CustomerID, resolvers ResolverDependencies) (ledger.TransactionInput, error) { | ||
| collections, err := collectFromPrioritizedCustomerFBO(ctx, customerID, t.Currency, t.Amount, resolvers) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("collect from prioritized FBO: %w", err) | ||
| } | ||
| if len(collections) == 0 { | ||
| return nil, nil | ||
| } | ||
|
|
||
| customerAccounts, err := resolvers.AccountService.GetCustomerAccounts(ctx, customerID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get customer accounts: %w", err) | ||
| } | ||
|
|
||
| accrued, err := customerAccounts.AccruedAccount.GetSubAccountForRoute(ctx, ledger.CustomerAccruedRouteParams{ | ||
| Currency: t.Currency, | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get accrued sub-account: %w", err) | ||
| } | ||
|
|
||
| entryInputs := make([]*EntryInput, 0, len(collections)+1) | ||
| totalCollected := alpacadecimal.Zero | ||
| for _, collection := range collections { | ||
| totalCollected = totalCollected.Add(collection.amount) | ||
| entryInputs = append(entryInputs, &EntryInput{ | ||
| address: collection.subAccount.Address(), | ||
| amount: collection.amount.Neg(), | ||
| }) | ||
| } | ||
|
|
||
| entryInputs = append(entryInputs, &EntryInput{ | ||
| address: accrued.Address(), | ||
| amount: totalCollected, | ||
| }) | ||
|
|
||
| return &TransactionInput{ | ||
| bookedAt: t.At, | ||
| entryInputs: entryInputs, | ||
| }, nil |
Contributor
There was a problem hiding this comment.
These new transfer templates are dropping cost_basis.
IssueCustomerReceivableTemplate, FundCustomerReceivableTemplate, and CoverCustomerReceivableTemplate now create FBO/receivable balances in {Currency, CostBasis} buckets, but these two templates resolve and collect by Currency only. With multiple cost bases in one currency, TransferCustomerReceivableToAccruedTemplate will hit the nil-cost-basis receivable bucket, and TransferCustomerFBOToAccruedTemplate can drain unrelated FBO credits. Once that dimension is lost here, the later earnings step can’t recover it.
💡 Suggested direction
type TransferCustomerFBOToAccruedTemplate struct {
At time.Time
Amount alpacadecimal.Decimal
Currency currencyx.Code
+ CostBasis *alpacadecimal.Decimal
}
func (t TransferCustomerFBOToAccruedTemplate) Validate() error {
if t.At.IsZero() {
return fmt.Errorf("at is required")
}
if err := ledger.ValidateTransactionAmount(t.Amount); err != nil {
return fmt.Errorf("amount: %w", err)
}
if err := ledger.ValidateCurrency(t.Currency); err != nil {
return fmt.Errorf("currency: %w", err)
}
+ if t.CostBasis != nil {
+ if err := ledger.ValidateCostBasis(*t.CostBasis); err != nil {
+ return fmt.Errorf("cost basis: %w", err)
+ }
+ }
return nil
}
-collections, err := collectFromPrioritizedCustomerFBO(ctx, customerID, t.Currency, t.Amount, resolvers)
+collections, err := collectFromPrioritizedCustomerFBO(ctx, customerID, t.Currency, t.CostBasis, t.Amount, resolvers)
type TransferCustomerReceivableToAccruedTemplate struct {
At time.Time
Amount alpacadecimal.Decimal
Currency currencyx.Code
+ CostBasis *alpacadecimal.Decimal
}
func (t TransferCustomerReceivableToAccruedTemplate) Validate() error {
if t.At.IsZero() {
return fmt.Errorf("at is required")
}
if err := ledger.ValidateTransactionAmount(t.Amount); err != nil {
return fmt.Errorf("amount: %w", err)
}
if err := ledger.ValidateCurrency(t.Currency); err != nil {
return fmt.Errorf("currency: %w", err)
}
+ if t.CostBasis != nil {
+ if err := ledger.ValidateCostBasis(*t.CostBasis); err != nil {
+ return fmt.Errorf("cost basis: %w", err)
+ }
+ }
return nil
}
receivable, err := customerAccounts.ReceivableAccount.GetSubAccountForRoute(ctx, ledger.CustomerReceivableRouteParams{
Currency: t.Currency,
+ CostBasis: t.CostBasis,
})You’d also want the matching signature/filter update in collectFromPrioritizedCustomerFBO(...).
Also applies to: 89-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/ledger/transactions/accrual.go` around lines 17 - 84, The resolve
path for TransferCustomerFBOToAccruedTemplate is losing the cost_basis
dimension—update resolve (TransferCustomerFBOToAccruedTemplate.resolve) and the
collection helper (collectFromPrioritizedCustomerFBO) to filter/return
collections by both Currency and CostBasis, propagate cost_basis into each
collection result, and include cost_basis when selecting the accrued sub-account
(ledger.CustomerAccruedRouteParams) and when building EntryInput entries so
debits/credits are applied to the correct {Currency, CostBasis} buckets; ensure
any signatures/types for collectFromPrioritizedCustomerFBO and the collection
struct are changed accordingly so cost_basis flows through the whole transfer.
^ Conflicts: ^ openmeter/ledger/account/service/service.go ^ openmeter/ledger/historical/adapter/ledger_test.go
GAlexIHU
force-pushed
the
feat/ledger-charges-rebased
branch
from
82aa595 to
d468769
Compare
turip
approved these changes
Mar 23, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
openmeter/ledger/routing.go (1)
165-187:⚠️ Potential issue | 🟠 MajorPartial route filters are getting over-validated here.
Line 182 converts a
RouteFilterinto a fullRoute, so filters like{CostBasis: ...},{CreditPriority: ...}or{Features: ...}now fail on missing currency. This same path also keepsTaxCode: ptr("")different fromnil, even thoughBuildRoutingKeyV1collapses both to the same routing-key value, so the query/persistence side drifts from the routing-key semantics.🛠️ Suggested fix
func (r Route) Normalize() (Route, error) { if err := r.Validate(); err != nil { return Route{}, err } normalized := r normalized.Features = SortedFeatures(r.Features) + if normalized.TaxCode != nil && *normalized.TaxCode == "" { + normalized.TaxCode = nil + } return normalized, nil } // Normalize canonicalizes route filter values before querying. func (f RouteFilter) Normalize() (RouteFilter, error) { - if f.Currency == "" && f.TaxCode == nil && len(f.Features) == 0 && f.CostBasis == nil && f.CreditPriority == nil { - return f, nil - } - - normalized, err := Route(f).Normalize() - if err != nil { - return RouteFilter{}, err - } - - return RouteFilter(normalized), nil + normalized := f + normalized.Features = SortedFeatures(f.Features) + if normalized.TaxCode != nil && *normalized.TaxCode == "" { + normalized.TaxCode = nil + } + if normalized.Currency != "" { + if err := ValidateCurrency(normalized.Currency); err != nil { + return RouteFilter{}, err + } + } + if normalized.CreditPriority != nil { + if err := ValidateCreditPriority(*normalized.CreditPriority); err != nil { + return RouteFilter{}, err + } + } + if normalized.CostBasis != nil { + if err := ValidateCostBasis(*normalized.CostBasis); err != nil { + return RouteFilter{}, err + } + } + + return normalized, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/routing.go` around lines 165 - 187, RouteFilter.Normalize should not cast the filter to a full Route (Route(f).Normalize()) because that enforces full-route validation (e.g. requiring Currency) and preserves empty-string TaxCode as non-nil; instead, implement normalization locally: collapse TaxCode empty string to nil, sort Features via SortedFeatures when len(Features)>0, and otherwise leave CostBasis/CreditPriority/Currency as-is so partial filters are accepted; update RouteFilter.Normalize (and keep Route.Normalize unchanged) to perform only these targeted canonicalizations and not call Route.Validate or Route.Normalize, ensuring routing-key semantics (empty TaxCode == nil) match BuildRoutingKeyV1.openmeter/ledger/chargeadapter/creditpurchase.go (1)
145-153:⚠️ Potential issue | 🟠 MajorZero-amount authorizations should still short-circuit.
The other two credit-purchase entry points already noop on
CreditAmount.IsZero(), but this one falls through intoFundCustomerReceivableTemplate. That makes$0authorizations behave differently and can turn a harmless no-op into a ledger error path.🛠️ Suggested fix
func (h *creditPurchaseHandler) OnCreditPurchasePaymentAuthorized(ctx context.Context, charge chargecreditpurchase.Charge) (ledgertransaction.GroupReference, error) { if err := charge.Validate(); err != nil { return ledgertransaction.GroupReference{}, err } + + if charge.Intent.CreditAmount.IsZero() { + return ledgertransaction.GroupReference{}, nil + } externalSettlement, err := charge.Intent.Settlement.AsExternalSettlement()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/chargeadapter/creditpurchase.go` around lines 145 - 153, In OnCreditPurchasePaymentAuthorized, short-circuit zero-amount authorizations like the other handlers by checking charge.CreditAmount.IsZero() early (before calling charge.Intent.Settlement.AsExternalSettlement() or FundCustomerReceivableTemplate) and return a zero-value ledgertransaction.GroupReference and nil error; update the method (identified by OnCreditPurchasePaymentAuthorized and using FundCustomerReceivableTemplate and CreditAmount.IsZero()) to perform this early noop to keep $0 authorizations consistent.openmeter/ledger/chargeadapter/flatfee.go (1)
171-220:⚠️ Potential issue | 🟠 MajorBase the wash → receivable leg on the usage amount that actually made it into
recognitionAmount.
recognitionAmountis capped by settled accrued balance, butreceivableReplenishmentstill uses the fullcharge.State.AccruedUsage.Totals.Total. In a partial-balance case, that overfunds receivable relative to the earnings recognition you actually posted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/chargeadapter/flatfee.go` around lines 171 - 220, The receivable replenishment currently uses charge.State.AccruedUsage.Totals.Total even when recognitionAmount is capped by settled accrued balance, which can overfund receivable; change receivableReplenishment so it is the lesser of charge.State.AccruedUsage.Totals.Total and recognitionAmount (i.e., set receivableReplenishment = min(charge.State.AccruedUsage.Totals.Total, recognitionAmount)) before building templates (use the existing variables receivableReplenishment, recognitionAmount and charge.State.AccruedUsage.Totals.Total to locate and update the logic).openmeter/ledger/transactions/accrual.go (1)
89-93:⚠️ Potential issue | 🟠 Major
TransferCustomerReceivableToAccruedTemplatestill can’t hit the right receivable bucket whencost_basisis set.
IssueCustomerReceivableTemplateandFundCustomerReceivableTemplatenow create receivable balances in{currency, cost_basis}routes, but this template resolvesCustomerReceivableRouteParamswith currency only. As soon as a cost basis is present, you’ll move value out of the nil-cost-basis receivable sub-account and leave the real outstanding bucket untouched. If accrued is supposed to stay basis-aware too, the positive leg needs the same dimension.Also applies to: 117-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/transactions/accrual.go` around lines 89 - 93, TransferCustomerReceivableToAccruedTemplate currently only includes currency so it resolves CustomerReceivableRouteParams without cost_basis and moves value from the nil-cost-basis receivable sub-account; add a CostBasis field to TransferCustomerReceivableToAccruedTemplate (same type used by IssueCustomerReceivableTemplate/FundCustomerReceivableTemplate) and include that CostBasis when constructing CustomerReceivableRouteParams for the accrued/positive leg so the transfer targets the {currency, cost_basis} receivable bucket; apply the same change to the equivalent code paths around the 117-125 region to ensure accrued balances remain basis-aware.
🧹 Nitpick comments (1)
openmeter/ledger/routingrules/routingrules_test.go (1)
17-80: I’d add oneCommitGroup-level rejection test too.These cases give nice unit coverage for
DefaultValidator, but they don’t prove the new wiring inhistorical.Ledger.CommitGroup. A small negative commit test — e.g. trying to persist an FBO → Earnings pair and asserting the routing-rule error bubbles back — would close that gap nicely.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/routingrules/routingrules_test.go` around lines 17 - 80, Add a CommitGroup-level negative test to ensure routing-rule rejections bubble up from persistence: create a test that constructs the same forbidden entry pair (use transactionstestutils.AnyEntryInput with ledger.AccountTypeCustomerFBO and ledger.AccountTypeEarnings, matching currency and opposite sign amounts) and call historical.Ledger.CommitGroup to persist them, then assert CommitGroup returns an error and that the error message contains "ledger routing rule violated"; this verifies the wiring between routingrules.DefaultValidator and historical.Ledger.CommitGroup rather than only validating validator.ValidateEntries.
🤖 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/validations.go`:
- Around line 66-83: In ValidateTransactionInputWith, validate each entry first
to avoid nil-entry panics: iterate transaction.EntryInputs() and call
ValidateEntryInput(entry) (and return ErrEntryInvalid if an entry is nil) before
calling ValidateInvariance; then call ValidateInvariance (passing the validated
slice of EntryInput values rather than mapping and dereferencing entry.Amount()
earlier). This touches ValidateTransactionInputWith, ValidateEntryInput,
ValidateInvariance and prevents dereferencing EntryInput.Amount() on a nil
entry.
---
Duplicate comments:
In `@openmeter/ledger/chargeadapter/creditpurchase.go`:
- Around line 145-153: In OnCreditPurchasePaymentAuthorized, short-circuit
zero-amount authorizations like the other handlers by checking
charge.CreditAmount.IsZero() early (before calling
charge.Intent.Settlement.AsExternalSettlement() or
FundCustomerReceivableTemplate) and return a zero-value
ledgertransaction.GroupReference and nil error; update the method (identified by
OnCreditPurchasePaymentAuthorized and using FundCustomerReceivableTemplate and
CreditAmount.IsZero()) to perform this early noop to keep $0 authorizations
consistent.
In `@openmeter/ledger/chargeadapter/flatfee.go`:
- Around line 171-220: The receivable replenishment currently uses
charge.State.AccruedUsage.Totals.Total even when recognitionAmount is capped by
settled accrued balance, which can overfund receivable; change
receivableReplenishment so it is the lesser of
charge.State.AccruedUsage.Totals.Total and recognitionAmount (i.e., set
receivableReplenishment = min(charge.State.AccruedUsage.Totals.Total,
recognitionAmount)) before building templates (use the existing variables
receivableReplenishment, recognitionAmount and
charge.State.AccruedUsage.Totals.Total to locate and update the logic).
In `@openmeter/ledger/routing.go`:
- Around line 165-187: RouteFilter.Normalize should not cast the filter to a
full Route (Route(f).Normalize()) because that enforces full-route validation
(e.g. requiring Currency) and preserves empty-string TaxCode as non-nil;
instead, implement normalization locally: collapse TaxCode empty string to nil,
sort Features via SortedFeatures when len(Features)>0, and otherwise leave
CostBasis/CreditPriority/Currency as-is so partial filters are accepted; update
RouteFilter.Normalize (and keep Route.Normalize unchanged) to perform only these
targeted canonicalizations and not call Route.Validate or Route.Normalize,
ensuring routing-key semantics (empty TaxCode == nil) match BuildRoutingKeyV1.
In `@openmeter/ledger/transactions/accrual.go`:
- Around line 89-93: TransferCustomerReceivableToAccruedTemplate currently only
includes currency so it resolves CustomerReceivableRouteParams without
cost_basis and moves value from the nil-cost-basis receivable sub-account; add a
CostBasis field to TransferCustomerReceivableToAccruedTemplate (same type used
by IssueCustomerReceivableTemplate/FundCustomerReceivableTemplate) and include
that CostBasis when constructing CustomerReceivableRouteParams for the
accrued/positive leg so the transfer targets the {currency, cost_basis}
receivable bucket; apply the same change to the equivalent code paths around the
117-125 region to ensure accrued balances remain basis-aware.
---
Nitpick comments:
In `@openmeter/ledger/routingrules/routingrules_test.go`:
- Around line 17-80: Add a CommitGroup-level negative test to ensure
routing-rule rejections bubble up from persistence: create a test that
constructs the same forbidden entry pair (use
transactionstestutils.AnyEntryInput with ledger.AccountTypeCustomerFBO and
ledger.AccountTypeEarnings, matching currency and opposite sign amounts) and
call historical.Ledger.CommitGroup to persist them, then assert CommitGroup
returns an error and that the error message contains "ledger routing rule
violated"; this verifies the wiring between routingrules.DefaultValidator and
historical.Ledger.CommitGroup rather than only validating
validator.ValidateEntries.
🪄 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: d0471e06-3475-4b49-a9b2-66b59f73bdd3
⛔ Files ignored due to path filters (8)
openmeter/ent/db/ledgersubaccountroute.gois excluded by!**/ent/db/**openmeter/ent/db/ledgersubaccountroute/ledgersubaccountroute.gois excluded by!**/ent/db/**openmeter/ent/db/ledgersubaccountroute/where.gois excluded by!**/ent/db/**openmeter/ent/db/ledgersubaccountroute_create.gois excluded by!**/ent/db/**openmeter/ent/db/ledgersubaccountroute_update.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (60)
.agents/skills/ledger/SKILL.md.agents/skills/rebase/SKILL.mdAGENTS.mdapp/common/ledger.goopenmeter/ent/schema/ledger_account.goopenmeter/ledger/account.goopenmeter/ledger/account/account.goopenmeter/ledger/account/account_business.goopenmeter/ledger/account/account_customer.goopenmeter/ledger/account/adapter/repo_test.goopenmeter/ledger/account/adapter/subaccount.goopenmeter/ledger/account/address.goopenmeter/ledger/account/service/service.goopenmeter/ledger/account/subaccount.goopenmeter/ledger/accounts.goopenmeter/ledger/annotations.goopenmeter/ledger/chargeadapter/creditpurchase.goopenmeter/ledger/chargeadapter/creditpurchase_test.goopenmeter/ledger/chargeadapter/flatfee.goopenmeter/ledger/chargeadapter/flatfee_test.goopenmeter/ledger/errors.goopenmeter/ledger/historical/adapter/ledger.goopenmeter/ledger/historical/adapter/ledger_test.goopenmeter/ledger/historical/adapter/sumentries_query.goopenmeter/ledger/historical/entry.goopenmeter/ledger/historical/ledger.goopenmeter/ledger/historical/transaction.goopenmeter/ledger/ledger_fx_test.goopenmeter/ledger/ledger_test.goopenmeter/ledger/primitives.goopenmeter/ledger/query.goopenmeter/ledger/resolvers/account.goopenmeter/ledger/routing.goopenmeter/ledger/routing_test.goopenmeter/ledger/routing_validator.goopenmeter/ledger/routingrules/defaults.goopenmeter/ledger/routingrules/routingrules.goopenmeter/ledger/routingrules/routingrules_test.goopenmeter/ledger/routingrules/view.goopenmeter/ledger/testutil/generate.goopenmeter/ledger/testutil/wire.goopenmeter/ledger/testutil/wire_gen.goopenmeter/ledger/testutils/deps.goopenmeter/ledger/testutils/integration.goopenmeter/ledger/transactions/accrual.goopenmeter/ledger/transactions/accrual_test.goopenmeter/ledger/transactions/collection.goopenmeter/ledger/transactions/customer.goopenmeter/ledger/transactions/customer_test.goopenmeter/ledger/transactions/earnings.goopenmeter/ledger/transactions/earnings_test.goopenmeter/ledger/transactions/fx.goopenmeter/ledger/transactions/resolve.goopenmeter/ledger/transactions/resolve_test.goopenmeter/ledger/transactions/testenv_test.goopenmeter/ledger/validations.gopkg/timeutil/openperiod.gopkg/timeutil/openperiod_test.gotools/migrate/migrations/20260323132723_ledger-route-cost-basis.down.sqltools/migrate/migrations/20260323132723_ledger-route-cost-basis.up.sql
💤 Files with no reviewable changes (3)
- openmeter/ledger/testutil/generate.go
- openmeter/ledger/testutil/wire.go
- openmeter/ledger/testutil/wire_gen.go
✅ Files skipped from review due to trivial changes (16)
- openmeter/ledger/ledger_test.go
- openmeter/ledger/routing_validator.go
- tools/migrate/migrations/20260323132723_ledger-route-cost-basis.up.sql
- openmeter/ledger/transactions/resolve_test.go
- openmeter/ledger/routing_test.go
- openmeter/ledger/routingrules/defaults.go
- openmeter/ledger/testutils/deps.go
- openmeter/ledger/transactions/collection.go
- openmeter/ledger/transactions/accrual_test.go
- openmeter/ledger/transactions/testenv_test.go
- openmeter/ledger/chargeadapter/creditpurchase_test.go
- openmeter/ledger/annotations.go
- .agents/skills/rebase/SKILL.md
- openmeter/ledger/routingrules/view.go
- openmeter/ledger/historical/adapter/sumentries_query.go
- .agents/skills/ledger/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (18)
- pkg/timeutil/openperiod.go
- openmeter/ledger/account/account.go
- openmeter/ledger/historical/transaction.go
- pkg/timeutil/openperiod_test.go
- openmeter/ledger/transactions/earnings_test.go
- app/common/ledger.go
- openmeter/ledger/transactions/fx.go
- openmeter/ledger/account/account_business.go
- openmeter/ledger/account/adapter/repo_test.go
- openmeter/ledger/chargeadapter/flatfee_test.go
- openmeter/ledger/transactions/customer_test.go
- openmeter/ledger/transactions/earnings.go
- openmeter/ledger/account/service/service.go
- openmeter/ledger/account/subaccount.go
- AGENTS.md
- openmeter/ledger/historical/adapter/ledger_test.go
- openmeter/ledger/testutils/integration.go
- openmeter/ledger/account/account_customer.go
Comment on lines
+66
to
+83
| func ValidateTransactionInputWith(ctx context.Context, transaction TransactionInput, routingValidator RoutingValidator) error { | ||
| if transaction == nil { | ||
| return ErrTransactionInputRequired | ||
| } | ||
|
|
||
| // Let's validate that the entries add up | ||
| if err := ValidateInvariance(ctx, lo.Map(transaction.EntryInputs(), func(e EntryInput, _ int) EntryInput { | ||
| return e | ||
| })); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Let's validate routing | ||
| if err := ValidateRouting(ctx, transaction.EntryInputs()); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Let's validate the entries themselves | ||
| for _, entry := range transaction.EntryInputs() { | ||
| if err := ValidateEntryInput(ctx, entry); err != nil { | ||
| return fmt.Errorf("invalid entry: %w", err) | ||
| return err | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Validate entries before you sum them.
Right now a nil entry still panics in ValidateInvariance() because entry.Amount() is dereferenced before ValidateEntryInput() gets a chance to return ErrEntryInvalid.
💡 Suggested fix
func ValidateTransactionInputWith(ctx context.Context, transaction TransactionInput, routingValidator RoutingValidator) error {
if transaction == nil {
return ErrTransactionInputRequired
}
+ for _, entry := range transaction.EntryInputs() {
+ if err := ValidateEntryInput(ctx, entry); err != nil {
+ return err
+ }
+ }
+
// Let's validate that the entries add up
if err := ValidateInvariance(ctx, lo.Map(transaction.EntryInputs(), func(e EntryInput, _ int) EntryInput {
return e
})); err != nil {
return err
}
-
- // Let's validate the entries themselves
- for _, entry := range transaction.EntryInputs() {
- if err := ValidateEntryInput(ctx, entry); err != nil {
- return err
- }
- }
if routingValidator != nil {
if err := routingValidator.ValidateEntries(transaction.EntryInputs()); err != nil {
return err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/ledger/validations.go` around lines 66 - 83, In
ValidateTransactionInputWith, validate each entry first to avoid nil-entry
panics: iterate transaction.EntryInputs() and call ValidateEntryInput(entry)
(and return ErrEntryInvalid if an entry is nil) before calling
ValidateInvariance; then call ValidateInvariance (passing the validated slice of
EntryInput values rather than mapping and dereferencing entry.Amount() earlier).
This touches ValidateTransactionInputWith, ValidateEntryInput,
ValidateInvariance and prevents dereferencing EntryInput.Amount() on a nil
entry.
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.
Overview
Implements charge adapters, + related & needed ledger chagnes
Changes
flatfeeandcreditpurchasecustomer_accruedas a customer account typecost_basisSummary by CodeRabbit
Release Notes
New Features
Improvements
Documentation