feat: ledger account provisioning#4055
Merged
Merged
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds ledger namespace provisioning and customer-ledger hook wiring, introduces distributed locking for account provisioning, separates business-account provisioning (EnsureBusinessAccounts) from retrieval, reorders startup to run migrations before default-namespace creation, and adds concurrency/idempotency tests for resolvers and hooks. Changes
Sequence DiagramsequenceDiagram
participant App as Server/App
participant DB as Database
participant NSM as NamespaceManager
participant LedgerNH as LedgerNamespaceHandler
participant LedgerR as LedgerResolver
participant Locker as DistributedLocker
App->>DB: Migrate schema
App->>NSM: Register LedgerNamespaceHandler
App->>NSM: Register other handlers (e.g., Kafka)
App->>NSM: Create default namespace (initNamespace)
NSM->>LedgerNH: CreateNamespace(namespace)
LedgerNH->>LedgerR: EnsureBusinessAccounts(ctx, namespace)
LedgerR->>Locker: Acquire provisioning lock (5s timeout)
alt Lock acquired
LedgerR->>DB: List existing business accounts
alt Missing accounts
LedgerR->>DB: Create missing business accounts (wash, earnings, brokerage)
end
LedgerR->>Locker: Release lock
LedgerR-->>LedgerNH: BusinessAccounts
else Lock timeout
LedgerR-->>LedgerNH: Return lock timeout error
end
LedgerNH-->>NSM: Namespace creation result
NSM-->>App: Default namespace ready or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested Labels
Suggested Reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
59daf10 to
3ac6c7d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/ledger/resolvers/account.go (1)
50-91:⚠️ Potential issue | 🟠 MajorCustomer provisioning can still leave a namespace half-provisioned.
The new
openmeter/customer/service/ledger_hook_test.gohappy-path test creates a customer in a fresh namespace without ever runningEnsureBusinessAccounts, so this method can succeed while the three business accounts are still missing. Downstream flows likeopenmeter/ledger/transactions/earnings.go:45-65callGetBusinessAccountsdirectly, so the first business-account-dependent ledger op in that namespace can still fail withErrBusinessAccountMissing. I’d either ensure business accounts here first or reject customer provisioning until namespace bootstrap has already run.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/resolvers/account.go` around lines 50 - 91, CreateCustomerAccounts can leave a namespace half-provisioned because it doesn't ensure the shared business accounts exist; before creating per-customer accounts call or verify namespace bootstrap by invoking the existing EnsureBusinessAccounts (or a Repo.IsNamespaceBootstrapped check) for ns and handle errors, e.g. call s.AccountService.EnsureBusinessAccounts(ctx, ns) (or return a clear error/retry if the namespace isn't bootstrapped) after lockCustomerProvisioning and before iterating ledger.CustomerAccountTypes so downstream GetBusinessAccounts calls won't see missing business accounts.
🧹 Nitpick comments (2)
cmd/billing-worker/main.go (1)
86-90: Consider hoisting default namespace once for clearer logs and reuse.This works as-is, but grabbing the namespace once makes startup flow cleaner and gives better log context if provisioning fails.
♻️ Small cleanup suggestion
- _, err = app.LedgerAccountResolver.EnsureBusinessAccounts(ctx, app.NamespaceManager.GetDefaultNamespace()) + defaultNamespace := app.NamespaceManager.GetDefaultNamespace() + _, err = app.LedgerAccountResolver.EnsureBusinessAccounts(ctx, defaultNamespace) if err != nil { - logger.Error("failed to provision ledger business accounts", "error", err) + logger.Error("failed to provision ledger business accounts", "namespace", defaultNamespace, "error", err) os.Exit(1) } // Provision sandbox app - err = app.AppRegistry.SandboxProvisioner(ctx, app.NamespaceManager.GetDefaultNamespace()) + err = app.AppRegistry.SandboxProvisioner(ctx, defaultNamespace)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/billing-worker/main.go` around lines 86 - 90, Hoist the default namespace into a local variable before calling EnsureBusinessAccounts so you reuse it and include it in logs; e.g., call ns := app.NamespaceManager.GetDefaultNamespace() then pass ns to app.LedgerAccountResolver.EnsureBusinessAccounts(ctx, ns) and, on error, log the namespace in logger.Error("failed to provision ledger business accounts", "namespace", ns, "error", err) before exiting to provide clearer startup context.openmeter/customer/service/ledger_hook_test.go (1)
60-106: This rollback test doesn’t cover partial ledger writes yet.
failingCustomerAccountProvisionerfails before any account or mapping is inserted, so this only proves error propagation plus customer rollback. It won’t catch a regression where the real hook creates one or two ledger rows and then fails. I’d add an integration case that fails after the first ledger write and asserts both the customer and ledger rows are gone.As per coding guidelines
**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/customer/service/ledger_hook_test.go` around lines 60 - 106, Add a new test that simulates a partial ledger write failure (e.g., TestCustomerService_CreateCustomerRollsBackOnPartialLedgerFailure) instead of only using failingCustomerAccountProvisioner which fails pre-write; implement a new provisioner type (e.g., partialFailingCustomerAccountProvisioner) that implements CreateCustomerAccounts by inserting one account/mapping (using the same code path the real hook uses) and then returning a non-nil error, register that hook via ledgerresolvers.NewCustomerLedgerHook and env.CustomerService.RegisterHooks, call env.CustomerService.CreateCustomer, assert it returns the injected error and that created is nil, then verify both the customer (env.CustomerService.GetCustomer) and any ledger rows (query the ledger store/repo used by the hook, or call the ledger read API) are not present; reuse symbols from the diff: CustomerLedgerHook, CreateCustomerAccounts, env.CustomerService and ledger.CustomerAccounts to locate the code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Line 52: Replace the time-relative phrasing "now migrates" on Line 52 with
durable wording; rephrase the sentence to read something like
"`cmd/server/main.go` migrates the database before creating the default
namespace. Register namespace handlers before `initNamespace(...)` if they must
provision the default namespace during startup." This keeps the guidance
evergreen and references `cmd/server/main.go` and `initNamespace(...)` so
readers can locate the relevant startup behavior.
In `@cmd/server/main.go`:
- Around line 108-114: The comment above the namespace registration is
incorrect; it mentions "Register Kafka Ingest Namespace Handler" while the code
calls app.NamespaceManager.RegisterHandler(app.LedgerNamespaceHandler). Update
the comment to accurately describe the action (e.g., "Register Ledger Namespace
Handler before creating the default namespace...") so it matches the code, or if
the intent was to register the Kafka handler instead, replace
app.LedgerNamespaceHandler with the correct handler reference. Locate the call
to app.NamespaceManager.RegisterHandler and the symbol
app.LedgerNamespaceHandler to make the change.
In `@openmeter/ledger/resolvers/account.go`:
- Around line 311-318: The current lockProvisioning converts any
context.DeadlineExceeded from Locker.LockForTX into lockr.ErrLockTimeout, which
hides upstream cancellations; change the error handling in lockProvisioning
(around the call to s.Locker.LockForTX) to distinguish whether the parent ctx
was canceled/expired: if errors.Is(err, context.DeadlineExceeded) then check
ctx.Err() and return ctx.Err() (preserving the caller's deadline/cancellation)
otherwise return lockr.ErrLockTimeout; reference the function lockProvisioning,
the call s.Locker.LockForTX, and the sentinel lockr.ErrLockTimeout when making
this change.
---
Outside diff comments:
In `@openmeter/ledger/resolvers/account.go`:
- Around line 50-91: CreateCustomerAccounts can leave a namespace
half-provisioned because it doesn't ensure the shared business accounts exist;
before creating per-customer accounts call or verify namespace bootstrap by
invoking the existing EnsureBusinessAccounts (or a Repo.IsNamespaceBootstrapped
check) for ns and handle errors, e.g. call
s.AccountService.EnsureBusinessAccounts(ctx, ns) (or return a clear error/retry
if the namespace isn't bootstrapped) after lockCustomerProvisioning and before
iterating ledger.CustomerAccountTypes so downstream GetBusinessAccounts calls
won't see missing business accounts.
---
Nitpick comments:
In `@cmd/billing-worker/main.go`:
- Around line 86-90: Hoist the default namespace into a local variable before
calling EnsureBusinessAccounts so you reuse it and include it in logs; e.g.,
call ns := app.NamespaceManager.GetDefaultNamespace() then pass ns to
app.LedgerAccountResolver.EnsureBusinessAccounts(ctx, ns) and, on error, log the
namespace in logger.Error("failed to provision ledger business accounts",
"namespace", ns, "error", err) before exiting to provide clearer startup
context.
In `@openmeter/customer/service/ledger_hook_test.go`:
- Around line 60-106: Add a new test that simulates a partial ledger write
failure (e.g.,
TestCustomerService_CreateCustomerRollsBackOnPartialLedgerFailure) instead of
only using failingCustomerAccountProvisioner which fails pre-write; implement a
new provisioner type (e.g., partialFailingCustomerAccountProvisioner) that
implements CreateCustomerAccounts by inserting one account/mapping (using the
same code path the real hook uses) and then returning a non-nil error, register
that hook via ledgerresolvers.NewCustomerLedgerHook and
env.CustomerService.RegisterHooks, call env.CustomerService.CreateCustomer,
assert it returns the injected error and that created is nil, then verify both
the customer (env.CustomerService.GetCustomer) and any ledger rows (query the
ledger store/repo used by the hook, or call the ledger read API) are not
present; reuse symbols from the diff: CustomerLedgerHook,
CreateCustomerAccounts, env.CustomerService and ledger.CustomerAccounts to
locate the code to change.
🪄 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: dc8f0ec0-1328-4a98-b32b-ae154735d164
📒 Files selected for processing (21)
AGENTS.mdapp/common/customer.goapp/common/ledger.goapp/common/ledger_test.gocmd/billing-worker/main.gocmd/billing-worker/wire.gocmd/billing-worker/wire_gen.gocmd/server/main.gocmd/server/wire.gocmd/server/wire_gen.goopenmeter/customer/service/ledger_hook_test.goopenmeter/ledger/account.goopenmeter/ledger/errors.goopenmeter/ledger/ledger_fx_test.goopenmeter/ledger/resolvers/account.goopenmeter/ledger/resolvers/account_test.goopenmeter/ledger/resolvers/hooks.goopenmeter/ledger/resolvers/namespace.goopenmeter/ledger/testutils/deps.goopenmeter/ledger/testutils/integration.gotest/credits/sanity_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
openmeter/ledger/account.go (1)
35-45: Keep the account-type catalogs andValidate()on one source of truth.
CustomerAccountTypesandBusinessAccountTypeslook like the canonical sets now, butAccountType.Validate()still hardcodes the same values separately. The next account-type addition can easily update one path and miss the other, leaving provisioning and validation out of sync.Also applies to: 53-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/account.go` around lines 35 - 45, Account-type lists and validation are duplicated: update AccountType.Validate() to use the canonical slices (CustomerAccountTypes and BusinessAccountTypes) instead of hardcoding values; e.g., create a helper that iterates the combined slice (CustomerAccountTypes + BusinessAccountTypes) or a single AllAccountTypes slice and have Validate() check membership against that, ensuring future additions only change the canonical list(s) (CustomerAccountTypes/BusinessAccountTypes or AllAccountTypes) and not Validate() itself.openmeter/ledger/resolvers/account.go (1)
168-196: Skip the secondListAccountsround-trip after provisioning.
EnsureBusinessAccountsalready has the fullexistingmap after creating the missing accounts, butreturn s.GetBusinessAccounts(...)forces anotherlistBusinessAccountsByTypecall before returning. A small shared helper that buildsledger.BusinessAccountsfrom the map would keep the validation/conversion logic in one place and trim one DB read from every provisioning run. As per coding guidelines, "Performance should be a priority in critical code paths. Anything related to event ingestion, message processing, database operations (regardless of database) should be vetted for potential performance bottlenecks."Also applies to: 199-250
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/resolvers/account.go` around lines 168 - 196, The EnsureBusinessAccounts function does unnecessary second DB read by calling GetBusinessAccounts after populating the existing map; instead, create a small helper (e.g., buildBusinessAccountsFromMap or similar) that converts the map returned by listBusinessAccountsByType into ledger.BusinessAccounts and return that directly from EnsureBusinessAccounts (and the similar block around GetBusinessAccounts usage at the later code region). Update EnsureBusinessAccounts to use existing (the map keyed by accountType) to construct and return ledger.BusinessAccounts without calling GetBusinessAccounts or listBusinessAccountsByType again, keeping validation/conversion logic centralized in the new helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/billing-worker/main.go`:
- Around line 86-90: The error handlers that call os.Exit(1) skip deferred
cleanup; before each os.Exit(1) add a call to cleanup() so resources are
released. Specifically, update the error branches around
app.LedgerAccountResolver.EnsureBusinessAccounts(ctx,
app.NamespaceManager.GetDefaultNamespace()) and the other similar early-init
error handler to invoke cleanup() immediately before calling os.Exit(1), keeping
the existing logger.Error calls and error handling intact.
---
Nitpick comments:
In `@openmeter/ledger/account.go`:
- Around line 35-45: Account-type lists and validation are duplicated: update
AccountType.Validate() to use the canonical slices (CustomerAccountTypes and
BusinessAccountTypes) instead of hardcoding values; e.g., create a helper that
iterates the combined slice (CustomerAccountTypes + BusinessAccountTypes) or a
single AllAccountTypes slice and have Validate() check membership against that,
ensuring future additions only change the canonical list(s)
(CustomerAccountTypes/BusinessAccountTypes or AllAccountTypes) and not
Validate() itself.
In `@openmeter/ledger/resolvers/account.go`:
- Around line 168-196: The EnsureBusinessAccounts function does unnecessary
second DB read by calling GetBusinessAccounts after populating the existing map;
instead, create a small helper (e.g., buildBusinessAccountsFromMap or similar)
that converts the map returned by listBusinessAccountsByType into
ledger.BusinessAccounts and return that directly from EnsureBusinessAccounts
(and the similar block around GetBusinessAccounts usage at the later code
region). Update EnsureBusinessAccounts to use existing (the map keyed by
accountType) to construct and return ledger.BusinessAccounts without calling
GetBusinessAccounts or listBusinessAccountsByType again, keeping
validation/conversion logic centralized in the new helper.
🪄 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: 483263a8-9fa3-45b3-a4fa-36092117c994
📒 Files selected for processing (21)
AGENTS.mdapp/common/customer.goapp/common/ledger.goapp/common/ledger_test.gocmd/billing-worker/main.gocmd/billing-worker/wire.gocmd/billing-worker/wire_gen.gocmd/server/main.gocmd/server/wire.gocmd/server/wire_gen.goopenmeter/customer/service/ledger_hook_test.goopenmeter/ledger/account.goopenmeter/ledger/errors.goopenmeter/ledger/ledger_fx_test.goopenmeter/ledger/resolvers/account.goopenmeter/ledger/resolvers/account_test.goopenmeter/ledger/resolvers/hooks.goopenmeter/ledger/resolvers/namespace.goopenmeter/ledger/testutils/deps.goopenmeter/ledger/testutils/integration.gotest/credits/sanity_test.go
✅ Files skipped from review due to trivial changes (4)
- AGENTS.md
- openmeter/ledger/testutils/deps.go
- openmeter/ledger/errors.go
- openmeter/ledger/resolvers/namespace.go
🚧 Files skipped from review as they are similar to previous changes (8)
- cmd/billing-worker/wire.go
- cmd/server/main.go
- app/common/ledger_test.go
- openmeter/ledger/resolvers/hooks.go
- openmeter/customer/service/ledger_hook_test.go
- cmd/server/wire.go
- openmeter/ledger/resolvers/account_test.go
- app/common/customer.go
93d15ee to
0ed733e
Compare
Merged
3ac6c7d to
3b403c6
Compare
…regenerate Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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/resolvers/account.go (1)
199-250:⚠️ Potential issue | 🟡 MinorEnsure GetBusinessAccounts is provisioned before use in transaction resolvers.
This is a breaking change—
GetBusinessAccountsnow requires priorEnsureBusinessAccountsprovisioning. The transaction resolvers inearnings.go,fx.go, andcustomer.goall call it directly.The good news: this is mostly handled automatically.
CreateNamespacetriggers provisioning via the namespace handler, so transactions that happen within an existing namespace will work fine. The billing-worker also explicitly provisions on startup, which is correct.Just keep an eye on any new code paths that might invoke transaction resolvers before a namespace is fully initialized—they'd hit
ErrBusinessAccountMissingif provisioning hasn't happened first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/resolvers/account.go` around lines 199 - 250, GetBusinessAccounts can now return ErrBusinessAccountMissing unless provisioning has run; update all transaction resolver call sites (notably in the resolvers referenced by earnings.go, fx.go, and customer.go) to call or await EnsureBusinessAccounts for the namespace before invoking GetBusinessAccounts, and handle the error path by retrying or returning a clear error if provisioning fails; specifically, insert a precondition that invokes EnsureBusinessAccounts(ctx, namespace) (or the appropriate namespace provisioning helper) prior to calling AccountResolver.GetBusinessAccounts and propagate or wrap ErrBusinessAccountMissing if provisioning cannot be completed.
🧹 Nitpick comments (3)
openmeter/ledger/account/account.go (1)
75-79: Reasonable defensive guard, consider using a sentinel error.The nil check prevents a nil pointer panic, and the TODO acknowledges this is a temporary workaround. One small improvement: wrapping a sentinel error would make it easier for callers to handle this case programmatically rather than relying on string matching.
var ErrQuerierNotConfigured = errors.New("querier not configured")Then:
return nil, fmt.Errorf("account %s in namespace %s cannot query balances: %w", a.data.ID.ID, a.data.ID.Namespace, ErrQuerierNotConfigured)Not blocking since this is already flagged as a hack to be fixed later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/account/account.go` around lines 75 - 79, Introduce a sentinel error (e.g., ErrQuerierNotConfigured) and return it wrapped instead of a plain formatted string so callers can check the condition programmatically; replace the current nil-guard return that references a.services.Querier and a.data.ID.ID/a.data.ID.Namespace with a wrapped error using %w and the sentinel (ErrQuerierNotConfigured) so the original context message is preserved while allowing errors.Is checks by callers.openmeter/ledger/account.go (1)
35-45: Keep these slices in sync with the constants.These slices duplicate the account type constants defined above. If a new account type is added (like the commented-out
AccountTypeCustomerBreakage), don't forget to update both the const block and the corresponding slice, plus theValidate()method.A minor improvement could be generating these programmatically or adding a compile-time check, but that's probably overkill for now.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/account.go` around lines 35 - 45, The CustomerAccountTypes and BusinessAccountTypes slices are duplicating the AccountType constants and can fall out of sync; update these slices whenever you add/remove an AccountType (e.g., AccountTypeCustomerBreakage) and also update the Validate() method to include the new type; to avoid future drift, consider replacing the hard-coded slices with a single authoritative source (e.g., a function or var that builds the slice from the defined constants) or add a compile-time/checking test that asserts every AccountType constant appears in one of CustomerAccountTypes or BusinessAccountTypes, and adjust references to CustomerAccountTypes, BusinessAccountTypes and Validate() accordingly.app/common/ledger.go (1)
69-74: Acknowledged tech debt, but worth tracking.The TODO is appreciated - this clearly documents the workaround for the circular dependency. Just want to flag that this setup is a bit fragile: if anyone later calls
GetBalance()on an account retrieved through this internalaccountSvc, it'll fail at runtime with"querier is not configured".Currently safe because
lockAccountsForTransactionInputsonly usesLock()andAsCustomerAccount()(no balance queries), but a future refactor could easily miss this constraint.Might be worth creating a tracking issue if there isn't one already, so the architectural fix doesn't get lost in the shuffle.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/common/ledger.go` around lines 69 - 74, Account construction uses accountservice.New with ledgeraccount.AccountLiveServices missing Querier which makes any future call to GetBalance on accounts from this service fail with a vague "querier is not configured"; create a tracking issue and reference its ID in the TODO, and replace the nil/hack with a small explicit stub implementation for the Querier interface (e.g., a ledgeraccount.QuerierStub) that returns a clear, descriptive error or panics when GetBalance is called so callers fail fast and include guidance to use the real Querier; update the comment near accountservice.New/accountRepo/ledgeraccount.AccountLiveServices/locker to note that lockAccountsForTransactionInputs currently only relies on Lock and AsCustomerAccount and that the stub prevents silent misuse.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@openmeter/ledger/resolvers/account.go`:
- Around line 199-250: GetBusinessAccounts can now return
ErrBusinessAccountMissing unless provisioning has run; update all transaction
resolver call sites (notably in the resolvers referenced by earnings.go, fx.go,
and customer.go) to call or await EnsureBusinessAccounts for the namespace
before invoking GetBusinessAccounts, and handle the error path by retrying or
returning a clear error if provisioning fails; specifically, insert a
precondition that invokes EnsureBusinessAccounts(ctx, namespace) (or the
appropriate namespace provisioning helper) prior to calling
AccountResolver.GetBusinessAccounts and propagate or wrap
ErrBusinessAccountMissing if provisioning cannot be completed.
---
Nitpick comments:
In `@app/common/ledger.go`:
- Around line 69-74: Account construction uses accountservice.New with
ledgeraccount.AccountLiveServices missing Querier which makes any future call to
GetBalance on accounts from this service fail with a vague "querier is not
configured"; create a tracking issue and reference its ID in the TODO, and
replace the nil/hack with a small explicit stub implementation for the Querier
interface (e.g., a ledgeraccount.QuerierStub) that returns a clear, descriptive
error or panics when GetBalance is called so callers fail fast and include
guidance to use the real Querier; update the comment near
accountservice.New/accountRepo/ledgeraccount.AccountLiveServices/locker to note
that lockAccountsForTransactionInputs currently only relies on Lock and
AsCustomerAccount and that the stub prevents silent misuse.
In `@openmeter/ledger/account.go`:
- Around line 35-45: The CustomerAccountTypes and BusinessAccountTypes slices
are duplicating the AccountType constants and can fall out of sync; update these
slices whenever you add/remove an AccountType (e.g.,
AccountTypeCustomerBreakage) and also update the Validate() method to include
the new type; to avoid future drift, consider replacing the hard-coded slices
with a single authoritative source (e.g., a function or var that builds the
slice from the defined constants) or add a compile-time/checking test that
asserts every AccountType constant appears in one of CustomerAccountTypes or
BusinessAccountTypes, and adjust references to CustomerAccountTypes,
BusinessAccountTypes and Validate() accordingly.
In `@openmeter/ledger/account/account.go`:
- Around line 75-79: Introduce a sentinel error (e.g., ErrQuerierNotConfigured)
and return it wrapped instead of a plain formatted string so callers can check
the condition programmatically; replace the current nil-guard return that
references a.services.Querier and a.data.ID.ID/a.data.ID.Namespace with a
wrapped error using %w and the sentinel (ErrQuerierNotConfigured) so the
original context message is preserved while allowing errors.Is checks by
callers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2f4d031d-d6eb-47f5-a2fc-41a475c93b21
📒 Files selected for processing (23)
AGENTS.mdapp/common/customer.goapp/common/ledger.goapp/common/ledger_test.gocmd/billing-worker/main.gocmd/billing-worker/wire.gocmd/billing-worker/wire_gen.gocmd/jobs/internal/wire_gen.gocmd/server/main.gocmd/server/wire.gocmd/server/wire_gen.goopenmeter/customer/service/ledger_hook_test.goopenmeter/ledger/account.goopenmeter/ledger/account/account.goopenmeter/ledger/errors.goopenmeter/ledger/ledger_fx_test.goopenmeter/ledger/resolvers/account.goopenmeter/ledger/resolvers/account_test.goopenmeter/ledger/resolvers/hooks.goopenmeter/ledger/resolvers/namespace.goopenmeter/ledger/testutils/deps.goopenmeter/ledger/testutils/integration.gotest/credits/sanity_test.go
✅ Files skipped from review due to trivial changes (6)
- AGENTS.md
- openmeter/ledger/testutils/integration.go
- openmeter/ledger/resolvers/namespace.go
- openmeter/customer/service/ledger_hook_test.go
- test/credits/sanity_test.go
- openmeter/ledger/resolvers/account_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- openmeter/ledger/testutils/deps.go
- cmd/billing-worker/wire.go
- cmd/server/wire.go
- cmd/server/main.go
- openmeter/ledger/errors.go
- app/common/customer.go
- app/common/ledger_test.go
- cmd/billing-worker/wire_gen.go
3b403c6 to
c418182
Compare
turip
approved these changes
Apr 2, 2026
This was referenced Apr 2, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Fixes #(issue)
Notes for reviewer
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation