feat(ledger): add credit_then_invoice custom currency fx support - #4854
Open
mark-vass-konghq wants to merge 4 commits into
Open
mark-vass-konghq wants to merge 4 commits into
mark-vass-konghq wants to merge 4 commits into
Conversation
mark-vass-konghq
added
release-note/feature
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCustom-currency overages now create fiat receivables through atomic ledger transactions. Payment flows use invoice currency and persisted cost basis. Lineage stores managed currency identity, and earnings recognition skips custom currencies. ChangesCustom currency identity and lineage
Charge adapter settlement
Earnings recognition
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
mark-vass-konghq
force-pushed
the
feat/add-cc-fx-support-to-ledger-credit_then_invoice
branch
from
August 4, 2026 16:15
827444d to
2d176f3
Compare
mark-vass-konghq
marked this pull request as ready for review
August 4, 2026 16:39
![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: 2
🧹 Nitpick comments (3)
openmeter/ledger/chargeadapter/flatfee_customcurrency_test.go (1)
59-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider
InexactFloat64()withrequire.Equalfor the balance assertions.These amounts (0, 10, 40, -10) are well within float precision.
require.True(t, x.Equal(y))prints onlyfalseon failure, so a wrong balance gives no numbers to work from. Some calls here add a"got %s"message, but most balance assertions do not. The same pattern repeats at lines 223-224, 240-241, and 246-248.♻️ Example for the FBO and accrued assertions
- require.True(t, env.sumBalance(t, fboSubAccount).Equal(alpacadecimal.Zero)) - require.True(t, env.sumBalance(t, env.customOpenReceivableSubAccountForFlatFee(t, customCurrencyIdentity, &settlementCurrency, costBasis)).Equal(alpacadecimal.Zero)) + require.Equal(t, 0.0, env.sumBalance(t, fboSubAccount).InexactFloat64()) + require.Equal(t, 0.0, env.sumBalance(t, env.customOpenReceivableSubAccountForFlatFee(t, customCurrencyIdentity, &settlementCurrency, costBasis)).InexactFloat64()) // The consumed amount is accrued natively, preserving cost basis and fiat provenance. accruedSubAccount := env.customAccruedSubAccountForFlatFee(t, customCurrencyIdentity, &settlementCurrency, &costBasis) - require.True(t, env.sumBalance(t, accruedSubAccount).Equal(alpacadecimal.NewFromInt(40))) + require.Equal(t, 40.0, env.sumBalance(t, accruedSubAccount).InexactFloat64())As per coding guidelines: "When precision permits, compare
alpacadecimal.DecimalthroughInexactFloat64()withrequire.Equal; inline one-off expected balances."🤖 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/flatfee_customcurrency_test.go` around lines 59 - 71, Update the balance assertions in the affected test, including the repeated assertions around the FBO, accrued, receivable, and brokerage checks, to compare each balance’s InexactFloat64() result with an inline expected float using require.Equal. Preserve the existing expected amounts and apply the same diagnostic-friendly pattern to the referenced later assertions.Source: Coding guidelines
openmeter/ledger/chargeadapter/flatfee.go (1)
430-448: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOne invoice cost-basis rule, copied into two adapters. Both helpers encode the same domain rule: fiat charges settle at par, and custom-currency charges settle at the persisted
ResolvedCostBasis. The accrual leg and the payment leg must land on the same ledger route, so the two copies have to stay identical forever. Extracting the rule once removes that coupling risk.
openmeter/ledger/chargeadapter/flatfee.go#L430-L448: replaceflatFeeInvoiceCostBasiswith a call to a shared helper that takes the charge currency and the resolved cost-basis state.openmeter/ledger/chargeadapter/usagebased.go#L419-L437: replaceusageBasedInvoiceCostBasiswith a call to the same shared helper.🤖 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/flatfee.go` around lines 430 - 448, Extract the duplicated invoice cost-basis rule from flatFeeInvoiceCostBasis in openmeter/ledger/chargeadapter/flatfee.go:430-448 and usageBasedInvoiceCostBasis in openmeter/ledger/chargeadapter/usagebased.go:419-437 into one shared helper accepting the charge currency and resolved cost-basis state. Update both adapters to call it, preserving par settlement for fiat, persisted ResolvedCostBasis for custom currencies, and the existing missing-cost-basis error.openmeter/billing/charges/service/base_test.go (1)
460-467: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInline this trivial test wrapper.
newFiatCurrencyonly invokescurrencyx.NewFiatCurrencyand asserts that it did not fail. Inline those operations at the test site and remove this helper.As per coding guidelines, “Do not extract trivial or single-use helpers …; inline pass-through wrappers,” and do not add local
must-style wrappers.Proposed change
--- a/openmeter/billing/charges/service/advance_test.go +++ b/openmeter/billing/charges/service/advance_test.go @@ - costBasisIntent := costbasis.NewIntent(costbasis.ManualIntent{ - FiatCurrency: s.newFiatCurrency("USD"), + fiatCurrency, err := currencyx.NewFiatCurrency("USD") + s.Require().NoError(err) + + costBasisIntent := costbasis.NewIntent(costbasis.ManualIntent{ + FiatCurrency: fiatCurrency, @@ - _, err := s.Charges.usageBasedService.Create(ctx, usagebased.CreateInput{ + _, err = s.Charges.usageBasedService.Create(ctx, usagebased.CreateInput{ --- a/openmeter/billing/charges/service/base_test.go +++ b/openmeter/billing/charges/service/base_test.go @@ -func (s *BaseSuite) newFiatCurrency(code currencyx.Code) *currencyx.FiatCurrency { - s.T().Helper() - - fiatCurrency, err := currencyx.NewFiatCurrency(code) - s.Require().NoError(err) - - return fiatCurrency -}🤖 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/service/base_test.go` around lines 460 - 467, Remove the newFiatCurrency helper method from BaseSuite and inline its implementation at all call sites within the test file. At each location where newFiatCurrency is invoked with a code parameter, replace it with a direct call to currencyx.NewFiatCurrency followed by s.Require().NoError to assert the error handling inline, then assign the result to the variable as before.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openmeter/billing/charges/service/advance_test.go`:
- Around line 353-378: Update the costBasisIntent fixture in the test around
newUsageBasedCharge to use EUR for costBasisIntent.FiatCurrency instead of USD,
while keeping the expected result currency as USD. Leave the charge setup and
assertions otherwise unchanged.
In `@openmeter/ledger/chargeadapter/flatfee_test.go`:
- Around line 1205-1239: Ensure both transactionGroupEntries helpers assert that
the queried ledger entries are non-empty before returning: add
require.NotEmpty(t, entries) in flatfee_test.go at lines 1205-1239 and
usagebased_test.go at lines 906-940, after the query error assertion and before
the return.
---
Nitpick comments:
In `@openmeter/billing/charges/service/base_test.go`:
- Around line 460-467: Remove the newFiatCurrency helper method from BaseSuite
and inline its implementation at all call sites within the test file. At each
location where newFiatCurrency is invoked with a code parameter, replace it with
a direct call to currencyx.NewFiatCurrency followed by s.Require().NoError to
assert the error handling inline, then assign the result to the variable as
before.
In `@openmeter/ledger/chargeadapter/flatfee_customcurrency_test.go`:
- Around line 59-71: Update the balance assertions in the affected test,
including the repeated assertions around the FBO, accrued, receivable, and
brokerage checks, to compare each balance’s InexactFloat64() result with an
inline expected float using require.Equal. Preserve the existing expected
amounts and apply the same diagnostic-friendly pattern to the referenced later
assertions.
In `@openmeter/ledger/chargeadapter/flatfee.go`:
- Around line 430-448: Extract the duplicated invoice cost-basis rule from
flatFeeInvoiceCostBasis in openmeter/ledger/chargeadapter/flatfee.go:430-448 and
usageBasedInvoiceCostBasis in
openmeter/ledger/chargeadapter/usagebased.go:419-437 into one shared helper
accepting the charge currency and resolved cost-basis state. Update both
adapters to call it, preserving par settlement for fiat, persisted
ResolvedCostBasis for custom currencies, and the existing missing-cost-basis
error.
🪄 Autofix
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 Plus
Run ID: 6b427db5-7588-4c71-8bcd-0b6e16383741
⛔ Files ignored due to path filters (9)
openmeter/ent/db/creditrealizationlineage.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage/creditrealizationlineage.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage/where.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage_create.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage_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/**openmeter/ent/db/runtime.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (25)
openmeter/billing/charges/README.mdopenmeter/billing/charges/flatfee/handler.goopenmeter/billing/charges/lineage/adapter/lineage.goopenmeter/billing/charges/lineage/lineage_test.goopenmeter/billing/charges/lineage/service.goopenmeter/billing/charges/lineage/service/service.goopenmeter/billing/charges/service/advance.goopenmeter/billing/charges/service/advance_test.goopenmeter/billing/charges/service/base_test.goopenmeter/billing/charges/service/lineage_test.goopenmeter/billing/charges/service/usagebased_costbasis_test.goopenmeter/billing/charges/usagebased/handler.goopenmeter/ent/schema/creditrealizationlineage.goopenmeter/ledger/chargeadapter/flatfee.goopenmeter/ledger/chargeadapter/flatfee_customcurrency_test.goopenmeter/ledger/chargeadapter/flatfee_test.goopenmeter/ledger/chargeadapter/helpers.goopenmeter/ledger/chargeadapter/usagebased.goopenmeter/ledger/chargeadapter/usagebased_customcurrency_test.goopenmeter/ledger/chargeadapter/usagebased_test.goopenmeter/ledger/recognizer/recognize.goopenmeter/ledger/recognizer/service_test.goopenmeter/ledger/transactions/accrual.gotools/migrate/migrations/20260804095904_add_lineage_custom_currency_identity.down.sqltools/migrate/migrations/20260804095904_add_lineage_custom_currency_identity.up.sql
💤 Files with no reviewable changes (1)
- openmeter/billing/charges/service/usagebased_costbasis_test.go
mark-vass-konghq
force-pushed
the
feat/add-cc-fx-support-to-ledger-credit_then_invoice
branch
from
August 5, 2026 09:12
16f9347 to
180ae17
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
mark-vass-konghq
force-pushed
the
feat/add-cc-fx-support-to-ledger-credit_then_invoice
branch
from
August 5, 2026 09:12
180ae17 to
6a2bacf
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tools/migrate/migrations/20260804095904_add_lineage_custom_currency_identity.up.sql (1)
2-2: 🚀 Performance & Scalability | 🔵 TrivialVerify the migration lock window before merge.
Changing
currencywithALTER COLUMN TYPErequires anACCESS EXCLUSIVElock. On a large or busycredit_realization_lineagestable, concurrent reads and writes can wait during the migration. Check the table size and expected duration, then use a maintenance window or an additive rollout if the lock is not acceptable.🤖 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 `@tools/migrate/migrations/20260804095904_add_lineage_custom_currency_identity.up.sql` at line 2, Assess the lock impact of the ALTER TABLE operation in migration 20260804095904_add_lineage_custom_currency_identity, including the size and traffic of credit_realization_lineages and the expected ALTER COLUMN duration. Schedule execution during an approved maintenance window, or replace the direct currency type change with an additive rollout if the ACCESS EXCLUSIVE lock is not acceptable.Source: Linters/SAST tools
openmeter/ledger/chargeadapter/flatfee_test.go (1)
1227-1248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWorth hoisting
transactionAnnotationsintoIntegrationEnvtoo.This PR already moved the entries query into the shared harness as
TransactionGroupEntries. The annotations query is the same shape and appears to exist per test file (the usage-based custom-currency test callsenv.transactionAnnotationsas well), so moving it next toTransactionGroupEntrieswould finish the cleanup nicely.While you are there:
transactionBookedAtTimesat line 1217 assertsrequire.NotEmpty(t, transactions)but this helper does not. Callers are currently protected byrequire.ElementsMatchon three template codes, so nothing silently passes today, but matching the sibling helper keeps it safe for future callers.As per coding guidelines: "put suite-wide behavior in the shared harness instead of exposing per-test knobs."
♻️ Proposed assertion tweak (before the wider move)
All(t.Context()) require.NoError(t, err) + require.NotEmpty(t, transactions, "expected at least one ledger transaction for group") out := make([]models.Annotations, 0, len(transactions))🤖 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/flatfee_test.go` around lines 1227 - 1248, Move transactionAnnotations from flatFeeHandlerTestEnv into the shared IntegrationEnv alongside TransactionGroupEntries, updating callers—including usage-based custom-currency tests—to use the shared helper. Preserve its namespace/group filtering, ordering, and annotation collection behavior, and add require.NotEmpty on the queried transactions to match transactionBookedAtTimes.Source: Coding guidelines
openmeter/ledger/chargeadapter/usagebased_customcurrency_test.go (2)
58-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDecimal assertion style drifts between the two new custom-currency test files. Both files assert the same kinds of balances, but one uses
require.True(...Equal(...))and the other mostly usesInexactFloat64()withrequire.Equal. All the expected values here (10, 40, 60, -10) are safely representable, so theInexactFloat64form works everywhere and gives much clearer failure output.
openmeter/ledger/chargeadapter/usagebased_customcurrency_test.go#L58-L75: convert the balance and result assertions torequire.Equal(t, <float>, ....InexactFloat64()).openmeter/ledger/chargeadapter/flatfee_customcurrency_test.go#L108-L114: convert the accrued-entry amount assertion on line 110 to the same form so the file is uniform.As per coding guidelines: "When precision permits, compare
alpacadecimal.DecimalthroughInexactFloat64()withrequire.Equal; inline one-off expected balances."🤖 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_customcurrency_test.go` around lines 58 - 75, The decimal assertions in openmeter/ledger/chargeadapter/usagebased_customcurrency_test.go lines 58-75 should use require.Equal with InexactFloat64() and inline expected values for the result and balance checks, replacing Equal-based require.True assertions. Apply the same assertion style to the accrued-entry amount in openmeter/ledger/chargeadapter/flatfee_customcurrency_test.go lines 108-114; no other assertions require changes.Source: Coding guidelines
728-731: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
currencyparameter is now unused here.After the switch to
AccruedSubAccountForCurrency,currency currencyx.Codeno longer affects the result. Dropping it (and updating the two call sites) keeps the helper honest about what it needs.🤖 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_customcurrency_test.go` around lines 728 - 731, Remove the unused currencyx.Code parameter from usageBasedHandlerTestEnv.customUnknownAccruedSubAccountForUsageBased, then update both call sites to stop passing currency while preserving the existing AccruedSubAccountForCurrency arguments and behavior.openmeter/ledger/chargeadapter/flatfee_customcurrency_test.go (1)
186-205: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
given/when/thenintent comments for the lifecycle test.This one walks accrual → authorization → settlement, so a couple of short intent markers would make the phases easy to scan. The existing inline comments already carry most of the meaning, so this is only a polish item.
As per coding guidelines: "Begin non-trivial service or lifecycle subtests with concise
given,when, andthenintent comments."🤖 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/flatfee_customcurrency_test.go` around lines 186 - 205, Update TestOnFlatFeeCustomCurrencyPaymentLifecycle with concise given, when, and then intent comments marking the accrual, authorization, and settlement phases respectively. Keep the existing test logic and inline comments unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openmeter/billing/charges/README.md`:
- Line 162: Update the wording in the charges README so the phrase uses the
hyphenated form, changing the existing “end to end” text to “end-to-end” while
leaving the rest of the sentence unchanged.
---
Nitpick comments:
In `@openmeter/ledger/chargeadapter/flatfee_customcurrency_test.go`:
- Around line 186-205: Update TestOnFlatFeeCustomCurrencyPaymentLifecycle with
concise given, when, and then intent comments marking the accrual,
authorization, and settlement phases respectively. Keep the existing test logic
and inline comments unchanged.
In `@openmeter/ledger/chargeadapter/flatfee_test.go`:
- Around line 1227-1248: Move transactionAnnotations from flatFeeHandlerTestEnv
into the shared IntegrationEnv alongside TransactionGroupEntries, updating
callers—including usage-based custom-currency tests—to use the shared helper.
Preserve its namespace/group filtering, ordering, and annotation collection
behavior, and add require.NotEmpty on the queried transactions to match
transactionBookedAtTimes.
In `@openmeter/ledger/chargeadapter/usagebased_customcurrency_test.go`:
- Around line 58-75: The decimal assertions in
openmeter/ledger/chargeadapter/usagebased_customcurrency_test.go lines 58-75
should use require.Equal with InexactFloat64() and inline expected values for
the result and balance checks, replacing Equal-based require.True assertions.
Apply the same assertion style to the accrued-entry amount in
openmeter/ledger/chargeadapter/flatfee_customcurrency_test.go lines 108-114; no
other assertions require changes.
- Around line 728-731: Remove the unused currencyx.Code parameter from
usageBasedHandlerTestEnv.customUnknownAccruedSubAccountForUsageBased, then
update both call sites to stop passing currency while preserving the existing
AccruedSubAccountForCurrency arguments and behavior.
In
`@tools/migrate/migrations/20260804095904_add_lineage_custom_currency_identity.up.sql`:
- Line 2: Assess the lock impact of the ALTER TABLE operation in migration
20260804095904_add_lineage_custom_currency_identity, including the size and
traffic of credit_realization_lineages and the expected ALTER COLUMN duration.
Schedule execution during an approved maintenance window, or replace the direct
currency type change with an additive rollout if the ACCESS EXCLUSIVE lock is
not acceptable.
🪄 Autofix
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 Plus
Run ID: 81f48bcf-785e-4211-ab6d-12054d337de4
⛔ Files ignored due to path filters (9)
openmeter/ent/db/creditrealizationlineage.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage/creditrealizationlineage.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage/where.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage_create.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage_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/**openmeter/ent/db/runtime.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (29)
openmeter/billing/charges/README.mdopenmeter/billing/charges/flatfee/charge.goopenmeter/billing/charges/flatfee/handler.goopenmeter/billing/charges/lineage/adapter/lineage.goopenmeter/billing/charges/lineage/lineage_test.goopenmeter/billing/charges/lineage/service.goopenmeter/billing/charges/lineage/service/service.goopenmeter/billing/charges/service/advance.goopenmeter/billing/charges/service/advance_test.goopenmeter/billing/charges/service/base_test.goopenmeter/billing/charges/service/lineage_test.goopenmeter/billing/charges/service/usagebased_costbasis_test.goopenmeter/billing/charges/usagebased/charge.goopenmeter/billing/charges/usagebased/handler.goopenmeter/ent/schema/creditrealizationlineage.goopenmeter/ledger/chargeadapter/creditpurchase_test.goopenmeter/ledger/chargeadapter/flatfee.goopenmeter/ledger/chargeadapter/flatfee_customcurrency_test.goopenmeter/ledger/chargeadapter/flatfee_test.goopenmeter/ledger/chargeadapter/helpers.goopenmeter/ledger/chargeadapter/usagebased.goopenmeter/ledger/chargeadapter/usagebased_customcurrency_test.goopenmeter/ledger/chargeadapter/usagebased_test.goopenmeter/ledger/recognizer/recognize.goopenmeter/ledger/recognizer/service_test.goopenmeter/ledger/testutils/integration.goopenmeter/ledger/transactions/accrual.gotools/migrate/migrations/20260804095904_add_lineage_custom_currency_identity.down.sqltools/migrate/migrations/20260804095904_add_lineage_custom_currency_identity.up.sql
💤 Files with no reviewable changes (1)
- openmeter/billing/charges/service/usagebased_costbasis_test.go
🚧 Files skipped from review as they are similar to previous changes (16)
- openmeter/ent/schema/creditrealizationlineage.go
- openmeter/billing/charges/service/lineage_test.go
- openmeter/ledger/recognizer/service_test.go
- openmeter/billing/charges/lineage/service/service.go
- openmeter/billing/charges/lineage/lineage_test.go
- openmeter/billing/charges/service/advance.go
- openmeter/ledger/chargeadapter/usagebased_test.go
- openmeter/billing/charges/flatfee/handler.go
- openmeter/billing/charges/usagebased/handler.go
- openmeter/ledger/transactions/accrual.go
- openmeter/ledger/chargeadapter/flatfee.go
- openmeter/ledger/chargeadapter/usagebased.go
- openmeter/billing/charges/service/advance_test.go
- openmeter/billing/charges/lineage/adapter/lineage.go
- openmeter/ledger/recognizer/recognize.go
- openmeter/billing/charges/lineage/service.go
| balance queries, and historical migration. | ||
| Ledger-backed charge adapters implement this boundary directly: | ||
|
|
||
| - credit allocation and correction stay in the custom currency end to end |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the hyphenated form.
Change end to end to end-to-end on Line 162.
🧰 Tools
🪛 LanguageTool
[grammar] ~162-~162: Use a hyphen to join words.
Context: ...rrection stay in the custom currency end to end ([credit_only](#settlement-seman...
(QB_NEW_EN_HYPHEN)
🤖 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/README.md` at line 162, Update the wording in the
charges README so the phrase uses the hyphenated form, changing the existing
“end to end” text to “end-to-end” while leaving the rest of the sentence
unchanged.
Source: Linters/SAST tools
mark-vass-konghq
force-pushed
the
feat/add-cc-fx-support-to-ledger-credit_then_invoice
branch
from
August 5, 2026 11:03
6a2bacf to
4f9580a
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/migrate/migrations/20260804095904_add_lineage_custom_currency_identity.up.sql (1)
2-2: 🩺 Stability & Availability | 🔵 TrivialVerify the migration lock impact before deployment.
ALTER TABLE ... ALTER COLUMN ... TYPE ...takes anACCESS EXCLUSIVElock. On a busycredit_realization_lineagestable, lock acquisition can block reads and writes. Confirm that this widening is metadata-only for the supported PostgreSQL version and fits the migration window or lock-timeout policy.🤖 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 `@tools/migrate/migrations/20260804095904_add_lineage_custom_currency_identity.up.sql` at line 2, Review the migration statement for credit_realization_lineages before deployment: verify that widening currency to character varying(24) is metadata-only on the supported PostgreSQL version, assess its ACCESS EXCLUSIVE lock impact during the migration window, and apply the project’s established lock-timeout policy if required.Source: Linters/SAST tools
🤖 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
`@tools/migrate/migrations/20260804095904_add_lineage_custom_currency_identity.up.sql`:
- Line 2: Review the migration statement for credit_realization_lineages before
deployment: verify that widening currency to character varying(24) is
metadata-only on the supported PostgreSQL version, assess its ACCESS EXCLUSIVE
lock impact during the migration window, and apply the project’s established
lock-timeout policy if required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 25672f2f-5af8-442d-8d47-9f79a2ab12a8
⛔ Files ignored due to path filters (9)
openmeter/ent/db/creditrealizationlineage.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage/creditrealizationlineage.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage/where.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage_create.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage_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/**openmeter/ent/db/runtime.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (29)
openmeter/billing/charges/README.mdopenmeter/billing/charges/flatfee/charge.goopenmeter/billing/charges/flatfee/handler.goopenmeter/billing/charges/lineage/adapter/lineage.goopenmeter/billing/charges/lineage/lineage_test.goopenmeter/billing/charges/lineage/service.goopenmeter/billing/charges/lineage/service/service.goopenmeter/billing/charges/service/advance.goopenmeter/billing/charges/service/advance_test.goopenmeter/billing/charges/service/base_test.goopenmeter/billing/charges/service/lineage_test.goopenmeter/billing/charges/service/usagebased_costbasis_test.goopenmeter/billing/charges/usagebased/charge.goopenmeter/billing/charges/usagebased/handler.goopenmeter/ent/schema/creditrealizationlineage.goopenmeter/ledger/chargeadapter/creditpurchase_test.goopenmeter/ledger/chargeadapter/flatfee.goopenmeter/ledger/chargeadapter/flatfee_customcurrency_test.goopenmeter/ledger/chargeadapter/flatfee_test.goopenmeter/ledger/chargeadapter/helpers.goopenmeter/ledger/chargeadapter/usagebased.goopenmeter/ledger/chargeadapter/usagebased_customcurrency_test.goopenmeter/ledger/chargeadapter/usagebased_test.goopenmeter/ledger/recognizer/recognize.goopenmeter/ledger/recognizer/service_test.goopenmeter/ledger/testutils/integration.goopenmeter/ledger/transactions/accrual.gotools/migrate/migrations/20260804095904_add_lineage_custom_currency_identity.down.sqltools/migrate/migrations/20260804095904_add_lineage_custom_currency_identity.up.sql
💤 Files with no reviewable changes (1)
- openmeter/billing/charges/service/usagebased_costbasis_test.go
🚧 Files skipped from review as they are similar to previous changes (25)
- openmeter/ledger/recognizer/service_test.go
- openmeter/billing/charges/usagebased/handler.go
- openmeter/billing/charges/lineage/service/service.go
- openmeter/billing/charges/usagebased/charge.go
- openmeter/ledger/chargeadapter/flatfee_test.go
- openmeter/billing/charges/flatfee/charge.go
- openmeter/ledger/chargeadapter/usagebased_test.go
- openmeter/ledger/chargeadapter/creditpurchase_test.go
- openmeter/billing/charges/service/advance.go
- openmeter/ent/schema/creditrealizationlineage.go
- openmeter/billing/charges/service/base_test.go
- openmeter/billing/charges/lineage/lineage_test.go
- openmeter/ledger/recognizer/recognize.go
- openmeter/ledger/chargeadapter/helpers.go
- openmeter/ledger/transactions/accrual.go
- openmeter/ledger/chargeadapter/flatfee.go
- openmeter/ledger/testutils/integration.go
- openmeter/billing/charges/flatfee/handler.go
- openmeter/ledger/chargeadapter/usagebased.go
- openmeter/billing/charges/lineage/adapter/lineage.go
- openmeter/billing/charges/service/lineage_test.go
- openmeter/billing/charges/lineage/service.go
- openmeter/ledger/chargeadapter/flatfee_customcurrency_test.go
- openmeter/billing/charges/service/advance_test.go
- openmeter/ledger/chargeadapter/usagebased_customcurrency_test.go
This was referenced Aug 6, 2026
mark-vass-konghq
force-pushed
the
feat/add-cc-fx-support-to-ledger-credit_then_invoice
branch
3 times, most recently
from
August 7, 2026 11:13
a639b9a to
7f8590d
Compare
mark-vass-konghq
force-pushed
the
feat/add-cc-fx-support-to-ledger-credit_then_invoice
branch
from
August 7, 2026 16:41
7f8590d to
273d3e3
Compare
|
This pull request has been inactive for 45 days. Push a commit or leave a comment describing the next step to remove the |
github-actions
Bot
added
the
stale
This branch has not been deployed
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implement ledger-side FX handling for custom-currency
credit_then_invoicecharges.The uncovered custom-currency overage is now booked using the same accounting semantics as a credit purchase:
All three operations are committed in one atomic ledger transaction group.
Why
Custom-currency overage invoice lines represent credit purchases. Previously, the ledger implementation did not fully preserve the corresponding credit-purchase source/spend attribution.
The ledger flow must therefore behave like an actual credit purchase while ensuring that the temporary custom-currency balance never becomes spendable.
Implementation
SourceChargeIDSourceChargeIDandSpendChargeIDSourceChargeIDSpendChargeIDbehavior for ordinary fiat charges.Test coverage
Added PostgreSQL-backed flow coverage for both usage-based and flat-fee charges:
Summary by CodeRabbit
New Features
Bug Fixes
Greptile Summary
The PR adds ledger-side handling for custom-currency
credit_then_invoiceoverages while preserving native credit lineage and settling the resulting invoice in fiat.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Sequence Diagram
Reviews (11): Last reviewed commit: "fix: remove guards" | Re-trigger Greptile
Context used (3)