chore: upgrade deps#102
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 12 minutes and 33 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (41)
WalkthroughThis pull request upgrades the project from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/api/handler_balances_create_test.go (1)
51-57:⚠️ Potential issue | 🟡 MinorIsolate the expiration validation case.
This case is named
with expiration, butwallet.MainBalanceis already invalid, so the test can pass without exercisingExpiresAtvalidation.🧪 Proposed test fix
{ name: "with expiration", request: wallet.CreateBalance{ - Name: wallet.MainBalance, + Name: uuid.NewString(), ExpiresAt: ptr(time.Now().Add(10 * time.Second)), }, expectedStatusCode: http.StatusBadRequest,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/handler_balances_create_test.go` around lines 51 - 57, The test case "with expiration" is not isolating expiration validation because it uses wallet.MainBalance (already invalid); update the test to use a valid balance name (e.g., a non-reserved name or the same valid value used in other passing cases) in the wallet.CreateBalance payload, keep ExpiresAt set to ptr(time.Now().Add(10 * time.Second)), and assert the expectedStatusCode and expectedErrorCode still reflect the expiration validation failure; adjust only the Name field in that case so the test exercises ExpiresAt validation rather than failing on wallet.MainBalance.cmd/serve.go (1)
45-68:⚠️ Potential issue | 🔴 CriticalChange
fx.Decoratetofx.Providefor*http.Client; no upstream provider exists.
fx.Decoraterequires an existing provider to wrap. Here, nothing in the options array provides*http.Client— neitherobservefx,authnfx, norservice.New(). The decorator will be silently ignored by Fx, and whenwallet.Module'sDefaultLedgertries to resolve its*http.Clientdependency at startup, Fx will fail with a "missing dependencies" error.Since
GetHTTPClient()takes no parameter and returns*http.Client, this should befx.Provide(...)instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/serve.go` around lines 45 - 68, The code uses fx.Decorate to register GetHTTPClient which provides *http.Client but fx.Decorate expects an existing provider to wrap, so replace the fx.Decorate(...) call that wraps GetHTTPClient with fx.Provide(...) so Fx actually registers the *http.Client provider; update the options array where GetHTTPClient is currently passed into fx.Decorate (referencing GetHTTPClient, fx.Decorate, fx.Provide, wallet.Module and DefaultLedger) to use fx.Provide with the same function arguments so wallet.Module's *http.Client dependency resolves at startup.
🧹 Nitpick comments (3)
pkg/api/handler_balances_list_test.go (1)
11-15: Misleadingsharedapialias — it now points to thepaginatepackage.In
pkg/api/utils.go,sharedapialiasesgithub.com/formancehq/go-libs/v5/pkg/transport/apiwhilepaginateis imported separately forpaginate.Cursor[T]. Heresharedapiis aliased to thepaginatepackage itself, so subsequentsharedapi.Cursor[...]usages actually referencepaginate.Cursor. Works, but the alias is confusing and inconsistent with the production file. Consider renaming the alias topaginate(or dropping the alias entirely) for consistency.♻️ Suggested alias rename
- sharedapi "github.com/formancehq/go-libs/v5/pkg/storage/bun/paginate" + "github.com/formancehq/go-libs/v5/pkg/storage/bun/paginate"Then replace
sharedapi.Cursor[...]withpaginate.Cursor[...]throughout the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/handler_balances_list_test.go` around lines 11 - 15, The test import alias `sharedapi` incorrectly points to the `paginate` package making `sharedapi.Cursor[...]` refer to `paginate.Cursor`; change the import alias to `paginate` (or remove the alias) in the import block and then replace all occurrences of `sharedapi.Cursor[...]` with `paginate.Cursor[...]` so the alias matches production usage; ensure any other references to `sharedapi` in this file are updated or left pointing to the transport/api package if needed.pkg/api/handler_transactions_list_test.go (1)
12-19: Consider extracting the repeated transaction mapper.The migration itself looks good. If this test gets touched again, a small helper would avoid keeping the two identical
shared.V2Transactionmapping blocks in sync.♻️ Optional refactor
+ mapTransaction := func(from shared.V2Transaction) shared.V2Transaction { + return shared.V2Transaction{ + ID: from.ID, + Metadata: from.Metadata, + PostCommitVolumes: nil, + Postings: from.Postings, + PreCommitVolumes: nil, + Reference: from.Reference, + Reverted: from.Reverted, + Timestamp: from.Timestamp, + } + } + var testEnv *testEnv testEnv = newTestEnv( WithListTransactions(func(ctx context.Context, ledger string, query wallet.ListTransactionsQuery) (*shared.V2TransactionsCursorResponseCursor, error) { @@ - Data: collections.Map(transactions[page*pageSize:(page+1)*pageSize], func(from shared.V2Transaction) shared.V2Transaction { - return shared.V2Transaction{ - ID: from.ID, - Metadata: from.Metadata, - PostCommitVolumes: nil, - Postings: from.Postings, - PreCommitVolumes: nil, - Reference: from.Reference, - Reverted: from.Reverted, - Timestamp: from.Timestamp, - } - }), + Data: collections.Map(transactions[page*pageSize:(page+1)*pageSize], mapTransaction), @@ - Data: collections.Map(transactions[:pageSize], func(from shared.V2Transaction) shared.V2Transaction { - return shared.V2Transaction{ - ID: from.ID, - Metadata: from.Metadata, - PostCommitVolumes: nil, - Postings: from.Postings, - PreCommitVolumes: nil, - Reference: from.Reference, - Reverted: from.Reverted, - Timestamp: from.Timestamp, - } - }), + Data: collections.Map(transactions[:pageSize], mapTransaction),Also applies to: 66-77, 89-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/handler_transactions_list_test.go` around lines 12 - 19, Two identical shared.V2Transaction mapping blocks are duplicated in the test; extract them into a single helper to avoid drift. Create a small helper function (e.g., makeV2Transaction or mapToV2Transaction) that returns a shared.V2Transaction built from the same inputs used in the two blocks, update both places (the two shared.V2Transaction construction sites currently duplicated) to call that helper, and keep any field initialization or pointer/metadata handling centralized in that helper so the test uses the single source of truth.cmd/serve.go (1)
88-104: Pre-existing: instrumented transport is bypassed for API calls.Not introduced by this PR, but worth noting while touching this code: when
clientID != "",clientCredentialsConfig.Client(...)returns a client whoseTransportisoauth2.Transport{Base: nil}, which defaults tohttp.DefaultTransport. Theoauth2.HTTPClientcontext value is used only for token fetching. As a result,observe.NewRoundTripperinstrumentation applies only to token fetches, not to ledger API calls made through this client.If full request-path tracing is desired, set
Baseon the returned transport (or wrap the returned client'sTransport) to the instrumented one.Proposed fix
- return clientCredentialsConfig.Client(context.WithValue(ctx, oauth2.HTTPClient, httpClient)), nil + oauthClient := clientCredentialsConfig.Client(context.WithValue(ctx, oauth2.HTTPClient, httpClient)) + if t, ok := oauthClient.Transport.(*oauth2.Transport); ok { + t.Base = httpClient.Transport + } + return oauthClient, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/serve.go` around lines 88 - 104, GetHTTPClient currently creates an instrumented transport via observe.NewRoundTripper but when clientID != "" it returns clientcredentials.Config.Client(...) whose oauth2.Transport has Base==nil so API calls bypass instrumentation; modify GetHTTPClient so after calling clientCredentialsConfig.Client(...) you set/ensure the returned client's Transport uses the instrumented transport (either by setting returnedClient.Transport.(*oauth2.Transport).Base = observe.NewRoundTripper(http.DefaultTransport, debug) or by wrapping/assigning returnedClient.Transport = observe.NewRoundTripper(returnedClient.Transport, debug)) so both token fetches and ledger API calls go through the instrumented roundtripper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/serve.go`:
- Around line 45-68: The code uses fx.Decorate to register GetHTTPClient which
provides *http.Client but fx.Decorate expects an existing provider to wrap, so
replace the fx.Decorate(...) call that wraps GetHTTPClient with fx.Provide(...)
so Fx actually registers the *http.Client provider; update the options array
where GetHTTPClient is currently passed into fx.Decorate (referencing
GetHTTPClient, fx.Decorate, fx.Provide, wallet.Module and DefaultLedger) to use
fx.Provide with the same function arguments so wallet.Module's *http.Client
dependency resolves at startup.
In `@pkg/api/handler_balances_create_test.go`:
- Around line 51-57: The test case "with expiration" is not isolating expiration
validation because it uses wallet.MainBalance (already invalid); update the test
to use a valid balance name (e.g., a non-reserved name or the same valid value
used in other passing cases) in the wallet.CreateBalance payload, keep ExpiresAt
set to ptr(time.Now().Add(10 * time.Second)), and assert the expectedStatusCode
and expectedErrorCode still reflect the expiration validation failure; adjust
only the Name field in that case so the test exercises ExpiresAt validation
rather than failing on wallet.MainBalance.
---
Nitpick comments:
In `@cmd/serve.go`:
- Around line 88-104: GetHTTPClient currently creates an instrumented transport
via observe.NewRoundTripper but when clientID != "" it returns
clientcredentials.Config.Client(...) whose oauth2.Transport has Base==nil so API
calls bypass instrumentation; modify GetHTTPClient so after calling
clientCredentialsConfig.Client(...) you set/ensure the returned client's
Transport uses the instrumented transport (either by setting
returnedClient.Transport.(*oauth2.Transport).Base =
observe.NewRoundTripper(http.DefaultTransport, debug) or by wrapping/assigning
returnedClient.Transport = observe.NewRoundTripper(returnedClient.Transport,
debug)) so both token fetches and ledger API calls go through the instrumented
roundtripper.
In `@pkg/api/handler_balances_list_test.go`:
- Around line 11-15: The test import alias `sharedapi` incorrectly points to the
`paginate` package making `sharedapi.Cursor[...]` refer to `paginate.Cursor`;
change the import alias to `paginate` (or remove the alias) in the import block
and then replace all occurrences of `sharedapi.Cursor[...]` with
`paginate.Cursor[...]` so the alias matches production usage; ensure any other
references to `sharedapi` in this file are updated or left pointing to the
transport/api package if needed.
In `@pkg/api/handler_transactions_list_test.go`:
- Around line 12-19: Two identical shared.V2Transaction mapping blocks are
duplicated in the test; extract them into a single helper to avoid drift. Create
a small helper function (e.g., makeV2Transaction or mapToV2Transaction) that
returns a shared.V2Transaction built from the same inputs used in the two
blocks, update both places (the two shared.V2Transaction construction sites
currently duplicated) to call that helper, and keep any field initialization or
pointer/metadata handling centralized in that helper so the test uses the single
source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85d0ee6a-5a38-4079-96ad-e0e4c61e6986
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (41)
cmd/root.gocmd/serve.gopkg/api/handler_balances_create_test.gopkg/api/handler_balances_list_test.gopkg/api/handler_holds_confirm.gopkg/api/handler_holds_confirm_test.gopkg/api/handler_holds_get_test.gopkg/api/handler_holds_list_test.gopkg/api/handler_holds_void.gopkg/api/handler_holds_void_test.gopkg/api/handler_transactions_list_test.gopkg/api/handler_wallets_create_test.gopkg/api/handler_wallets_credit.gopkg/api/handler_wallets_credit_test.gopkg/api/handler_wallets_debit.gopkg/api/handler_wallets_debit_test.gopkg/api/handler_wallets_get_test.gopkg/api/handler_wallets_list_test.gopkg/api/handler_wallets_patch.gopkg/api/handler_wallets_patch_test.gopkg/api/handler_wallets_summary_test.gopkg/api/module.gopkg/api/router.gopkg/api/utils.gopkg/api/utils_test.gopkg/balance.gopkg/credit.gopkg/debit.gopkg/hold.gopkg/ledger_interface.gopkg/manager.gopkg/metadata.gopkg/testserver/testserver.gopkg/wallet.gotest/e2e/balances_test.gotest/e2e/create_test.gotest/e2e/credit_test.gotest/e2e/debit_test.gotest/e2e/suite_test.gotest/e2e/summary_test.gotest/e2e/transactions_test.go
gfyrag
force-pushed
the
chore/upgrade-deps
branch
from
759e735 to
534417b
Compare
2 participants
No description provided.