feat: add currency resolver#4756
Conversation
chrisgacsal
added
the
release-note/ignore
📝 WalkthroughWalkthroughCurrency listing now supports currency type, ID, and union or intersection matching. A namespace-aware resolver handles single and batch currency references, with API mapping, dependency wiring, and tests updated. ChangesCurrency filtering and resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant namespacedResolver
participant resolver
participant currenciesService
namespacedResolver->>resolver: delegate resolution with namespace
resolver->>currenciesService: ListCurrencies with ID and Code filters
currenciesService-->>resolver: paginated currencies
resolver-->>namespacedResolver: resolved currency map or error
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openmeter/currencies/currencyresolver/resolver.go (1)
140-148: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider classifying the "multiple active currencies" error.
This new ambiguity guard (e.g. a custom currency sharing a code with a fiat ISO code in the same namespace) returns a bare
fmt.Errorf, unlike similar conflict/ambiguity situations elsewhere in this package (e.g.adapter/currencies.go'sCreateCurrencyusesmodels.NewGenericConflictErrorfor a comparable "already exists" case). Worth considering wrapping this in a recognizable domain error type so callers/observability tooling can distinguish "ambiguous namespace data" from a generic internal error — though it's not clear "conflict" is the exact right classification here since this is discovered incidentally at read time rather than caused by the current request.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/currencies/currencyresolver/resolver.go` around lines 140 - 148, Classify the duplicate active-currency condition in the resolver’s byCode validation instead of returning a bare fmt.Errorf. Introduce or reuse a recognizable domain error type for ambiguous namespace data, preserving the existing error message and early-return behavior so callers can distinguish this condition from generic internal failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openmeter/currencies/resolver.go`:
- Around line 20-30: Update CurrencyRef.Validate to wrap the missing ID/code
error with models.NewNillableGenericValidationError, preserving the existing
message. Adjust the exact error assertion in resolver_test.go to expect the
helper’s “validation error:” prefix while retaining
models.IsGenericValidationError compatibility.
---
Nitpick comments:
In `@openmeter/currencies/currencyresolver/resolver.go`:
- Around line 140-148: Classify the duplicate active-currency condition in the
resolver’s byCode validation instead of returning a bare fmt.Errorf. Introduce
or reuse a recognizable domain error type for ambiguous namespace data,
preserving the existing error message and early-return behavior so callers can
distinguish this condition from generic internal failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c8e41ae3-30b5-43ee-af57-bb1262ac20bb
📒 Files selected for processing (11)
api/v3/handlers/currencies/list.goapi/v3/handlers/currencies/list_test.goapp/common/currency.goopenmeter/currencies/adapter/currencies.goopenmeter/currencies/adapter/currencies_test.goopenmeter/currencies/currencyresolver/resolver.goopenmeter/currencies/currencyresolver/resolver_test.goopenmeter/currencies/resolver.goopenmeter/currencies/service.goopenmeter/currencies/service/service.goopenmeter/currencies/service/service_test.go
|
@coderabbitai Regarding the nitpick about classifying the duplicate active-currency-code condition: I am leaving this as an internal error intentionally. This state violates persisted-domain invariants: active custom codes are unique per namespace, and custom-currency validation rejects fiat codes. It is not a conflict caused by the request, so GenericConflictError would expose corrupted or legacy server data as a client-correctable 409. If machine-readable classification becomes necessary, an internal data-integrity sentinel would be more accurate than a generic conflict. |
|
Tip For best results, initiate chat on the files or code changes.
🔎🐇 ✏️ Learnings added
|
2 participants
Overview
Add currency resolver to
currenciespackage which can be used for resolving Currency byidorcode.Fixes #(issue)
Notes for reviewer
Summary by CodeRabbit
currency_type, plusidandcodefiltering with configurable matching-all vs matching-any behavior.Greptile Summary
This PR adds currency resolution by ID or code. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "fix: classify invalid currency reference..." | Re-trigger Greptile
Context used (3)