refactor(billing): improves batch signing client#2138
Conversation
WalkthroughReplaces single-tx execution with a DataLoader-driven batched signAndBroadcast pipeline (semaphore-serialized batch processing, retry executors, memoized address/chainId, lifecycle dispose), swaps connectWithSigner for createWithEndpoint, updates providers/consumers/tests, and adds async memoization utility and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant BatchSvc as BatchSigningClientService
participant Loader as DataLoader
participant Signer as signBatch
participant Broadcaster as broadcastBatch
participant ChainErr as ChainErrorService
Note over Caller,BatchSvc: Batched signAndBroadcast flow
Caller->>BatchSvc: signAndBroadcast(messages, options)
activate BatchSvc
BatchSvc->>Loader: load({ messages, options })
deactivate BatchSvc
par Accumulate requests
Loader->>Loader: collect and batch requests until flush
end
Loader->>BatchSvc: flush batch
activate BatchSvc
BatchSvc->>Signer: signBatch(inputs) %% sequential per-input signing
activate Signer
Signer-->>BatchSvc: signedTxs
deactivate Signer
BatchSvc->>Broadcaster: broadcastBatch(signedTxs) %% sequential broadcast, tx-cache checks
activate Broadcaster
Broadcaster-->>BatchSvc: tx results / errors
deactivate Broadcaster
alt All success
BatchSvc-->>Caller: resolve IndexedTx[] (all settled)
else Partial/Failure
BatchSvc->>ChainErr: handle/retry/log
BatchSvc-->>Caller: partial results (Promise.allSettled)
end
deactivate BatchSvc
sequenceDiagram
participant App
participant OldFactory as connectWithSigner (old)
participant NewFactory as createWithEndpoint (new)
Note over App,NewFactory: Factory method change for creating signing clients
rect rgb(240,240,240)
App->>OldFactory: connectWithSigner(...) -- removed
end
rect rgb(225,245,225)
App->>NewFactory: createWithEndpoint(endpoint, signer, options)
NewFactory->>NewFactory: build HttpClient + Comet38Client
NewFactory-->>App: SyncSigningStargateClient
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to focus review on:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/api/env/.env.functional.test(1 hunks)apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts(1 hunks)apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts(1 hunks)apps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts(1 hunks)apps/api/src/billing/providers/signing-client.provider.ts(3 hunks)apps/api/src/billing/services/balances/balances.service.ts(1 hunks)apps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts(3 hunks)apps/api/src/billing/services/managed-signer/managed-signer.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/src/billing/services/balances/balances.service.tsapps/api/src/billing/services/managed-signer/managed-signer.service.tsapps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.tsapps/api/src/billing/providers/signing-client.provider.tsapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.tsapps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.tsapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/src/billing/services/balances/balances.service.tsapps/api/src/billing/services/managed-signer/managed-signer.service.tsapps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.tsapps/api/src/billing/providers/signing-client.provider.tsapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.tsapps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.tsapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()to mock dependencies in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test.
**/*.spec.{ts,tsx}: Usesetupfunction instead ofbeforeEachin test files
setupfunction must be at the bottom of the rootdescribeblock in test files
setupfunction creates an object under test and returns it
setupfunction should accept a single parameter with inline type definition
Don't use shared state insetupfunction
Don't specify return type ofsetupfunction
Files:
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: baktun14
PR: akash-network/console#1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
🧬 Code graph analysis (5)
apps/api/src/billing/services/balances/balances.service.ts (1)
packages/http-sdk/src/deployment/deployment-http.service.ts (1)
DeploymentInfo(103-103)
apps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts (2)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (1)
SignAndBroadcastOptions(23-33)apps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts (1)
SyncSigningStargateClient(11-25)
apps/api/src/billing/providers/signing-client.provider.ts (1)
apps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts (1)
SyncSigningStargateClient(11-25)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts (5)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (1)
BatchSigningClientService(61-384)apps/api/test/seeders/akash-address.seeder.ts (1)
createAkashAddress(4-12)apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(7-65)apps/api/test/mocks/config-service.mock.ts (1)
mockConfigService(16-25)apps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts (1)
SyncSigningStargateClient(11-25)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (3)
apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(7-65)apps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts (1)
SyncSigningStargateClient(11-25)apps/api/src/core/services/tracing/tracing.service.ts (1)
withSpan(165-182)
⏰ 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)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (1)
apps/api/src/billing/services/balances/balances.service.ts (1)
87-92: LGTM! Clean refactor that improves readability.Directly returning the reduce result eliminates an unnecessary intermediate variable, making the code more concise without changing behavior.
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts
Show resolved
Hide resolved
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts
Show resolved
Hide resolved
apps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts
Outdated
Show resolved
Hide resolved
apps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2138 +/- ##
==========================================
- Coverage 46.83% 46.44% -0.40%
==========================================
Files 1014 1004 -10
Lines 28748 28372 -376
Branches 7460 7396 -64
==========================================
- Hits 13465 13176 -289
+ Misses 14887 14801 -86
+ Partials 396 395 -1
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts
Outdated
Show resolved
Hide resolved
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts
Outdated
Show resolved
Hide resolved
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts
Outdated
Show resolved
Hide resolved
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts
Outdated
Show resolved
Hide resolved
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts
Outdated
Show resolved
Hide resolved
apps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts
Outdated
Show resolved
Hide resolved
apps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts(1 hunks)apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts(1 hunks)apps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts(1 hunks)apps/api/src/billing/providers/signing-client.provider.ts(3 hunks)apps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts(3 hunks)apps/api/src/caching/helpers.spec.ts(2 hunks)apps/api/src/caching/helpers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/src/billing/providers/signing-client.provider.ts
- apps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/src/caching/helpers.tsapps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.tsapps/api/src/caching/helpers.spec.tsapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.tsapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/src/caching/helpers.tsapps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.tsapps/api/src/caching/helpers.spec.tsapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.tsapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()to mock dependencies in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test.
**/*.spec.{ts,tsx}: Usesetupfunction instead ofbeforeEachin test files
setupfunction must be at the bottom of the rootdescribeblock in test files
setupfunction creates an object under test and returns it
setupfunction should accept a single parameter with inline type definition
Don't use shared state insetupfunction
Don't specify return type ofsetupfunction
Files:
apps/api/src/caching/helpers.spec.tsapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
📚 Learning: 2025-10-31T11:26:42.123Z
Learnt from: stalniy
Repo: akash-network/console PR: 2138
File: apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts:379-382
Timestamp: 2025-10-31T11:26:42.123Z
Learning: In TypeScript/JavaScript, the pattern of checking a cached value and then performing an async operation to fetch it without proper synchronization is race condition unsafe:
```typescript
private async getAddress() {
if (!this.address) {
this.address = await this.wallet.getFirstAddress();
}
return this.address;
}
```
Multiple concurrent calls can all pass the `if (!this.address)` check before any of them sets the value, leading to duplicate async operations. This should be flagged as a race condition. Proper synchronization (mutex, atomic promise caching, or guaranteed single-threaded execution) is required.
Applied to files:
apps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.tsapps/api/src/caching/helpers.spec.tsapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts
📚 Learning: 2025-07-24T17:00:52.361Z
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
Applied to files:
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts
🧬 Code graph analysis (4)
apps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts (2)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (1)
SignAndBroadcastOptions(24-34)apps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts (1)
SyncSigningStargateClient(11-25)
apps/api/src/caching/helpers.spec.ts (1)
apps/api/src/caching/helpers.ts (1)
memoizeAsync(113-133)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (5)
apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(7-65)apps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts (1)
SyncSigningStargateClient(11-25)apps/api/src/caching/helpers.ts (1)
memoizeAsync(113-133)packages/logging/src/services/logger/logger.service.ts (1)
error(107-109)apps/api/src/core/services/tracing/tracing.service.ts (1)
withSpan(165-182)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts (5)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (1)
BatchSigningClientService(62-376)apps/api/test/seeders/akash-address.seeder.ts (1)
createAkashAddress(4-12)apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(7-65)apps/api/test/mocks/config-service.mock.ts (1)
mockConfigService(16-25)apps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts (1)
SyncSigningStargateClient(11-25)
⏰ 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)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (5)
apps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts (1)
30-41: LGTM! The await fix resolves the critical disposal issue.The addition of
awaiton line 34 ensures thatclient.signAndBroadcastcompletes before thefinallyblock executes, preventing premature disposal of the client. This addresses the race condition that was flagged in the previous review.apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts (1)
29-192: LGTM! Test suite follows coding guidelines and covers key scenarios.The test suite properly:
- Uses the
setupfunction at the bottom of the describe block as per guidelines- Covers successful batching, error handling, and retry scenarios
- Avoids shared state and follows the inline type definition pattern
- Uses
jest-mock-extendedfor mocking as requiredapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (3)
119-129: LGTM!memoizeAsyncresolves the race condition.Using
memoizeAsyncfor bothgetChainIdandgetAddressproperly handles concurrent calls by caching the promise atomically. Multiple concurrent invocations will all receive the same promise, preventing duplicate async operations. This addresses the race condition concern from the previous review.Based on learnings.
180-212: Well-structured batch signing and broadcasting flow.The
signAndBroadcastmethod properly:
- Enqueues requests through the DataLoader for batching
- Uses retry logic with exponential backoff for sequence mismatch errors
- Converts chain errors to application errors via the error service
- Retrieves and validates the final transaction result
The overall refactor introduces a robust batching mechanism with proper retry semantics.
322-328: Code is correct; review comment is factually incorrect.CosmJS's
broadcastTxSyncreturns a string containing the transaction hash, not aBroadcastTxSyncResponseobject. The code correctly pushes this string directly to theresultsarray on line 324, matching the behavior ofbroadcastTxwhich returnsDeliverTxResponse.transactionHash(also a string) on line 327. No type inconsistency exists, and no fix is needed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (1)
323-329: Return hash string frombroadcastTxSync, not the full response object.Line 325 pushes the entire
BroadcastTxSyncResponseobject, but the return type isResult<string, unknown>[]expecting a transaction hash string. Downstream code (line 200) passes this togetTx(), which will fail when trying to query for a stringified object.Extract and convert the hash to hex before returning:
try { if (index < signResults.length - 1) { const response = await this.client.broadcastTxSync(txBytes); - results.push(Ok(response)); + const txHash = toHex(response.hash); + results.push(Ok(txHash)); } else { const lastDelivery = await this.client.broadcastTx(txBytes); results.push(Ok(lastDelivery.transactionHash)); }
🧹 Nitpick comments (1)
apps/api/env/.env.functional.test (1)
40-42: Minor: Consider reordering for alphabetical consistency.Static analysis flags that
CONSOLE_WEB_PAYMENT_LINK(line 41) should precedeFEATURE_FLAGS_ENABLE_ALL(line 40) for alphabetical ordering. While environment files don't enforce strict ordering semantically, aligning with this convention improves maintainability.-FEATURE_FLAGS_ENABLE_ALL=true CONSOLE_WEB_PAYMENT_LINK=http://localhost:3000/payment +FEATURE_FLAGS_ENABLE_ALL=true STD_OUT_LOG_FORMAT=pretty
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/env/.env.functional.test(1 hunks)apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts(1 hunks)apps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.tsapps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.tsapps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
📚 Learning: 2025-10-31T11:26:42.123Z
Learnt from: stalniy
Repo: akash-network/console PR: 2138
File: apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts:379-382
Timestamp: 2025-10-31T11:26:42.123Z
Learning: In TypeScript/JavaScript, the pattern of checking a cached value and then performing an async operation to fetch it without proper synchronization is race condition unsafe:
```typescript
private async getAddress() {
if (!this.address) {
this.address = await this.wallet.getFirstAddress();
}
return this.address;
}
```
Multiple concurrent calls can all pass the `if (!this.address)` check before any of them sets the value, leading to duplicate async operations. This should be flagged as a race condition. Proper synchronization (mutex, atomic promise caching, or guaranteed single-threaded execution) is required.
Applied to files:
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.tsapps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts
📚 Learning: 2025-07-24T17:00:52.361Z
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
Applied to files:
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.tsapps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts
🧬 Code graph analysis (1)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (5)
apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(7-65)apps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts (1)
SyncSigningStargateClient(20-55)apps/api/src/caching/helpers.ts (1)
memoizeAsync(113-133)packages/logging/src/services/logger/logger.service.ts (1)
error(107-109)apps/api/src/core/services/tracing/tracing.service.ts (1)
withSpan(165-182)
🪛 dotenv-linter (4.0.0)
apps/api/env/.env.functional.test
[warning] 41-41: [UnorderedKey] The CONSOLE_WEB_PAYMENT_LINK key should go before the FEATURE_FLAGS_ENABLE_ALL key
(UnorderedKey)
⏰ 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)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (15)
apps/api/env/.env.functional.test (2)
37-37: Disabling telemetry sampling for functional tests is correct.Setting
AMPLITUDE_SAMPLING=0prevents test execution telemetry from polluting analytics data, which is a best practice for test environments.
42-42: Pretty-printed logging enhances test readability.The addition of
STD_OUT_LOG_FORMAT=prettyimproves log output readability during functional test execution and aligns with the PR's enhanced logging configuration.apps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts (2)
29-33: LGTM!The
createWithEndpointfactory method correctly implements the new HTTP-based client creation flow. The synchronous instantiation pattern aligns with the confirmed behavior ofComet38Client.createand provides a clean, simplified interface for creating signing clients.
52-54: LGTM!The
connected()method provides a useful health check mechanism by invoking thestatus()RPC call. This allows callers to verify connectivity before performing operations, which is valuable for the batching workflow.apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (11)
21-53: LGTM!The new interfaces and type definitions are well-structured and properly documented. The
CreateWithSignerFntype correctly expects a synchronous return, aligning with thecreateWithEndpointimplementation.
82-87: LGTM!The DataLoader configuration is correct. Disabling cache is appropriate for transaction operations, and the batching interval provides a configurable window for collecting multiple requests.
95-98: LGTM!The retry strategy for sequence mismatches is well-designed with appropriate backoff parameters. The string matching on "account sequence mismatch" is a practical approach given that chain errors are typically string-based.
106-109: LGTM!The transaction retrieval retry logic appropriately accounts for blockchain confirmation latency with a longer max delay (7 seconds) compared to signing operations.
119-129: LGTM!The memoized helpers correctly use
memoizeAsyncto cache both the chain ID and wallet address. This implementation properly addresses the race condition identified in previous reviews by caching the promise itself rather than checking and setting values separately. Based on learnings.
155-166: LGTM!The constructor correctly uses the synchronous factory function to initialize the client, aligning with the
createWithEndpointimplementation pattern.
180-212: LGTM!The
signAndBroadcastmethod correctly orchestrates the batching workflow with retry logic, proper error handling viachainErrorService, and transaction retrieval. The logging provides good observability into the signing lifecycle.
229-236: LGTM!The semaphore-based serialization ensures that batches are processed sequentially, preventing sequence number conflicts. The try-finally block guarantees proper cleanup.
263-294: LGTM!The batch signing implementation correctly manages sequence numbers by incrementing only after successful signing, preventing sequence gaps. The per-input error handling provides good isolation, allowing the batch to continue even if individual transactions fail.
353-355: LGTM!The
getTxmethod correctly uses the retry executor to handle cases where transactions are not immediately available after broadcasting.
369-376: LGTM!The fee estimation logic correctly simulates transactions and applies a safety multiplier to prevent out-of-gas errors. The optional granter support integrates cleanly into the fee structure.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/src/billing/providers/signing-client.provider.ts(3 hunks)apps/api/src/console.ts(1 hunks)apps/api/src/core/services/start-server/start-server.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/src/console.tsapps/api/src/billing/providers/signing-client.provider.tsapps/api/src/core/services/start-server/start-server.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/src/console.tsapps/api/src/billing/providers/signing-client.provider.tsapps/api/src/core/services/start-server/start-server.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
📚 Learning: 2025-08-21T04:03:23.490Z
Learnt from: stalniy
Repo: akash-network/console PR: 1827
File: apps/api/src/console.ts:146-154
Timestamp: 2025-08-21T04:03:23.490Z
Learning: The tsyringe container in the Akash Network console project has been extended with a custom dispose() method that iterates over all registered instances and calls dispose() on each one that implements it, enabling proper resource cleanup during shutdown.
Applied to files:
apps/api/src/console.tsapps/api/src/core/services/start-server/start-server.ts
📚 Learning: 2025-08-21T04:03:23.490Z
Learnt from: stalniy
Repo: akash-network/console PR: 1827
File: apps/api/src/console.ts:146-154
Timestamp: 2025-08-21T04:03:23.490Z
Learning: In the Akash Network console project, services like JobQueueService implement dispose() methods for resource cleanup, and the tsyringe container has been extended with a dispose() method that iterates over all registered instances and calls their dispose() methods, enabling proper shutdown orchestration.
Applied to files:
apps/api/src/billing/providers/signing-client.provider.tsapps/api/src/core/services/start-server/start-server.ts
| export const disposeSigningClients = async () => { | ||
| const clients = await Promise.all([ | ||
| container.resolve<BatchSigningClientService>(MANAGED_MASTER_SIGNING_CLIENT), | ||
| container.resolve<BatchSigningClientService>(UAKT_TOP_UP_MASTER_SIGNING_CLIENT), | ||
| container.resolve<BatchSigningClientService>(USDC_TOP_UP_MASTER_SIGNING_CLIENT) | ||
| ]); | ||
|
|
||
| await Promise.all(clients.map(client => client.dispose())); | ||
| }; |
There was a problem hiding this comment.
Dispose the real signing client instances rather than new ones
Each registration above still relies on container.register(..., { useFactory }) with the default Lifecycle.Transient. In tsyringe, that means every container.resolve(<token>) call produces a brand-new BatchSigningClientService. The disposeSigningClients() helper therefore constructs three fresh services, immediately disposes those, and never touches the live instances your consumers are holding—so the actual signing clients stay open.
Please either promote these registrations to Lifecycle.Singleton (so resolves share the same instance and container.dispose() works) or otherwise track the concrete instances when they are created, and dispose that set here.
A minimal fix:
-import { container, inject } from "tsyringe";
+import { container, inject, Lifecycle } from "tsyringe";
...
container.register(MANAGED_MASTER_SIGNING_CLIENT, {
useFactory: c =>
new BatchSigningClientService(
c.resolve(BillingConfigService),
c.resolve(MANAGED_MASTER_WALLET),
c.resolve(TYPE_REGISTRY),
SyncSigningStargateClient.createWithEndpoint.bind(SyncSigningStargateClient),
c.resolve(ChainErrorService),
MANAGED_MASTER_SIGNING_CLIENT
)
-});
+ ,
+ lifecycle: Lifecycle.Singleton
+});Repeat for the other two registrations so disposeSigningClients() hits the actual cached instances.
Committable suggestion skipped: line range outside the PR's diff.
| const { disposeSigningClients } = require("./billing/providers/signing-client.provider"); | ||
|
|
||
| await disposeSigningClients(); | ||
| await Promise.all([closeConnections(), chainDb.close(), container.dispose()]); | ||
| }); |
There was a problem hiding this comment.
Protect the rest of shutdown when signing client disposal fails
Here too, any rejection from disposeSigningClients() aborts the subsequent Promise.all(...), leaving database connections and the container alive. Please swallow/log those errors but still run the remaining cleanup.
Consider:
- await disposeSigningClients();
- await Promise.all([closeConnections(), chainDb.close(), container.dispose()]);
+ try {
+ await disposeSigningClients();
+ } catch (error) {
+ logger.error({ event: "SIGNING_CLIENT_DISPOSE_ERROR", error });
+ }
+
+ await Promise.all([closeConnections(), chainDb.close(), container.dispose()]);📝 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.
| const { disposeSigningClients } = require("./billing/providers/signing-client.provider"); | |
| await disposeSigningClients(); | |
| await Promise.all([closeConnections(), chainDb.close(), container.dispose()]); | |
| }); | |
| const { disposeSigningClients } = require("./billing/providers/signing-client.provider"); | |
| try { | |
| await disposeSigningClients(); | |
| } catch (error) { | |
| logger.error({ event: "SIGNING_CLIENT_DISPOSE_ERROR", error }); | |
| } | |
| await Promise.all([closeConnections(), chainDb.close(), container.dispose()]); | |
| }); |
🤖 Prompt for AI Agents
In apps/api/src/console.ts around lines 126 to 130, calling await
disposeSigningClients() can reject and abort the following Promise.all cleanup;
wrap the disposeSigningClients() call in a try/catch, log the caught error
(using the existing logger or console.error) and do not rethrow so the code
proceeds to await Promise.all([closeConnections(), chainDb.close(),
container.dispose()]) to ensure DBs and container are always cleaned up.
| return disposeSigningClients() | ||
| .then(() => container.dispose()) | ||
| .catch(error => { | ||
| logger.error({ event: "CONTAINER_DISPOSE_ERROR", error }); | ||
| }); |
There was a problem hiding this comment.
Ensure container disposal still runs when signing client cleanup fails
If disposeSigningClients() rejects, the current .then(() => container.dispose()) never executes, so the container (and everything registered on it) remains undisposed. That’s a major reliability regression: a transient issue in one signing client would leak DB pools, queue workers, etc., and block shutdown. Please run container disposal in a finally-style block so it always executes, and log signing-client failures separately.
Apply this diff:
- const disposeContainerOnce = once(() => {
- logger.info({ event: "DISPOSING_CONTAINER" });
- return disposeSigningClients()
- .then(() => container.dispose())
- .catch(error => {
- logger.error({ event: "CONTAINER_DISPOSE_ERROR", error });
- });
- });
+ const disposeContainerOnce = once(async () => {
+ logger.info({ event: "DISPOSING_CONTAINER" });
+
+ try {
+ await disposeSigningClients();
+ } catch (error) {
+ logger.error({ event: "SIGNING_CLIENT_DISPOSE_ERROR", error });
+ }
+
+ try {
+ await container.dispose();
+ } catch (error) {
+ logger.error({ event: "CONTAINER_DISPOSE_ERROR", error });
+ }
+ });🤖 Prompt for AI Agents
In apps/api/src/core/services/start-server/start-server.ts around lines 32 to
36, the current chain stops container.dispose() if disposeSigningClients()
rejects; change it to always run container.dispose() in a finally-style step:
call disposeSigningClients(), catch and log its error separately, then in a
finally handler run container.dispose() (and catch/log any error from
container.dispose() as well) so container disposal is guaranteed even when
signing-client cleanup fails.
| * @throws If the connection cannot be established. | ||
| */ | ||
| async connected() { | ||
| await this.forceGetCometClient().status(); |
There was a problem hiding this comment.
question(non-blocking): is this race condition safe?
There was a problem hiding this comment.
what race conditions do you mean?
There was a problem hiding this comment.
if this method is called by many callers, everybody will try to get status. So, instead of one there will be many calls to this.forceGetCometClient().status()
ygrishajev
force-pushed
the
feature/billing
branch
from
e1280e2 to
6ae3f12
Compare
ygrishajev
force-pushed
the
feature/billing
branch
from
6ae3f12 to
5935d9a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts (1)
44-62: Race condition: concurrent getClient calls can create duplicate clients.Multiple concurrent calls with the same key can all pass the
!this.clientsByAddress.has(key)check before any of them sets the value, resulting in multiple client instances being created. The latersetcalls overwrite earlier ones, potentially leaking resources and violating the deduplication contract.Based on learnings.
Apply proper synchronization, such as:
- A mutex/lock around the check-and-create block
- Atomic promise caching (store the creation promise, not just the result)
- A semaphore or similar construct to serialize getClient per key
Example with promise caching:
+private readonly clientCreationPromises: Map<string, Promise<CachedClient>> = new Map(); + private getClient(mnemonic: string, addressIndex?: number): CachedClient { const key = `${crypto.createHash("sha256").update(mnemonic).digest("hex")}/${addressIndex ?? 0}`; - if (!this.clientsByAddress.has(key)) { - this.logger.debug({ event: "DEDUPE_SIGNING_CLIENT_CREATE", key }); - this.clientsByAddress.set(key, { - key: key, - client: new BatchSigningClientService( - this.config, - new Wallet(mnemonic, addressIndex), - this.registry, - SyncSigningStargateClient.createWithEndpoint.bind(SyncSigningStargateClient), - this.chainErrorService - ) - }); - } + if (!this.clientsByAddress.has(key)) { + if (!this.clientCreationPromises.has(key)) { + const promise = this.createClient(key, mnemonic, addressIndex); + this.clientCreationPromises.set(key, promise); + promise.finally(() => this.clientCreationPromises.delete(key)); + } + await this.clientCreationPromises.get(key); + } return this.clientsByAddress.get(key)!; } + +private async createClient(key: string, mnemonic: string, addressIndex?: number): Promise<CachedClient> { + this.logger.debug({ event: "DEDUPE_SIGNING_CLIENT_CREATE", key }); + const cached = { + key: key, + client: new BatchSigningClientService( + this.config, + new Wallet(mnemonic, addressIndex), + this.registry, + SyncSigningStargateClient.createWithEndpoint.bind(SyncSigningStargateClient), + this.chainErrorService + ) + }; + this.clientsByAddress.set(key, cached); + return cached; +}Note: This requires making
getClientasync and updating its callers.
♻️ Duplicate comments (1)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (1)
320-329: Return a hash string forbroadcastTxSync, not the whole response.Right now we push
Ok(response)whereresponseis aBroadcastTxSyncResponse. WhensignAndBroadcastlater callsgetTx(result.val), it stringifies to"[object Object]", every lookup fails, and we throw “Failed to sign and broadcast transaction.” Convert the sync response into the actual hash (and treat non‑zero codes as errors) before wrapping it.- const response = await this.client.broadcastTxSync(txBytes); - results.push(Ok(response)); + const response = await this.client.broadcastTxSync(txBytes); + if (response.code && response.code !== 0) { + results.push(Err(new Error(response.log ?? "broadcastTxSync failed"))); + continue; + } + const txHash = toHex(response.hash); + results.push(Ok(txHash));
🧹 Nitpick comments (1)
apps/api/env/.env.functional.test (1)
40-41: Fix key ordering to comply with static analysis.The static analysis tool flagged that
CONSOLE_WEB_PAYMENT_LINK(line 41) should appear beforeFEATURE_FLAGS_ENABLE_ALL(line 40) in alphabetical order.Apply this diff to fix the ordering:
-FEATURE_FLAGS_ENABLE_ALL=true -CONSOLE_WEB_PAYMENT_LINK=http://localhost:3000/payment +CONSOLE_WEB_PAYMENT_LINK=http://localhost:3000/payment +FEATURE_FLAGS_ENABLE_ALL=true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/api/env/.env.functional.test(1 hunks)apps/api/src/background-jobs-app.ts(1 hunks)apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts(1 hunks)apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts(1 hunks)apps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts(2 hunks)apps/api/src/billing/providers/signing-client.provider.ts(3 hunks)apps/api/src/billing/services/balances/balances.service.ts(1 hunks)apps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts(3 hunks)apps/api/src/billing/services/managed-signer/managed-signer.service.ts(1 hunks)apps/api/src/caching/helpers.spec.ts(2 hunks)apps/api/src/caching/helpers.ts(1 hunks)apps/api/src/console.ts(1 hunks)apps/api/src/core/services/start-server/start-server.ts(2 hunks)apps/api/src/rest-app.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/api/src/core/services/start-server/start-server.ts
- apps/api/src/caching/helpers.ts
- apps/api/src/console.ts
- apps/api/src/billing/services/balances/balances.service.ts
- apps/api/src/rest-app.ts
- apps/api/src/billing/providers/signing-client.provider.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/src/background-jobs-app.tsapps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.tsapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.tsapps/api/src/billing/services/managed-signer/managed-signer.service.tsapps/api/src/caching/helpers.spec.tsapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.tsapps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/src/background-jobs-app.tsapps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.tsapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.tsapps/api/src/billing/services/managed-signer/managed-signer.service.tsapps/api/src/caching/helpers.spec.tsapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.tsapps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()to mock dependencies in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test.
**/*.spec.{ts,tsx}: Usesetupfunction instead ofbeforeEachin test files
setupfunction must be at the bottom of the rootdescribeblock in test files
setupfunction creates an object under test and returns it
setupfunction should accept a single parameter with inline type definition
Don't use shared state insetupfunction
Don't specify return type ofsetupfunction
Files:
apps/api/src/caching/helpers.spec.tsapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
📚 Learning: 2025-08-21T04:03:23.490Z
Learnt from: stalniy
Repo: akash-network/console PR: 1827
File: apps/api/src/console.ts:146-154
Timestamp: 2025-08-21T04:03:23.490Z
Learning: In the Akash Network console project, services like JobQueueService implement dispose() methods for resource cleanup, and the tsyringe container has been extended with a dispose() method that iterates over all registered instances and calls their dispose() methods, enabling proper shutdown orchestration.
Applied to files:
apps/api/src/background-jobs-app.ts
📚 Learning: 2025-10-31T11:26:42.123Z
Learnt from: stalniy
Repo: akash-network/console PR: 2138
File: apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts:379-382
Timestamp: 2025-10-31T11:26:42.123Z
Learning: In TypeScript/JavaScript, the pattern of checking a cached value and then performing an async operation to fetch it without proper synchronization is race condition unsafe:
```typescript
private async getAddress() {
if (!this.address) {
this.address = await this.wallet.getFirstAddress();
}
return this.address;
}
```
Multiple concurrent calls can all pass the `if (!this.address)` check before any of them sets the value, leading to duplicate async operations. This should be flagged as a race condition. Proper synchronization (mutex, atomic promise caching, or guaranteed single-threaded execution) is required.
Applied to files:
apps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.tsapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.tsapps/api/src/caching/helpers.spec.tsapps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts
📚 Learning: 2025-07-24T17:00:52.361Z
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
Applied to files:
apps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.tsapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts
🧬 Code graph analysis (5)
apps/api/src/background-jobs-app.ts (3)
apps/api/src/rest-app.ts (1)
bootstrap(201-207)apps/api/src/core/services/start-server/start-server.ts (1)
startServer(18-66)apps/api/src/billing/providers/signing-client.provider.ts (1)
disposeSigningClients(52-60)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (5)
apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(7-65)apps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts (1)
SyncSigningStargateClient(20-55)apps/api/src/caching/helpers.ts (1)
memoizeAsync(113-133)packages/logging/src/services/logger/logger.service.ts (2)
LoggerService(39-183)error(142-144)apps/api/src/core/services/tracing/tracing.service.ts (1)
withSpan(165-182)
apps/api/src/caching/helpers.spec.ts (1)
apps/api/src/caching/helpers.ts (1)
memoizeAsync(113-133)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts (4)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (1)
BatchSigningClientService(62-377)apps/api/test/seeders/akash-address.seeder.ts (1)
createAkashAddress(4-12)apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(7-65)apps/api/test/mocks/config-service.mock.ts (1)
mockConfigService(16-25)
apps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts (2)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (1)
SignAndBroadcastOptions(24-34)apps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts (1)
SyncSigningStargateClient(20-55)
🪛 dotenv-linter (4.0.0)
apps/api/env/.env.functional.test
[warning] 41-41: [UnorderedKey] The CONSOLE_WEB_PAYMENT_LINK key should go before the FEATURE_FLAGS_ENABLE_ALL key
(UnorderedKey)
⏰ 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)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (6)
apps/api/env/.env.functional.test (2)
37-37: Amplitude sampling disabled for functional tests.Disabling amplitude sampling (set to
0) is appropriate for a functional test environment, as it prevents telemetry events from being generated during test execution. This change looks good.
42-42: New log format setting improves test readability.Adding
STD_OUT_LOG_FORMAT=prettyenhances the readability of functional test logs, which supports better debugging and easier interpretation of test output.apps/api/src/background-jobs-app.ts (1)
6-6: LGTM! Import follows established patterns.The import is necessary for resource cleanup and matches the pattern used in rest-app.ts.
apps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts (3)
6-6: LGTM: Import updated to match new API.The import change from
ExecuteTxOptionstoSignAndBroadcastOptionscorrectly aligns with the refactored batch signing client interface.
34-34: Good fix:awaitprevents premature disposal.Adding
awaitensures the broadcast completes before the finally block runs, correctly addressing the previous review comment.
55-55: LGTM: Client factory binding updated to new API.The change from
connectWithSignertocreateWithEndpointcorrectly aligns with the refactoredSyncSigningStargateClientinterface, as shown in the relevant code snippets.
|
|
||
| app.route("/", healthzRouter); | ||
| const server = await startServer(app, LoggerService.forContext("BACKGROUND_JOBS"), process, { port }); | ||
| const server = await startServer(app, LoggerService.forContext("BACKGROUND_JOBS"), process, { port, beforeEnd: () => disposeSigningClients() }); |
There was a problem hiding this comment.
Critical: async disposal callback is not awaited, risking incomplete cleanup.
The disposeSigningClients function is async and performs resource cleanup, but the startServer implementation in apps/api/src/core/services/start-server/start-server.ts (line 33) calls options?.beforeEnd?.() without awaiting it. This means the server may shut down before the signing clients are fully disposed, potentially causing resource leaks or orphaned connections.
The startServer implementation should be updated to await the beforeEnd callback:
await options?.beforeEnd?.();This issue affects both this file and rest-app.ts which uses the same pattern. Based on learnings, proper disposal is critical for resource cleanup in this project.
🤖 Prompt for AI Agents
In apps/api/src/background-jobs-app.ts around line 14, the async disposal
callback disposeSigningClients is passed to startServer but startServer
currently invokes options?.beforeEnd?.() without awaiting it; update the
start-server implementation
(apps/api/src/core/services/start-server/start-server.ts, around line 33) to
await the callback (i.e., await options?.beforeEnd?.()) so disposal completes
before shutdown, and apply the same change for the identical pattern used in
rest-app.ts to ensure all async cleanup is awaited.
| if (!client.hasPendingTransactions && this.clientsByAddress.has(key)) { | ||
| this.logger.debug({ event: "DEDUPE_SIGNING_CLIENT_CLEAN_UP", key }); | ||
| this.clientsByAddress.delete(key); | ||
| client.dispose(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Potential race condition in concurrent cleanup.
If executeManagedTx is called concurrently with the same key, multiple executions share the same client instance. When one finishes and enters the finally block, it could delete from the map and dispose the client while another concurrent call is about to use the same client reference. The check !client.hasPendingTransactions may not provide sufficient synchronization against this race.
Consider:
- Call A finishes transaction, checks
hasPendingTransactions(false), proceeds to delete/dispose - Call B starts with same key, calls
getClient, receives the same client reference - Call A disposes the client
- Call B attempts
signAndBroadcaston the disposed client
Proper synchronization (mutex, promise coordination, or reference counting) would prevent disposing a client while it's being acquired or used by concurrent callers.
Run the following script to check if there are concurrent usage patterns that could trigger this race:
🏁 Script executed:
#!/bin/bash
# Search for concurrent or batched calls to executeManagedTx
rg -nP -C5 'executeManagedTx' --type=ts -g '!*.spec.ts' -g '!*.test.ts'Length of output: 11803
I'll examine the full implementation of the dedupe-signing-client service and search for concurrent usage patterns.
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the full dedupe-signing-client.service.ts file to see the complete finally block
cat apps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts
Length of output: 2483
🏁 Script executed:
#!/bin/bash
# Search for Promise.all or batch patterns with executeManagedTx
rg -nP 'Promise\.all.*executeManagedTx|map.*executeManagedTx' --type=ts -g '!*.spec.ts' -g '!*.test.ts' -C3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there are any concurrent/parallel patterns when calling executeManagedTx
rg -nP 'Promise\.(all|race|any)|forkJoin|combineLatest' --type=ts apps/api/src/deployment/services -C3 | head -50Length of output: 6303
Let me examine the BatchSigningClientService to understand the hasPendingTransactions and dispose behavior:
🏁 Script executed:
#!/bin/bash
# View the BatchSigningClientService implementation
cat apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.tsLength of output: 13991
🏁 Script executed:
#!/bin/bash
# Search for how executeManagedTx is actually called - check if concurrent calls with same key are possible
rg -nP 'executeManagedTx.*walletIndex|walletId' --type=ts apps/api/src/billing/services apps/api/src/deployment/services -C3 | head -80Length of output: 9820
Race condition confirmed: multiple concurrent executeManagedTx calls share the same client instance, and concurrent finally blocks race on disposal.
When top-up-managed-deployments.service.ts calls Promise.allSettled on multiple owners with the same walletId, concurrent executeManagedTx calls with identical mnemonic and walletIndex retrieve the same cached client from clientsByAddress. Both calls' signAndBroadcast are batched by the DataLoader, and when the batch completes, their finally blocks execute concurrently. Both check !client.hasPendingTransactions (true, batch done) and clientsByAddress.has(key) (true), causing a race where one deletes and disposes while the other still holds a reference or is about to use it.
Proper synchronization (mutex, reference counting, or preventing concurrent acquisition per key) is required before disposing.
| const fn = jest.fn().mockResolvedValue("cached"); | ||
| const memoized = memoizeAsync(fn); | ||
|
|
||
| await memoized(); | ||
| const result = memoized(); | ||
|
|
||
| expect(result).resolves.toBe("cached"); | ||
| expect(fn).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
Await the .resolves assertion so Jest actually checks it.
Without await, the promise returned by expect(result).resolves may resolve after the test finishes, giving false positives or unhandled rejections. Await the assertion (or return it) to keep the test reliable.
- expect(result).resolves.toBe("cached");
+ await expect(result).resolves.toBe("cached");📝 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.
| const fn = jest.fn().mockResolvedValue("cached"); | |
| const memoized = memoizeAsync(fn); | |
| await memoized(); | |
| const result = memoized(); | |
| expect(result).resolves.toBe("cached"); | |
| expect(fn).toHaveBeenCalledTimes(1); | |
| const fn = jest.fn().mockResolvedValue("cached"); | |
| const memoized = memoizeAsync(fn); | |
| await memoized(); | |
| const result = memoized(); | |
| await expect(result).resolves.toBe("cached"); | |
| expect(fn).toHaveBeenCalledTimes(1); |
🤖 Prompt for AI Agents
In apps/api/src/caching/helpers.spec.ts around lines 349 to 356, the test uses
expect(result).resolves.toBe("cached") but does not await or return that
assertion; update the test to await the resolves assertion (e.g., await
expect(result).resolves.toBe("cached")) so Jest waits for the promise resolution
and the assertion is actually executed, keeping the test reliable and preventing
false positives.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Improvements