feat(api): Add API Handlers to the v3 currencies API#3891
Merged
Conversation
mark-vass-konghq
added
the
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 full currency feature: domain models and service interface, adapter and service implementations, HTTP handlers for listing/creating currencies and cost bases, and wiring into the v3 server/router and application DI (including test stubs). Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler
participant Service
participant Adapter
participant Database
Client->>Handler: HTTP request (e.g., POST /v3/currencies)
Handler->>Handler: Decode request, build input
Handler->>Service: Call CreateCurrency/CreateCostBasis/List/GetCostBases
Service->>Adapter: Delegate operation
Adapter->>Database: Run query/insert (within tx if needed)
Database-->>Adapter: Result / Error
Adapter-->>Service: Return domain result or error
Service-->>Handler: Return result or error
Handler->>Handler: Map to API type, translate errors, set status
Handler-->>Client: HTTP response (200/201/4xx)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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/currencies-v3-api-handlers
branch
from
de8cf05 to
394616b
Compare
![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: 16
🧹 Nitpick comments (10)
openmeter/currencies/adapter.go (1)
16-21:CreateCurrencyleaks a gobl type while every other method uses local types.
ListCurrenciesreturns[]CurrencyandCreateCostBasisreturns*CostBasis, butCreateCurrencyreturns*currency.Deffrom the gobl package. This forces callers to deal with an external type inconsistently — ideallyCreateCurrencyshould return a*Currency(local) to keep the interface self-contained. Same pattern holds forCurrencyServiceinservice.go.♻️ Proposed change
type CurrenciesAdapter interface { ListCurrencies(ctx context.Context) ([]Currency, error) - CreateCurrency(ctx context.Context, params CreateCurrencyInput) (*currency.Def, error) + CreateCurrency(ctx context.Context, params CreateCurrencyInput) (*Currency, error) CreateCostBasis(ctx context.Context, params CreateCostBasisInput) (*CostBasis, error) GetCostBasesByCurrencyID(ctx context.Context, currencyID string) ([]CostBasis, error) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/currencies/adapter.go` around lines 16 - 21, The CreateCurrency method on CurrenciesAdapter (and the corresponding method on CurrencyService in service.go) currently returns the external type *currency.Def which leaks the gobl package; change the signature to return *Currency (the local type) and update all implementations and callers to construct/return a local *Currency instead of *currency.Def (convert or map fields from currency.Def where implementations currently build/receive it), ensuring CreateCurrency(ctx context.Context, params CreateCurrencyInput) (*Currency, error) is used consistently across CurrenciesAdapter and CurrencyService.openmeter/ent/schema/custom_currencies.go (1)
17-25: Replace localTimeMixinwithentutils.TimeMixinfor consistency.You've already got
entutilsimported here, andTimeMixinis actually defined over inpkg/framework/entutils/mixins.go. Pretty much every other schema file usesentutils.TimeMixin{}directly. The local version here is a trimmed-down copy with onlycreated_at(whereas the standard one also includesupdated_at).If you're intentionally keeping just
created_atbecause these currency definitions are immutable, that's a fair design choice—just drop the localTimeMixindefinition and useentutils.TimeMixin{}from the mixin list instead. Keeps things DRY and in sync with the rest of the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ent/schema/custom_currencies.go` around lines 17 - 25, Remove the local TimeMixin definition in custom_currencies.go and replace uses of the local TimeMixin with entutils.TimeMixin{} in the schema mixin list; specifically, delete the type TimeMixin struct / func (TimeMixin) Fields block and update the schema's Mixin declarations to reference entutils.TimeMixin{} so the file uses the shared mixin (which includes created_at and updated_at) and stays consistent with other schemas.api/spec/src/v3/openmeter.tsp (1)
81-87: Tagging currencies underShared.BillingTag— intentional?Both endpoints land under the "Billing" section of the generated API docs. Currencies feel like a broader domain concept (not strictly billing-specific), so a dedicated
CurrenciesTagwith its own@tagMetadatamight give them better visibility. That said, if co-locating them with billing profiles is the intended UX, this is fine as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/src/v3/openmeter.tsp` around lines 81 - 87, The current endpoints CurrenciesEndpoints and CostBasesEndpoints are tagged with Shared.BillingTag which groups them under Billing; if you want them surfaced separately create a new tag (e.g., CurrenciesTag) and apply it to the interfaces instead. Change the `@tag`(Shared.BillingTag) annotations on CurrenciesEndpoints and CostBasesEndpoints to `@tag`(Shared.CurrenciesTag) (or similar), and add a corresponding `@tagMetadata` declaration for CurrenciesTag (title/description) in the spec so generated docs show a distinct Currencies section; if you intend them to remain under Billing leave the tags as-is.api/spec/src/v3/currencies/custom.tsp (1)
1-2: Missing explicit import forbase.tsp.
CurrencyBaseandCustomCurrencyCodeboth come frombase.tsp, but it's not imported here. This works in practice becauseindex.tsppulls everything into the compilation unit, butcost-bases.tspin the same PR explicitly importscost-basis.tspfor itsCostBasisdependency — it's worth being consistent.📦 Proposed fix
import "../shared/index.tsp"; +import "./base.tsp";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/src/v3/currencies/custom.tsp` around lines 1 - 2, Add an explicit import for base.tsp at the top of this module so CurrencyBase and CustomCurrencyCode are resolved directly rather than relying on index.tsp; update the imports in custom.tsp to include base.tsp (so symbols CurrencyBase and CustomCurrencyCode are imported explicitly) to match the explicit-import style used by cost-bases.tsp and ensure consistent dependency declaration.api/spec/src/v3/currencies/cost-bases/cost-bases.tsp (1)
10-15: Consider whether pagination metadata belongs here.
CostBasesis a bare array wrapper with nototal,page, orlimitfields. That's totally fine if cost bases per currency are expected to stay small, but worth a deliberate call — adding pagination later is a breaking API change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/src/v3/currencies/cost-bases/cost-bases.tsp` around lines 10 - 15, The CostBases model currently wraps only data: CostBasis[] which makes adding pagination later a breaking change; decide whether lists can grow and, if so, change model CostBases to include pagination metadata (e.g., total: number, page: number, limit: number or nextCursor: string) alongside data: CostBasis[] and update any handlers/serializers that return CostBases (search for CostBases and any endpoints referencing it) to populate those fields; if you intentionally expect small lists, add a code comment on the CostBases model documenting the deliberate choice to remain an unpaginated array.api/spec/src/v3/currencies/base.tsp (1)
12-31: Consider adding length constraints onsymbol.
namehas@minLength(1)/@maxLength(256)butsymbolhas no constraints at all. It could be submitted as an empty string or an unreasonably long value. A short max (e.g., 10–12 characters) and a@minLength(1)would tighten validation nicely.Proposed fix
/** * Symbol representation of the currency. */ `@visibility`(Lifecycle.Create, Lifecycle.Read) + `@minLength`(1) + `@maxLength`(12) symbol: string;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/src/v3/currencies/base.tsp` around lines 12 - 31, The CurrencyBase model's symbol field lacks length validation allowing empty or excessively long strings; update the symbol property in model CurrencyBase to add `@minLength`(1) and a sensible `@maxLength` (e.g., `@maxLength`(12)) so it cannot be empty or unreasonably long, keeping the existing `@visibility`(Lifecycle.Create, Lifecycle.Read) decorator intact.api/spec/src/v3/currencies/operations.tsp (1)
18-24: Pagination metadata without pagination parameters?
CurrenciesResponseuses@pageItemsondata, which typically signals a paginated list. But thelist()operation doesn't accept any pagination query params (page, pageSize, cursor, etc.). If the currency list is expected to remain small enough to always return everything, this might be fine — but it could confuse SDK consumers who see paginated response shapes without a way to paginate. Worth a quick sanity check on intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/src/v3/currencies/operations.tsp` around lines 18 - 24, CurrenciesResponse currently marks data with `@pageItems` which implies pagination, but the list() operation has no pagination params; either remove the `@pageItems` decorator from the CurrenciesResponse model (CurrenciesResponse.data) if you intend to always return the full list, or add standard pagination query parameters to the list() operation (e.g., page, pageSize or cursor) and implement handling so the `@pageItems` response matches the operation behavior; update the model or operation accordingly (look for CurrenciesResponse and the list() operation definitions).openmeter/currencies/adapter/currencies.go (1)
135-143: Potential nil-pointer dereference ifCurrencyedge isn't loaded.Line 138 accesses
costBasis.Edges.Currency.IDdirectly. WhileWithCurrency()on line 127 should ensure it's loaded, a defensive nil check would prevent a panic if the edge is missing (e.g., orphaned record, schema migration issue).Proposed guard
return lo.Map(costBases, func(costBasis *entdb.CurrencyCostBasis, _ int) currencies.CostBasis { + currencyID := "" + if costBasis.Edges.Currency != nil { + currencyID = costBasis.Edges.Currency.ID + } return currencies.CostBasis{ ID: costBasis.ID, - CurrencyID: costBasis.Edges.Currency.ID, + CurrencyID: currencyID, FiatCode: costBasis.FiatCode, Rate: costBasis.Rate, EffectiveFrom: costBasis.EffectiveFrom, } }), nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/currencies/adapter/currencies.go` around lines 135 - 143, The mapping dereferences costBasis.Edges.Currency.ID without a nil check; modify the code that converts costBases into []currencies.CostBasis to defensively handle missing Currency edge: iterate over costBases (or change the lo.Map callback) and if costBasis.Edges.Currency == nil, return an error (or skip/assign zero value) instead of dereferencing it; update the function that returns the mapped slice to propagate that error (or ensure the mapping sets CurrencyID to a safe zero value) and keep the reference to WithCurrency() in mind when locating the code to change.openmeter/currencies/service/service.go (1)
17-21: Consider validating the adapter isn't nil.If someone accidentally passes a
niladapter toNew(), you'll get a nil-pointer dereference at call time with no helpful error message. A quick guard keeps things explicit and fail-fast.Proposed fix
-func New(adapter currencies.Adapter) *Service { +func New(adapter currencies.Adapter) (*Service, error) { + if adapter == nil { + return nil, errors.New("adapter is required") + } return &Service{ adapter: adapter, - } + }, nil }Note: this would require updating the call sites to handle the error. An alternative is to just panic — the existing codebase might have a convention for this.
#!/bin/bash # Check how other services in the codebase handle nil adapter/dependency checks in constructors rg -n 'func New\(' --type=go -g '*/service/service.go' -A 10 | head -80🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/currencies/service/service.go` around lines 17 - 21, The constructor New currently accepts an adapter that may be nil, causing later nil-pointer panics; update New(adapter currencies.Adapter) to validate adapter != nil and return (*Service, error) with a clear error (e.g., "nil adapter") when missing, and update all call sites to handle the error, or if project convention prefers, document and use a deliberate panic instead; locate the New function and the Service struct in service.go and ensure subsequent uses of Service.adapter are safe after this change.api/v3/api.gen.go (1)
893-894:BillingCurrency.Codetyped as plainstringinstead ofCurrencyCode// Code Standard ISO 4217 code. Code string `json:"code"`Every other currency-code field in this file uses
CurrencyCode(e.g.,BillingCostBasis.FiatCode CurrencyCode,BillingCustomer.Currency *CurrencyCode). Usingstringhere is inconsistent and loses the semantic signal, even thoughCurrencyCode = stringunder the hood.✨ Suggested upstream spec change (fix in TypeSpec)
-Code string `json:"code"` +Code CurrencyCode `json:"code"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/api.gen.go` around lines 893 - 894, BillingCurrency.Code is declared as plain string but should use the existing CurrencyCode alias for consistency and semantic clarity; update the BillingCurrency struct field declaration (BillingCurrency.Code) to use CurrencyCode instead of string, keep the json tag `json:"code"` unchanged, and run/adjust any affected usages or tests that assume a string type to accept CurrencyCode where needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/spec/src/v3/currencies/cost-bases/cost-basis.tsp`:
- Around line 14-18: The doc comment for the field fiat_code is misleading:
update the comment for fiat_code (type Shared.CurrencyCode, visibility
Lifecycle.Create/Read) to state that it is the fiat currency code used for the
exchange rate applied to this cost basis (e.g., "Fiat currency code used for the
exchange rate"), and explicitly note that the parent currency id is provided via
the request path parameter rather than this field so API consumers are not
confused.
In `@api/spec/src/v3/currencies/cost-bases/operations.tsp`:
- Around line 20-28: The doc comment above the get_cost_bases operation is
singular ("Get a cost basis") but the operationId ("get-cost-bases"), summary
("Get cost bases"), and return type (CostBases) are plural; update the doc
comment for the get_cost_bases method to a plural form such as "Get cost bases"
so the comment matches the operationId, `@summary`, and return type (CostBases).
In `@api/spec/src/v3/currencies/currency.tsp`:
- Around line 18-23: Update the doc comment for the "code" field to reflect that
it accepts both standard ISO 4217 codes and custom codes (it is typed as
Shared.CurrencyCode | CustomCurrencyCode). Locate the "code" field declaration
and change the comment from "Standard ISO 4217 code." to something like "ISO
4217 code or custom currency code." so API consumers aren't misled about
supported values.
In `@api/spec/src/v3/currencies/custom.tsp`:
- Around line 12-16: Update the doc comment for the field `code:
CustomCurrencyCode` to clarify that this is a custom scalar following an ISO
4217-like format (e.g., 3-letter uppercase) but is not an official/registered
ISO 4217 currency code; reference `CustomCurrencyCode` in the comment and
optionally give a short example (e.g., "USD-like, 3 uppercase letters") and
intended validation behavior so readers won't assume it's an official ISO code.
In `@api/v3/api.gen.go`:
- Around line 883-884: Update the TypeSpec comment for the FiatCode field to
accurately describe it: change the misleading "currency id" wording to indicate
it's an ISO 4217 currency code (string) — e.g., "The ISO 4217 currency code this
cost basis belongs to." Locate the FiatCode field on the TypeSpec that generates
the FiatCode CurrencyCode `json:"fiat_code"` line and replace the comment so it
clearly distinguishes this from the separate currencyId ULID path parameter.
- Around line 2048-2052: The generated request-body aliases
CreateCustomCurrencyJSONRequestBody = BillingCustomCurrency and
CreateCostBasisJSONRequestBody = BillingCostBasis reuse full response types that
include a non-optional Id field, forcing callers to supply server-generated
ULIDs; update the upstream TypeSpec OpenAPI spec to introduce dedicated create
request types (e.g., CreateCustomCurrencyRequest and CreateCostBasisRequest,
modeled like CreateCustomerRequest) that omit the server-generated Id (or make
it optional/pointer with omitempty) and then re-generate api.gen.go, or
alternatively document and intentionally support client-side ULID generation in
the spec if IDs must be client-provided.
In `@api/v3/handlers/currencies/create_cost_basis.go`:
- Around line 30-36: The handler is discarding the created CostBasis by
assigning the result of h.currencyService.CreateCostBasis to _ and returning
nil; fix the handler to capture the created entity (e.g. created, err :=
h.currencyService.CreateCostBasis(ctx, request)), return that value as the
CreateCostBasisResponse on success, and keep returning the error when err != nil
so the client receives the CreateResponse<CostBasis> instead of an empty body;
update references to CreateCostBasisRequest/CreateCostBasisResponse and ensure
the returned object matches the API's CreateResponse<CostBasis> shape.
In `@api/v3/handlers/currencies/create.go`:
- Around line 30-36: The handler is discarding the created currency and mapping
every error to a 409; change the CreateCurrency handler so it captures the
result from h.currencyService.CreateCurrency (don’t assign to `_`) and return
that value as the CreateCurrencyResponse body, and differentiate error types
instead of blanket-wrapping every error with apierrors.NewConflictError: detect
the specific "already exists" / conflict condition from the service error (or
use a typed sentinel/error check from the currency service) and only then return
apierrors.NewConflictError(ctx, err, "Currency already exists"); for all other
errors return an appropriate error (e.g., apierrors.NewInternalError or
propagate the original error) so validation/db errors aren’t misreported.
In `@openmeter/currencies/adapter/currencies.go`:
- Around line 57-68: The final lo.Map call is a no-op because it reconstructs
each currencies.Currency identical to the input; replace the lo.Map usage by
returning the appended slice directly (use append(customCurrencies, defs...))
and return that with nil error, and then remove the now-unused lo import; locate
the code using lo.Map and the variables customCurrencies and defs in
currencies.go to make the change.
- Around line 100-103: CreateCostBasisInput.Rate is currently float32 and is
passed to alpacadecimal.NewFromFloat32 in the a.db.CurrencyCostBasis.Create()
chain (SetRate), which loses precision for financial rates; change the input to
accept a string (Shared.Numeric) or alpacadecimal.Decimal instead of float32,
update the TypeSpec/API to use Shared.Numeric, and in currencies.go replace
alpacadecimal.NewFromFloat32(params.Rate) with
alpacadecimal.NewFromString(params.Rate) (or assign the Decimal directly if you
accept Decimal) including validation and clear error handling for parse failures
before calling SetRate on the CostBasis model.
- Around line 91-98: In CreateCostBasis, avoid the TOCTOU by calling time.Now()
once into a local variable (e.g., now := time.Now()) and use that single
timestamp for both the default effectiveFrom and the validation check against
params.EffectiveFrom; specifically, replace the two time.Now() calls with now,
validate params.EffectiveFrom.Before(now) and set effectiveFrom =
*params.EffectiveFrom when provided, otherwise set effectiveFrom = now, keeping
the existing error path that returns models.NewGenericConflictError.
In `@openmeter/currencies/models.go`:
- Around line 9-18: The Currency struct is missing JSON tags for several fields;
update the struct definition for Currency to add json tags: set ID to
`json:"id"`, IsCustom to `json:"is_custom"`, DisambiguateSymbol to
`json:"disambiguate_symbol,omitempty"`, and Subunits to `json:"subunits"`
(keeping other existing tags like `symbol,omitempty` and
`smallest_denomination,omitempty` unchanged) so JSON serialization matches the
project's naming convention; modify the tags on the Currency type in models.go
where the fields ID, IsCustom, DisambiguateSymbol, and Subunits are declared.
- Line 38: The CreateCostBasisInput.Rate field currently uses float32 which
causes precision loss for financial data; change the Rate field's type to
alpacadecimal.Decimal (the same type used by CostBasis.Rate), update any code
that constructs or parses CreateCostBasisInput to use alpacadecimal.Decimal, and
add/import the alpacadecimal package in openmeter/currencies/models.go so
serialization/deserialization and DB mapping use the arbitrary-precision Decimal
type instead of float32.
- Around line 20-25: The CreateCurrencyInput.SmallestDenomination is accepted by
the API but never persisted because the CustomCurrency Ent schema lacks that
field and the adapter method CreateCurrency() doesn't set it; update the
persistence layer by adding a SmallestDenomination (int8 or appropriate DB type)
field to the CustomCurrency Ent schema and regenerate ent code, then modify the
adapter's CreateCurrency() to call
SetSmallestDenomination(input.SmallestDenomination) when building the create
query; also add the corresponding DB migration and update any tests that assert
currency persistence to include SmallestDenomination.
In `@openmeter/currencies/service.go`:
- Around line 9-13: The service interface leaks a third‑party type and has a
precision mismatch: update CurrencyService to consistently use your domain
Currency types (change CreateCurrency to return your domain Currency or
*Currency instead of *currency.Def and keep ListCurrencies returning []Currency)
and remove any references to currency.Def from the service contract so the
adapter/mapper layer performs gobl <-> domain conversion; also change
CreateCostBasisInput.Rate from float32 to alpacadecimal.Decimal so it matches
CostBasis.Rate (alpacadecimal.Decimal) to avoid precision loss when creating
cost bases.
In `@tools/migrate/migrations/20260220154446_add_custom_currency.up.sql`:
- Line 11: Remove the redundant unique index statements that duplicate the
PRIMARY KEY b-tree for custom_currencies.id and for the currencycostbasis_id
index (the CREATE UNIQUE INDEX "customcurrency_id" and the analogous
currencycostbasis_id statement); delete those CREATE UNIQUE INDEX lines from the
migration (or change them to non-unique only if a separate non-PK index is
actually required) and, if these were auto-generated by Ent/Atlas, adjust the
Ent/Atlas schema generation settings to avoid emitting unique indexes on primary
key columns.
---
Nitpick comments:
In `@api/spec/src/v3/currencies/base.tsp`:
- Around line 12-31: The CurrencyBase model's symbol field lacks length
validation allowing empty or excessively long strings; update the symbol
property in model CurrencyBase to add `@minLength`(1) and a sensible `@maxLength`
(e.g., `@maxLength`(12)) so it cannot be empty or unreasonably long, keeping the
existing `@visibility`(Lifecycle.Create, Lifecycle.Read) decorator intact.
In `@api/spec/src/v3/currencies/cost-bases/cost-bases.tsp`:
- Around line 10-15: The CostBases model currently wraps only data: CostBasis[]
which makes adding pagination later a breaking change; decide whether lists can
grow and, if so, change model CostBases to include pagination metadata (e.g.,
total: number, page: number, limit: number or nextCursor: string) alongside
data: CostBasis[] and update any handlers/serializers that return CostBases
(search for CostBases and any endpoints referencing it) to populate those
fields; if you intentionally expect small lists, add a code comment on the
CostBases model documenting the deliberate choice to remain an unpaginated
array.
In `@api/spec/src/v3/currencies/custom.tsp`:
- Around line 1-2: Add an explicit import for base.tsp at the top of this module
so CurrencyBase and CustomCurrencyCode are resolved directly rather than relying
on index.tsp; update the imports in custom.tsp to include base.tsp (so symbols
CurrencyBase and CustomCurrencyCode are imported explicitly) to match the
explicit-import style used by cost-bases.tsp and ensure consistent dependency
declaration.
In `@api/spec/src/v3/currencies/operations.tsp`:
- Around line 18-24: CurrenciesResponse currently marks data with `@pageItems`
which implies pagination, but the list() operation has no pagination params;
either remove the `@pageItems` decorator from the CurrenciesResponse model
(CurrenciesResponse.data) if you intend to always return the full list, or add
standard pagination query parameters to the list() operation (e.g., page,
pageSize or cursor) and implement handling so the `@pageItems` response matches
the operation behavior; update the model or operation accordingly (look for
CurrenciesResponse and the list() operation definitions).
In `@api/spec/src/v3/openmeter.tsp`:
- Around line 81-87: The current endpoints CurrenciesEndpoints and
CostBasesEndpoints are tagged with Shared.BillingTag which groups them under
Billing; if you want them surfaced separately create a new tag (e.g.,
CurrenciesTag) and apply it to the interfaces instead. Change the
`@tag`(Shared.BillingTag) annotations on CurrenciesEndpoints and
CostBasesEndpoints to `@tag`(Shared.CurrenciesTag) (or similar), and add a
corresponding `@tagMetadata` declaration for CurrenciesTag (title/description) in
the spec so generated docs show a distinct Currencies section; if you intend
them to remain under Billing leave the tags as-is.
In `@api/v3/api.gen.go`:
- Around line 893-894: BillingCurrency.Code is declared as plain string but
should use the existing CurrencyCode alias for consistency and semantic clarity;
update the BillingCurrency struct field declaration (BillingCurrency.Code) to
use CurrencyCode instead of string, keep the json tag `json:"code"` unchanged,
and run/adjust any affected usages or tests that assume a string type to accept
CurrencyCode where needed.
In `@openmeter/currencies/adapter.go`:
- Around line 16-21: The CreateCurrency method on CurrenciesAdapter (and the
corresponding method on CurrencyService in service.go) currently returns the
external type *currency.Def which leaks the gobl package; change the signature
to return *Currency (the local type) and update all implementations and callers
to construct/return a local *Currency instead of *currency.Def (convert or map
fields from currency.Def where implementations currently build/receive it),
ensuring CreateCurrency(ctx context.Context, params CreateCurrencyInput)
(*Currency, error) is used consistently across CurrenciesAdapter and
CurrencyService.
In `@openmeter/currencies/adapter/currencies.go`:
- Around line 135-143: The mapping dereferences costBasis.Edges.Currency.ID
without a nil check; modify the code that converts costBases into
[]currencies.CostBasis to defensively handle missing Currency edge: iterate over
costBases (or change the lo.Map callback) and if costBasis.Edges.Currency ==
nil, return an error (or skip/assign zero value) instead of dereferencing it;
update the function that returns the mapped slice to propagate that error (or
ensure the mapping sets CurrencyID to a safe zero value) and keep the reference
to WithCurrency() in mind when locating the code to change.
In `@openmeter/currencies/service/service.go`:
- Around line 17-21: The constructor New currently accepts an adapter that may
be nil, causing later nil-pointer panics; update New(adapter currencies.Adapter)
to validate adapter != nil and return (*Service, error) with a clear error
(e.g., "nil adapter") when missing, and update all call sites to handle the
error, or if project convention prefers, document and use a deliberate panic
instead; locate the New function and the Service struct in service.go and ensure
subsequent uses of Service.adapter are safe after this change.
In `@openmeter/ent/schema/custom_currencies.go`:
- Around line 17-25: Remove the local TimeMixin definition in
custom_currencies.go and replace uses of the local TimeMixin with
entutils.TimeMixin{} in the schema mixin list; specifically, delete the type
TimeMixin struct / func (TimeMixin) Fields block and update the schema's Mixin
declarations to reference entutils.TimeMixin{} so the file uses the shared mixin
(which includes created_at and updated_at) and stays consistent with other
schemas.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (28)
api/spec/package-lock.jsonis excluded by!**/package-lock.jsonapi/v3/openapi.yamlis excluded by!**/openapi.yamlgo.sumis excluded by!**/*.sum,!**/*.sumopenmeter/ent/db/client.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis/currencycostbasis.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis/where.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis_create.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis_delete.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis_query.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis_update.gois excluded by!**/ent/db/**openmeter/ent/db/customcurrency.gois excluded by!**/ent/db/**openmeter/ent/db/customcurrency/customcurrency.gois excluded by!**/ent/db/**openmeter/ent/db/customcurrency/where.gois excluded by!**/ent/db/**openmeter/ent/db/customcurrency_create.gois excluded by!**/ent/db/**openmeter/ent/db/customcurrency_delete.gois excluded by!**/ent/db/**openmeter/ent/db/customcurrency_query.gois excluded by!**/ent/db/**openmeter/ent/db/customcurrency_update.gois excluded by!**/ent/db/**openmeter/ent/db/ent.gois excluded by!**/ent/db/**openmeter/ent/db/expose.gois excluded by!**/ent/db/**openmeter/ent/db/hook/hook.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/paginate.gois excluded by!**/ent/db/**openmeter/ent/db/predicate/predicate.gois excluded by!**/ent/db/**openmeter/ent/db/runtime.gois excluded by!**/ent/db/**openmeter/ent/db/tx.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (33)
api/spec/src/v3/currencies/base.tspapi/spec/src/v3/currencies/cost-bases/cost-bases.tspapi/spec/src/v3/currencies/cost-bases/cost-basis.tspapi/spec/src/v3/currencies/cost-bases/operations.tspapi/spec/src/v3/currencies/currency.tspapi/spec/src/v3/currencies/custom.tspapi/spec/src/v3/currencies/index.tspapi/spec/src/v3/currencies/operations.tspapi/spec/src/v3/openmeter.tspapi/v3/api.gen.goapi/v3/handlers/currencies/convert.goapi/v3/handlers/currencies/create.goapi/v3/handlers/currencies/create_cost_basis.goapi/v3/handlers/currencies/get_cost_bases.goapi/v3/handlers/currencies/handler.goapi/v3/handlers/currencies/list.goapi/v3/server/routes.goapi/v3/server/server.goapp/common/currency.gocmd/server/main.gocmd/server/wire.gocmd/server/wire_gen.goopenmeter/currencies/adapter.goopenmeter/currencies/adapter/adapter.goopenmeter/currencies/adapter/currencies.goopenmeter/currencies/models.goopenmeter/currencies/service.goopenmeter/currencies/service/service.goopenmeter/ent/schema/custom_currencies.goopenmeter/server/router/router.goopenmeter/server/server.gotools/migrate/migrations/20260220154446_add_custom_currency.down.sqltools/migrate/migrations/20260220154446_add_custom_currency.up.sql
Comment on lines
+14
to
+18
| /** | ||
| * The currency id this cost basis belongs to. | ||
| */ | ||
| @visibility(Lifecycle.Create, Lifecycle.Read) | ||
| fiat_code: Shared.CurrencyCode; |
Contributor
There was a problem hiding this comment.
fiat_code doc comment is misleading.
The comment says "The currency id this cost basis belongs to" — but fiat_code is the fiat currency code used in the exchange rate, not the parent currency ID (which comes from the path parameter). This will confuse API consumers.
📝 Suggested fix
- /**
- * The currency id this cost basis belongs to.
- */
+ /**
+ * The fiat currency code used for the exchange rate (e.g., "USD", "EUR").
+ */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/spec/src/v3/currencies/cost-bases/cost-basis.tsp` around lines 14 - 18,
The doc comment for the field fiat_code is misleading: update the comment for
fiat_code (type Shared.CurrencyCode, visibility Lifecycle.Create/Read) to state
that it is the fiat currency code used for the exchange rate applied to this
cost basis (e.g., "Fiat currency code used for the exchange rate"), and
explicitly note that the parent currency id is provided via the request path
parameter rather than this field so API consumers are not confused.
| PRIMARY KEY ("id") | ||
| ); | ||
| -- create index "customcurrency_id" to table: "custom_currencies" | ||
| CREATE UNIQUE INDEX "customcurrency_id" ON "custom_currencies" ("id"); |
Contributor
There was a problem hiding this comment.
Redundant unique index on the primary key column.
custom_currencies.id is already a PRIMARY KEY, which implicitly creates a unique B-tree index. The separate CREATE UNIQUE INDEX "customcurrency_id" is redundant. Same applies to currencycostbasis_id on line 26. These are harmless but add unnecessary storage overhead. (These are typically auto-generated by Atlas/Ent — consider whether the Ent schema configuration can suppress them.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/migrate/migrations/20260220154446_add_custom_currency.up.sql` at line
11, Remove the redundant unique index statements that duplicate the PRIMARY KEY
b-tree for custom_currencies.id and for the currencycostbasis_id index (the
CREATE UNIQUE INDEX "customcurrency_id" and the analogous currencycostbasis_id
statement); delete those CREATE UNIQUE INDEX lines from the migration (or change
them to non-unique only if a separate non-PK index is actually required) and, if
these were auto-generated by Ent/Atlas, adjust the Ent/Atlas schema generation
settings to avoid emitting unique indexes on primary key columns.
394616b to
f0b0991
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (9)
openmeter/currencies/adapter/currencies.go (3)
91-98: Doubletime.Now()call — TOCTOU concern.
time.Now()is called on line 92 and again on line 94, producing two different instants. Capture it once.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/currencies/adapter/currencies.go` around lines 91 - 98, In CreateCostBasis, avoid the TOCTOU by calling time.Now() once: capture now := time.Now() at the start, use now for the Before() check against params.EffectiveFrom and use now as the default for effectiveFrom when params.EffectiveFrom is nil; update references in the CreateCostBasis function (variables effectiveFrom and params.EffectiveFrom checks) to use the single captured now.
57-68: The finallo.Mapis a no-op — just return the appended slice directly.This copies each
Currencyfield-by-field into an identicalCurrency. No transformation is happening.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/currencies/adapter/currencies.go` around lines 57 - 68, The lo.Map call is a no-op because it reconstructs identical currencies; replace the lo.Map(append(customCurrencies, defs...), ...) code with a direct return of the combined slice (return append(customCurrencies, defs...), nil) so you don't copy fields unnecessarily; update the return in the function that currently uses lo.Map and remove the now-unused mapping closure and import if unused.
100-103:float32for financial rate loses precision.
params.Rateisfloat32, giving only ~7 significant digits — insufficient for exchange rates. The API spec correctly usesShared.Numeric(string-based), so the Go input model should accept astringoralpacadecimal.Decimalto preserve precision end-to-end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/currencies/adapter/currencies.go` around lines 100 - 103, The code uses params.Rate (a float32) and alpacadecimal.NewFromFloat32 when creating CurrencyCostBasis which loses precision; change the input model so params.Rate is a string (or alpacadecimal.Decimal) matching Shared.Numeric, then parse/convert that string to a high-precision decimal before calling CurrencyCostBasis.Create(). Replace alpacadecimal.NewFromFloat32 with the string-based constructor/parsing API (e.g., alpacadecimal.NewFromString or accept an alpacadecimal.Decimal directly), add validation/error handling for parse failures, and update any callers or DTOs that populate params.Rate to pass the string/decimal instead of float32.api/spec/src/v3/currencies/cost-bases/operations.tsp (1)
20-22: Doc comment says "Get a cost basis" (singular) but the operation returns a collection.The
operationId,@summary, and return type are all plural (CostBases), but the doc says singular. Small inconsistency for generated docs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/src/v3/currencies/cost-bases/operations.tsp` around lines 20 - 22, The doc comment "Get a cost basis." is inconsistent with the rest of the operation (operationId, `@summary`, and return type which are plural CostBases); update the JSDoc/top comment to a plural form (e.g., "Get cost bases.") so it matches the operationId, `@summary`, and the return type returned by the operation in operations.tsp (look for the operationId and `@summary` that reference CostBases and change the surrounding comment accordingly).api/spec/src/v3/currencies/cost-bases/cost-basis.tsp (1)
14-18:fiat_codedoc comment is still misleading.The comment says "The currency id this cost basis belongs to" but
fiat_codeis the fiat currency code for the exchange rate, not the parent currency ID. This was flagged previously and appears unresolved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/src/v3/currencies/cost-bases/cost-basis.tsp` around lines 14 - 18, The doc comment for the field fiat_code is incorrect: it describes a parent currency id but fiat_code actually represents the fiat currency code used for the exchange rate; update the comment on fiat_code (type Shared.CurrencyCode) to clearly state it is the fiat currency code for the exchange rate (e.g., "The fiat currency code used to express the exchange rate for this cost basis."), leaving visibility and type annotations unchanged.api/v3/handlers/currencies/create_cost_basis.go (1)
30-35:⚠️ Potential issue | 🔴 CriticalBug: Created cost basis is discarded — response will always be empty.
Same issue as
CreateCurrency: the result ofCreateCostBasisis assigned to_and thrown away, so the client always gets a 201 with null body instead of the created resource.🐛 Proposed fix
func(ctx context.Context, request CreateCostBasisRequest) (CreateCostBasisResponse, error) { - _, err := h.currencyService.CreateCostBasis(ctx, request) + costBasis, err := h.currencyService.CreateCostBasis(ctx, request) if err != nil { return nil, err } - return nil, nil + return costBasis, nil },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/currencies/create_cost_basis.go` around lines 30 - 35, The handler is discarding the created value by assigning the result of h.currencyService.CreateCostBasis to `_`, causing a nil response; change the call to capture the created resource (e.g., costBasis, err := h.currencyService.CreateCostBasis(ctx, request)), propagate the error if non-nil, and return the captured costBasis as the CreateCostBasisResponse with nil error so the client receives the created resource.api/v3/handlers/currencies/create.go (1)
30-36:⚠️ Potential issue | 🔴 CriticalTwo bugs: discarded result and blanket 409 on all errors.
These issues were flagged in a previous review and still appear to be unresolved:
- Discarded result (Line 31): The
*currency.Defreturned byCreateCurrencyis assigned to_and thrown away. The client always gets a 201 with a null/empty body.- All errors treated as 409 Conflict (Line 33): Every error — including validation failures, DB errors, etc. — gets wrapped in
NewConflictError. Only actual uniqueness conflicts should be 409.Proposed fix
func(ctx context.Context, request CreateCurrencyRequest) (CreateCurrencyResponse, error) { - _, err := h.currencyService.CreateCurrency(ctx, request) + result, err := h.currencyService.CreateCurrency(ctx, request) if err != nil { - return nil, apierrors.NewConflictError(ctx, err, "Currency already exists") + // TODO: differentiate conflict errors from other error types + return nil, err } - return nil, nil + return result, nil },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/currencies/create.go` around lines 30 - 36, The handler currently discards the *currency.Def returned by h.currencyService.CreateCurrency and maps every error to a 409; fix by capturing the created currency (assign the first return value to a variable, not _), use it to build and return a proper CreateCurrencyResponse with the created resource and a 201, and replace the blanket apierrors.NewConflictError usage with error-type checking: if err matches the service's uniqueness error (e.g., currency.ErrAlreadyExists or DB unique-violation sentinel) then return NewConflictError, if it is a validation error return a BadRequest error, otherwise return an InternalServerError; update the anonymous function handling CreateCurrencyRequest/CreateCurrencyResponse to perform these checks and return the created currency on success.api/v3/api.gen.go (2)
2150-2154: Create request bodies reuse full response types —Idis required on create🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/api.gen.go` around lines 2150 - 2154, The generated request-body aliases CreateCustomCurrencyJSONRequestBody and CreateCostBasisJSONRequestBody incorrectly reuse the full response types BillingCustomCurrency and BillingCostBasis (which include server-assigned Ids), causing Id to be required for create calls; replace these aliases with new request-only types (e.g., CreateCustomCurrencyRequestBody, CreateCostBasisRequestBody) or structs that omit or make the Id field optional/omitted, and update any references to use the new types so create endpoints accept payloads without Ids (look for CreateCustomCurrencyJSONRequestBody, CreateCostBasisJSONRequestBody, BillingCustomCurrency, and BillingCostBasis to locate the code to change).
883-884: MisleadingFiatCodecomment🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/api.gen.go` around lines 883 - 884, The comment for the FiatCode field is misleading; update the comment above the FiatCode CurrencyCode `json:"fiat_code"` field to clearly describe it as the fiat currency code for the cost basis (e.g., "FiatCode The fiat currency code for this cost basis (ISO 4217).") so it accurately reflects the field's meaning; locate the FiatCode field in the struct in api.gen.go and replace the existing comment with the corrected one.
🧹 Nitpick comments (3)
openmeter/ent/schema/custom_currencies.go (1)
83-87: Consider the timestamp precision for the uniqueness guaranteeThe composite index on
(currency, fiat_code, effective_from)makes sense for the append-only temporal design. One small gotcha:effective_fromdefaults totime.Now, so two concurrent inserts for the same(currency, fiat_code)within the same timestamp precision window (microseconds in Postgres) could technically race past the unique constraint. If strict deduplication matters here, you might want to handle this at the application layer or consider a partial/advisory lock on insert.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ent/schema/custom_currencies.go` around lines 83 - 87, The composite unique index defined in CurrencyCostBasis.Indexes() can still allow races because effective_from is set with time.Now (application-side) and concurrent inserts may share the same DB timestamp precision; to fix, normalise or control the timestamp and/or serialize inserts: either (A) truncate or round effective_from in the code that creates CurrencyCostBasis (e.g., in the constructor/creation helper or CreateCurrencyCostBasis) to a consistent precision (microsecond or millisecond) before persisting, or (B) move the default to the DB (use a database-side now() with sufficient precision) and add an application-level check or an advisory/transactional lock around the insert path (the function that performs the insert) to prevent concurrent duplicates; pick one approach and apply it to the code which sets/creates effective_from and the creation method that inserts CurrencyCostBasis.openmeter/currencies/adapter.go (1)
18-18:CreateCurrencyreturning*currency.Defcreates a semantic mismatch and wastes the response.The adapter creates a
CustomCurrencyin the database but returns*currency.Deffrom the gobl library — a type designed for ISO currency definitions. OnlyISOCode,Name, andSymbolare populated; other fields likeISONumericandSubunitsstay zero-valued, which is misleading for a custom currency.Even worse, the HTTP handler in
api/v3/handlers/currencies/create.godiscards this response entirely (line 31:_, err := h.currencyService.CreateCurrency(...)) and returnsnilto the client instead of the created currency details.Return
*Currencyfrom your domain model instead. It's the natural fit—it already has theIsCustomflag and all relevant fields, keeps the contract consistent withListCurrencies, and avoids the unnecessary coupling to an external type.Suggested change
type CurrenciesAdapter interface { ListCurrencies(ctx context.Context) ([]Currency, error) - CreateCurrency(ctx context.Context, params CreateCurrencyInput) (*currency.Def, error) + CreateCurrency(ctx context.Context, params CreateCurrencyInput) (*Currency, error) CreateCostBasis(ctx context.Context, params CreateCostBasisInput) (*CostBasis, error) GetCostBasesByCurrencyID(ctx context.Context, currencyID string) ([]CostBasis, error) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/currencies/adapter.go` at line 18, The adapter's CreateCurrency currently returns *currency.Def (gobl) while it persists a domain CustomCurrency, causing semantic mismatch and incomplete fields; change the CreateCurrency signature in the adapter and its service interface to return *Currency (your domain model) instead of *currency.Def, update the adapter implementation that creates CustomCurrency to populate and return a *Currency (including IsCustom, ISOCode, Name, Symbol, Subunits, ISONumeric), and update the HTTP handler in api/v3/handlers/currencies/create.go to capture and return that *Currency (instead of discarding the result with "_, err := ..."), keeping ListCurrencies' contract consistent with the new type.api/v3/api.gen.go (1)
893-894:BillingCurrency.Codetyped asstringinstead of the project'sCurrencyCodealiasEvery other place in this file that carries a currency code uses
CurrencyCode(e.g.,BillingAppStripeCreateCheckoutSessionRequestOptions.Currency,BillingCustomer.Currency). Using plainstringhere loses that semantic clarity, even though they're both= stringunderneath. Worth a quick TypeSpec tweak to align.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/api.gen.go` around lines 893 - 894, BillingCurrency.Code is declared as a plain string which breaks consistency with the project's CurrencyCode alias; update the field type to CurrencyCode (preserving the `json:"code"` tag) in the BillingCurrency type declaration so it matches other currency fields like BillingAppStripeCreateCheckoutSessionRequestOptions.Currency and BillingCustomer.Currency, and regenerate or re-run the TypeSpec change so all usages compile against the CurrencyCode alias.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/spec/src/v3/currencies/base.tsp`:
- Around line 26-30: The `symbol` field lacks length validation and can accept
empty or very long strings; update the `symbol` property in the currency TSP
(the field named symbol) to mirror `name` by adding `@minLength`(1) and
`@maxLength`(10) (or the same max used in the Ent schema) on the existing
`@visibility`(Lifecycle.Create, Lifecycle.Read) line so the API spec enforces a
non-empty symbol with a 1–10 character limit, keeping the spec consistent with
the DB schema.
- Around line 18-24: The JSDoc for the shared CurrencyBase field 'name' is
misleading ("Custom Currency name"); update the comment to a neutral description
such as "Currency name" to reflect that CurrencyBase is extended by both
Currency and CustomCurrency; locate the 'name: string' property in the
CurrencyBase definition (annotations: `@visibility`(Lifecycle.Create,
Lifecycle.Read), `@minLength`(1), `@maxLength`(256)) and replace the doc comment
accordingly.
In `@api/spec/src/v3/currencies/cost-bases/cost-basis.tsp`:
- Around line 26-31: The doc comment for the effective_from field uses
Go-specific jargon ("time.Now()"); update the comment on the effective_from
property (annotated with `@visibility`(Lifecycle.Create, Lifecycle.Read) and type
Shared.DateTime) to replace "will set to time.Now() by the system" with a
public-friendly phrase such as "will default to the current time" and ensure the
sentence reads grammatically correct (e.g., "If not provided, it will default to
the current time.").
In `@api/v3/handlers/currencies/create_cost_basis.go`:
- Line 13: The CreateCostBasisInput.Rate is currently a float32 which loses
precision; change the input type to string (so CreateCostBasisRequest /
currencies.CreateCostBasisInput accepts a string for Rate) and update the
conversion logic that produces alpacadecimal.Decimal (the code in the adapter
that converts inputs to Decimal) to parse the string into alpacadecimal.Decimal
instead of converting from float32; ensure JSON unmarshalling accepts the string
and the Decimal parsing handles validation/errors consistently (refer to
CreateCostBasisRequest, CreateCostBasisInput.Rate, and the alpacadecimal.Decimal
conversion in the currencies adapter).
In `@api/v3/handlers/currencies/list.go`:
- Around line 17-18: The response type is wrong: replace the bare slice alias
ListCurrenciesResponse with the spec-shaped envelope (e.g., CurrenciesResponse {
Data []v3.BillingCurrency }) and update ListCurrenciesHandler's signature and
return value to produce {"data": [...] } instead of a raw array; locate the
types and the handler that reference ListCurrenciesResponse and
ListCurrenciesHandler and change them to use the new CurrenciesResponse struct
(and adjust any code that constructs/returns the response to populate the Data
field with the []v3.BillingCurrency).
In `@api/v3/server/routes.go`:
- Around line 161-167: The UpsertTaxCode function is missing a closing brace
which causes the following ListCurrencies function to be nested and the file to
not compile; fix by adding the missing closing curly brace '}' immediately after
the unimplemented.UpsertTaxCode(...) line to terminate func (s *Server)
UpsertTaxCode, ensuring func (s *Server) ListCurrencies starts at top-level;
verify the file compiles and that UpsertTaxCode and ListCurrencies are separate
functions.
In `@openmeter/currencies/adapter/currencies.go`:
- Around line 135-143: The mapping over costBases can panic when
costBasis.Edges.Currency is nil; update the lo.Map callback in currencies.go
(the conversion from *entdb.CurrencyCostBasis to currencies.CostBasis) to guard
against costBasis.Edges.Currency == nil — either skip/omit that entry or
populate CurrencyID with a zero/nullable value and/or return an error; ensure
the callback checks costBasis.Edges.Currency before accessing .ID and handles
the missing edge consistently (e.g., filter out nil-edge entries before mapping
or return an explanatory error from the enclosing function).
- Around line 129-133: The ent IsNotFound branch after the Query().All() call is
dead code because All() returns an empty slice rather than an error; remove the
entdb.IsNotFound check or replace it with an explicit existence check on the
CustomCurrency entity (e.g., use
client.CustomCurrency.Query().Where(customcurrency.IDEQ(currencyID)).Exist(ctx)
or .Only(ctx)) and if that existence check fails return
models.NewGenericNotFoundError(fmt.Errorf("cost basis with id: %s not found",
currencyID)); otherwise keep the existing error wrapping for other failures from
the cost-basis query.
- Around line 46-55: The return value sets
currencies.Currency.SmallestDenomination using an unsafe int8 cast of
def.SmallestDenomination; either remove the cast if
currencies.Currency.SmallestDenomination already matches
def.SmallestDenomination's type, or add a bounds check before casting to prevent
silent truncation—validate def.SmallestDenomination is within int8 range (e.g.
>= -128 && <= 127) and handle out-of-range values (return an error, clamp, or
use a safer type) so the assignment to currencies.Currency.SmallestDenomination
is explicit and safe.
In `@openmeter/ent/schema/custom_currencies.go`:
- Line 40: Add a uniqueness constraint to the currency identifier: modify the
CustomCurrency schema so the code field is unique by appending .Unique() to
field.String("code").NotEmpty().MinLen(3).MaxLen(24).Immutable(); if uniqueness
should be scoped per-tenant/namespace instead, remove .Unique() and instead add
a composite unique index in CustomCurrency's Indexes() method (similar to the
CurrencyCostBasis pattern) that includes the namespace/tenant field plus "code".
- Around line 31-36: CustomCurrency.Mixin currently returns entutils.IDMixin and
TimeMixin but omits the NamespaceMixin, which makes custom currencies global
rather than tenant-scoped; update the CustomCurrency Mixin() to include
entutils.NamespaceMixin{} alongside entutils.IDMixin{} and TimeMixin{} so the
schema follows the multi-tenant pattern used elsewhere and enforces namespace
isolation.
- Around line 17-25: Add a uniqueness constraint and tenant scoping: update the
custom currency schema so the "code" field (code) is marked .Unique() if
currencies are global, or if they are tenant-scoped add NamespaceMixin (or an
explicit "namespace" field) and create a composite unique index on (namespace,
code); also keep the existing TimeMixin as-is. Reference the TimeMixin type and
the schema's code field (and NamespaceMixin if added) when making these changes.
---
Duplicate comments:
In `@api/spec/src/v3/currencies/cost-bases/cost-basis.tsp`:
- Around line 14-18: The doc comment for the field fiat_code is incorrect: it
describes a parent currency id but fiat_code actually represents the fiat
currency code used for the exchange rate; update the comment on fiat_code (type
Shared.CurrencyCode) to clearly state it is the fiat currency code for the
exchange rate (e.g., "The fiat currency code used to express the exchange rate
for this cost basis."), leaving visibility and type annotations unchanged.
In `@api/spec/src/v3/currencies/cost-bases/operations.tsp`:
- Around line 20-22: The doc comment "Get a cost basis." is inconsistent with
the rest of the operation (operationId, `@summary`, and return type which are
plural CostBases); update the JSDoc/top comment to a plural form (e.g., "Get
cost bases.") so it matches the operationId, `@summary`, and the return type
returned by the operation in operations.tsp (look for the operationId and
`@summary` that reference CostBases and change the surrounding comment
accordingly).
In `@api/v3/api.gen.go`:
- Around line 2150-2154: The generated request-body aliases
CreateCustomCurrencyJSONRequestBody and CreateCostBasisJSONRequestBody
incorrectly reuse the full response types BillingCustomCurrency and
BillingCostBasis (which include server-assigned Ids), causing Id to be required
for create calls; replace these aliases with new request-only types (e.g.,
CreateCustomCurrencyRequestBody, CreateCostBasisRequestBody) or structs that
omit or make the Id field optional/omitted, and update any references to use the
new types so create endpoints accept payloads without Ids (look for
CreateCustomCurrencyJSONRequestBody, CreateCostBasisJSONRequestBody,
BillingCustomCurrency, and BillingCostBasis to locate the code to change).
- Around line 883-884: The comment for the FiatCode field is misleading; update
the comment above the FiatCode CurrencyCode `json:"fiat_code"` field to clearly
describe it as the fiat currency code for the cost basis (e.g., "FiatCode The
fiat currency code for this cost basis (ISO 4217).") so it accurately reflects
the field's meaning; locate the FiatCode field in the struct in api.gen.go and
replace the existing comment with the corrected one.
In `@api/v3/handlers/currencies/create_cost_basis.go`:
- Around line 30-35: The handler is discarding the created value by assigning
the result of h.currencyService.CreateCostBasis to `_`, causing a nil response;
change the call to capture the created resource (e.g., costBasis, err :=
h.currencyService.CreateCostBasis(ctx, request)), propagate the error if
non-nil, and return the captured costBasis as the CreateCostBasisResponse with
nil error so the client receives the created resource.
In `@api/v3/handlers/currencies/create.go`:
- Around line 30-36: The handler currently discards the *currency.Def returned
by h.currencyService.CreateCurrency and maps every error to a 409; fix by
capturing the created currency (assign the first return value to a variable, not
_), use it to build and return a proper CreateCurrencyResponse with the created
resource and a 201, and replace the blanket apierrors.NewConflictError usage
with error-type checking: if err matches the service's uniqueness error (e.g.,
currency.ErrAlreadyExists or DB unique-violation sentinel) then return
NewConflictError, if it is a validation error return a BadRequest error,
otherwise return an InternalServerError; update the anonymous function handling
CreateCurrencyRequest/CreateCurrencyResponse to perform these checks and return
the created currency on success.
In `@openmeter/currencies/adapter/currencies.go`:
- Around line 91-98: In CreateCostBasis, avoid the TOCTOU by calling time.Now()
once: capture now := time.Now() at the start, use now for the Before() check
against params.EffectiveFrom and use now as the default for effectiveFrom when
params.EffectiveFrom is nil; update references in the CreateCostBasis function
(variables effectiveFrom and params.EffectiveFrom checks) to use the single
captured now.
- Around line 57-68: The lo.Map call is a no-op because it reconstructs
identical currencies; replace the lo.Map(append(customCurrencies, defs...), ...)
code with a direct return of the combined slice (return append(customCurrencies,
defs...), nil) so you don't copy fields unnecessarily; update the return in the
function that currently uses lo.Map and remove the now-unused mapping closure
and import if unused.
- Around line 100-103: The code uses params.Rate (a float32) and
alpacadecimal.NewFromFloat32 when creating CurrencyCostBasis which loses
precision; change the input model so params.Rate is a string (or
alpacadecimal.Decimal) matching Shared.Numeric, then parse/convert that string
to a high-precision decimal before calling CurrencyCostBasis.Create(). Replace
alpacadecimal.NewFromFloat32 with the string-based constructor/parsing API
(e.g., alpacadecimal.NewFromString or accept an alpacadecimal.Decimal directly),
add validation/error handling for parse failures, and update any callers or DTOs
that populate params.Rate to pass the string/decimal instead of float32.
---
Nitpick comments:
In `@api/v3/api.gen.go`:
- Around line 893-894: BillingCurrency.Code is declared as a plain string which
breaks consistency with the project's CurrencyCode alias; update the field type
to CurrencyCode (preserving the `json:"code"` tag) in the BillingCurrency type
declaration so it matches other currency fields like
BillingAppStripeCreateCheckoutSessionRequestOptions.Currency and
BillingCustomer.Currency, and regenerate or re-run the TypeSpec change so all
usages compile against the CurrencyCode alias.
In `@openmeter/currencies/adapter.go`:
- Line 18: The adapter's CreateCurrency currently returns *currency.Def (gobl)
while it persists a domain CustomCurrency, causing semantic mismatch and
incomplete fields; change the CreateCurrency signature in the adapter and its
service interface to return *Currency (your domain model) instead of
*currency.Def, update the adapter implementation that creates CustomCurrency to
populate and return a *Currency (including IsCustom, ISOCode, Name, Symbol,
Subunits, ISONumeric), and update the HTTP handler in
api/v3/handlers/currencies/create.go to capture and return that *Currency
(instead of discarding the result with "_, err := ..."), keeping ListCurrencies'
contract consistent with the new type.
In `@openmeter/ent/schema/custom_currencies.go`:
- Around line 83-87: The composite unique index defined in
CurrencyCostBasis.Indexes() can still allow races because effective_from is set
with time.Now (application-side) and concurrent inserts may share the same DB
timestamp precision; to fix, normalise or control the timestamp and/or serialize
inserts: either (A) truncate or round effective_from in the code that creates
CurrencyCostBasis (e.g., in the constructor/creation helper or
CreateCurrencyCostBasis) to a consistent precision (microsecond or millisecond)
before persisting, or (B) move the default to the DB (use a database-side now()
with sufficient precision) and add an application-level check or an
advisory/transactional lock around the insert path (the function that performs
the insert) to prevent concurrent duplicates; pick one approach and apply it to
the code which sets/creates effective_from and the creation method that inserts
CurrencyCostBasis.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (28)
api/spec/package-lock.jsonis excluded by!**/package-lock.jsonapi/v3/openapi.yamlis excluded by!**/openapi.yamlgo.sumis excluded by!**/*.sum,!**/*.sumopenmeter/ent/db/client.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis/currencycostbasis.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis/where.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis_create.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis_delete.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis_query.gois excluded by!**/ent/db/**openmeter/ent/db/currencycostbasis_update.gois excluded by!**/ent/db/**openmeter/ent/db/customcurrency.gois excluded by!**/ent/db/**openmeter/ent/db/customcurrency/customcurrency.gois excluded by!**/ent/db/**openmeter/ent/db/customcurrency/where.gois excluded by!**/ent/db/**openmeter/ent/db/customcurrency_create.gois excluded by!**/ent/db/**openmeter/ent/db/customcurrency_delete.gois excluded by!**/ent/db/**openmeter/ent/db/customcurrency_query.gois excluded by!**/ent/db/**openmeter/ent/db/customcurrency_update.gois excluded by!**/ent/db/**openmeter/ent/db/ent.gois excluded by!**/ent/db/**openmeter/ent/db/expose.gois excluded by!**/ent/db/**openmeter/ent/db/hook/hook.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/paginate.gois excluded by!**/ent/db/**openmeter/ent/db/predicate/predicate.gois excluded by!**/ent/db/**openmeter/ent/db/runtime.gois excluded by!**/ent/db/**openmeter/ent/db/tx.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (33)
api/spec/src/v3/currencies/base.tspapi/spec/src/v3/currencies/cost-bases/cost-bases.tspapi/spec/src/v3/currencies/cost-bases/cost-basis.tspapi/spec/src/v3/currencies/cost-bases/operations.tspapi/spec/src/v3/currencies/currency.tspapi/spec/src/v3/currencies/custom.tspapi/spec/src/v3/currencies/index.tspapi/spec/src/v3/currencies/operations.tspapi/spec/src/v3/openmeter.tspapi/v3/api.gen.goapi/v3/handlers/currencies/convert.goapi/v3/handlers/currencies/create.goapi/v3/handlers/currencies/create_cost_basis.goapi/v3/handlers/currencies/get_cost_bases.goapi/v3/handlers/currencies/handler.goapi/v3/handlers/currencies/list.goapi/v3/server/routes.goapi/v3/server/server.goapp/common/currency.gocmd/server/main.gocmd/server/wire.gocmd/server/wire_gen.goopenmeter/currencies/adapter.goopenmeter/currencies/adapter/adapter.goopenmeter/currencies/adapter/currencies.goopenmeter/currencies/models.goopenmeter/currencies/service.goopenmeter/currencies/service/service.goopenmeter/ent/schema/custom_currencies.goopenmeter/server/router/router.goopenmeter/server/server.gotools/migrate/migrations/20260220154446_add_custom_currency.down.sqltools/migrate/migrations/20260220154446_add_custom_currency.up.sql
🚧 Files skipped from review as they are similar to previous changes (14)
- api/spec/src/v3/openmeter.tsp
- api/spec/src/v3/currencies/operations.tsp
- api/spec/src/v3/currencies/cost-bases/cost-bases.tsp
- openmeter/server/server.go
- api/v3/handlers/currencies/get_cost_bases.go
- openmeter/currencies/models.go
- api/v3/handlers/currencies/handler.go
- openmeter/currencies/service.go
- tools/migrate/migrations/20260220154446_add_custom_currency.up.sql
- api/spec/src/v3/currencies/index.tsp
- api/spec/src/v3/currencies/currency.tsp
- api/spec/src/v3/currencies/custom.tsp
- tools/migrate/migrations/20260220154446_add_custom_currency.down.sql
- openmeter/currencies/service/service.go
Comment on lines
+26
to
+31
| /** | ||
| * An ISO-8601 timestamp representation of the date from which the cost basis is effective. | ||
| * If not provided, it will be effective immediately and will set to time.Now() by the system. | ||
| */ | ||
| @visibility(Lifecycle.Create, Lifecycle.Read) | ||
| effective_from?: Shared.DateTime; |
Contributor
There was a problem hiding this comment.
Avoid Go-specific jargon in the API spec.
The doc says "will set to time.Now() by the system" — API consumers won't know what time.Now() means. Something like "will default to the current time" reads much better for a public-facing spec.
Suggested fix
/**
* An ISO-8601 timestamp representation of the date from which the cost basis is effective.
- * If not provided, it will be effective immediately and will set to time.Now() by the system.
+ * If not provided, it defaults to the current time and takes effect immediately.
*/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/spec/src/v3/currencies/cost-bases/cost-basis.tsp` around lines 26 - 31,
The doc comment for the effective_from field uses Go-specific jargon
("time.Now()"); update the comment on the effective_from property (annotated
with `@visibility`(Lifecycle.Create, Lifecycle.Read) and type Shared.DateTime) to
replace "will set to time.Now() by the system" with a public-friendly phrase
such as "will default to the current time" and ensure the sentence reads
grammatically correct (e.g., "If not provided, it will default to the current
time.").
|
|
||
| func (CustomCurrency) Fields() []ent.Field { | ||
| return []ent.Field{ | ||
| field.String("code").NotEmpty().MinLen(3).MaxLen(24).Immutable(), |
Contributor
There was a problem hiding this comment.
Missing uniqueness constraint on code
Without .Unique() on code, the database will happily let you insert two CustomCurrency rows with the same code (e.g., two "BTC" entries). Since code looks like the natural identifier for a currency, it should be unique — at minimum globally, and ideally scoped per-namespace if this is per-tenant.
🛠️ Proposed fix
- field.String("code").NotEmpty().MinLen(3).MaxLen(24).Immutable(),
+ field.String("code").NotEmpty().MinLen(3).MaxLen(24).Immutable().Unique(),Or, if per-tenant uniqueness is needed, add a composite unique index instead (see Indexes()) — see the CurrencyCostBasis pattern for reference.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| field.String("code").NotEmpty().MinLen(3).MaxLen(24).Immutable(), | |
| field.String("code").NotEmpty().MinLen(3).MaxLen(24).Immutable().Unique(), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/ent/schema/custom_currencies.go` at line 40, Add a uniqueness
constraint to the currency identifier: modify the CustomCurrency schema so the
code field is unique by appending .Unique() to
field.String("code").NotEmpty().MinLen(3).MaxLen(24).Immutable(); if uniqueness
should be scoped per-tenant/namespace instead, remove .Unique() and instead add
a composite unique index in CustomCurrency's Indexes() method (similar to the
CurrencyCostBasis pattern) that includes the namespace/tenant field plus "code".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
openmeter/currencies/adapter/currencies.go (4)
54-62: Still a no-oplo.Map— the slice can be returned directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/currencies/adapter/currencies.go` around lines 54 - 62, The lo.Map call is a no-op because it maps currencies.Currency to the same struct; replace the lo.Map invocation and simply return the merged slice created by append(customCurrencies, defs...), i.e. remove lo.Map and the mapping lambda and return the combined slice (using the existing identifiers customCurrencies, defs and currencies.Currency) directly, preserving the existing return of nil error.
86-92: Doubletime.Now()TOCTOU andfloat32rate precision are still unaddressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/currencies/adapter/currencies.go` around lines 86 - 92, The code uses time.Now() twice causing a TOCTOU bug and still uses float32 for rates; capture the current time once (e.g., now := time.Now()) and use that single timestamp to validate params.EffectiveFrom and set effectiveFrom, and replace the float32 rate usage with a higher-precision type (e.g., float64 or a decimal type like shopspring/decimal) so the rate handling and comparisons use the new type; update any references to params.EffectiveFrom, effectiveFrom, and the Rate field/variable to use the single now variable and the chosen higher-precision rate type.
97-97:float32rate precision issue is still unaddressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/currencies/adapter/currencies.go` at line 97, The code currently converts params.Rate using alpacadecimal.NewFromFloat32 which loses precision; change the conversion to use a higher-precision constructor (e.g., alpacadecimal.NewFromFloat64 or NewFromString) and/or ensure params.Rate is a float64 or string to preserve precision before calling SetRate on the currency adapter; update the call site that constructs the rate (replace NewFromFloat32(params.Rate) with NewFromFloat64(params.Rate) or NewFromString(fmt.Sprintf("%v", params.Rate))) and adjust the params.Rate type if necessary so SetRate receives the high-precision alpacadecimal value.
123-132: DeadIsNotFoundcheck and nil-pointer risk onEdges.Currencyare still unaddressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/currencies/adapter/currencies.go` around lines 123 - 132, The entdb.IsNotFound(err) branch is ineffective and there's an NPE risk when accessing costBasis.Edges.Currency.ID; replace the dead NotFound check with an explicit empty-result check (if len(costBases) == 0) and return models.NewGenericNotFoundError(fmt.Errorf("cost basis with id: %s not found", currencyID)); additionally, when mapping costBases to currencies.CostBasis, guard against nil edges by validating costBasis.Edges != nil and costBasis.Edges.Currency != nil (and return a clear error or skip/handle that record as appropriate) so you don't dereference a nil costBasis.Edges.Currency.ID.
🧹 Nitpick comments (2)
openmeter/currencies/adapter/currencies.go (1)
21-63: Merged result isn't sorted — mixed ordering could surprise API consumers.Custom currencies are fetched in code-ascending order, but GOBL definitions come in their own internal order, so the combined list has an inconsistent sort. A final
sort.Slice(or alo-based sort) byCodewould make the output deterministic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/currencies/adapter/currencies.go` around lines 21 - 63, The combined currency list returned by ListCurrencies is not deterministically ordered because customCurrencies (queried by customcurrency.FieldCode) and defs (from currency.Definitions()) are concatenated without a final sort; modify ListCurrencies to sort the merged slice by the Currency.Code field before returning (e.g., use sort.Slice on the appended slice or an lo-based sort) so the output is consistently ordered for API consumers.openmeter/server/server_test.go (1)
139-412: New currency endpoints aren't covered inTestRoutes.All other resource types (events, meters, portal, debug) have at least a smoke-test entry here. Adding a few basic cases for the new endpoints — even just asserting a
200onGET /currenciesand a400on bad input — would follow the established pattern and catch wiring regressions early.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/server/server_test.go` around lines 139 - 412, Add smoke tests for the new currency endpoints inside the TestRoutes tests table: update the tests slice in TestRoutes to include at least one GET case for path "/api/v1/currencies" asserting status http.StatusOK and one negative case (e.g. POST or GET with invalid input to "/api/v1/currencies" or "/api/v1/currencies/{id}") asserting http.StatusBadRequest; use the same testRequest/testResponse structs used by existing cases (refer to TestRoutes, testRequest, testResponse, and the tests variable) so the new entries follow the established pattern and will catch routing/wiring regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/currencies/adapter/currencies.go`:
- Around line 29-52: customCurrencies and defs can contain duplicate currency
codes (e.g., a custom "USD" plus ISO "USD"); before creating defs (from
currency.Definitions()) build a set of custom currency codes from
currencyRecords/customCurrencies (normalizing case if needed) and filter out any
def whose ISOCode matches a custom code; update the lo.Filter/lo.Map pipeline
that produces defs to exclude those codes so the merged list doesn't contain
duplicates (refer to customCurrencies, currencyRecords, currency.Definitions(),
and the defs variable).
- Around line 65-83: Change the CreateCurrency flow to return the full domain
model and use it in the handler: update the adapter method signature
adapter.CreateCurrency(ctx, params) to return (*currencies.Currency, error)
instead of (*currency.Def, error) and have it return the created ent record
mapped into currencies.Currency (preserving ID, Code, Name, Symbol); update the
CurrencyService interface and all implementations (including NoopCurrencyService
in server_test.go) to match the new signature; then modify the handler in
api/v3/handlers/currencies/create.go to capture the service result (don’t
discard with `_`), pass it through MapCurrencyToAPI (or equivalent mapping used
by ListCurrencies) and return that mapped response instead of nil, nil so the
API returns ID, Code, Name and Symbol.
In `@openmeter/server/server_test.go`:
- Around line 1658-1660: The doc comment for NoopCurrencyService incorrectly
mentions `currency.Service`; update the comment above the `NoopCurrencyService`
declaration to reference the actual interface `currencies.CurrencyService` so it
accurately documents that `NoopCurrencyService` implements
`currencies.CurrencyService` for testing; ensure the comment text and any
package qualifier match the `currencies` package name used in the file.
---
Duplicate comments:
In `@openmeter/currencies/adapter/currencies.go`:
- Around line 54-62: The lo.Map call is a no-op because it maps
currencies.Currency to the same struct; replace the lo.Map invocation and simply
return the merged slice created by append(customCurrencies, defs...), i.e.
remove lo.Map and the mapping lambda and return the combined slice (using the
existing identifiers customCurrencies, defs and currencies.Currency) directly,
preserving the existing return of nil error.
- Around line 86-92: The code uses time.Now() twice causing a TOCTOU bug and
still uses float32 for rates; capture the current time once (e.g., now :=
time.Now()) and use that single timestamp to validate params.EffectiveFrom and
set effectiveFrom, and replace the float32 rate usage with a higher-precision
type (e.g., float64 or a decimal type like shopspring/decimal) so the rate
handling and comparisons use the new type; update any references to
params.EffectiveFrom, effectiveFrom, and the Rate field/variable to use the
single now variable and the chosen higher-precision rate type.
- Line 97: The code currently converts params.Rate using
alpacadecimal.NewFromFloat32 which loses precision; change the conversion to use
a higher-precision constructor (e.g., alpacadecimal.NewFromFloat64 or
NewFromString) and/or ensure params.Rate is a float64 or string to preserve
precision before calling SetRate on the currency adapter; update the call site
that constructs the rate (replace NewFromFloat32(params.Rate) with
NewFromFloat64(params.Rate) or NewFromString(fmt.Sprintf("%v", params.Rate)))
and adjust the params.Rate type if necessary so SetRate receives the
high-precision alpacadecimal value.
- Around line 123-132: The entdb.IsNotFound(err) branch is ineffective and
there's an NPE risk when accessing costBasis.Edges.Currency.ID; replace the dead
NotFound check with an explicit empty-result check (if len(costBases) == 0) and
return models.NewGenericNotFoundError(fmt.Errorf("cost basis with id: %s not
found", currencyID)); additionally, when mapping costBases to
currencies.CostBasis, guard against nil edges by validating costBasis.Edges !=
nil and costBasis.Edges.Currency != nil (and return a clear error or skip/handle
that record as appropriate) so you don't dereference a nil
costBasis.Edges.Currency.ID.
---
Nitpick comments:
In `@openmeter/currencies/adapter/currencies.go`:
- Around line 21-63: The combined currency list returned by ListCurrencies is
not deterministically ordered because customCurrencies (queried by
customcurrency.FieldCode) and defs (from currency.Definitions()) are
concatenated without a final sort; modify ListCurrencies to sort the merged
slice by the Currency.Code field before returning (e.g., use sort.Slice on the
appended slice or an lo-based sort) so the output is consistently ordered for
API consumers.
In `@openmeter/server/server_test.go`:
- Around line 139-412: Add smoke tests for the new currency endpoints inside the
TestRoutes tests table: update the tests slice in TestRoutes to include at least
one GET case for path "/api/v1/currencies" asserting status http.StatusOK and
one negative case (e.g. POST or GET with invalid input to "/api/v1/currencies"
or "/api/v1/currencies/{id}") asserting http.StatusBadRequest; use the same
testRequest/testResponse structs used by existing cases (refer to TestRoutes,
testRequest, testResponse, and the tests variable) so the new entries follow the
established pattern and will catch routing/wiring regressions.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/v3/server/routes.goopenmeter/currencies/adapter/currencies.goopenmeter/currencies/models.goopenmeter/server/server_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- openmeter/currencies/models.go
Comment on lines
+29
to
+52
| customCurrencies := lo.Map(currencyRecords, func(currency *entdb.CustomCurrency, _ int) currencies.Currency { | ||
| return currencies.Currency{ | ||
| ID: currency.ID, | ||
| Code: currency.Code, | ||
| Name: currency.Name, | ||
| Symbol: currency.Symbol, | ||
| IsCustom: true, | ||
| } | ||
| }) | ||
|
|
||
| defs := lo.Map(lo.Filter( | ||
| currency.Definitions(), | ||
| func(def *currency.Def, _ int) bool { | ||
| // NOTE: this filters out non-iso currencies such as crypto | ||
| return def.ISONumeric != "" | ||
| }, | ||
| ), func(def *currency.Def, _ int) currencies.Currency { | ||
| return currencies.Currency{ | ||
| Code: def.ISOCode.String(), | ||
| Name: def.Name, | ||
| Symbol: def.Symbol, | ||
| IsCustom: false, | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
No deduplication between custom and ISO currencies — duplicate codes can slip through.
If someone creates a custom currency with a code that also exists as an ISO currency (e.g. "USD"), both entries end up in the merged list. The Ent schema has no cross-table uniqueness guarantee here. At minimum, consider filtering out ISO entries whose code matches any custom currency code before appending.
💡 Suggested approach
+ customCodes := lo.SliceToMap(customCurrencies, func(c currencies.Currency) (string, struct{}) {
+ return c.Code, struct{}{}
+ })
+
defs := lo.Map(lo.Filter(
currency.Definitions(),
func(def *currency.Def, _ int) bool {
- return def.ISONumeric != ""
+ _, isOverridden := customCodes[def.ISOCode.String()]
+ return def.ISONumeric != "" && !isOverridden
},
), ...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/currencies/adapter/currencies.go` around lines 29 - 52,
customCurrencies and defs can contain duplicate currency codes (e.g., a custom
"USD" plus ISO "USD"); before creating defs (from currency.Definitions()) build
a set of custom currency codes from currencyRecords/customCurrencies
(normalizing case if needed) and filter out any def whose ISOCode matches a
custom code; update the lo.Filter/lo.Map pipeline that produces defs to exclude
those codes so the merged list doesn't contain duplicates (refer to
customCurrencies, currencyRecords, currency.Definitions(), and the defs
variable).
2014854 to
e92a87a
Compare
mark-vass-konghq
force-pushed
the
feat/currencies-v3-api-handlers
branch
7 times, most recently
from
da8cca9 to
d1f8917
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (8)
openmeter/currencies/adapter/currencies.go (5)
86-93:⚠️ Potential issue | 🟡 MinorDouble
time.Now()call — still a subtle TOCTOU.Lines 87 and 89 call
time.Now()separately, soeffectiveFromcould be set to a slightly different instant than the one used for validation.🛠️ Proposed fix
func (a *adapter) CreateCostBasis(ctx context.Context, params currencies.CreateCostBasisInput) (*currencies.CostBasis, error) { - effectiveFrom := time.Now() + now := time.Now() + effectiveFrom := now if params.EffectiveFrom != nil { - if params.EffectiveFrom.Before(time.Now()) { + if params.EffectiveFrom.Before(now) { return nil, models.NewGenericConflictError(fmt.Errorf("effective from must be in the future")) } effectiveFrom = *params.EffectiveFrom }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/currencies/adapter/currencies.go` around lines 86 - 93, The CreateCostBasis function uses time.Now() twice causing a TOCTOU drift; capture the current time once into a local variable (e.g., now := time.Now()) at the start of CreateCostBasis and use that single timestamp for both validation (params.EffectiveFrom.Before(now)) and setting effectiveFrom (defaulting to now) so effectiveFrom and the validation reference the same instant.
28-51:⚠️ Potential issue | 🟠 MajorCustom currencies and ISO currencies can still have duplicate codes.
A custom currency with code
"USD"and the ISO definition for"USD"would both end up in the returned list. There's no cross-table uniqueness guarantee at the DB level. Worth filtering out ISO entries whose code is already covered by a custom entry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/currencies/adapter/currencies.go` around lines 28 - 51, customCurrencies and defs can contain duplicate currency codes (e.g., a custom "USD" plus ISO "USD"); update the adapter to remove ISO definitions whose ISOCode matches any custom currency code by building a set of custom codes from customCurrencies (derived from currencyRecords) and filtering currency.Definitions() (the def.ISOCode.String() values) before mapping to defs so that defs excludes entries already present in customCurrencies; keep the existing filtering on def.ISONumeric and mapping to currencies.Currency.
53-61: Still a no-oplo.Maphere — just return the appended slice directly.This
lo.Mapreconstructs eachcurrencies.Currencyfield-by-field into an identical struct, effectively doing nothing.♻️ Proposed fix
- return lo.Map(append(customCurrencies, defs...), func(def currencies.Currency, _ int) currencies.Currency { - return currencies.Currency{ - ID: def.ID, - Code: def.Code, - Name: def.Name, - Symbol: def.Symbol, - IsCustom: def.IsCustom, - } - }), nil + return append(customCurrencies, defs...), nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/currencies/adapter/currencies.go` around lines 53 - 61, The code is doing a no-op transformation by calling lo.Map over append(customCurrencies, defs...) and rebuilding identical currencies.Currency structs; replace the lo.Map call with directly returning the appended slice (return append(customCurrencies, defs...), nil) so you don't recreate identical structs, and remove the lo.Map usage/import if it becomes unused; reference symbols: lo.Map, customCurrencies, defs, and currencies.Currency.
130-138:⚠️ Potential issue | 🔴 CriticalPotential nil panic on
costBasis.Edges.Currency— still unguarded.If the related
CustomCurrencyedge fails to load for any row,costBasis.Edges.Currencywill beniland.IDon line 133 will panic.🛡️ Proposed guard
return lo.Map(costBases, func(costBasis *entdb.CurrencyCostBasis, _ int) currencies.CostBasis { + var currencyID string + if costBasis.Edges.Currency != nil { + currencyID = costBasis.Edges.Currency.ID + } return currencies.CostBasis{ ID: costBasis.ID, - CurrencyID: costBasis.Edges.Currency.ID, + CurrencyID: currencyID, FiatCode: costBasis.FiatCode, Rate: costBasis.Rate, EffectiveFrom: costBasis.EffectiveFrom, } }), nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/currencies/adapter/currencies.go` around lines 130 - 138, The mapping over costBases in the lo.Map call can panic when costBasis.Edges.Currency is nil; update the mapper used when constructing currencies.CostBasis to guard access to costBasis.Edges and costBasis.Edges.Currency (e.g., check if costBasis.Edges == nil || costBasis.Edges.Currency == nil) and handle that case safely—either skip that entry, return a currencies.CostBasis with an empty/zero CurrencyID or return an error upstream depending on existing function semantics; ensure you reference the lo.Map mapper, currencies.CostBasis construction, and costBasis.Edges.Currency when making the change.
116-129:⚠️ Potential issue | 🟠 Major
entdb.IsNotFoundafterAll()is still dead code.Ent's
All()returns an empty slice (not a not-found error) when no records match. This branch can never be reached. If you want a not-found response when the parent currency doesn't exist, you'll need an explicit existence check onCustomCurrencyfirst.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/currencies/adapter/currencies.go` around lines 116 - 129, The entdb.IsNotFound branch in GetCostBasesByCurrencyID is dead code because CurrencyCostBasis.Query().All(ctx) returns an empty slice rather than a not-found error; either remove the entdb.IsNotFound handling and return the empty slice (nil error) or add an explicit existence check on the parent currency before querying cost bases (call a.db.CustomCurrency.Query().Where(customcurrency.ID(currencyID)).Exist(ctx) and if false return models.NewGenericNotFoundError(fmt.Errorf(...))). Update GetCostBasesByCurrencyID to perform the chosen behavior and eliminate the unreachable entdb.IsNotFound branch.api/v3/handlers/currencies/create_cost_basis.go (2)
14-14:⚠️ Potential issue | 🔴 Critical
CreateCostBasisResponse = struct{}means the client always gets an empty 201 body.This is paired with the issue below — the response type should be
*currencies.CostBasisto return the created resource.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/currencies/create_cost_basis.go` at line 14, The response type is currently an empty struct (CreateCostBasisResponse) so POST returns an empty 201 body; change CreateCostBasisResponse to use the created resource type (e.g., *currencies.CostBasis) and ensure the handler that constructs the response (the create cost-basis handler using CreateCostBasisResponse) returns the newly created currencies.CostBasis instance so the client receives the created resource in the 201 response.
30-36:⚠️ Potential issue | 🔴 CriticalStill discarding the created cost basis — 201 body will always be empty.
The result of
CreateCostBasisis thrown away on line 31 and the handler returns an empty struct. The client gets no info about what was just created.🛠️ Proposed fix
type ( CreateCostBasisRequest = currencies.CreateCostBasisInput - CreateCostBasisResponse = struct{} + CreateCostBasisResponse = *currencies.CostBasis CreateCostBasisHandler = httptransport.HandlerWithArgs[CreateCostBasisRequest, CreateCostBasisResponse, string] )func(ctx context.Context, request CreateCostBasisRequest) (CreateCostBasisResponse, error) { - _, err := h.currencyService.CreateCostBasis(ctx, request) + costBasis, err := h.currencyService.CreateCostBasis(ctx, request) if err != nil { - return CreateCostBasisResponse{}, err + return nil, err } - return CreateCostBasisResponse{}, nil + return costBasis, nil },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/currencies/create_cost_basis.go` around lines 30 - 36, The handler is discarding the created cost basis; change the call to capture the result from h.currencyService.CreateCostBasis (e.g., cb, err := h.currencyService.CreateCostBasis(ctx, request)), handle err as before, and return a CreateCostBasisResponse populated with the created cost-basis value (instead of an empty struct) so the client receives the created resource; update the return to return CreateCostBasisResponse{...appropriate field...: cb}, nil using the response field name expected by CreateCostBasisResponse.openmeter/server/server_test.go (1)
1657-1658:⚠️ Potential issue | 🟡 MinorDoc comment still says
currency.Service— should becurrencies.CurrencyService.The comment on line 1657 refers to the wrong type name.
✏️ Suggested fix
-// NoopCurrencyService implements currency.Service with no-op operations +// NoopCurrencyService implements currencies.CurrencyService with no-op operations // for use in testing🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/server/server_test.go` around lines 1657 - 1658, Update the doc comment for NoopCurrencyService to reference the correct type name currencies.CurrencyService instead of currency.Service; locate the comment above the NoopCurrencyService declaration and change the text to "NoopCurrencyService implements currencies.CurrencyService with no-op operations for use in testing" (or equivalent) so the doc matches the actual type.
🧹 Nitpick comments (3)
openmeter/currencies/adapter.go (1)
14-19: Return type inconsistency betweenCreateCurrencyandCreateCostBasis
CreateCurrencyreturns a value(Currency, error)whileCreateCostBasisreturns a pointer(*CostBasis, error). For single-entity create operations like these, it's nice to be consistent. Since neither nil carries special meaning here (both always return a fully populated result on success), you could align them to both return values — or both pointers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/currencies/adapter.go` around lines 14 - 19, The CurrenciesAdapter interface mixes return types for create operations—CreateCurrency returns (Currency, error) while CreateCostBasis returns (*CostBasis, error); make them consistent by choosing either value or pointer for both. Update the CreateCostBasis signature to return (CostBasis, error) or change CreateCurrency to return (*Currency, error) so both create methods use the same style, and adjust all implementations of CurrenciesAdapter and any callers of CreateCurrency/CreateCostBasis to match the chosen signature (including constructors, repository/service methods, and tests) to ensure compilation.openmeter/server/server_test.go (1)
1657-1677: No test cases for the currency endpoints inTestRoutes.
NoopCurrencyServiceis wired into the test server, butTestRoutesdoesn't exercise any currency API routes. Given that the handler is now registered on the router, it'd be great to add at least a smoke-test case (e.g.,GET /api/v1/currencies→200 OKwith an empty list) to confirm routing is actually hooked up correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/server/server_test.go` around lines 1657 - 1677, Add a smoke test in TestRoutes that exercises the registered currency handler: use the existing test server setup that wires NoopCurrencyService and perform an HTTP GET to "/api/v1/currencies", then assert the response status is 200 and the body is an empty list (or JSON array) to verify routing is hooked up; place this case alongside the other route tests in TestRoutes and reference NoopCurrencyService/handler/router to locate where the test server is configured.openmeter/server/router/router.go (1)
15-15: Import aliascurrencyHandlerclashes with the struct field of the same name.Every other handler import in this file uses a
x<type>driverorx<type>handlerstyle alias that's distinct from the field name (e.g.,billinghttpdriver,apphttpdriver). UsingcurrencyHandleras both the import alias and the struct field name makescurrencyHandler currencyHandler.Handlerlook a bit self-referential and breaks that convention. Renaming the alias keeps things consistent:♻️ Suggested rename
-currencyHandler "github.com/openmeterio/openmeter/api/v3/handlers/currencies" +currencyhttphandler "github.com/openmeterio/openmeter/api/v3/handlers/currencies"Then in the
Routerstruct (line 235):-currencyHandler currencyHandler.Handler +currencyHandler currencyhttphandler.HandlerAnd in
NewRouter(line 474):-router.currencyHandler = currencyHandler.New( +router.currencyHandler = currencyhttphandler.New(Also applies to: 235-235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/server/router/router.go` at line 15, The import alias currencyHandler conflicts with the Router struct field of the same name and breaks the existing alias convention; rename the import alias (e.g., to currencyhttpdriver) and update all usages accordingly so the Router field stays named currencyHandler but its type/reference uses the new alias (change occurrences like "currencyHandler currencyHandler.Handler" to "currencyHandler currencyhttpdriver.Handler" and update references in NewRouter and any other usages).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@api/v3/handlers/currencies/create_cost_basis.go`:
- Line 14: The response type is currently an empty struct
(CreateCostBasisResponse) so POST returns an empty 201 body; change
CreateCostBasisResponse to use the created resource type (e.g.,
*currencies.CostBasis) and ensure the handler that constructs the response (the
create cost-basis handler using CreateCostBasisResponse) returns the newly
created currencies.CostBasis instance so the client receives the created
resource in the 201 response.
- Around line 30-36: The handler is discarding the created cost basis; change
the call to capture the result from h.currencyService.CreateCostBasis (e.g., cb,
err := h.currencyService.CreateCostBasis(ctx, request)), handle err as before,
and return a CreateCostBasisResponse populated with the created cost-basis value
(instead of an empty struct) so the client receives the created resource; update
the return to return CreateCostBasisResponse{...appropriate field...: cb}, nil
using the response field name expected by CreateCostBasisResponse.
In `@openmeter/currencies/adapter/currencies.go`:
- Around line 86-93: The CreateCostBasis function uses time.Now() twice causing
a TOCTOU drift; capture the current time once into a local variable (e.g., now
:= time.Now()) at the start of CreateCostBasis and use that single timestamp for
both validation (params.EffectiveFrom.Before(now)) and setting effectiveFrom
(defaulting to now) so effectiveFrom and the validation reference the same
instant.
- Around line 28-51: customCurrencies and defs can contain duplicate currency
codes (e.g., a custom "USD" plus ISO "USD"); update the adapter to remove ISO
definitions whose ISOCode matches any custom currency code by building a set of
custom codes from customCurrencies (derived from currencyRecords) and filtering
currency.Definitions() (the def.ISOCode.String() values) before mapping to defs
so that defs excludes entries already present in customCurrencies; keep the
existing filtering on def.ISONumeric and mapping to currencies.Currency.
- Around line 53-61: The code is doing a no-op transformation by calling lo.Map
over append(customCurrencies, defs...) and rebuilding identical
currencies.Currency structs; replace the lo.Map call with directly returning the
appended slice (return append(customCurrencies, defs...), nil) so you don't
recreate identical structs, and remove the lo.Map usage/import if it becomes
unused; reference symbols: lo.Map, customCurrencies, defs, and
currencies.Currency.
- Around line 130-138: The mapping over costBases in the lo.Map call can panic
when costBasis.Edges.Currency is nil; update the mapper used when constructing
currencies.CostBasis to guard access to costBasis.Edges and
costBasis.Edges.Currency (e.g., check if costBasis.Edges == nil ||
costBasis.Edges.Currency == nil) and handle that case safely—either skip that
entry, return a currencies.CostBasis with an empty/zero CurrencyID or return an
error upstream depending on existing function semantics; ensure you reference
the lo.Map mapper, currencies.CostBasis construction, and
costBasis.Edges.Currency when making the change.
- Around line 116-129: The entdb.IsNotFound branch in GetCostBasesByCurrencyID
is dead code because CurrencyCostBasis.Query().All(ctx) returns an empty slice
rather than a not-found error; either remove the entdb.IsNotFound handling and
return the empty slice (nil error) or add an explicit existence check on the
parent currency before querying cost bases (call
a.db.CustomCurrency.Query().Where(customcurrency.ID(currencyID)).Exist(ctx) and
if false return models.NewGenericNotFoundError(fmt.Errorf(...))). Update
GetCostBasesByCurrencyID to perform the chosen behavior and eliminate the
unreachable entdb.IsNotFound branch.
In `@openmeter/server/server_test.go`:
- Around line 1657-1658: Update the doc comment for NoopCurrencyService to
reference the correct type name currencies.CurrencyService instead of
currency.Service; locate the comment above the NoopCurrencyService declaration
and change the text to "NoopCurrencyService implements
currencies.CurrencyService with no-op operations for use in testing" (or
equivalent) so the doc matches the actual type.
---
Nitpick comments:
In `@openmeter/currencies/adapter.go`:
- Around line 14-19: The CurrenciesAdapter interface mixes return types for
create operations—CreateCurrency returns (Currency, error) while CreateCostBasis
returns (*CostBasis, error); make them consistent by choosing either value or
pointer for both. Update the CreateCostBasis signature to return (CostBasis,
error) or change CreateCurrency to return (*Currency, error) so both create
methods use the same style, and adjust all implementations of CurrenciesAdapter
and any callers of CreateCurrency/CreateCostBasis to match the chosen signature
(including constructors, repository/service methods, and tests) to ensure
compilation.
In `@openmeter/server/router/router.go`:
- Line 15: The import alias currencyHandler conflicts with the Router struct
field of the same name and breaks the existing alias convention; rename the
import alias (e.g., to currencyhttpdriver) and update all usages accordingly so
the Router field stays named currencyHandler but its type/reference uses the new
alias (change occurrences like "currencyHandler currencyHandler.Handler" to
"currencyHandler currencyhttpdriver.Handler" and update references in NewRouter
and any other usages).
In `@openmeter/server/server_test.go`:
- Around line 1657-1677: Add a smoke test in TestRoutes that exercises the
registered currency handler: use the existing test server setup that wires
NoopCurrencyService and perform an HTTP GET to "/api/v1/currencies", then assert
the response status is 200 and the body is an empty list (or JSON array) to
verify routing is hooked up; place this case alongside the other route tests in
TestRoutes and reference NoopCurrencyService/handler/router to locate where the
test server is configured.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (21)
api/v3/handlers/currencies/convert.goapi/v3/handlers/currencies/create.goapi/v3/handlers/currencies/create_cost_basis.goapi/v3/handlers/currencies/get_cost_bases.goapi/v3/handlers/currencies/handler.goapi/v3/handlers/currencies/list.goapi/v3/server/routes.goapi/v3/server/server.goapp/common/currency.gocmd/server/main.gocmd/server/wire.gocmd/server/wire_gen.goopenmeter/currencies/adapter.goopenmeter/currencies/adapter/adapter.goopenmeter/currencies/adapter/currencies.goopenmeter/currencies/models.goopenmeter/currencies/service.goopenmeter/currencies/service/service.goopenmeter/server/router/router.goopenmeter/server/server.goopenmeter/server/server_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- openmeter/currencies/models.go
- openmeter/currencies/service.go
- api/v3/handlers/currencies/handler.go
- api/v3/handlers/currencies/list.go
- api/v3/handlers/currencies/convert.go
- cmd/server/main.go
- app/common/currency.go
- cmd/server/wire.go
- api/v3/handlers/currencies/create.go
- openmeter/currencies/adapter/adapter.go
- api/v3/server/server.go
mark-vass-konghq
force-pushed
the
feat/currencies-v3-api-handlers
branch
from
d1f8917 to
680f6cb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
openmeter/currencies/adapter/currencies.go (5)
86-93:⚠️ Potential issue | 🟡 MinorTwo separate
time.Now()calls create a TOCTOU window.The
effectiveFromdefault is captured on line 87, but the validation on line 89 callstime.Now()again — these are different instants. Capture it once.🐛 Proposed fix
func (a *adapter) CreateCostBasis(ctx context.Context, params currencies.CreateCostBasisInput) (*currencies.CostBasis, error) { - effectiveFrom := time.Now() + now := time.Now() + effectiveFrom := now if params.EffectiveFrom != nil { - if params.EffectiveFrom.Before(time.Now()) { + if params.EffectiveFrom.Before(now) { return nil, models.NewGenericConflictError(fmt.Errorf("effective from must be in the future")) } effectiveFrom = *params.EffectiveFrom }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/currencies/adapter/currencies.go` around lines 86 - 93, The code in adapter.CreateCostBasis uses time.Now() twice creating a TOCTOU race between effectiveFrom assignment and the validation; capture the current time once (e.g., timeNow := time.Now()) at the start of the function and use that single variable for both the default effectiveFrom and the Before check against params.EffectiveFrom to ensure consistent time comparisons (update references to effectiveFrom and the Before check accordingly).
53-61: 🛠️ Refactor suggestion | 🟠 MajorThe final
lo.Mapis a no-op — it just copies every field fromCurrencytoCurrency.You can drop it entirely and return the appended slice directly.
♻️ Proposed simplification
- return lo.Map(append(customCurrencies, defs...), func(def currencies.Currency, _ int) currencies.Currency { - return currencies.Currency{ - ID: def.ID, - Code: def.Code, - Name: def.Name, - Symbol: def.Symbol, - IsCustom: def.IsCustom, - } - }), nil + return append(customCurrencies, defs...), nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/currencies/adapter/currencies.go` around lines 53 - 61, The lo.Map call at the end (mapping append(customCurrencies, defs...) into the same currencies.Currency struct) is a no-op; simplify by removing the lo.Map wrapper and return the appended slice of Currency directly (i.e., return the result of appending customCurrencies and defs, along with nil error). Update the return in the function that currently uses lo.Map so it returns the plain append(customCurrencies, defs...) slice and nil instead of mapping each def through currencies.Currency again.
130-138:⚠️ Potential issue | 🔴 CriticalPotential nil-pointer panic on
costBasis.Edges.Currency.ID.If
WithCurrency()can't load the edge (e.g., the parent currency was hard-deleted),costBasis.Edges.Currencywill beniland Line 133 will panic at runtime.🛡️ Suggested guard
return lo.Map(costBases, func(costBasis *entdb.CurrencyCostBasis, _ int) currencies.CostBasis { + var currencyID string + if costBasis.Edges.Currency != nil { + currencyID = costBasis.Edges.Currency.ID + } return currencies.CostBasis{ ID: costBasis.ID, - CurrencyID: costBasis.Edges.Currency.ID, + CurrencyID: currencyID, FiatCode: costBasis.FiatCode, Rate: costBasis.Rate, EffectiveFrom: costBasis.EffectiveFrom, } }), nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/currencies/adapter/currencies.go` around lines 130 - 138, The mapping can panic when costBasis.Edges.Currency is nil; update the lo.Map callback that constructs currencies.CostBasis to check costBasis.Edges.Currency != nil before accessing .ID and handle the nil case (e.g., set CurrencyID to a zero value or skip/return an error for that entry and/or log the anomaly). Concretely, inside the anonymous function that returns currencies.CostBasis, branch on costBasis.Edges.Currency being nil and populate CurrencyID safely (or filter out the entry) instead of directly using costBasis.Edges.Currency.ID.
28-51:⚠️ Potential issue | 🟠 MajorCustom + ISO currency dedup issue — a custom "USD" would appear twice in the list.
If someone creates a custom currency with a code like
"USD",ListCurrencieswould return both the custom entry and the GOBL ISO entry. There's no cross-table uniqueness guarantee, andCreateCurrencyonly checks for duplicate custom currency codes.💡 Suggested approach
+ customCodes := lo.SliceToMap(customCurrencies, func(c currencies.Currency) (string, struct{}) { + return c.Code, struct{}{} + }) + defs := lo.Map(lo.Filter( currency.Definitions(), func(def *currency.Def, _ int) bool { - return def.ISONumeric != "" + _, isOverridden := customCodes[def.ISOCode.String()] + return def.ISONumeric != "" && !isOverridden }, ), ...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/currencies/adapter/currencies.go` around lines 28 - 51, The current mapping builds customCurrencies and defs separately, causing duplicate entries when a custom currency code (e.g., "USD") matches an ISO definition; update the assembly logic in the ListCurrencies flow to deduplicate by currency code (case-insensitive) so only one entry is returned and ensure custom currencies override ISO ones. Concretely, after creating customCurrencies and defs (from currencyRecords and currency.Definitions()), create a map keyed by normalized code, insert all customCurrencies first (so they win), then insert defs only if the normalized code is not already present, and finally produce the resulting slice from that map; reference variables/functions currencyRecords, customCurrencies, defs, currency.Definitions(), and currencies.Currency when making the change.
124-128:⚠️ Potential issue | 🟡 Minor
IsNotFoundcheck afterAll()is dead code —All()returns an empty slice, neverNotFoundError.This branch will never be hit. If you want to surface a not-found error when the currency ID doesn't exist, you'll need an explicit existence check before querying cost bases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/currencies/adapter/currencies.go` around lines 124 - 128, The IsNotFound check after calling All() is dead code because All() returns an empty slice rather than a NotFound error; modify the logic to explicitly check for the existence of the currency before querying cost bases: perform an existence lookup (e.g., fetch or Exists call for the currency ID) using the same DB/ent client, return models.NewGenericNotFoundError(fmt.Errorf("currency with id: %s not found", currencyID)) if that existence check fails, and only then call the cost-bases query (the code that currently calls All() and handles err via entdb.IsNotFound). Update/remove the entdb.IsNotFound branch in the current All() error handling accordingly so it no longer expects NotFound from All().api/v3/handlers/currencies/create_cost_basis.go (2)
14-14:⚠️ Potential issue | 🔴 Critical
CreateCostBasisResponse = struct{}guarantees an empty{}response body — the created resource is never returned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/currencies/create_cost_basis.go` at line 14, CreateCostBasisResponse is defined as an empty struct which forces the API to return an empty JSON body; change the response type to include the created resource fields (e.g., id, amount, currency, created_at or reuse the existing CostBasis/CostBasisDTO type) so the handler returns the newly created resource. Update CreateCostBasisResponse to mirror the model/DTO used elsewhere (or replace with the model type) and ensure the create handler populates and returns that value instead of an empty struct.
30-36:⚠️ Potential issue | 🔴 CriticalThe service result is discarded — clients receive an empty
{}201 instead of the created cost basis.The handler calls
CreateCostBasisbut throws away the returned*currencies.CostBasis, so the client never gets the ID, rate, or any other fields. The fix spans the response type alias and the handler body:🐛 Proposed fix sketch
type ( CreateCostBasisRequest = currencies.CreateCostBasisInput - CreateCostBasisResponse = struct{} + CreateCostBasisResponse = currencies.CostBasis // or a dedicated API response type CreateCostBasisHandler = httptransport.HandlerWithArgs[CreateCostBasisRequest, CreateCostBasisResponse, string] )func(ctx context.Context, request CreateCostBasisRequest) (CreateCostBasisResponse, error) { - _, err := h.currencyService.CreateCostBasis(ctx, request) + costBasis, err := h.currencyService.CreateCostBasis(ctx, request) if err != nil { return CreateCostBasisResponse{}, err } - return CreateCostBasisResponse{}, nil + return *costBasis, nil },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/currencies/create_cost_basis.go` around lines 30 - 36, The handler currently discards the result from h.currencyService.CreateCostBasis and returns an empty CreateCostBasisResponse; update the response wiring so the created *currencies.CostBasis is returned to clients. Concretely: change the CreateCostBasisResponse type (or alias) to represent currencies.CostBasis (or include a field to hold it), and modify the anonymous handler function to capture the returned value from h.currencyService.CreateCostBasis(ctx, request) (e.g., cb, err := ...) and return CreateCostBasisResponse populated from cb instead of returning an empty struct; keep error handling the same and ensure the returned response marshals the CostBasis ID/rate/fields.openmeter/server/server_test.go (1)
1657-1659: Doc comment still references the wrong interface —currency.Serviceshould becurrencies.CurrencyService.✏️ Quick fix
-// NoopCurrencyService implements currency.Service with no-op operations +// NoopCurrencyService implements currencies.CurrencyService with no-op operations // for use in testing🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/server/server_test.go` around lines 1657 - 1659, The doc comment above the NoopCurrencyService declaration incorrectly references currency.Service; update the comment to reference the correct interface name currencies.CurrencyService so it matches the type assertion line var _ currencies.CurrencyService = (*NoopCurrencyService)(nil) and avoids the stale identifier; locate the comment immediately above NoopCurrencyService and change the text to mention currencies.CurrencyService.
🧹 Nitpick comments (1)
openmeter/currencies/service/service.go (1)
21-35: Consider adding lightweight input validation here as the service layer evolves.Right now everything passes straight to the adapter. As this grows, it'd be worth validating things like non-empty
currencyID, validCodeformat, and positiveRateat the service boundary rather than relying solely on database constraints.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/currencies/service/service.go` around lines 21 - 35, Add lightweight input validation in the Service methods before calling the adapter: in CreateCurrency validate fields on currencies.CreateCurrencyInput (e.g., non-empty Code, correct format) and return a clear error if invalid; in CreateCostBasis validate currencies.CreateCostBasisInput (non-empty CurrencyID and positive Rate) and return an error if invalid; in GetCostBasesByCurrencyID ensure currencyID is non-empty and return an error if not; for ListCurrencies no-op or basic context validation. Implement these checks at the start of the respective methods (ListCurrencies, CreateCurrency, CreateCostBasis, GetCostBasesByCurrencyID) and only call s.adapter.* if validation passes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/currencies/adapter/currencies.go`:
- Line 90: The code in currencies.go currently returns
models.NewGenericConflictError for the "effective from must be in the future"
case; replace that with the appropriate 400-level validation error (e.g.,
models.NewBadRequestError or models.NewValidationError) so the handler returns
HTTP 400 instead of 409. Locate the return using NewGenericConflictError in the
function that validates the "effective from" date and swap it to the
bad-request/validation error constructor while preserving the error message
text.
---
Duplicate comments:
In `@api/v3/handlers/currencies/create_cost_basis.go`:
- Line 14: CreateCostBasisResponse is defined as an empty struct which forces
the API to return an empty JSON body; change the response type to include the
created resource fields (e.g., id, amount, currency, created_at or reuse the
existing CostBasis/CostBasisDTO type) so the handler returns the newly created
resource. Update CreateCostBasisResponse to mirror the model/DTO used elsewhere
(or replace with the model type) and ensure the create handler populates and
returns that value instead of an empty struct.
- Around line 30-36: The handler currently discards the result from
h.currencyService.CreateCostBasis and returns an empty CreateCostBasisResponse;
update the response wiring so the created *currencies.CostBasis is returned to
clients. Concretely: change the CreateCostBasisResponse type (or alias) to
represent currencies.CostBasis (or include a field to hold it), and modify the
anonymous handler function to capture the returned value from
h.currencyService.CreateCostBasis(ctx, request) (e.g., cb, err := ...) and
return CreateCostBasisResponse populated from cb instead of returning an empty
struct; keep error handling the same and ensure the returned response marshals
the CostBasis ID/rate/fields.
In `@openmeter/currencies/adapter/currencies.go`:
- Around line 86-93: The code in adapter.CreateCostBasis uses time.Now() twice
creating a TOCTOU race between effectiveFrom assignment and the validation;
capture the current time once (e.g., timeNow := time.Now()) at the start of the
function and use that single variable for both the default effectiveFrom and the
Before check against params.EffectiveFrom to ensure consistent time comparisons
(update references to effectiveFrom and the Before check accordingly).
- Around line 53-61: The lo.Map call at the end (mapping
append(customCurrencies, defs...) into the same currencies.Currency struct) is a
no-op; simplify by removing the lo.Map wrapper and return the appended slice of
Currency directly (i.e., return the result of appending customCurrencies and
defs, along with nil error). Update the return in the function that currently
uses lo.Map so it returns the plain append(customCurrencies, defs...) slice and
nil instead of mapping each def through currencies.Currency again.
- Around line 130-138: The mapping can panic when costBasis.Edges.Currency is
nil; update the lo.Map callback that constructs currencies.CostBasis to check
costBasis.Edges.Currency != nil before accessing .ID and handle the nil case
(e.g., set CurrencyID to a zero value or skip/return an error for that entry
and/or log the anomaly). Concretely, inside the anonymous function that returns
currencies.CostBasis, branch on costBasis.Edges.Currency being nil and populate
CurrencyID safely (or filter out the entry) instead of directly using
costBasis.Edges.Currency.ID.
- Around line 28-51: The current mapping builds customCurrencies and defs
separately, causing duplicate entries when a custom currency code (e.g., "USD")
matches an ISO definition; update the assembly logic in the ListCurrencies flow
to deduplicate by currency code (case-insensitive) so only one entry is returned
and ensure custom currencies override ISO ones. Concretely, after creating
customCurrencies and defs (from currencyRecords and currency.Definitions()),
create a map keyed by normalized code, insert all customCurrencies first (so
they win), then insert defs only if the normalized code is not already present,
and finally produce the resulting slice from that map; reference
variables/functions currencyRecords, customCurrencies, defs,
currency.Definitions(), and currencies.Currency when making the change.
- Around line 124-128: The IsNotFound check after calling All() is dead code
because All() returns an empty slice rather than a NotFound error; modify the
logic to explicitly check for the existence of the currency before querying cost
bases: perform an existence lookup (e.g., fetch or Exists call for the currency
ID) using the same DB/ent client, return
models.NewGenericNotFoundError(fmt.Errorf("currency with id: %s not found",
currencyID)) if that existence check fails, and only then call the cost-bases
query (the code that currently calls All() and handles err via
entdb.IsNotFound). Update/remove the entdb.IsNotFound branch in the current
All() error handling accordingly so it no longer expects NotFound from All().
In `@openmeter/server/server_test.go`:
- Around line 1657-1659: The doc comment above the NoopCurrencyService
declaration incorrectly references currency.Service; update the comment to
reference the correct interface name currencies.CurrencyService so it matches
the type assertion line var _ currencies.CurrencyService =
(*NoopCurrencyService)(nil) and avoids the stale identifier; locate the comment
immediately above NoopCurrencyService and change the text to mention
currencies.CurrencyService.
---
Nitpick comments:
In `@openmeter/currencies/service/service.go`:
- Around line 21-35: Add lightweight input validation in the Service methods
before calling the adapter: in CreateCurrency validate fields on
currencies.CreateCurrencyInput (e.g., non-empty Code, correct format) and return
a clear error if invalid; in CreateCostBasis validate
currencies.CreateCostBasisInput (non-empty CurrencyID and positive Rate) and
return an error if invalid; in GetCostBasesByCurrencyID ensure currencyID is
non-empty and return an error if not; for ListCurrencies no-op or basic context
validation. Implement these checks at the start of the respective methods
(ListCurrencies, CreateCurrency, CreateCostBasis, GetCostBasesByCurrencyID) and
only call s.adapter.* if validation passes.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (23)
api/v3/handlers/currencies/convert.goapi/v3/handlers/currencies/create.goapi/v3/handlers/currencies/create_cost_basis.goapi/v3/handlers/currencies/get_cost_bases.goapi/v3/handlers/currencies/handler.goapi/v3/handlers/currencies/list.goapi/v3/server/routes.goapi/v3/server/server.goapp/common/currency.gocmd/server/main.gocmd/server/wire.gocmd/server/wire_gen.goopenmeter/currencies/adapter.goopenmeter/currencies/adapter/adapter.goopenmeter/currencies/adapter/currencies.goopenmeter/currencies/models.goopenmeter/currencies/service.goopenmeter/currencies/service/service.goopenmeter/server/router/router.goopenmeter/server/server.goopenmeter/server/server_test.gotools/migrate/migrations/20260223191305_add_custom_currency.down.sqltools/migrate/migrations/20260223191305_add_custom_currency.up.sql
🚧 Files skipped from review as they are similar to previous changes (13)
- api/v3/handlers/currencies/get_cost_bases.go
- cmd/server/main.go
- api/v3/handlers/currencies/convert.go
- openmeter/currencies/adapter/adapter.go
- api/v3/handlers/currencies/handler.go
- openmeter/currencies/adapter.go
- api/v3/handlers/currencies/list.go
- openmeter/currencies/service.go
- api/v3/handlers/currencies/create.go
- openmeter/server/router/router.go
- openmeter/currencies/models.go
- openmeter/server/server.go
- cmd/server/wire.go
mark-vass-konghq
force-pushed
the
feat/currencies-v3-api-ent
branch
3 times, most recently
from
588dc17 to
4223e19
Compare
mark-vass-konghq
force-pushed
the
feat/currencies-v3-api-handlers
branch
from
680f6cb to
e561b01
Compare
mark-vass-konghq
force-pushed
the
feat/currencies-v3-api-ent
branch
2 times, most recently
from
81218fb to
0c492f3
Compare
mark-vass-konghq
force-pushed
the
feat/currencies-v3-api-handlers
branch
from
237b5d1 to
ef6eaa7
Compare
mark-vass-konghq
force-pushed
the
feat/currencies-v3-api-ent
branch
from
1dd820b to
535baa8
Compare
mark-vass-konghq
force-pushed
the
feat/currencies-v3-api-ent
branch
2 times, most recently
from
7514cba to
c8b2e0d
Compare
tothandras
force-pushed
the
feat/currencies-v3-api-ent
branch
2 times, most recently
from
6717db1 to
8cfc1f3
Compare
mark-vass-konghq
force-pushed
the
feat/currencies-v3-api-ent
branch
2 times, most recently
from
6af5848 to
1553d1e
Compare
mark-vass-konghq
force-pushed
the
feat/currencies-v3-api-handlers
branch
from
6835e4e to
624c645
Compare
mark-vass-konghq
force-pushed
the
feat/currencies-v3-api-ent
branch
2 times, most recently
from
9a79c79 to
589747a
Compare
turip
reviewed
Mar 3, 2026
| func NewCurrencyService(logger *slog.Logger, db *entdb.Client) (currencies.CurrencyService, error) { | ||
| adapter, err := currencyAdapter.New(currencyAdapter.Config{ | ||
| Client: db, | ||
| Logger: logger.WithGroup("currency.postgres"), |
Member
There was a problem hiding this comment.
Suggested change
| Logger: logger.WithGroup("currency.postgres"), |
Let's not do this.
Comment on lines
+92
to
+103
| curr, err := a.db.CustomCurrency.Create(). | ||
| SetNamespace(params.Namespace). | ||
| SetCode(params.Code). | ||
| SetName(params.Name). | ||
| SetSymbol(params.Symbol). | ||
| Save(ctx) | ||
| if err != nil { | ||
| if entdb.IsConstraintError(err) { | ||
| return v3.BillingCurrencyCustom{}, models.NewGenericConflictError(fmt.Errorf("currency with code %s already exists", params.Code)) | ||
| } | ||
| return v3.BillingCurrencyCustom{}, fmt.Errorf("failed to create currency: %w", err) | ||
| } |
| } | ||
|
|
||
| type CurrenciesAdapter interface { | ||
| ListCurrencies(ctx context.Context, params ListCurrenciesInput) ([]v3.BillingCurrency, int, error) |
Member
There was a problem hiding this comment.
Suggested change
| ListCurrencies(ctx context.Context, params ListCurrenciesInput) ([]v3.BillingCurrency, int, error) | |
| ListCurrencies(ctx context.Context, params ListCurrenciesInput) (pagination.Result[Currency], error) |
| } | ||
|
|
||
| func (s *Service) ListCurrencies(ctx context.Context, params currencies.ListCurrenciesInput) ([]v3.BillingCurrency, int, error) { | ||
| return s.adapter.ListCurrencies(ctx, params) |
Member
There was a problem hiding this comment.
Suggested change
| return s.adapter.ListCurrencies(ctx, params) | |
| return transaction.Run(ctx, s.adapter, func(ctx context.Context) (pagination.Result[currencies.Currency], error) { | |
| return s.adapter.ListCurrencies(ctx, params) | |
| }) |
Comment on lines
+54
to
+68
| for _, def := range lo.Filter(currency.Definitions(), func(def *currency.Def, _ int) bool { | ||
| // NOTE: this filters out non-iso currencies such as crypto | ||
| return def.ISONumeric != "" | ||
| }) { | ||
| var item v3.BillingCurrency | ||
| if err := item.FromBillingCurrencyFiat(v3.BillingCurrencyFiat{ | ||
| Code: def.ISOCode.String(), | ||
| Name: def.Name, | ||
| Symbol: &def.Symbol, | ||
| Type: v3.BillingCurrencyFiatTypeFiat, | ||
| }); err != nil { | ||
| return nil, 0, fmt.Errorf("failed to construct BillingCurrencyFiat: %w", err) | ||
| } | ||
| all = append(all, item) | ||
| } |
Comment on lines
+37
to
+50
| for _, c := range currencyRecords { | ||
| var item v3.BillingCurrency | ||
| if err := item.FromBillingCurrencyCustom(v3.BillingCurrencyCustom{ | ||
| Id: c.ID, | ||
| Code: c.Code, | ||
| Name: c.Name, | ||
| Symbol: &c.Symbol, | ||
| Type: v3.BillingCurrencyCustomTypeCustom, | ||
| CreatedAt: &c.CreatedAt, | ||
| }); err != nil { | ||
| return nil, 0, fmt.Errorf("failed to construct BillingCurrencyCustom: %w", err) | ||
| } | ||
| all = append(all, item) | ||
| } |
| SetCurrencyID(params.CurrencyID). | ||
| SetFiatCode(params.FiatCode). | ||
| SetRate(params.Rate). | ||
| SetEffectiveFrom(effectiveFrom). |
Member
There was a problem hiding this comment.
Suggested change
| SetEffectiveFrom(effectiveFrom). | |
| SetEffectiveFrom(effectiveFrom.In(time.UTC)). |
Comment on lines
+36
to
+40
| Namespace string | ||
| CurrencyID string | ||
| FiatCode string `json:"fiat_code"` | ||
| Rate alpacadecimal.Decimal `json:"rate"` | ||
| EffectiveFrom *time.Time `json:"effective_from"` |
mark-vass-konghq
force-pushed
the
feat/currencies-v3-api-handlers
branch
3 times, most recently
from
e5bfc48 to
f132f0f
Compare
turip
reviewed
Mar 3, 2026
| ) | ||
|
|
||
| type Currency struct { | ||
| ID string `json:"id"` |
Member
There was a problem hiding this comment.
use the models package (models.ManagedModel, models.NamespacedID)
Comment on lines
+44
to
+49
| switch *i.FilterType { | ||
| case CurrencyTypeCustom, CurrencyTypeFiat: | ||
| // valid | ||
| default: | ||
| errs = append(errs, fmt.Errorf("invalid filter_type: %s", *i.FilterType)) | ||
| } |
Member
There was a problem hiding this comment.
Let's move this into a validator function for the filter type.
| } | ||
|
|
||
| type CostBasis struct { | ||
| ID string `json:"id"` |
| "github.com/getkin/kin-openapi/openapi3filter" | ||
|
|
||
| "github.com/openmeterio/openmeter/api" | ||
| currencyHandler "github.com/openmeterio/openmeter/api/v3/handlers/currencies" |
Member
There was a problem hiding this comment.
Suggested change
| currencyHandler "github.com/openmeterio/openmeter/api/v3/handlers/currencies" | |
| currencyhandler "github.com/openmeterio/openmeter/api/v3/handlers/currencies" |
Comment on lines
+47
to
+56
| return currencies.Currency{ | ||
| ID: c.ID, | ||
| Namespace: c.Namespace, | ||
| Code: c.Code, | ||
| Name: c.Name, | ||
| Symbol: c.Symbol, | ||
| CreatedAt: c.CreatedAt, | ||
| UpdatedAt: &c.UpdatedAt, | ||
| DeletedAt: c.DeletedAt, | ||
| } |
Member
There was a problem hiding this comment.
Let's create a mapCurrencyFromDB(*entdb.CustomCurrency) currencies.Currency and let's reuse that.
| /** | ||
| * The ID of the custom currency this cost basis belongs to. | ||
| */ | ||
| @visibility(Lifecycle.Read) |
Member
There was a problem hiding this comment.
Let's remove this please, as it's available from the context.
| if err != nil { | ||
| return CreateCurrencyResponse{}, apierrors.NewConflictError(ctx, err, "Currency already exists") | ||
| } | ||
| return v3.BillingCurrencyCustom{ |
mark-vass-konghq
force-pushed
the
feat/currencies-v3-api-handlers
branch
from
e442f49 to
adbec0f
Compare
mark-vass-konghq
force-pushed
the
feat/currencies-v3-api-handlers
branch
from
adbec0f to
87d982f
Compare
turip
reviewed
Mar 4, 2026
Comment on lines
+136
to
+147
| costBases, err := q.All(ctx) | ||
| if err != nil { | ||
| return pagination.Result[currencies.CostBasis]{}, fmt.Errorf("failed to list cost bases: %w", err) | ||
| } | ||
|
|
||
| return pagination.Result[currencies.CostBasis]{ | ||
| Page: params.Page, | ||
| TotalCount: total, | ||
| Items: lo.Map(costBases, func(cb *entdb.CurrencyCostBasis, _ int) currencies.CostBasis { | ||
| return mapCostBasisFromDB(cb) | ||
| }), | ||
| }, nil |
Member
There was a problem hiding this comment.
Please use query.Paginate instead so that you are not fetching all the records. Example:
openmeter/openmeter/customer/adapter/customer.go
Lines 104 to 107 in 15cd3a0
|
|
||
| if i.FilterType != nil { | ||
| if err := i.FilterType.Validate(); err != nil { | ||
| errs = append(errs, fmt.Errorf("invalid filter_type: %w", err)) |
Member
There was a problem hiding this comment.
Suggested change
| errs = append(errs, fmt.Errorf("invalid filter_type: %w", err)) | |
| errs = append(errs, fmt.Errorf("filter_type: %w", err)) |
This is super nit, but in validators it's worth being terse as these will concatenate into something like:
invalid filter_type: invalid currency type: bugous type
Comment on lines
+131
to
+133
| if i.Rate.IsNegative() { | ||
| errs = append(errs, errors.New("rate must be non-negative")) | ||
| } |
Member
There was a problem hiding this comment.
I would prefer that this should be positive (e.g. let's not allow free credits for now)
| systems = [ "x86_64-linux" "x86_64-darwin" "aarch64-darwin" "aarch64-linux" ]; | ||
|
|
||
| perSystem = { config, self', inputs', pkgs, lib, system, ... }: rec { | ||
| # FIX: Removed the 'system' argument to fix evaluation warnings |
Member
There was a problem hiding this comment.
Please move flake updates into a seperate pr, that we can review seperately.
We prefer small PRs when possible and this change has nothing to do with currencies api.
turip
approved these changes
Mar 4, 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
3 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.
Features
Summary by CodeRabbit