chore: Replace local currency package with go-libs version #500
Conversation
WalkthroughThis change removes the internal Changes
Sequence Diagram(s)Not applicable: No new features or control flow changes introduced. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #500 +/- ##
==========================================
- Coverage 69.55% 69.55% -0.01%
==========================================
Files 628 626 -2
Lines 32401 32302 -99
==========================================
- Hits 22536 22467 -69
+ Misses 8620 8590 -30
Partials 1245 1245 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
go.mod
Outdated
| github.com/adyen/adyen-go-api-library/v7 v7.3.1 | ||
| github.com/bombsimon/logrusr/v3 v3.1.0 | ||
| github.com/formancehq/go-libs/v3 v3.0.0 | ||
| github.com/formancehq/go-libs/v3 v3.0.1-0.20250730083246-96260bc6aaa5 |
There was a problem hiding this comment.
should we do a release for go libs instead? Not sure 🤷
I'll have the same problem in not too long, so maybe not worth it yet?
There was a problem hiding this comment.
Oh this explains why it started to complain in my PR. dependabot downgraded go-libs 😨
| ) *chi.Mux { | ||
| return NewRouter(backend, info, healthController, a, debug, versions...) | ||
| }, fx.ParamTags(``, ``, ``, ``, `group:"apiVersions"`))), | ||
| }, fx.ParamTags(``, ``, ``, ``, `group:"apiVersions"`), fx.ResultTags(`name:"apiRouter"`))), |
There was a problem hiding this comment.
can we do it the other way around -- make sure any router provided by go-libs are only for the go-libs stuff?
Probably wishful thinking, but it'd make the lib less "intrusive"
I also don't get why it's a problem just now 😂
There was a problem hiding this comment.
I don't fully understand the fx magic either... I'll make a separate PR in go-libs to add annotations to the profiling module that was interferring.
There was a problem hiding this comment.
There was a problem hiding this comment.
I approved it; can we see if it works as we expect in payment (i.e. without the annotation)
We can keep the annotation regardless, I guess it wouldn't hurt, but I'm curious if that annotation is "use only if matching" or more of a "best attempt"
![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
🔭 Outside diff range comments (4)
internal/connectors/plugins/public/stripe/create_transfers.go (1)
40-42: Potential loss of precision when down-casting big.Int to int64
pi.Amount.Int64()truncates if the value exceedsmath.MaxInt64, silently corrupting large-value transfers.-Amount: pi.Amount.Int64(), +// Reject amounts that cannot fit in int64 to avoid silent truncation +if !pi.Amount.IsInt64() { + return models.PSPPayment{}, errorsutils.NewWrappedError( + fmt.Errorf("amount %s exceeds int64", pi.Amount.String()), + models.ErrInvalidRequest, + ) +} +Amount: pi.Amount.Int64(),internal/connectors/plugins/public/currencycloud/transfers.go (1)
72-80: Silent “unsupported currency” case returns success – this is a bug
translateTransferToPaymentreturns(models.PSPPayment{}, nil)when the currency isn’t insupportedCurrenciesWithDecimal.
Callers will treat this as a successful conversion yet get an empty payment object.Apply something like:
- precision, ok := supportedCurrenciesWithDecimal[from.Currency] - if !ok { - return models.PSPPayment{}, nil - } + precision, ok := supportedCurrenciesWithDecimal[from.Currency] + if !ok { + return models.PSPPayment{}, fmt.Errorf("unsupported currency: %s", from.Currency) + }Remember to add
fmtto the import list. This pattern appears inpayouts.gotoo—fix both spots.internal/connectors/plugins/public/currencycloud/payouts.go (1)
74-80: Same silent failure case as in transfers – needs fixingReturning
(models.PSPPayment{}, nil)on an unknown currency hides an error.Replicate the guard suggested for
transfers.go, e.g.:- precision, ok := supportedCurrenciesWithDecimal[from.Currency] - if !ok { - return models.PSPPayment{}, nil - } + precision, ok := supportedCurrenciesWithDecimal[from.Currency] + if !ok { + return models.PSPPayment{}, fmt.Errorf("unsupported currency: %s", from.Currency) + }internal/connectors/plugins/public/wise/payments.go (1)
66-70: Potential slice panic when fewer results thanPageSize
payments = payments[:req.PageSize](and the same forpaymentIDs) will panic iflen(payments) < req.PageSize.
Guard the length before slicing.-if !needMore { - payments = payments[:req.PageSize] - paymentIDs = paymentIDs[:req.PageSize] +if !needMore && len(payments) >= req.PageSize { + payments = payments[:req.PageSize] + paymentIDs = paymentIDs[:req.PageSize] }
♻️ Duplicate comments (10)
internal/connectors/plugins/public/stripe/payments.go (1)
11-11: SameFormatAssetconcern applies hereThis file also relies heavily on
currency.FormatAsset. Please ensure the verification suggested in the Stripe balances review passes for the whole codebase.internal/connectors/plugins/public/increase/payouts.go (1)
11-11: SameFormatAssetconcern applies hereBuild will fail if the function is missing or its signature changed in the external library. Covered by the earlier verification step.
internal/connectors/plugins/public/stripe/create_payouts.go (2)
40-42: Repeated int64 cast – apply same guardThe payout path repeats the
big.Int→int64cast. Guard against overflow exactly as suggested for transfers to keep behaviour consistent.
23-29: Confirm helper availabilityAgain, validate that
GetCurrencyAndPrecisionFromAssetexists in go-libs/v3.internal/connectors/plugins/public/bankingcircle/balances.go (1)
9-9: Same API-parity concern as noted for Moneycorpinternal/connectors/plugins/public/stripe/accounts.go (1)
8-8: Same API-parity concern as noted for Moneycorpinternal/connectors/plugins/public/bankingcircle/transfers.go (1)
8-8: Same API-parity concern as noted for Moneycorpinternal/connectors/plugins/public/generic/balances.go (1)
9-9: Same API-parity concern as noted for Moneycorpinternal/connectors/plugins/public/bankingcircle/currencies.go (1)
3-31: Import migration is fine – decimals still sourced from ISO 4217No further action. Same comment as in
generic/currencies.goregarding pinning the dependency applies here.internal/connectors/plugins/public/increase/currencies.go (1)
3-3: LGTM – import path updatedSame note as earlier about ensuring
go-libs/v3version is controlled.
🧹 Nitpick comments (8)
internal/connectors/plugins/public/atlar/balances.go (1)
8-8: Import switch OK, but map duplication worth reconsideringGood to see the move to the shared library.
Now thatgo-libs/v3/currencyowns currency metadata, the localsupportedCurrenciesWithDecimalmap is redundant and a future divergence hazard. Prefer using the library’s source of truth (e.g.,currency.PrecisionByCode) throughout.internal/connectors/plugins/public/wise/balances.go (1)
47-50: Error message could be more informativeWrapping the error from
GetAmountWithPrecisionFromStringwith context (currency code & value) would ease debugging.-amount, err := currency.GetAmountWithPrecisionFromString(balance.Amount.Value.String(), precision) +amountStr := balance.Amount.Value.String() +amount, err := currency.GetAmountWithPrecisionFromString(amountStr, precision) +if err != nil { + return models.FetchNextBalancesResponse{}, fmt.Errorf( + "parsing Wise amount %s (currency %s, precision %d): %w", + amountStr, balance.Amount.Currency, precision, err) +}internal/connectors/plugins/public/modulr/payouts.go (1)
9-9: Possible dead code: duplicated precision mapEven after switching to go-libs, every conversion still passes the local
supportedCurrenciesWithDecimalmap.
go-libs ships its own precision registry; keeping both tables increases the
risk of divergence.Consider deleting the local map and calling the simple helpers:
- curr, precision, err := currency.GetCurrencyAndPrecisionFromAsset(supportedCurrenciesWithDecimal, pi.Asset) + curr, precision, err := currency.GetCurrencyAndPrecision(pi.Asset)(and similar for
FormatAsset,GetAmountWithPrecisionFromString, …).Cleaner code and one single source of truth for currency metadata.
Also applies to: 20-34, 77-88
internal/connectors/plugins/public/modulr/transfers.go (1)
9-9: Same duplication comment as in payouts.goAll precision/format helpers still delegate to the hand-rolled
supportedCurrenciesWithDecimalinstead of leveraging go-libs’ internal
registry. Re-using the built-in helpers would shrink the codebase and remove
a maintenance hotspot.Also applies to: 20-34, 80-83, 90-90
internal/connectors/plugins/public/column/payments.go (1)
12-12: Nil-safe asset pointer
currency.FormatAssetreturns an empty string when the code is unknown; this
would still be wrapped inpointer.For(""), giving a non-nil pointer to an
empty asset and polluting downstream analytics.
You might want to guard the call or returnnilwhen the asset is not
recognised.Also applies to: 88-90
internal/connectors/plugins/public/generic/payments.go (1)
58-60: Minor readability nit – index withupdatedAtsdirectly
updatedAts[len(payments)-1]works because both slices are kept in sync, but using the slice’s own length is clearer and future-proof if their sizes ever diverge.- newState.LastUpdatedAtFrom = updatedAts[len(payments)-1] + newState.LastUpdatedAtFrom = updatedAts[len(updatedAts)-1]internal/connectors/plugins/public/currencycloud/currencies.go (1)
7-36: Consider deferring to the library’s precision map instead of duplicating values
supportedCurrenciesWithDecimalcopies entries straight fromcurrency.ISO4217Currencies. If you only want a subset, keeping a slice of allowed codes and looking them up in the library map on-demand removes duplication and maintenance effort.Optional refactor:
var supportedCurrencies = []string{ "AUD", "CAD", "CZK", /* … */ } func precision(code string) (int, bool) { for _, c := range supportedCurrencies { if c == code { p, ok := currency.ISO4217Currencies[code] return p, ok } } return 0, false }internal/connectors/plugins/public/column/accounts.go (1)
65-68: Off-by-one guard may over-fetch
if len(accounts) > pageSize { break }stops only when the slice length is greater thanpageSize; it allows one extra element before breaking.
If the intention is to cap at exactlypageSize, change the condition to>=.- if len(accounts) > pageSize { + if len(accounts) >= pageSize {
Fixes: PMNT-135