feat(ledger): add tax code and tax behavior to ledger types#4291
Open
mark-vass-konghq wants to merge 4 commits into
Open
feat(ledger): add tax code and tax behavior to ledger types#4291mark-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:
📝 WalkthroughWalkthroughAdds a TaxBehavior domain type (inclusive/exclusive), threads it through Route/RouteFilter/validation/normalization/routing-key generation, persists it in the schema, and updates DB predicates and transaction routing to distinguish absent vs explicitly nil TaxCode and TaxBehavior; tests expanded accordingly. ChangesTax Behavior & TaxCode Nil-Distinction
Sequence Diagram(s)sequenceDiagram
participant Template as Transaction Template
participant Router as Route/Normalize/Validate
participant Adapter as SubAccount Adapter
participant DB as Database (ledger_sub_account_routes)
participant Accrual as Accrual/Correction Logic
Template->>Router: Build Route (currency, costBasis, taxCode?, taxBehavior?)
Router->>Router: Normalize & Validate (preserve Some(nil) vs absent)
Router->>Adapter: ResolveOrCreateRoute (includes nillable taxBehavior/taxCode)
Adapter->>DB: Query/Insert with predicates (TaxCode/TaxBehavior or IsNil)
DB-->>Adapter: Route row (tax_behavior included)
Adapter-->>Router: Mapped ledger.Route (includes TaxBehavior)
Router->>Accrual: Provide routePairingKey (includes taxCode)
Accrual->>Accrual: Separate buckets by taxCode / taxBehavior
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mark-vass-konghq
force-pushed
the
feat/add-tax-code-and-tax-behavior-to-ledger-types
branch
from
2292b60 to
198e123
Compare
mark-vass-konghq
changed the base branch from
main
to
feat/test-charge-tax-config-carryover
mark-vass-konghq
changed the base branch from
feat/test-charge-tax-config-carryover
to
main
mark-vass-konghq
force-pushed
the
feat/add-tax-code-and-tax-behavior-to-ledger-types
branch
from
198e123 to
d11aa78
Compare
mark-vass-konghq
force-pushed
the
feat/add-tax-code-and-tax-behavior-to-ledger-types
branch
from
d11aa78 to
6cb0134
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/ledger/account/adapter/subaccount.go (1)
82-92:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
TaxBehavioris in the routing key but never persisted or read back.The new
Route.TaxBehaviorflows into the routing key (good — guarantees row uniqueness), but it's missing from the actual route persistence/read path:
resolveOrCreateRoutehere doesn't callSetNillableTaxBehavior(normalizedRoute.TaxBehavior).MapSubAccountData(Lines 234‑241) reconstructsledger.RoutewithoutTaxBehavior, so any sub-account loaded from the DB will always haveRoute.TaxBehavior == nil, even if it was set when the route was created.ListSubAccounts(Lines 166‑173) handlesTaxCodewithIsPresent()/Get()but doesn't add aTaxBehaviorpredicate, so filter callers asking for a specific tax behavior get unfiltered results.Per
openmeter/ent/schema/ledger_account.go(LedgerSubAccountRoute.Fields), there's also notax_behaviorcolumn on the schema, so this needs a schema field + migration before the persistence/filter wiring will work.🛠️ Sketch of what needs to land together
In the schema (
openmeter/ent/schema/ledger_account.go):field.String("tax_behavior"). GoType(ledger.TaxBehavior("")). Optional().Nillable().Immutable(),In
resolveOrCreateRoute:SetCurrency(string(normalizedRoute.Currency)). SetNillableTaxCode(normalizedRoute.TaxCode). + SetNillableTaxBehavior(normalizedRoute.TaxBehavior). SetFeatures(normalizedRoute.Features).In
MapSubAccountData:Route: ledger.Route{ Currency: currencyx.Code(dbRoute.Currency), TaxCode: dbRoute.TaxCode, + TaxBehavior: dbRoute.TaxBehavior, Features: dbRoute.Features,In
ListSubAccounts(and the equivalent block inhistorical/adapter/sumentries_query.go):if normalizedRoute.TaxCode.IsPresent() { ... } + if normalizedRoute.TaxBehavior.IsPresent() { + tb, _ := normalizedRoute.TaxBehavior.Get() + if tb != nil { + routePredicates = append(routePredicates, dbledgersubaccountroute.TaxBehavior(*tb)) + } else { + routePredicates = append(routePredicates, dbledgersubaccountroute.TaxBehaviorIsNil()) + } + }Happy to draft the schema field + migration + adapter wiring as a follow-up if helpful — just say the word.
🤖 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 82 - 92, The code forgets to persist, read back, and filter on Route.TaxBehavior: add a new nullable/optional tax_behavior field to the ent schema (LedgerSubAccountRoute.Fields) and create the migration, then in resolveOrCreateRoute call SetNillableTaxBehavior(normalizedRoute.TaxBehavior) when building the create, update MapSubAccountData to populate ledger.Route.TaxBehavior from the DB row, and add a TaxBehavior predicate in ListSubAccounts (and the analogous historical sumentries query) so queries honor requested tax behavior filters.
🧹 Nitpick comments (1)
openmeter/ledger/routing_test.go (1)
240-248: 💤 Low valueTiny test naming + literal nits.
Two small things:
- Function name has a casing typo:
TestBuildroutingKeyV1_...→TestBuildRoutingKeyV1_...to match the rest of the file.- The literal
TaxBehavior("exclusive")on Line 244 could use the typed const for consistency withTestBuildRoutingKeyV1_DifferentTaxBehavior_DifferentKeyabove.✏️ Suggested cleanup
-func TestBuildroutingKeyV1_WithTaxBehaviorAndTaxCode(t *testing.T) { +func TestBuildRoutingKeyV1_WithTaxBehaviorAndTaxCode(t *testing.T) { key, err := BuildRoutingKeyV1(Route{ Currency: currencyx.Code("USD"), TaxCode: lo.ToPtr("GST10"), - TaxBehavior: lo.ToPtr(TaxBehavior("exclusive")), + TaxBehavior: lo.ToPtr(TaxBehaviorExclusive), })🤖 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/routing_test.go` around lines 240 - 248, Rename the test function TestBuildroutingKeyV1_WithTaxBehaviorAndTaxCode to TestBuildRoutingKeyV1_WithTaxBehaviorAndTaxCode (fix casing) and replace the literal TaxBehavior("exclusive") with the package's typed const used elsewhere (e.g. TaxBehaviorExclusive) when constructing the Route passed to BuildRoutingKeyV1; this keeps naming consistent and uses the typed constant for TaxBehavior.
🤖 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.
Outside diff comments:
In `@openmeter/ledger/account/adapter/subaccount.go`:
- Around line 82-92: The code forgets to persist, read back, and filter on
Route.TaxBehavior: add a new nullable/optional tax_behavior field to the ent
schema (LedgerSubAccountRoute.Fields) and create the migration, then in
resolveOrCreateRoute call SetNillableTaxBehavior(normalizedRoute.TaxBehavior)
when building the create, update MapSubAccountData to populate
ledger.Route.TaxBehavior from the DB row, and add a TaxBehavior predicate in
ListSubAccounts (and the analogous historical sumentries query) so queries honor
requested tax behavior filters.
---
Nitpick comments:
In `@openmeter/ledger/routing_test.go`:
- Around line 240-248: Rename the test function
TestBuildroutingKeyV1_WithTaxBehaviorAndTaxCode to
TestBuildRoutingKeyV1_WithTaxBehaviorAndTaxCode (fix casing) and replace the
literal TaxBehavior("exclusive") with the package's typed const used elsewhere
(e.g. TaxBehaviorExclusive) when constructing the Route passed to
BuildRoutingKeyV1; this keeps naming consistent and uses the typed constant for
TaxBehavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e6014707-5187-4e08-b0e7-110d9c6b5520
📒 Files selected for processing (7)
openmeter/ledger/account/adapter/subaccount.goopenmeter/ledger/accounts.goopenmeter/ledger/errors.goopenmeter/ledger/historical/adapter/sumentries_query.goopenmeter/ledger/primitives.goopenmeter/ledger/routing.goopenmeter/ledger/routing_test.go
GAlexIHU
requested changes
May 5, 2026
Contributor
GAlexIHU
left a comment
GAlexIHU
left a comment
There was a problem hiding this comment.
thx, if you don't mind I'd prefer if a bit more stuff was collected on the branch that actually do something (ik unpopular opinion)
Contributor
Author
What changes needed to be done to not break our env? |
mark-vass-konghq
force-pushed
the
feat/add-tax-code-and-tax-behavior-to-ledger-types
branch
from
6cb0134 to
1427ce7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
openmeter/ledger/routing.go (2)
313-324:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift🚨 This is the env-breaking bit Mark asked about.
Appending
tax_behavior:to the V1 string changes the routing key value for every existing route, not just routes that opt into aTaxBehavior. Concretely:
- Existing rows in
ledger_sub_account_routeswere stored withrouting_key_version = "v1"and a value that has notax_behavior:segment.- After this change,
BuildRoutingKeyV1for the same logicalRouteproduces…|tax_behavior:null|….resolveOrCreateRoutelooks up by(namespace, account_id, routing_key_version, routing_key)— so post-deploy it will miss the existing row and try to insert a new one (or hitOnConflictwith mismatched key set), and any code that resolves a sub-account from a previously persisted key will fail.To avoid breaking deployed envs, please pick one of:
- Bump the routing key version (e.g., add
RoutingKeyVersionV2) and keepBuildRoutingKeyV1byte-identical to today. New routes get v2; existing v1 routes keep resolving. Best for a clean cutover.- Backfill migration that recomputes & rewrites
routing_keyfor all existing rows underv1before the new code is live, gated to roll out atomically with the binary.- Skip emitting the
tax_behavior:segment entirely whenTaxBehavior == nil(so existing routes hash identically), and only ever pin/normalize TaxBehavior into the key when explicitly set. This loses the "Some(nil) is meaningful" property in the key, but preserves it inRouteFilter.Option 1 is the lowest-risk path and matches the version field's reason for existing.
🤖 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/routing.go` around lines 313 - 324, The change added a new `tax_behavior:` segment to the V1 routing key which will break existing rows; instead add a new version constant (e.g., RoutingKeyVersionV2) and implement BuildRoutingKeyV2 that includes the `tax_behavior:` segment while leaving BuildRoutingKeyV1 byte-identical to its current output, then switch route-creation paths to emit RoutingKeyVersionV2/NewRoutingKey(..., value) for new routes (keep resolveOrCreateRoute able to read v1 rows unchanged). Ensure any helper called here (BuildRoutingKeyV1, BuildRoutingKeyV2, NewRoutingKey) are updated so V1 serialization is not modified.
242-285:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
RouteFieldTaxBehaviorcase tosameRouteFieldin routingrules.go.Your normalization refactor in routing.go correctly added
TaxBehaviorhandling alongside the existing fast-path checks. However, thesameRouteFieldfunction inopenmeter/ledger/routingrules/routingrules.godoesn't have a corresponding case for it. If a rule ever tries to compareTaxBehaviorfields, it'll return an error ("unknown route field: tax_behavior").Add the constant and switch case to keep them in sync:
Example fix pattern
const RouteFieldTaxBehavior RouteField = "tax_behavior" // Add to enum around line 168 // In sameRouteField, add: case RouteFieldTaxBehavior: return optionalTaxBehaviorEqual(left.TaxBehavior, right.TaxBehavior), nil🤖 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/routing.go` around lines 242 - 285, Add a new RouteField constant for TaxBehavior and handle it in sameRouteField: define RouteFieldTaxBehavior as "tax_behavior" alongside the other RouteField constants, then in the sameRouteField switch add a case RouteFieldTaxBehavior that returns optionalTaxBehaviorEqual(left.TaxBehavior, right.TaxBehavior), nil; ensure you reference the TaxBehavior field on the Route struct and the helper optionalTaxBehaviorEqual in routingrules.go so comparisons for tax_behavior no longer fall through to the unknown-field error.openmeter/ledger/account/adapter/subaccount.go (2)
234-241:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDon't forget TaxBehavior in the read path.
Once the persistence wiring lands (see the comment on lines 82-92), this struct literal also needs
TaxBehavior: dbRoute.TaxBehaviorso hydrated routes match what was written. Without it,SubAccountData.Route.TaxBehavioris permanentlynil.🤖 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 234 - 241, The ledger.Route struct literal in the read/hydration path is missing TaxBehavior, causing SubAccountData.Route.TaxBehavior to stay nil; update the Route initialization (where ledger.Route is constructed from dbRoute) to include TaxBehavior: dbRoute.TaxBehavior so the hydrated route mirrors what was persisted (ensure the change is applied alongside the persistence wiring referenced in the earlier comment).
82-92:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftAdd
tax_behaviorfield to LedgerSubAccountRoute persistence and wire it through all read/write paths.
BuildRoutingKeyV1hashesTaxBehaviorinto the routing key, but the database schema has notax_behaviorcolumn and the builder never callsSetNillableTaxBehavior(...). Two routes differing only byTaxBehaviorend up with different keys but identical persisted rows — they look the same on read-back. When code recomputes a routing key from a hydrated route, it'll produce a different key than what's stored becauseRoute.TaxBehavioris always nil, breaking the round-trip guarantee.This needs:
- Add
tax_behaviorfield to the ent schema (with migration)- Call
SetNillableTaxBehavior(normalizedRoute.TaxBehavior)in the Create builder (lines 82–92)- Read it back in
MapSubAccountDatawhen constructing theRoute- Fix the same omission in
openmeter/ledger/historical/adapter/ledger.goPair this fix with the routing-key version concern in
routing.gobefore landing.🤖 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 82 - 92, Add a nullable tax_behavior column to the ent schema and create the DB migration, then wire TaxBehavior through persistence: in subaccount.go update the Create builder for LedgerSubAccountRoute (the create variable built with r.db.LedgerSubAccountRoute.Create()) to call SetNillableTaxBehavior(normalizedRoute.TaxBehavior); update MapSubAccountData to read the tax_behavior field into the Route struct when hydrating routes; apply the same SetNillableTaxBehavior and read-back fix in openmeter/ledger/historical/adapter/ledger.go so historical writes/reads include TaxBehavior; finally ensure routing-key versioning in routing.go is addressed in the same change set so routing keys remain consistent.
🧹 Nitpick comments (3)
openmeter/ledger/routing.go (1)
28-33: 💤 Low valueQuick thought on the type.
TaxBehavioris a string-backed enum likeTransactionAuthorizationStatus— consistent and easy to reason about. One small consideration: there's no normalization (e.g., lower-casing) beforeValidate(), so"Inclusive"becomes invalid even though many APIs accept case-insensitive variants. If callers always use the constants, ignore this; if it ever flows in from user input or external systems, you'll want aNormalize/ParseTaxBehaviorhelper. Up to you.🤖 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/routing.go` around lines 28 - 33, The TaxBehavior string enum lacks normalization, so inputs like "Inclusive" will fail Validate(); add a ParseTaxBehavior (or TaxBehavior.Normalize) helper that trims and lowercases the incoming string and returns the corresponding TaxBehavior constant (TaxBehaviorInclusive/TaxBehaviorExclusive) or an error for invalid values, and update any usages (including the TaxBehavior.Validate method) to call this parser/normalizer when accepting external or user-provided input.openmeter/ledger/historical/adapter/sumentries_query.go (1)
124-131: 💤 Low valueLogic looks correct — minor DRY thought.
Predicate dispatch (
TaxCode(*tc)vsTaxCodeIsNil()) lines up perfectly with the ent definitions, and the surroundingCostBasisblock follows exactly the same shape. Since the same pattern now lives here and inopenmeter/ledger/account/adapter/subaccount.go(lines 166-173), it might be nice to extract a tiny generic helper like:♻️ Optional helper sketch
// optionalEqOrIsNil returns the predicate for an mo.Option[*T] filter // that supports absent / Some(nil) / Some(&v) semantics. func optionalEqOrIsNil[T any, P ~func(*sql.Selector)]( opt mo.Option[*T], eq func(T) P, isNil func() P, ) (P, bool) { if !opt.IsPresent() { var zero P return zero, false } v, _ := opt.Get() if v != nil { return eq(*v), true } return isNil(), true }Totally optional — happy to skip if you prefer to keep the call sites explicit.
🤖 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/historical/adapter/sumentries_query.go` around lines 124 - 131, The duplicated pattern for handling mo.Option[*T] filters (present/Some(nil)/Some(&v)) in the TaxCode and CostBasis predicate building should be extracted into a small generic helper (suggested name optionalEqOrIsNil) that returns a predicate and a bool indicating presence; implement optionalEqOrIsNil[T any, P ~func(*sql.Selector)](opt mo.Option[*T], eq func(T) P, isNil func() P) and replace the duplicated blocks in the functions that append ledgersubaccountroutedb.TaxCode/TaxCodeIsNil and the analogous CostBasis predicates to call the helper and append the returned predicate when present.openmeter/ledger/routing_test.go (1)
116-228: ⚡ Quick winSolid coverage on the new tax-routing surface. 🎯
Nice spread across:
TaxBehavior.Validate,Route.Validaterejecting bogus values,RouteFilter.NormalizepreservingSome(&v)andSome(nil), absent-stays-absent fast path, andRoute.Filter()pinning semantics. TheSome(nil)round-trip case in particular is exactly what the new option semantics need to be locked down with.One follow-up to consider after the persistence/version concerns from
routing.goandsubaccount.goare addressed: an in-code integration test that round-trips aRoutewithTaxBehaviorthroughEnsureSubAccount→GetSubAccountByIDwould catch the read/write gap if it ever regresses. Not blocking on this 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 `@openmeter/ledger/routing_test.go` around lines 116 - 228, Add an integration test that round-trips a Route containing a non-nil TaxBehavior through your persistence layer by calling EnsureSubAccount (to write) then GetSubAccountByID (to read) and asserting the returned SubAccount/Route still has the same TaxBehavior value (and nil vs present semantics); target the Route struct with a pinned TaxBehavior value (e.g., TaxBehaviorInclusive), use EnsureSubAccount and GetSubAccountByID to perform the write/read, and assert equality of the TaxBehavior pointer/presence to detect any serialization/versioning regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openmeter/ledger/routing_test.go`:
- Around line 240-248: Rename the test function
TestBuildroutingKeyV1_WithTaxBehaviorAndTaxCode to
TestBuildRoutingKeyV1_WithTaxBehaviorAndTaxCode (capitalize the "r" to match
other tests) and replace the string literal TaxBehavior("exclusive") with the
constant TaxBehaviorExclusive used elsewhere; update the Route construction in
TestBuildRoutingKeyV1_WithTaxBehaviorAndTaxCode to use
lo.ToPtr(TaxBehaviorExclusive) so the test name and the TaxBehavior value are
consistent with the rest of the file.
---
Outside diff comments:
In `@openmeter/ledger/account/adapter/subaccount.go`:
- Around line 234-241: The ledger.Route struct literal in the read/hydration
path is missing TaxBehavior, causing SubAccountData.Route.TaxBehavior to stay
nil; update the Route initialization (where ledger.Route is constructed from
dbRoute) to include TaxBehavior: dbRoute.TaxBehavior so the hydrated route
mirrors what was persisted (ensure the change is applied alongside the
persistence wiring referenced in the earlier comment).
- Around line 82-92: Add a nullable tax_behavior column to the ent schema and
create the DB migration, then wire TaxBehavior through persistence: in
subaccount.go update the Create builder for LedgerSubAccountRoute (the create
variable built with r.db.LedgerSubAccountRoute.Create()) to call
SetNillableTaxBehavior(normalizedRoute.TaxBehavior); update MapSubAccountData to
read the tax_behavior field into the Route struct when hydrating routes; apply
the same SetNillableTaxBehavior and read-back fix in
openmeter/ledger/historical/adapter/ledger.go so historical writes/reads include
TaxBehavior; finally ensure routing-key versioning in routing.go is addressed in
the same change set so routing keys remain consistent.
In `@openmeter/ledger/routing.go`:
- Around line 313-324: The change added a new `tax_behavior:` segment to the V1
routing key which will break existing rows; instead add a new version constant
(e.g., RoutingKeyVersionV2) and implement BuildRoutingKeyV2 that includes the
`tax_behavior:` segment while leaving BuildRoutingKeyV1 byte-identical to its
current output, then switch route-creation paths to emit
RoutingKeyVersionV2/NewRoutingKey(..., value) for new routes (keep
resolveOrCreateRoute able to read v1 rows unchanged). Ensure any helper called
here (BuildRoutingKeyV1, BuildRoutingKeyV2, NewRoutingKey) are updated so V1
serialization is not modified.
- Around line 242-285: Add a new RouteField constant for TaxBehavior and handle
it in sameRouteField: define RouteFieldTaxBehavior as "tax_behavior" alongside
the other RouteField constants, then in the sameRouteField switch add a case
RouteFieldTaxBehavior that returns optionalTaxBehaviorEqual(left.TaxBehavior,
right.TaxBehavior), nil; ensure you reference the TaxBehavior field on the Route
struct and the helper optionalTaxBehaviorEqual in routingrules.go so comparisons
for tax_behavior no longer fall through to the unknown-field error.
---
Nitpick comments:
In `@openmeter/ledger/historical/adapter/sumentries_query.go`:
- Around line 124-131: The duplicated pattern for handling mo.Option[*T] filters
(present/Some(nil)/Some(&v)) in the TaxCode and CostBasis predicate building
should be extracted into a small generic helper (suggested name
optionalEqOrIsNil) that returns a predicate and a bool indicating presence;
implement optionalEqOrIsNil[T any, P ~func(*sql.Selector)](opt mo.Option[*T], eq
func(T) P, isNil func() P) and replace the duplicated blocks in the functions
that append ledgersubaccountroutedb.TaxCode/TaxCodeIsNil and the analogous
CostBasis predicates to call the helper and append the returned predicate when
present.
In `@openmeter/ledger/routing_test.go`:
- Around line 116-228: Add an integration test that round-trips a Route
containing a non-nil TaxBehavior through your persistence layer by calling
EnsureSubAccount (to write) then GetSubAccountByID (to read) and asserting the
returned SubAccount/Route still has the same TaxBehavior value (and nil vs
present semantics); target the Route struct with a pinned TaxBehavior value
(e.g., TaxBehaviorInclusive), use EnsureSubAccount and GetSubAccountByID to
perform the write/read, and assert equality of the TaxBehavior pointer/presence
to detect any serialization/versioning regressions.
In `@openmeter/ledger/routing.go`:
- Around line 28-33: The TaxBehavior string enum lacks normalization, so inputs
like "Inclusive" will fail Validate(); add a ParseTaxBehavior (or
TaxBehavior.Normalize) helper that trims and lowercases the incoming string and
returns the corresponding TaxBehavior constant
(TaxBehaviorInclusive/TaxBehaviorExclusive) or an error for invalid values, and
update any usages (including the TaxBehavior.Validate method) to call this
parser/normalizer when accepting external or user-provided input.
🪄 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: bb9a8da1-0baa-443f-a9c3-cd7dcf836c6e
📒 Files selected for processing (7)
openmeter/ledger/account/adapter/subaccount.goopenmeter/ledger/accounts.goopenmeter/ledger/errors.goopenmeter/ledger/historical/adapter/sumentries_query.goopenmeter/ledger/primitives.goopenmeter/ledger/routing.goopenmeter/ledger/routing_test.go
mark-vass-konghq
force-pushed
the
feat/add-tax-code-and-tax-behavior-to-ledger-types
branch
2 times, most recently
from
3640692 to
619e85f
Compare
GAlexIHU
requested changes
May 7, 2026
mark-vass-konghq
force-pushed
the
feat/add-tax-code-and-tax-behavior-to-ledger-types
branch
from
619e85f to
6524242
Compare
mark-vass-konghq
force-pushed
the
feat/add-tax-code-and-tax-behavior-to-ledger-types
branch
from
6524242 to
6c21be8
Compare
mark-vass-konghq
force-pushed
the
feat/add-tax-code-and-tax-behavior-to-ledger-types
branch
from
ab5c054 to
f8bc9bf
Compare
mark-vass-konghq
force-pushed
the
feat/add-tax-code-and-tax-behavior-to-ledger-types
branch
from
f8bc9bf to
dae5e93
Compare
mark-vass-konghq
force-pushed
the
feat/add-tax-code-and-tax-behavior-to-ledger-types
branch
from
7e37992 to
7508a11
Compare
GAlexIHU
requested changes
May 15, 2026
Contributor
GAlexIHU
left a comment
GAlexIHU
left a comment
There was a problem hiding this comment.
ai was (not) (ab)used when writing these comments
mark-vass-konghq
force-pushed
the
feat/add-tax-code-and-tax-behavior-to-ledger-types
branch
from
7508a11 to
a0430cf
Compare
mark-vass-konghq
force-pushed
the
feat/add-tax-code-and-tax-behavior-to-ledger-types
branch
from
a0430cf to
9b9a156
Compare
mark-vass-konghq
force-pushed
the
feat/add-tax-code-and-tax-behavior-to-ledger-types
branch
from
9b9a156 to
805d185
Compare
mark-vass-konghq
force-pushed
the
feat/add-tax-code-and-tax-behavior-to-ledger-types
branch
from
805d185 to
c6826e2
Compare
GAlexIHU
requested changes
May 19, 2026
Contributor
GAlexIHU
left a comment
GAlexIHU
left a comment
There was a problem hiding this comment.
I think you forgot about TaxBehavior. I didn't go to find the exact lines in sanity where you miss asserting for it, but its missed. Please add & fix
| At time.Time | ||
| Amount alpacadecimal.Decimal | ||
| Currency currencyx.Code | ||
| TaxCode *string |
Contributor
There was a problem hiding this comment.
wouldn't you also need TaxBehavior?
mark-vass-konghq
force-pushed
the
feat/add-tax-code-and-tax-behavior-to-ledger-types
branch
from
1133d75 to
911d360
Compare
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 by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Chores