feat: add wire for charges#4058
Merged
Merged
Conversation
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a BillingRegistry (billing.Service plus optional ChargesRegistry), implements a new charges factory stack and ledger routing validator provider, and updates wiring and tests to construct ledger/charges/locker components from lower-level package constructors instead of app/common wiring. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant App as Application (wire)
participant Ledger as LedgerStack
participant BillingReg as BillingRegistry
participant BillingSvc as Billing.Service
participant ChargesReg as ChargesRegistry
participant DB as Database
Client->>App: start/init (reads conf.Credits)
App->>Ledger: construct LedgerStack (repos, validators, locker)
App->>BillingReg: call NewBillingRegistry(ledger, conf.Credits, ...)
BillingReg->>BillingSvc: newBillingService(...)
alt credits enabled
BillingReg->>ChargesReg: newChargesRegistry(...) (build adapters, handlers, services)
ChargesReg->>DB: initialize adapters/ledgers using DB
BillingReg->>BillingSvc: attach ChargesReg
end
BillingReg-->>App: return BillingRegistry (BillingSvc + optional ChargesReg)
App->>BillingSvc: use for provisioning / router / workers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/common/charges.go (1)
223-234: Consider a small deps struct fornewChargesRegistry.Ten positional arguments is starting to get pretty brittle here, especially with several related billing / ledger services in the mix. Bundling them into a config struct would make the wiring easier to scan and safer to extend.
As per coding guidelines,
**/*.go: In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/common/charges.go` around lines 223 - 234, The newChargesRegistry function signature is brittle with ten positional parameters; create a small deps/config struct (e.g., ChargesDeps or NewChargesConfig) that contains fields for *entdb.Client, *slog.Logger, *lockr.Locker, billing.Service, rating.Service, feature.FeatureConnector, streaming.Connector, ledger.Ledger, ledger.AccountResolver, and ledgeraccount.Service, update newChargesRegistry to accept that struct (or pointer) and construct the ChargesRegistry from its fields, then update all callers to build and pass the struct; keep the ChargesRegistry type and internal field names unchanged to minimize churn and add a short constructor helper if needed to preserve call-site clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/common/charges.go`:
- Around line 223-234: The newChargesRegistry function signature is brittle with
ten positional parameters; create a small deps/config struct (e.g., ChargesDeps
or NewChargesConfig) that contains fields for *entdb.Client, *slog.Logger,
*lockr.Locker, billing.Service, rating.Service, feature.FeatureConnector,
streaming.Connector, ledger.Ledger, ledger.AccountResolver, and
ledgeraccount.Service, update newChargesRegistry to accept that struct (or
pointer) and construct the ChargesRegistry from its fields, then update all
callers to build and pass the struct; keep the ChargesRegistry type and internal
field names unchanged to minimize churn and add a short constructor helper if
needed to preserve call-site clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 77269598-1fdf-49ba-b437-fd9dfee9331e
📒 Files selected for processing (1)
app/common/charges.go
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
8a6b686 to
4b44259
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/common/charges.go (1)
32-65: Optional: Consider adding brief doc comments to exported functions.Functions like
NewChargesMetaAdapter,NewChargesFlatFeeHandler, etc. are exported and could benefit from a one-liner doc comment explaining what they create. This helps with IDE autocomplete and godoc.📝 Example doc comments
+// NewChargesMetaAdapter creates the shared metadata adapter for charges. func NewChargesMetaAdapter( db *entdb.Client, logger *slog.Logger, ) (meta.Adapter, error) {+// NewChargesFlatFeeHandler creates the ledger-backed handler for flat-fee charges. func NewChargesFlatFeeHandler( ledgerService ledger.Ledger, accountResolver ledger.AccountResolver, accountService ledgeraccount.Service, ) flatfee.Handler {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/common/charges.go` around lines 32 - 65, Add single-line godoc comments above each exported constructor function to describe what they create and return: e.g., add comments for NewChargesMetaAdapter, NewChargesFlatFeeHandler, NewChargesCreditPurchaseHandler, and NewChargesUsageBasedHandler using the standard Go comment form "// NewChargesMetaAdapter ...", briefly stating the purpose (e.g., creates a charges meta.Adapter connected to entdb.Client and logger) and the return type; keep each comment one sentence and placed immediately above the corresponding function.app/common/app.go (1)
93-107: Minor: Parameter naming inconsistency.
NewAppSandboxFactoryusesbilling BillingRegistrywhile the other functions usebillingRegistry BillingRegistry. This is a small thing, but consistent naming helps readability.🔧 Optional rename for consistency
func NewAppSandboxFactory( appsConfig config.AppsConfiguration, appService app.Service, - billing BillingRegistry, + billingRegistry BillingRegistry, ) (*appsandbox.Factory, error) { factory, err := appsandbox.NewFactory(appsandbox.Config{ AppService: appService, - BillingService: billing.Billing, + BillingService: billingRegistry.Billing, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/common/app.go` around lines 93 - 107, Rename the parameter in NewAppSandboxFactory from billing to billingRegistry to match the naming used across other functions; update the function signature (NewAppSandboxFactory(appsConfig config.AppsConfiguration, appService app.Service, billingRegistry BillingRegistry)) and all internal references (e.g., BillingService: billingRegistry.Billing) so the BillingRegistry type name and usage are consistent with other functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/common/app.go`:
- Around line 93-107: Rename the parameter in NewAppSandboxFactory from billing
to billingRegistry to match the naming used across other functions; update the
function signature (NewAppSandboxFactory(appsConfig config.AppsConfiguration,
appService app.Service, billingRegistry BillingRegistry)) and all internal
references (e.g., BillingService: billingRegistry.Billing) so the
BillingRegistry type name and usage are consistent with other functions.
In `@app/common/charges.go`:
- Around line 32-65: Add single-line godoc comments above each exported
constructor function to describe what they create and return: e.g., add comments
for NewChargesMetaAdapter, NewChargesFlatFeeHandler,
NewChargesCreditPurchaseHandler, and NewChargesUsageBasedHandler using the
standard Go comment form "// NewChargesMetaAdapter ...", briefly stating the
purpose (e.g., creates a charges meta.Adapter connected to entdb.Client and
logger) and the return type; keep each comment one sentence and placed
immediately above the corresponding function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d32c8f7a-6f87-45d2-8850-a142111a4f27
📒 Files selected for processing (16)
AGENTS.mdapp/common/app.goapp/common/billing.goapp/common/charges.goapp/common/ledger.goapp/common/openmeter_billingworker.gocmd/billing-worker/wire_gen.gocmd/jobs/internal/wire.gocmd/jobs/internal/wire_gen.gocmd/server/main.gocmd/server/wire.gocmd/server/wire_gen.goopenmeter/billing/worker/worker.goopenmeter/ledger/testutils/deps.goopenmeter/ledger/testutils/integration.gotest/credits/sanity_test.go
💤 Files with no reviewable changes (1)
- openmeter/billing/worker/worker.go
✅ Files skipped from review due to trivial changes (2)
- AGENTS.md
- openmeter/ledger/testutils/deps.go
🚧 Files skipped from review as they are similar to previous changes (6)
- cmd/server/wire.go
- cmd/server/main.go
- cmd/jobs/internal/wire.go
- app/common/openmeter_billingworker.go
- app/common/ledger.go
- cmd/billing-worker/wire_gen.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/common/billing.go (1)
124-145: Consider collapsing the credits-only deps into one input.
locker,ledgerService,accountResolver, andaccountServiceare only used inside thecreditsConfig.Enabledbranch, but keeping them on the top-level constructor means every Wire graph has to grow with that optional path. You can already see that incmd/server/wire_gen.goLines 309-316, where the ledger stack is built before the branch is evaluated. A smallChargesDepsbundle or factory would keep this constructor easier to read and evolve.As per coding guidelines: "In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/common/billing.go` around lines 124 - 145, The constructor NewBillingRegistry currently takes locker, ledgerService, accountResolver, and accountService even though they are only used when creditsConfig.Enabled; create a small composite type (e.g., ChargesDeps) or a provider factory (e.g., ChargesDepsFactory func() (*ChargesDeps, error)) that groups locker, ledgerService, accountResolver, and accountService, change NewBillingRegistry to accept that bundle or factory instead of the individual symbols, and update the constructor body to only call or dereference the bundle when creditsConfig.Enabled (keep existing logic that uses those symbols but reference them via ChargesDeps.*); also update your dependency injection/wire wiring to construct the ChargesDeps only for the credits-enabled branch so the top-level graph no longer includes ledger/locker/account providers unconditionally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/common/billing.go`:
- Around line 124-145: The constructor NewBillingRegistry currently takes
locker, ledgerService, accountResolver, and accountService even though they are
only used when creditsConfig.Enabled; create a small composite type (e.g.,
ChargesDeps) or a provider factory (e.g., ChargesDepsFactory func()
(*ChargesDeps, error)) that groups locker, ledgerService, accountResolver, and
accountService, change NewBillingRegistry to accept that bundle or factory
instead of the individual symbols, and update the constructor body to only call
or dereference the bundle when creditsConfig.Enabled (keep existing logic that
uses those symbols but reference them via ChargesDeps.*); also update your
dependency injection/wire wiring to construct the ChargesDeps only for the
credits-enabled branch so the top-level graph no longer includes
ledger/locker/account providers unconditionally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1755d0e8-dbaa-4e84-9828-cfff9495ca55
📒 Files selected for processing (2)
app/common/billing.gocmd/server/wire_gen.go
tothandras
approved these changes
Apr 2, 2026
This was referenced Apr 2, 2026
Merged
Merged
This was referenced Apr 8, 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.
Overview
This patch initializes charges and ledger when the credits.enabled = true everywhere where we are using billing related logic.
The new BillingRegistry type enforces that whatever uses billing must be initializing it alongside charges.
Notes for reviewer
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores