feat(ledger): add custom currencies support (credit_only) - #4731
Conversation
mark-vass-konghq
added
release-note/feature
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughLedger routing now supports exchange source currencies and v3 keys. FX conversion uses customer receivable routes with explicit source amounts, transaction validation balances per currency, and invoice APIs expose unit configuration snapshots. ChangesLedger currency routing and persistence
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ConvertCurrencyTemplate
participant CustomerReceivable
participant Ledger
ConvertCurrencyTemplate->>CustomerReceivable: resolve exchange-aware source and target subaccounts
ConvertCurrencyTemplate->>Ledger: submit source and target currency postings
Ledger-->>ConvertCurrencyTemplate: validate per-currency totals and record transaction
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openmeter/billing/rating/service/rate/types.go (1)
32-46: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCollect all validation failures in
Validate().
pkg/models.NewNillableGenericValidationErroralready exists, so this can accumulate the checks intoerrsand returnmodels.NewNillableGenericValidationError(errors.Join(errs...))instead of stopping at the first missing field.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/rating/service/rate/types.go` around lines 32 - 46, Update PricerCalculateInput.Validate to collect every missing-field validation error in an errs slice instead of returning immediately; append failures for CurrencyCalculator, FullProgressivelyBilledServicePeriod, and StandardLineAccessor, then return models.NewNillableGenericValidationError(errors.Join(errs...)) so nil is returned when there are no errors.Source: Coding guidelines
openmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.go (1)
106-108: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard against nil
Currency.Good catch bringing in
currencyx.Currency! Just a heads-up: if anApplyInputhappens to be instantiated with anilCurrencyfield, callingi.Currency.Validate()right here will cause a panic. Might be worth adding a quick nil-check first to keep things super safe!🛡️ Proposed fix
- if err := i.Currency.Validate(); err != nil { - errs = append(errs, fmt.Errorf("currency: %w", err)) - } + if i.Currency == nil { + errs = append(errs, fmt.Errorf("currency is required")) + } else if err := i.Currency.Validate(); err != nil { + errs = append(errs, fmt.Errorf("currency: %w", err)) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.go` around lines 106 - 108, Update the validation logic in the ApplyInput reconciliation flow to check whether i.Currency is nil before calling Currency.Validate(); when nil, append an appropriate currency validation error instead of dereferencing it, while preserving the existing wrapped error behavior for non-nil currencies.
🧹 Nitpick comments (5)
pkg/currencyx/currency_test.go (1)
244-357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the remaining formatter and directional-rounding branches.
Please add custom cases using
WithDecimalMarkandWithThousandsSeparator, plus negative inputs forRoundUpandRoundDown. The current suite would miss broken setter wiring or reversed negative-rounding behavior.As per path instructions, “Make sure the tests are comprehensive and cover the changes.”
Also applies to: 415-427
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/currencyx/currency_test.go` around lines 244 - 357, Extend TestFormatAmount with custom-currency cases that configure WithDecimalMark and WithThousandsSeparator and assert both separators are reflected in formatted output. Add negative-value cases covering RoundUp and RoundDown, asserting their directional rounding behavior; keep the existing table-driven setup and currency construction flow.Source: Path instructions
openmeter/billing/charges/flatfee/charge.go (1)
186-188: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate stale error messages to reflect the new
currencytype.Hey! 👋 Just a tiny stylistic note. With the great refactor from
currencyx.Calculatortocurrencyx.Currencybuilt viaNewCurrencyBuilder, there are a few error messages left behind that still refer to a "currency calculator" instead of the new currency type. It might be nice to update these to keep the terminology consistent.
openmeter/billing/charges/flatfee/charge.go#L186-L188: Update the error message to"building currency: %w".openmeter/billing/charges/flatfee/service/linemapper.go#L18-L20: Update the error message to"building currency: %w".openmeter/billing/charges/flatfee/service/realizations/credittheninvoice.go#L73-L75: Update the error message to"building currency: %w".openmeter/billing/charges/flatfee/service/realizations/credittheninvoice.go#L261-L263: Update the error message to"building currency: %w".openmeter/billing/charges/usagebased/service/linemapper.go#L131-L133: Update the error message to"building currency: %w".openmeter/billing/charges/usagebased/service/linemapper.go#L198-L200: Update the parameter name tocurrencyand the error message to"currency is required".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/flatfee/charge.go` around lines 186 - 188, Update stale currency terminology in the listed error paths: in openmeter/billing/charges/flatfee/charge.go:186-188, openmeter/billing/charges/flatfee/service/linemapper.go:18-20, both sites in openmeter/billing/charges/flatfee/service/realizations/credittheninvoice.go:73-75 and 261-263, and openmeter/billing/charges/usagebased/service/linemapper.go:131-133, change the wrapped error prefix to “building currency: %w”. In openmeter/billing/charges/usagebased/service/linemapper.go:198-200, rename the parameter to currency and update the required-value error to “currency is required”.openmeter/ledger/chargeadapter/usagebased.go (1)
289-296: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the error message to reflect the new builder.
Since the code now dynamically builds a currency instead of fetching a calculator, consider tweaking the error context to accurately reflect the action being performed.
💡 Suggested tweak
currency, err := currencyx.NewCurrencyBuilder(currencyx.CurrencyTypeFiat). WithCode(intent.GetCurrency()). Build() if err != nil { - return nil, fmt.Errorf("get currency calculator: %w", err) + return nil, fmt.Errorf("build currency: %w", err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/ledger/chargeadapter/usagebased.go` around lines 289 - 296, Update the error context in the currency construction block of the charge calculation flow to describe building or creating the currency rather than fetching a currency calculator. Keep the existing wrapped error and validation behavior unchanged.openmeter/billing/charges/usagebased/service/run/correct.go (1)
51-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify redundant nil checks.
Hey! Since we're returning early just above if
i.CurrencyCalculatorisnil, we don't need to wrap the next validation step inif i.CurrencyCalculator != nilagain. We can simplify both of these methods nicely!
openmeter/billing/charges/usagebased/service/run/correct.go#L51-L59: remove theif i.CurrencyCalculator != nilwrapper inReconcileCreditRealizationsInput.Validate().openmeter/billing/charges/usagebased/service/run/correct.go#L165-L173: remove theif i.CurrencyCalculator != nilwrapper inCorrectAllCreditRealizationsInput.Validate().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/usagebased/service/run/correct.go` around lines 51 - 59, Remove the redundant nil-check wrappers in ReconcileCreditRealizationsInput.Validate() at openmeter/billing/charges/usagebased/service/run/correct.go:51-59 and CorrectAllCreditRealizationsInput.Validate() at openmeter/billing/charges/usagebased/service/run/correct.go:165-173. After each existing nil early return, call CurrencyCalculator.Validate() directly while preserving the current wrapped error behavior.openmeter/billing/rating/service/rate/tieredgraduated.go (1)
133-135: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the currency presence check.
Hey! Since the
||operator short-circuits in Go, if we make it past thei.Currency == nilcheck, we already knowi.Currencyis not nil. We can safely drop the redundanti.Currency != nil &&piece to make this a bit cleaner!♻️ Proposed fix
- if i.Currency == nil || i.Currency != nil && i.Currency.Details().Code == "" { + if i.Currency == nil || i.Currency.Details().Code == "" { return fmt.Errorf("currency is required") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/rating/service/rate/tieredgraduated.go` around lines 133 - 135, In the currency validation condition, simplify the redundant nil check after the short-circuiting `i.Currency == nil` check. Update the validation in the surrounding rate logic to directly test `i.Currency.Details().Code == ""` for the non-nil case, preserving the existing “currency is required” error behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/v3/handlers/customers/credits/convert.go`:
- Around line 106-112: Update the currency construction used before
purchaseAmount rounding to use the settlement currency (`inv.Currency` or
`ext.Currency`) that denominates `CreditAmount × CostBasis`, rather than
`charge.Intent.Currency`. Apply the same change to both currency-building
occurrences so `RoundToPrecision` uses the API-returned currency’s precision and
supports custom credit currencies.
In `@openmeter/app/stripe/calculator.go`:
- Around line 43-52: Update StripeCalculator.FormatAmount to delegate directly
to c.currency.FormatAmount with the original alpacadecimal.Decimal, removing the
integer special case and Float64 conversion. Preserve exact formatting and avoid
ignoring conversion accuracy.
In `@openmeter/billing/charges/creditpurchase/charge.go`:
- Around line 164-169: Update the normalization flow around the currency builder
and CreditAmount assignment to resolve a currencyx.Currency for both standard
and custom credit currencies, preserving custom precision instead of silently
skipping rounding. Thread the resolved currency into the normalization
operation, and explicitly propagate or return currency-resolution errors rather
than ignoring Build failures.
In `@openmeter/billing/charges/usagebased/service/run/preview.go`:
- Around line 77-84: Update validation methods to accumulate all errors in errs
and return models.NewNillableGenericValidationError(errors.Join(errs...))
instead of returning early. In
openmeter/billing/charges/usagebased/service/run/preview.go (77-84) and
service/statemachine.go (82-89), append CurrencyCalculator validation failures
and use else-if; statemachine.go must wrap the final return. Apply the same errs
accumulation and wrapped return in
openmeter/billing/charges/flatfee/service/realizations/creditsonly.go (33-40),
service/run/create.go (85-92), and service/run/credits.go (78-85). In
openmeter/billing/charges/usagebased/handler.go (82-89), replace the redundant
currency != nil check with an else-if chained to the nil check.
In `@openmeter/ledger/chargeadapter/creditpurchase.go`:
- Around line 463-468: Update the caller around the currency resolution and
advanceAttributions invocation to pass the already-resolved currencyx.Currency
instead of rebuilding charge.Intent.Currency as CurrencyTypeFiat. Remove the
redundant fiat-only construction and preserve the existing error handling for
resolving the currency, so custom credit currency codes reach allocation
successfully.
In `@openmeter/ledger/transactions/codes.go`:
- Line 28: Rename the legacy currency conversion constant from
legacyTemplateCodeConvertCurrency to
transactionTemplateCodeConvertCurrencyLegacy, preserving its
TransactionTemplateCode type and string value.
In `@openmeter/ledger/transactions/fx.go`:
- Around line 27-60: The Validate methods must aggregate all validation failures
instead of returning early, then return
models.NewNillableGenericValidationError(errors.Join(errs...)). In
openmeter/ledger/transactions/fx.go, update ConvertCurrencyTemplate.Validate to
append each prefixed amount, cost-basis, and currency error while preserving all
validation checks. In openmeter/ledger/transactions/customer.go, update the
source validation at lines 46-48 to append its prefixed error to the existing
aggregate; no other validation behavior should change.
- Around line 32-45: The FX transaction validation currently checks positivity
but not consistency between amounts and cost basis. In
openmeter/ledger/transactions/fx.go lines 32-45, update the validation around
ValidateTransactionAmount and ValidateCostBasis to verify SourceAmount equals
TargetAmount multiplied by CostBasis using the applicable currency rounding
rules. In openmeter/ledger/transactions/fx_test.go lines 72-156, add a positive
but inconsistent amount/cost-basis case and assert that transaction validation
rejects it.
In `@pkg/currencyx/allocation.go`:
- Around line 249-263: Update the currency validation flow to stop dereferencing
a nil currency after recording the “currency is required” error. Guard
currency.Validate and currency.IsRoundedToPrecision behind a non-nil check while
preserving amount validation and existing error accumulation.
In `@pkg/currencyx/currency.go`:
- Around line 176-193: Update both currency Validate methods, including
FiatCurrency.Validate and the validator covering the referenced later range, to
wrap each validation failure with its field context before adding it to errs.
Use the appropriate fields code, name, and precision, then preserve
errors.Join(errs...) inside models.NewNillableGenericValidationError so all
failures remain collected with structured attribution.
---
Outside diff comments:
In `@openmeter/billing/rating/service/rate/types.go`:
- Around line 32-46: Update PricerCalculateInput.Validate to collect every
missing-field validation error in an errs slice instead of returning
immediately; append failures for CurrencyCalculator,
FullProgressivelyBilledServicePeriod, and StandardLineAccessor, then return
models.NewNillableGenericValidationError(errors.Join(errs...)) so nil is
returned when there are no errors.
In `@openmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.go`:
- Around line 106-108: Update the validation logic in the ApplyInput
reconciliation flow to check whether i.Currency is nil before calling
Currency.Validate(); when nil, append an appropriate currency validation error
instead of dereferencing it, while preserving the existing wrapped error
behavior for non-nil currencies.
---
Nitpick comments:
In `@openmeter/billing/charges/flatfee/charge.go`:
- Around line 186-188: Update stale currency terminology in the listed error
paths: in openmeter/billing/charges/flatfee/charge.go:186-188,
openmeter/billing/charges/flatfee/service/linemapper.go:18-20, both sites in
openmeter/billing/charges/flatfee/service/realizations/credittheninvoice.go:73-75
and 261-263, and
openmeter/billing/charges/usagebased/service/linemapper.go:131-133, change the
wrapped error prefix to “building currency: %w”. In
openmeter/billing/charges/usagebased/service/linemapper.go:198-200, rename the
parameter to currency and update the required-value error to “currency is
required”.
In `@openmeter/billing/charges/usagebased/service/run/correct.go`:
- Around line 51-59: Remove the redundant nil-check wrappers in
ReconcileCreditRealizationsInput.Validate() at
openmeter/billing/charges/usagebased/service/run/correct.go:51-59 and
CorrectAllCreditRealizationsInput.Validate() at
openmeter/billing/charges/usagebased/service/run/correct.go:165-173. After each
existing nil early return, call CurrencyCalculator.Validate() directly while
preserving the current wrapped error behavior.
In `@openmeter/billing/rating/service/rate/tieredgraduated.go`:
- Around line 133-135: In the currency validation condition, simplify the
redundant nil check after the short-circuiting `i.Currency == nil` check. Update
the validation in the surrounding rate logic to directly test
`i.Currency.Details().Code == ""` for the non-nil case, preserving the existing
“currency is required” error behavior.
In `@openmeter/ledger/chargeadapter/usagebased.go`:
- Around line 289-296: Update the error context in the currency construction
block of the charge calculation flow to describe building or creating the
currency rather than fetching a currency calculator. Keep the existing wrapped
error and validation behavior unchanged.
In `@pkg/currencyx/currency_test.go`:
- Around line 244-357: Extend TestFormatAmount with custom-currency cases that
configure WithDecimalMark and WithThousandsSeparator and assert both separators
are reflected in formatted output. Add negative-value cases covering RoundUp and
RoundDown, asserting their directional rounding behavior; keep the existing
table-driven setup and currency construction flow.
🪄 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: 963426f0-fe72-436f-8c33-be299383ac22
⛔ 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 (87)
.agents/skills/currencyx/SKILL.md.agents/skills/currencyx/agents/openai.yamlapi/v3/handlers/customers/credits/convert.goopenmeter/app/stripe/calculator.goopenmeter/billing/charges/creditpurchase/charge.goopenmeter/billing/charges/creditpurchase/service/create.goopenmeter/billing/charges/creditpurchase/service/external_test.goopenmeter/billing/charges/flatfee/charge.goopenmeter/billing/charges/flatfee/handler.goopenmeter/billing/charges/flatfee/service/creditsonly.goopenmeter/billing/charges/flatfee/service/lineengine.goopenmeter/billing/charges/flatfee/service/linemapper.goopenmeter/billing/charges/flatfee/service/realizations/correct.goopenmeter/billing/charges/flatfee/service/realizations/creditsonly.goopenmeter/billing/charges/flatfee/service/realizations/credittheninvoice.goopenmeter/billing/charges/flatfee/service/realizations/preview.goopenmeter/billing/charges/models/creditrealization/correction.goopenmeter/billing/charges/models/creditrealization/correction_test.goopenmeter/billing/charges/models/creditrealization/realizations.goopenmeter/billing/charges/usagebased/handler.goopenmeter/billing/charges/usagebased/service/creditsonly_test.goopenmeter/billing/charges/usagebased/service/lineengine.goopenmeter/billing/charges/usagebased/service/linemapper.goopenmeter/billing/charges/usagebased/service/run/correct.goopenmeter/billing/charges/usagebased/service/run/create.goopenmeter/billing/charges/usagebased/service/run/credits.goopenmeter/billing/charges/usagebased/service/run/preview.goopenmeter/billing/charges/usagebased/service/statemachine.goopenmeter/billing/charges/usagebased/service/triggers.goopenmeter/billing/invoicedetailedline.goopenmeter/billing/invoicedetailedline_test.goopenmeter/billing/invoicelinediscount.goopenmeter/billing/models/creditsapplied/model.goopenmeter/billing/models/totals/model.goopenmeter/billing/models/totals/model_test.goopenmeter/billing/rating/detailedline.goopenmeter/billing/rating/detailedline_test.goopenmeter/billing/rating/service/detailedline.goopenmeter/billing/rating/service/mutator/credits_test.goopenmeter/billing/rating/service/mutator/discountpercentage.goopenmeter/billing/rating/service/rate/tieredgraduated.goopenmeter/billing/rating/service/rate/tieredgraduated_test.goopenmeter/billing/rating/service/rate/types.goopenmeter/billing/worker/subscriptionsync/service/reconcile.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchcharge_test.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchchargeflatfee.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchchargeusagebased.goopenmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.goopenmeter/billing/worker/subscriptionsync/service/sync.goopenmeter/billing/worker/subscriptionsync/service/targetstate/targetstate.goopenmeter/billing/worker/subscriptionsync/service/targetstate/targetstateitem.goopenmeter/ent/schema/ledger_account.goopenmeter/ledger/account/adapter/subaccount.goopenmeter/ledger/accounts.goopenmeter/ledger/accounts_test.goopenmeter/ledger/chargeadapter/creditpurchase.goopenmeter/ledger/chargeadapter/flatfee.goopenmeter/ledger/chargeadapter/flatfee_test.goopenmeter/ledger/chargeadapter/usagebased.goopenmeter/ledger/chargeadapter/usagebased_test.goopenmeter/ledger/collector/correct_test.goopenmeter/ledger/historical/adapter/sumentries_query.goopenmeter/ledger/ledger_fx_test.goopenmeter/ledger/primitives.goopenmeter/ledger/routing.goopenmeter/ledger/routing_test.goopenmeter/ledger/routingrules/defaults.goopenmeter/ledger/transactions/accrual.goopenmeter/ledger/transactions/codes.goopenmeter/ledger/transactions/correction_leg.goopenmeter/ledger/transactions/customer.goopenmeter/ledger/transactions/fx.goopenmeter/ledger/transactions/fx_test.goopenmeter/ledger/validations.goopenmeter/ledger/validations_test.gopkg/currencyx/allocation.gopkg/currencyx/allocation_test.gopkg/currencyx/code.gopkg/currencyx/costbasis.gopkg/currencyx/currency.gopkg/currencyx/currency_test.gopkg/currencyx/fiat.gopkg/currencyx/fiat_test.gopkg/currencyx/validation.gotest/app/stripe/invoice_credits_test.gotools/migrate/migrations/20260716143421_add_ledger_sub_account_source.down.sqltools/migrate/migrations/20260716143421_add_ledger_sub_account_source.up.sql
💤 Files with no reviewable changes (6)
- pkg/currencyx/fiat_test.go
- openmeter/ledger/ledger_fx_test.go
- pkg/currencyx/validation.go
- .agents/skills/currencyx/SKILL.md
- .agents/skills/currencyx/agents/openai.yaml
- pkg/currencyx/fiat.go
27b9295 to
df5d509
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/ledger/account/adapter/subaccount.go (1)
159-170: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBump slice capacity to 9.
Hey there! With the new
Sourcefield added, there are now up to 9 possible predicates appended in this block. Bumping the capacity up to 9 is a tiny optimization that will save a reallocation when a route filter has all its fields present.✨ Proposed fix
- routePredicates := make([]predicate.LedgerSubAccountRoute, 0, 8) + routePredicates := make([]predicate.LedgerSubAccountRoute, 0, 9) if normalizedRoute.Currency != "" { routePredicates = append(routePredicates, dbledgersubaccountroute.Currency(string(normalizedRoute.Currency))) } if normalizedRoute.Source.IsPresent() { source, _ := normalizedRoute.Source.Get() if source != nil { routePredicates = append(routePredicates, dbledgersubaccountroute.Source(*source)) } else { routePredicates = append(routePredicates, dbledgersubaccountroute.SourceIsNil()) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/ledger/account/adapter/subaccount.go` around lines 159 - 170, Increase the initial capacity of routePredicates in the subaccount route-filter construction from 8 to 9, keeping the existing predicate append logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@openmeter/ledger/account/adapter/subaccount.go`:
- Around line 159-170: Increase the initial capacity of routePredicates in the
subaccount route-filter construction from 8 to 9, keeping the existing predicate
append logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0cebdd96-1574-4487-8719-a4831f213358
⛔ 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 (20)
openmeter/ent/schema/ledger_account.goopenmeter/ledger/account/adapter/subaccount.goopenmeter/ledger/accounts.goopenmeter/ledger/accounts_test.goopenmeter/ledger/historical/adapter/sumentries_query.goopenmeter/ledger/ledger_fx_test.goopenmeter/ledger/primitives.goopenmeter/ledger/routing.goopenmeter/ledger/routing_test.goopenmeter/ledger/routingrules/defaults.goopenmeter/ledger/transactions/accrual.goopenmeter/ledger/transactions/codes.goopenmeter/ledger/transactions/correction_leg.goopenmeter/ledger/transactions/customer.goopenmeter/ledger/transactions/fx.goopenmeter/ledger/transactions/fx_test.goopenmeter/ledger/validations.goopenmeter/ledger/validations_test.gotools/migrate/migrations/20260716220228_add_ledger_sub_account_source.down.sqltools/migrate/migrations/20260716220228_add_ledger_sub_account_source.up.sql
💤 Files with no reviewable changes (1)
- openmeter/ledger/ledger_fx_test.go
🚧 Files skipped from review as they are similar to previous changes (15)
- openmeter/ledger/routingrules/defaults.go
- openmeter/ledger/transactions/accrual.go
- openmeter/ent/schema/ledger_account.go
- openmeter/ledger/transactions/codes.go
- openmeter/ledger/transactions/customer.go
- openmeter/ledger/accounts.go
- openmeter/ledger/primitives.go
- openmeter/ledger/historical/adapter/sumentries_query.go
- openmeter/ledger/validations.go
- openmeter/ledger/routing_test.go
- openmeter/ledger/validations_test.go
- openmeter/ledger/transactions/correction_leg.go
- openmeter/ledger/transactions/fx_test.go
- openmeter/ledger/routing.go
- openmeter/ledger/transactions/fx.go
|
✅ Action performedReview finished.
|
9101f5f to
dc13ad1
Compare
dc13ad1 to
562a50d
Compare
mark-vass-konghq
changed the title
ff20ce3 to
ff473c0
Compare
GAlexIHU
left a comment
GAlexIHU
left a comment
There was a problem hiding this comment.
please get rid "idempotency"
The ledger should correctly book whatever is requested, not invent business mechanisms. Why ever would 2 identical transactions be invalid in sequence? Imagine conceptually how bad it would be if you couldn't buy the same thing twice IRL
| field.String("routing_key").Immutable(), | ||
| // Literal routing values (denormalized from routing_key for query filtering; not FKs). | ||
| field.String("currency").Immutable(), | ||
| field.String("exchange_source_currency"). |
There was a problem hiding this comment.
nit: lets not call exchange_source_currency, the "exchange" only makes sense for CC and even then it's not necessarily an exchange, let's call it cost_basis_currency or revenue_source_currency or source_fiat revenue_fiat, something of the like... (should be meaningful for FIAT as well)
| annotations, | ||
| inputs..., | ||
| )) | ||
| transactionGroupInput := transactions.WithIdempotencyKey( |
There was a problem hiding this comment.
this is not needed & i think its conceptually dangerous, remove it please
- groups roughly map to business events
- charges lifecycle defines/protects payment flow correctness, its not our business
- conceptually, why would the ledger decline transactions? thats a business problem
| @@ -0,0 +1,190 @@ | |||
| package historical | |||
There was a problem hiding this comment.
why did you think this is needed?
| ledger.AnnotationBreakagePlanID, | ||
| } | ||
|
|
||
| type transactionGroupFingerprintPayload struct { |
There was a problem hiding this comment.
i think your fingerprinting only works if the adapter is invoked with the exact same input. that scenario wouldn't even happen in production, when it would be called with semantically same inputs we'd just receive a conflict. this is also a good sign as to why this ledger lvl idempotency doesnt make sense... also, why would two identical groups be invalid (conceptually from ledger side)?
| require.NotEmpty(t, authRef.TransactionGroupID) | ||
|
|
||
| // and: the same authorization event is replayed. | ||
| replayedAuthRef, err := env.handler.OnCreditPurchasePaymentAuthorized(t.Context(), authorizationInput) |
There was a problem hiding this comment.
this is not a meaningful testcase, what made you concerned with replay?
| field.String("idempotency_scope"). | ||
| Optional(). | ||
| Nillable(). | ||
| Immutable(), | ||
| field.String("idempotency_key"). | ||
| Optional(). | ||
| Nillable(). | ||
| Immutable(). | ||
| MaxLen(256), | ||
| field.String("input_fingerprint"). |
There was a problem hiding this comment.
please get rid of these (see other comments for justifications)
| // Re-denominate what the customer owes from the custom-currency IOU | ||
| // into the fiat amount actually being paid, before authorizing payment | ||
| // against it. Authorize/Settle only ever move real (fiat) money. | ||
| templates = append(templates, transactions.ConvertCurrencyTemplate{ |
There was a problem hiding this comment.
conversion should preserve everything present on the receivable account. you're dropping feature filters here, e.g.
# 100 ACME credits for api-calls costing $50:
+100 ACME receivable [no feature] ← conversion
-100 ACME receivable [api-calls] ← purchase
+$50 USD receivable [api-calls] ← authorization
-$50 USD receivable [no feature] ← conversion
647a48d to
9df4341
Compare
5437aab to
14ba071
Compare
9df4341 to
9df8492
Compare
798007b to
cb77977
Compare
| amount := creditAmount. | ||
| Mul(costBasis). | ||
| RoundBank(int32(fiatCurrency.Details().Precision)) |
There was a problem hiding this comment.
The ledger now uses banker's rounding, but the external-payment authorization and invoice gathering paths still calculate the same fiat amount with RoundToPrecision, which rounds midpoints away from zero. For a custom-currency purchase worth USD 1.005, those paths record 1.01 while this branch authorizes and settles 1.00 in the ledger. Apply the same rounding rule when creating the payment and invoice amounts so their records remain consistent with the ledger.
Prompt To Fix With AI
This is a comment left during a code review.
Path: openmeter/ledger/chargeadapter/creditpurchase.go
Line: 489-491
Comment:
**Settlement Records Still Diverge**
The ledger now uses banker's rounding, but the external-payment authorization and invoice gathering paths still calculate the same fiat amount with `RoundToPrecision`, which rounds midpoints away from zero. For a custom-currency purchase worth USD `1.005`, those paths record `1.01` while this branch authorizes and settles `1.00` in the ledger. Apply the same rounding rule when creating the payment and invoice amounts so their records remain consistent with the ledger.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
i think we should simplify the route currency representation before merging this. right now we already have the right domain abstraction for this: type Route struct {
Currency currencies.CurrencyReference
CostBasis *alpacadecimal.Decimal
CostBasisCurrency *currencyx.Code // fiat only
// other dimensions...
}a hydrated reference already contains everything the ledger needs:
the currencies pkg could own a stable serialization for ledger dimensions: func (r CurrencyReference) MarshalText() ([]byte, error)
func ParseCurrencyReference([]byte) (CurrencyReference, error)roughly: a hydrated ref serializes into the ledger dimension. parsing that value reconstructs a hydrated ref from the embedded snapshot. no currency service lookup, lifecycle loading, or cost-basis loading involved. this would let us keep one the result is:
the only related cleanup is that |
| // rounding difference the original booking's precision left behind. Partial | ||
| // corrections recompute the proportional source amount from the supplied | ||
| // cost basis. | ||
| func (t ConvertCurrencyTemplate) correct(scope CorrectionInput) ([]ledger.TransactionInput, error) { |
There was a problem hiding this comment.
i don't think we should implement this yet. credit purchases currently have no refund/payment-correction flow, so production code cannot reach it with the required exact fiat amount.
lets keep it explicitly unsupported until the charge layer provides both exact amounts.
| annotations := chargeAnnotationsForCreditPurchaseCharge(charge) | ||
| featureFilters := charge.Intent.FeatureFilters.Normalize() | ||
|
|
||
| settlementAmount, settlementCurrency, err := settlementPaymentAmount(charge.Intent.Currency, charge.Intent.Settlement, charge.Intent.CreditAmount, costBasis) |
There was a problem hiding this comment.
can we pass the exact FiatAmount through PaymentEventInput instead of recalculating it here?
charges already calculates and persists this value for both external and invoice settlements. the ledger should book that same amount so rounding has one owner and the payment realization always matches the ledger.
| func (i GetBalanceServiceInput) bookedRoute() ledger.RouteFilter { | ||
| route := i.featureRoute() | ||
| route.Currency = i.Currency | ||
| route.Currency = currencies.NewCurrencyReference(i.Currency) |
There was a problem hiding this comment.
can we add an explicit fiat-only guard here?
lets keep custom currency support in customerbalance out of scope for this PR
|
Too many files changed for review. ( Bypass the limit by tagging |
GAlexIHU
left a comment
GAlexIHU
left a comment
There was a problem hiding this comment.
Nice job! Lets fix these two small ones and then it should be good to merge
| fiatAmount := charge.Intent.CreditAmount | ||
| if charge.Intent.Currency.IsCustom() { | ||
| fiatAmount = fiatCurrency.RoundToPrecision( | ||
| charge.Intent.CreditAmount.Mul(externalSettlement.CostBasis), | ||
| ) | ||
| } |
There was a problem hiding this comment.
no, costbasis is still meaningful for fiat currencies, now FIAT purchases disregard costbasis, just remove the IsCustom branching
|
|
||
| serialized, err := reference.MarshalText() | ||
| require.NoError(t, err) | ||
| require.Equal(t, "custom:v1:CREDITS:"+custom.ID+":2", string(serialized)) |
There was a problem hiding this comment.
char ":" is unfortunately allowed for custom currency keying, lets use | as the delimiter, that is guarded against
69b7ec7 to
f99c388
Compare
| // GetCurrency returns the fiat currency real money settles in. Promotional | ||
| // settlements never move real money, so they return an empty currency rather | ||
| // than an error. | ||
| func (s Settlement) GetCurrency() (currencyx.FiatCode, error) { |
There was a problem hiding this comment.
nit: shouldn't this return *currencyx.FiatCode, error instead? (using lo.ToPtr)
| // NewCustomCurrency builds a custom currency value for tests that only need | ||
| // the resolved calculator (code, precision) and a stable fixture managed ID, | ||
| // not a persisted custom_currencies row. | ||
| func NewCustomCurrency[T ~string](t testing.TB, code T, precision uint32) currencies.Currency { |
There was a problem hiding this comment.
why do we have a subpackage for these two methods? also why's that subpackage called currency (it can get unnecessarily confusing)... can these live in the parent testutils please?
| }) | ||
| } | ||
|
|
||
| func ParseCurrencyReference(value []byte) (CurrencyReference, error) { |
There was a problem hiding this comment.
very very nit: this convenience is nice, but as we have encoding.TextMarshaller it could we nice if we implemented encoding.TextUnmarshaller and this just wrapped that (still this API is nicer at place of use)
we don't do XML, v3 AIP filters honor textmarshallers, there wouldn't be other interplay (+ we already implement marshaller)
2 participants
Summary
This PR adds the ledger foundation for custom-currency
credit_onlyflows.Custom credits can now be issued, allocated, corrected, and routed without
applying fiat precision or coalescing different managed currencies that share
the same code.
What changed
(
CurrencyReference/ managed custom-currency ID).credits.
different managed IDs.
credit-void ledger paths.
and passed the exact
FiatAmountthrough lifecycle events, so payment recordsand ledger entries use the same amount without ledger-side recalculation.
Credit-only behavior
For a custom-currency
credit_onlycharge:PR.
Tests
Added and updated coverage for:
authorization, and settlement flows;