feat: add db support for credit purchase costbasis pinning - #4870
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughCredit purchases now use charge-level cost-basis models with dedicated persistence, legacy-row migration, validation, and resolved-rate consumers. Invoice, external payment, ledger, API, grant, schema, and test flows use the new representations. ChangesCredit purchase cost-basis lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CreditPurchaseAdapter
participant Database
participant ChargeBase
participant CreditPurchaseService
participant LedgerAdapter
participant CreditAPI
CreditPurchaseAdapter->>Database: Lock and upgrade legacy charge
CreditPurchaseAdapter->>Database: Persist schema and cost-basis state
CreditPurchaseService->>ChargeBase: GetResolvedCostBasis()
ChargeBase-->>CreditPurchaseService: Return fiat currency and rate
LedgerAdapter->>ChargeBase: GetResolvedCostBasis()
ChargeBase-->>LedgerAdapter: Return fiat currency and rate
CreditAPI->>ChargeBase: GetResolvedCostBasis()
ChargeBase-->>CreditAPI: Return fiat currency and rate
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
turip
marked this pull request as ready for review
August 6, 2026 16:57
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
openmeter/billing/charges/creditpurchase/settlement_test.go (1)
74-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNice touch pinning the exact compat JSON. Two new validation branches are still uncovered.
Locking the wire format with
require.JSONEqis exactly right for a payload that old binaries must keep reading.Two rules added in
PersistedSettlement.Validatehave no direct test here:
- A promotional settlement that carries
Currency,CostBasis, orInitialStatusmust fail with"promotional settlement cannot contain payment compatibility fields".- An external settlement must require
InitialStatus, and a non-external settlement must reject it.Both are cheap table cases, and they guard the compat contract that the whole schema-level rollout depends on.
💚 Suggested additional cases
func TestPersistedSettlementValidateFieldCombinations(t *testing.T) { currency := currencyx.FiatCode("USD") costBasis := alpacadecimal.NewFromFloat(0.5) status := CreatedInitialPaymentSettlementStatus for _, tc := range []struct { name string settlement PersistedSettlement wantErr string }{ { name: "promotional rejects payment compatibility fields", settlement: PersistedSettlement{Type: SettlementTypePromotional, CostBasis: &costBasis}, wantErr: "promotional settlement cannot contain payment compatibility fields", }, { name: "external requires initial status", settlement: PersistedSettlement{Type: SettlementTypeExternal, Currency: ¤cy, CostBasis: &costBasis}, wantErr: "initial status is required", }, { name: "invoice rejects initial status", settlement: PersistedSettlement{Type: SettlementTypeInvoice, Currency: ¤cy, CostBasis: &costBasis, InitialStatus: &status}, wantErr: "initial status is only valid for external settlement", }, } { t.Run(tc.name, func(t *testing.T) { err := tc.settlement.Validate() require.ErrorContains(t, err, tc.wantErr) require.True(t, models.IsGenericValidationError(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/charges/creditpurchase/settlement_test.go` around lines 74 - 95, Extend settlement validation tests with table-driven cases targeting PersistedSettlement.Validate: verify promotional settlements reject any payment compatibility field, external settlements require InitialStatus, and non-external settlements reject InitialStatus. Assert each expected validation message and confirm the error is a generic validation error.tools/migrate/migrations/20260806163516_credit_purchase_cost_basis_bridge.up.sql (1)
2-2: 🩺 Stability & Availability | 🔵 TrivialHeads-up on lock duration; no change requested to the generated SQL.
Squawk flags the five new CHECK constraints as validating without
NOT VALID. The new columns are added in the same statement, so existing rows satisfy every constraint by construction, and this file is an Atlas-generated artifact. The only practical consideration is that PostgreSQL still takes anACCESS EXCLUSIVElock and scans the table while validating. Ifcharge_credit_purchasesis large in production, plan the deploy window or set alock_timeoutfor the migration session.🤖 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/20260806163516_credit_purchase_cost_basis_bridge.up.sql` at line 2, No SQL changes are required because the new columns and constraints are valid for existing rows by construction. Review the migration using charge_credit_purchases and its five CHECK constraints, and address only deployment planning: schedule it during an appropriate window or configure a migration-session lock_timeout if the table may be large.Source: Linters/SAST tools
openmeter/billing/charges/creditpurchase/adapter/mapper.go (1)
157-183: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSmall DRY win: hoist the legacy settlement currency lookup.
Both branches call
dbEntity.Settlement.GetCurrency()and repeat the nil check with only the error text differing. Reading it once before thecurrency.IsCustom()split removes the duplication.♻️ Proposed refactor
+ fiatCode, err := dbEntity.Settlement.GetCurrency() + if err != nil { + return mappedCostBasis{}, fmt.Errorf("getting legacy settlement currency: %w", err) + } + if fiatCode == nil { + return mappedCostBasis{}, errors.New("legacy cost basis requires a settlement currency") + } + if !currency.IsCustom() { - fiatCode, err := dbEntity.Settlement.GetCurrency() - if err != nil { - return mappedCostBasis{}, fmt.Errorf("getting legacy settlement currency: %w", err) - } - if fiatCode == nil { - return mappedCostBasis{}, errors.New("legacy fiat cost basis requires a settlement currency") - } if currencyx.Code(*fiatCode) != currency.GetCode() { return mappedCostBasis{}, fmt.Errorf("settlement currency %q must match credit currency %q", *fiatCode, currency.GetCode()) } return mappedCostBasis{CostBasis: lo.ToPtr(creditpurchase.NewCostBasis(creditpurchase.FiatCostBasis{Rate: rate}))}, nil } - fiatCode, err := dbEntity.Settlement.GetCurrency() - if err != nil { - return mappedCostBasis{}, fmt.Errorf("getting legacy settlement currency: %w", err) - } - if fiatCode == nil { - return mappedCostBasis{}, errors.New("legacy custom-currency cost basis requires a settlement currency") - } - fiatCurrency, err := currencyx.NewFiatCurrency(*fiatCode)🤖 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/creditpurchase/adapter/mapper.go` around lines 157 - 183, Hoist the dbEntity.Settlement.GetCurrency() call and its shared error handling before the currency.IsCustom() branch, then perform the nil validation inside each branch using its branch-specific error message. Reuse the single fiatCode result in both the standard and custom currency paths, preserving their existing mapping and validation behavior.openmeter/billing/charges/creditpurchase/adapter/charge.go (1)
97-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider moving the cost-basis branch into a named helper.
This closure now carries settlement dispatch, cost-basis type switching, and persistence mapping in one nested block of about 90 lines. A named helper such as
applyCostBasis(ctx, tx, create, in) (*db.ChargeCreditPurchaseCostBasis, *creditpurchase.ResolvedCostBasis, error)would keepCreateChargereadable and make the promotional / fiat / custom-currency paths easier to test. No behavior change needed.As per coding guidelines: "Do not hide type switching, validation, persistence mapping, or meaningful domain translation inside local closures; use named helpers and reserve inline callbacks for obvious, tiny logic."
🤖 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/creditpurchase/adapter/charge.go` around lines 97 - 186, Extract the cost-basis type-switching and persistence logic from the settlement dispatch in CreateCharge into a named applyCostBasis helper returning the created cost basis, resolved cost basis, and error. Keep promotional settlement handling and settlement dispatch in CreateCharge, pass the existing context, transaction, charge builder, and input into the helper, and preserve all current validation, mapping, persistence, and error behavior.Source: Coding guidelines
openmeter/billing/charges/creditpurchase/costbasis_test.go (1)
61-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the resolved-rate validation.
TestResolvedCostBasisValidateonly checks the missing fiat currency case. Add a case with a valid fiat currency andalpacadecimal.Zerorate. This ensures thatResolvedCostBasis.Validate()continues to reject a non-positive resolved rate.Proposed test case
func TestResolvedCostBasisValidate(t *testing.T) { require.ErrorContains(t, (ResolvedCostBasis{ Rate: alpacadecimal.NewFromInt(1), }).Validate(), "fiat currency is required") + + usd, err := currencyx.NewFiatCurrency("USD") + require.NoError(t, err) + require.ErrorContains(t, (ResolvedCostBasis{ + FiatCurrency: usd, + Rate: alpacadecimal.Zero, + }).Validate(), "rate must be positive") }As per path instructions, “Make sure the tests are comprehensive and cover the changes.”
🤖 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/creditpurchase/costbasis_test.go` around lines 61 - 65, Extend TestResolvedCostBasisValidate with a case that sets a valid fiat currency and alpacadecimal.Zero rate, then assert Validate() returns the expected non-positive resolved-rate error. Keep the existing missing-currency assertion unchanged.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openmeter/billing/charges/creditpurchase/adapter/mapper.go`:
- Around line 67-69: Update the credit-purchase migration/backfill flow used
before ListCharges enables Charge.Validate, adding a query that identifies
legacy fiat rows where settlementCurrency differs from the credit currency and
either fixes them or fails the migration. Ensure mismatched rows cannot remain
when adapter.mapper.FromDB validates mapped charges.
In `@openmeter/billing/charges/service/creditpurchase_test.go`:
- Around line 319-357: Add a guard at the start of the “legacy row upgrades on
write” subtest to verify that the shared createdCharge was successfully
initialized before dereferencing createdCharge.State.CostBasisID; skip or fail
this subtest cleanly when creation failed, preserving the existing upgrade
assertions.
In `@openmeter/ent/schema/chargescreditpurchase.go`:
- Line 112: Update the cost_basis_schema_level_settlement_fields check in
chargescreditpurchase.go to explicitly require settlement_type IS NOT NULL when
schema_level is 2, ensuring NULL cannot satisfy the constraint. Apply the
identical constraint expression to the paired migration so the generated schema
and migration remain consistent.
---
Nitpick comments:
In `@openmeter/billing/charges/creditpurchase/adapter/charge.go`:
- Around line 97-186: Extract the cost-basis type-switching and persistence
logic from the settlement dispatch in CreateCharge into a named applyCostBasis
helper returning the created cost basis, resolved cost basis, and error. Keep
promotional settlement handling and settlement dispatch in CreateCharge, pass
the existing context, transaction, charge builder, and input into the helper,
and preserve all current validation, mapping, persistence, and error behavior.
In `@openmeter/billing/charges/creditpurchase/adapter/mapper.go`:
- Around line 157-183: Hoist the dbEntity.Settlement.GetCurrency() call and its
shared error handling before the currency.IsCustom() branch, then perform the
nil validation inside each branch using its branch-specific error message. Reuse
the single fiatCode result in both the standard and custom currency paths,
preserving their existing mapping and validation behavior.
In `@openmeter/billing/charges/creditpurchase/costbasis_test.go`:
- Around line 61-65: Extend TestResolvedCostBasisValidate with a case that sets
a valid fiat currency and alpacadecimal.Zero rate, then assert Validate()
returns the expected non-positive resolved-rate error. Keep the existing
missing-currency assertion unchanged.
In `@openmeter/billing/charges/creditpurchase/settlement_test.go`:
- Around line 74-95: Extend settlement validation tests with table-driven cases
targeting PersistedSettlement.Validate: verify promotional settlements reject
any payment compatibility field, external settlements require InitialStatus, and
non-external settlements reject InitialStatus. Assert each expected validation
message and confirm the error is a generic validation error.
In
`@tools/migrate/migrations/20260806163516_credit_purchase_cost_basis_bridge.up.sql`:
- Line 2: No SQL changes are required because the new columns and constraints
are valid for existing rows by construction. Review the migration using
charge_credit_purchases and its five CHECK constraints, and address only
deployment planning: schedule it during an appropriate window or configure a
migration-session lock_timeout if the table may be large.
🪄 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: 88efeb19-fda0-4305-bb95-e980159f07f8
⛔ Files ignored due to path filters (10)
openmeter/ent/db/chargecreditpurchase.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchase/chargecreditpurchase.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchase/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchase_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchase_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/**openmeter/ent/db/setorclear.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (37)
api/v3/handlers/customers/credits/convert.goopenmeter/billing/charges/README.mdopenmeter/billing/charges/creditpurchase/adapter/charge.goopenmeter/billing/charges/creditpurchase/adapter/costbasis.goopenmeter/billing/charges/creditpurchase/adapter/funded_credit_activity_test.goopenmeter/billing/charges/creditpurchase/adapter/mapper.goopenmeter/billing/charges/creditpurchase/adapter/mapper_test.goopenmeter/billing/charges/creditpurchase/charge.goopenmeter/billing/charges/creditpurchase/costbasis.goopenmeter/billing/charges/creditpurchase/costbasis_test.goopenmeter/billing/charges/creditpurchase/service/create.goopenmeter/billing/charges/creditpurchase/service/external_test.goopenmeter/billing/charges/creditpurchase/service/invoice_test.goopenmeter/billing/charges/creditpurchase/service/lineengine.goopenmeter/billing/charges/creditpurchase/service/promotional_test.goopenmeter/billing/charges/creditpurchase/service/realizations/service.goopenmeter/billing/charges/creditpurchase/service/realizations_test.goopenmeter/billing/charges/creditpurchase/settlement.goopenmeter/billing/charges/creditpurchase/settlement_test.goopenmeter/billing/charges/service/creditpurchase_test.goopenmeter/billing/charges/service/taxcode_test.goopenmeter/billing/creditgrant/service.goopenmeter/billing/creditgrant/service/service.goopenmeter/billing/creditgrant/service_test.goopenmeter/ent/schema/chargescreditpurchase.goopenmeter/ledger/chargeadapter/creditpurchase.goopenmeter/ledger/chargeadapter/creditpurchase_customcurrency_test.goopenmeter/ledger/chargeadapter/creditpurchase_test.goopenmeter/ledger/customerbalance/service_test.goopenmeter/ledger/customerbalance/testenv_test.gotest/app/stripe/invoice_credits_test.gotest/credits/base.gotest/credits/credit_then_invoice_test.gotest/credits/sanity_lifecycle_test.gotest/credits/sanity_test.gotools/migrate/migrations/20260806163516_credit_purchase_cost_basis_bridge.down.sqltools/migrate/migrations/20260806163516_credit_purchase_cost_basis_bridge.up.sql
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@tools/migrate/migrations/20260806163516_credit_purchase_cost_basis_bridge.up.sql`:
- Line 2: The fiat_cost_basis constraint in the migration currently permits
numeric NaN; update the fiat_cost_basis check to require both non-NaN and
positive values while preserving NULL allowance, and add a migration test
asserting that numeric 'NaN' is rejected.
- Line 2: The migration’s new CHECK constraints currently validate while the
table is locked. Update the ALTER TABLE statement in the credit purchase
cost-basis migration to add each constraint with NOT VALID, add a separate
validation step outside the deploy-lock operation, and update the
schema/migration generator responsible for these constraints so future generated
migrations preserve this pattern.
🪄 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: a33c48c5-c65c-499f-ad24-e08b87a6c4db
⛔ Files ignored due to path filters (2)
openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (7)
openmeter/billing/charges/creditpurchase/adapter/charge.goopenmeter/billing/charges/creditpurchase/costbasis_test.goopenmeter/billing/charges/creditpurchase/settlement_test.goopenmeter/billing/charges/service/creditpurchase_test.goopenmeter/ent/schema/chargescreditpurchase.gotools/migrate/migrations/20260806163516_credit_purchase_cost_basis_bridge.down.sqltools/migrate/migrations/20260806163516_credit_purchase_cost_basis_bridge.up.sql
🚧 Files skipped from review as they are similar to previous changes (4)
- openmeter/billing/charges/creditpurchase/costbasis_test.go
- openmeter/ent/schema/chargescreditpurchase.go
- openmeter/billing/charges/creditpurchase/adapter/charge.go
- openmeter/billing/charges/service/creditpurchase_test.go
turip
changed the title
turip
force-pushed
the
feat/credit-purchase-costbasis-bridge
branch
from
August 7, 2026 08:58
b9fefa4 to
97768be
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 (2)
tools/migrate/credit_purchase_cost_basis_test.go (1)
27-31: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover all rejection branches of
fiat_cost_basis_positive.The test checks only
NaN. The migration also rejects0and negative values throughfiat_cost_basis > 0. Add separate cases for both values, using a fresh transaction or savepoint for each expected failure. Otherwise, a regression that permits non-positive finite cost bases can pass this test.As per path instructions, tests should cover behavior introduced by the PR.
🤖 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/credit_purchase_cost_basis_test.go` around lines 27 - 31, Extend the rejection coverage in the credit purchase cost-basis test around the existing charge_credit_purchases INSERT to add separate expected-failure cases for fiat_cost_basis values 0 and a negative number, in addition to NaN. Use a fresh transaction or savepoint for each case so one failed INSERT does not affect subsequent assertions, and verify each error references fiat_cost_basis_positive.Source: Path instructions
openmeter/billing/charges/creditpurchase/adapter/mapper.go (1)
156-192: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSmall cleanup: hoist the duplicated settlement-currency lookup, and explain the synthesized
ResolvedAt.Two small things here, both optional:
dbEntity.Settlement.GetCurrency()plus the nil check appears twice (lines 158-164 and 172-178). Only the error text differs. Hoisting it above theIsCustombranch removes the duplication.ResolvedAt: dbEntity.CreatedAt.UTC()synthesizes a resolution timestamp for legacy rows. That is non-obvious domain intent, so a short comment stating that legacy rows never recorded a resolution time and the row creation time stands in for it would help the next reader.♻️ Proposed cleanup
+ fiatCode, err := dbEntity.Settlement.GetCurrency() + if err != nil { + return mappedCostBasis{}, fmt.Errorf("getting legacy settlement currency: %w", err) + } + if fiatCode == nil { + return mappedCostBasis{}, errors.New("legacy cost basis requires a settlement currency") + } + if !currency.IsCustom() { - fiatCode, err := dbEntity.Settlement.GetCurrency() - if err != nil { - return mappedCostBasis{}, fmt.Errorf("getting legacy settlement currency: %w", err) - } - if fiatCode == nil { - return mappedCostBasis{}, errors.New("legacy fiat cost basis requires a settlement currency") - } if currencyx.Code(*fiatCode) != currency.GetCode() { return mappedCostBasis{}, fmt.Errorf("settlement currency %q must match credit currency %q", *fiatCode, currency.GetCode()) } return mappedCostBasis{CostBasis: lo.ToPtr(creditpurchase.NewCostBasis(creditpurchase.FiatCostBasis{Rate: rate}))}, nil } - fiatCode, err := dbEntity.Settlement.GetCurrency() - if err != nil { - return mappedCostBasis{}, fmt.Errorf("getting legacy settlement currency: %w", err) - } - if fiatCode == nil { - return mappedCostBasis{}, errors.New("legacy custom-currency cost basis requires a settlement currency") - } - fiatCurrency, err := currencyx.NewFiatCurrency(*fiatCode) if err != nil { return mappedCostBasis{}, fmt.Errorf("mapping legacy settlement currency: %w", err) } intent := costbasis.NewIntent(costbasis.ManualIntent{ FiatCurrency: fiatCurrency, Rate: rate, }) + // Legacy rows never persisted a resolution timestamp; the row creation time + // is the closest available approximation for the resolved cost basis. state := costbasis.State{ CostBasis: rate, ResolvedAt: dbEntity.CreatedAt.UTC(), }One caveat on the merged error text: the two current messages distinguish the fiat and custom-currency cases. If that distinction helps operators triage, keep the branches separate and only add the comment.
🤖 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/creditpurchase/adapter/mapper.go` around lines 156 - 192, In the mapper logic around the currency handling branch, hoist the shared dbEntity.Settlement.GetCurrency() call and error handling above the currency.IsCustom() split, while preserving the existing fiat/custom nil-error distinction if it aids triage. Add a brief comment next to State.ResolvedAt explaining that legacy rows lack a recorded resolution time and use dbEntity.CreatedAt.UTC() as the substitute.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
`@tools/migrate/migrations/20260806163516_credit_purchase_cost_basis_bridge.up.sql`:
- Line 2: Update the five CHECK constraints in the migration statement to use
NOT VALID, preserving their existing definitions. Add a separate migration step
outside the deployment-lock window that validates each
constraint—cost_basis_schema_level_settlement_fields, fiat_cost_basis_positive,
initial_payment_settlement_status, schema_level, and settlement_type—using the
project’s Atlas/migrate-check generation workflow.
---
Nitpick comments:
In `@openmeter/billing/charges/creditpurchase/adapter/mapper.go`:
- Around line 156-192: In the mapper logic around the currency handling branch,
hoist the shared dbEntity.Settlement.GetCurrency() call and error handling above
the currency.IsCustom() split, while preserving the existing fiat/custom
nil-error distinction if it aids triage. Add a brief comment next to
State.ResolvedAt explaining that legacy rows lack a recorded resolution time and
use dbEntity.CreatedAt.UTC() as the substitute.
In `@tools/migrate/credit_purchase_cost_basis_test.go`:
- Around line 27-31: Extend the rejection coverage in the credit purchase
cost-basis test around the existing charge_credit_purchases INSERT to add
separate expected-failure cases for fiat_cost_basis values 0 and a negative
number, in addition to NaN. Use a fresh transaction or savepoint for each case
so one failed INSERT does not affect subsequent assertions, and verify each
error references fiat_cost_basis_positive.
🪄 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: 7fd285da-7b65-4805-a77e-22bb4ecf844d
⛔ Files ignored due to path filters (10)
openmeter/ent/db/chargecreditpurchase.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchase/chargecreditpurchase.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchase/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchase_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchase_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/**openmeter/ent/db/setorclear.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (38)
api/v3/handlers/customers/credits/convert.goopenmeter/billing/charges/README.mdopenmeter/billing/charges/creditpurchase/adapter/charge.goopenmeter/billing/charges/creditpurchase/adapter/costbasis.goopenmeter/billing/charges/creditpurchase/adapter/funded_credit_activity_test.goopenmeter/billing/charges/creditpurchase/adapter/mapper.goopenmeter/billing/charges/creditpurchase/adapter/mapper_test.goopenmeter/billing/charges/creditpurchase/charge.goopenmeter/billing/charges/creditpurchase/costbasis.goopenmeter/billing/charges/creditpurchase/costbasis_test.goopenmeter/billing/charges/creditpurchase/service/create.goopenmeter/billing/charges/creditpurchase/service/external_test.goopenmeter/billing/charges/creditpurchase/service/invoice_test.goopenmeter/billing/charges/creditpurchase/service/lineengine.goopenmeter/billing/charges/creditpurchase/service/promotional_test.goopenmeter/billing/charges/creditpurchase/service/realizations/service.goopenmeter/billing/charges/creditpurchase/service/realizations_test.goopenmeter/billing/charges/creditpurchase/settlement.goopenmeter/billing/charges/creditpurchase/settlement_test.goopenmeter/billing/charges/service/creditpurchase_test.goopenmeter/billing/charges/service/taxcode_test.goopenmeter/billing/creditgrant/service.goopenmeter/billing/creditgrant/service/service.goopenmeter/billing/creditgrant/service_test.goopenmeter/ent/schema/chargescreditpurchase.goopenmeter/ledger/chargeadapter/creditpurchase.goopenmeter/ledger/chargeadapter/creditpurchase_customcurrency_test.goopenmeter/ledger/chargeadapter/creditpurchase_test.goopenmeter/ledger/customerbalance/service_test.goopenmeter/ledger/customerbalance/testenv_test.gotest/app/stripe/invoice_credits_test.gotest/credits/base.gotest/credits/credit_then_invoice_test.gotest/credits/sanity_lifecycle_test.gotest/credits/sanity_test.gotools/migrate/credit_purchase_cost_basis_test.gotools/migrate/migrations/20260806163516_credit_purchase_cost_basis_bridge.down.sqltools/migrate/migrations/20260806163516_credit_purchase_cost_basis_bridge.up.sql
🚧 Files skipped from review as they are similar to previous changes (33)
- openmeter/billing/creditgrant/service.go
- openmeter/billing/charges/creditpurchase/service/realizations/service.go
- test/credits/sanity_lifecycle_test.go
- openmeter/billing/charges/README.md
- openmeter/billing/charges/creditpurchase/adapter/funded_credit_activity_test.go
- openmeter/billing/charges/creditpurchase/service/create.go
- openmeter/billing/charges/creditpurchase/service/external_test.go
- openmeter/billing/charges/creditpurchase/service/lineengine.go
- openmeter/billing/charges/service/taxcode_test.go
- openmeter/ledger/chargeadapter/creditpurchase.go
- openmeter/ledger/chargeadapter/creditpurchase_customcurrency_test.go
- openmeter/billing/creditgrant/service/service.go
- openmeter/ent/schema/chargescreditpurchase.go
- openmeter/billing/charges/creditpurchase/adapter/costbasis.go
- api/v3/handlers/customers/credits/convert.go
- openmeter/ledger/customerbalance/testenv_test.go
- openmeter/billing/charges/creditpurchase/costbasis_test.go
- openmeter/billing/creditgrant/service_test.go
- openmeter/billing/charges/creditpurchase/service/invoice_test.go
- openmeter/ledger/customerbalance/service_test.go
- openmeter/billing/charges/creditpurchase/costbasis.go
- openmeter/billing/charges/creditpurchase/service/realizations_test.go
- test/credits/sanity_test.go
- openmeter/billing/charges/creditpurchase/adapter/charge.go
- openmeter/billing/charges/creditpurchase/adapter/mapper_test.go
- openmeter/billing/charges/creditpurchase/settlement.go
- test/credits/base.go
- openmeter/billing/charges/creditpurchase/service/promotional_test.go
- openmeter/billing/charges/creditpurchase/settlement_test.go
- openmeter/billing/charges/service/creditpurchase_test.go
- openmeter/ledger/chargeadapter/creditpurchase_test.go
- test/app/stripe/invoice_credits_test.go
- test/credits/credit_then_invoice_test.go
chrisgacsal
approved these changes
Aug 7, 2026
This was referenced Aug 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why
This is the first deployment step for credit-purchase cost-basis pinning. It establishes a stable, online-compatible database schema before introducing the new cost-basis resolver, while keeping existing credit purchases readable and writable during rollout.
The change unblocks subsequent cost-basis pinning work without weakening custom-currency cost-basis validation.
Deployment behavior
Validation
make lint-go-fastmake migrate-checkPOSTGRES_HOST=127.0.0.1 go test -tags=dynamic ./openmeter/billing/charges/creditpurchase/... ./openmeter/billing/charges/service ./openmeter/billing/creditgrant/... ./openmeter/ledger/chargeadapter ./openmeter/ledger/customerbalance ./test/app/stripe ./test/creditsTicket: OM-436
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Greptile Summary
The PR introduces schema-level 2 persistence for credit-purchase settlement and cost-basis state while retaining compatibility with legacy rows.
Confidence Score: 5/5
The PR appears safe to merge because no blocking failure remains within the eligible follow-up review scope.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD Read[Read credit purchase] --> Level{Schema level} Level -->|Level 1| Legacy[Map settlement and cost basis from compatibility JSON] Level -->|Level 2| Native[Map settlement and cost basis from dedicated columns and edge] Legacy --> Write{Business write?} Write -->|No| Domain[Return domain charge] Write -->|Yes| Lock[Lock legacy row] Lock --> Materialize[Materialize settlement type and cost basis] Materialize --> Upgrade[Set schema level 2] Upgrade --> Persist[Apply business write in transaction] Native --> Domain Persist --> DomainReviews (4): Last reviewed commit: "fix: reject NaN credit purchase cost bas..." | Re-trigger Greptile
Context used (3)