feat(billing): uses ws transport for signer implementation#2124
feat(billing): uses ws transport for signer implementation#2124ygrishajev wants to merge 2 commits intomainfrom
Conversation
WalkthroughSwitches a functional test RPC endpoint; replaces SyncSigningStargateClient connect/create API with a new static init that uses WebsocketClient + Comet38Client; BatchSigningClientService now constructs a concrete SyncSigningStargateClient via create/init, adds disconnect()/dispose(), and DedupeSigningClientService calls client.disconnect() during cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant DedupeService as DedupeSigningClientService
participant BatchSvc as BatchSigningClientService
participant SyncClient as SyncSigningStargateClient
participant Transport as WebsocketClient/Comet38
Caller->>DedupeService: executeManagedTx(tx...)
DedupeService->>BatchSvc: request executeTxBatch / simulate
BatchSvc->>BatchSvc: constructor called with createWithSigner
BatchSvc->>SyncClient: createWithSigner/init(...)
SyncClient->>Transport: new WebsocketClient -> Comet38Client.create()
Transport-->>SyncClient: cometClient
SyncClient-->>BatchSvc: SyncSigningStargateClient instance
BatchSvc->>SyncClient: simulate/submit tx(s)
SyncClient-->>BatchSvc: result / error
BatchSvc-->>DedupeService: result
Note over DedupeService,BatchSvc: cleanup path
DedupeService->>BatchSvc: disconnect() / dispose()
BatchSvc->>SyncClient: disconnect()
SyncClient->>Transport: close websocket
Transport-->>SyncClient: closed
SyncClient-->>BatchSvc: disconnected
DedupeService->>DedupeService: remove client entry
DedupeService-->>Caller: final result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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: 0
🧹 Nitpick comments (2)
apps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts (1)
36-41: Consider error handling for disconnect() in the finally block.The
disconnect()call in the finally block is correct for resource cleanup. However, ifdisconnect()throws an exception, it could mask the original error from the try block or cause unexpected behavior.Apply this diff to add defensive error handling:
} finally { if (!client.hasPendingTransactions && this.clientsByAddress.has(key)) { this.logger.debug({ event: "DEDUPE_SIGNING_CLIENT_CLEAN_UP", key }); this.clientsByAddress.delete(key); - await client.disconnect(); + try { + await client.disconnect(); + } catch (error) { + this.logger.error({ event: "DEDUPE_SIGNING_CLIENT_DISCONNECT_ERROR", key, error }); + } } }apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (1)
85-92: Consider adding error handling to the disconnect methods.The
disconnect()method is confirmed to exist onSigningStargateClient(inherited fromStargateClientin @cosmjs/stargate), so the implementation is correct. However, since these methods are called during cleanup, wrapping the disconnect call in a try-catch block would prevent errors from propagating and allow graceful degradation:async disconnect() { try { const client = await this.clientAsPromised; client.disconnect(); } catch (error) { // Log error } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
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(1 hunks)apps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts(1 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/services/dedupe-signing-client/dedupe-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/services/dedupe-signing-client/dedupe-signing-client.service.tsapps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts
🪛 dotenv-linter (4.0.0)
apps/api/env/.env.functional.test
[warning] 6-6: [UnorderedKey] The RPC_NODE_ENDPOINT key should go before the UAKT_TOP_UP_MASTER_WALLET_MNEMONIC 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 (3)
apps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts (2)
11-19: Verified: The endpoint parameter type change does not break any existing code.All call sites already pass string values (RPC endpoint URLs), and no code uses the
HttpEndpointtype. The narrowing fromstring | HttpEndpointtostringis safe.
16-18: Consider potential connection initialization timing issues.The transport implementation changed from
connectComet()(which likely established connection) to constructingWebsocketClientand wrapping it withComet38Client.create().The
WebsocketClientconstructor is synchronous, and the connection might not be established until the first RPC call. This could lead to:
- Delayed error detection if the endpoint is unreachable
- First RPC call latency including connection establishment
apps/api/env/.env.functional.test (1)
5-6: RPC endpoint configuration verified successfully.The endpoint
https://rpc.sandbox.akt.dev/rpcis accessible and responds correctly to RPC health checks. The CosmJSWebsocketClientlibrary automatically convertshttps://URLs towss://(WebSocket Secure) protocol internally, so the configuration is correct as-is.
❌ 13 Tests Failed:
View the top 3 failed test(s) by shortest run time
View the full list of 4 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
| return this.clientAsPromised; | ||
| } | ||
|
|
||
| const client = await backOff( |
There was a problem hiding this comment.
issue(blocking): this is race condition vulnerable
There was a problem hiding this comment.
Actionable comments posted: 1
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/lib/batch-signing-client/batch-signing-client.service.ts (1)
92-117: Race condition: concurrent getClient() calls may create multiple clients.If
getClient()is invoked concurrently before the first call completes, both calls will pass theif (this.clientAsPromised)check and each will initialize a separate client. This can lead to multiple WebSocket connections and resource leaks.While
executeTxBatchBlocking()uses the semaphore for protection,getCachedAccountInfo()(line 159) andsimulate()(line 318) callgetClient()without synchronization.Apply this diff to coordinate concurrent initialization:
+private clientInitPromise?: Promise<SyncSigningStargateClient>; + private async getClient(): Promise<SyncSigningStargateClient> { if (this.clientAsPromised) { return this.clientAsPromised; } + + if (this.clientInitPromise) { + return this.clientInitPromise; + } - const client = await backOff( + this.clientInitPromise = backOff( () => this.connectWithSigner(this.config.get("RPC_NODE_ENDPOINT"), this.wallet, { registry: this.registry }).then(async client => { this.chainId = await client.getChainId(); return client; }), { maxDelay: 10_000, startingDelay: 500, timeMultiple: 2, numOfAttempts: 7, jitter: "full" } - ); + ) + .then(client => { + this.clientAsPromised = client; + this.clientInitPromise = undefined; + return client; + }) + .catch(error => { + this.clientInitPromise = undefined; + throw error; + }); - this.clientAsPromised = client; - - return client; + return this.clientInitPromise; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/net/src/generated/netConfigData.tsis excluded by!**/generated/**
📒 Files selected for processing (1)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts(8 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.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.ts
🧬 Code graph analysis (1)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (1)
apps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts (1)
SyncSigningStargateClient(10-36)
⏰ 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). (9)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: Validate local packages
🔇 Additional comments (6)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (6)
41-41: LGTM: Type change supports lazy initialization.The type change from
Promise<SyncSigningStargateClient>toSyncSigningStargateClient | undefinedcorrectly supports the new lazy initialization pattern.
119-124: LGTM: Correct cleanup pattern.The
removeClient()method properly checks for client existence before disconnecting and correctly clears the reference.
159-159: LGTM: Correctly uses lazy initialization.The update to use
this.getClient()is consistent with the lazy initialization pattern.
210-210: LGTM: Correct sequence mismatch handling.Using
removeClient()to reset the client on sequence mismatch is correct. The next operation will lazily re-initialize viagetClient().
227-227: LGTM: Correctly uses lazy initialization.The update to use
await this.getClient()is consistent with the lazy initialization pattern.
318-318: LGTM: Correctly uses lazy initialization.The update to use
await this.getClient()is consistent with the lazy initialization pattern.
… call stack Also uses sync transport client instantiation
ygrishajev
force-pushed
the
feature/billing
branch
from
c4cd5ec to
578de9c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (1)
84-86:initmethod contributes to race condition.This private async method is called from the constructor without await, causing the race condition flagged in lines 58-61. See the earlier comment for fix options.
🧹 Nitpick comments (2)
apps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts (1)
39-39: Add error handling for disconnect.If
disconnect()throws during cleanup (e.g., WebSocket already closed), the error would propagate from thefinallyblock and potentially mask the original transaction result or error. Consider wrapping the disconnect call in a try-catch.Apply this diff:
- await client.disconnect(); + try { + client.disconnect(); + } catch (error) { + this.logger.warn({ event: "DEDUPE_SIGNING_CLIENT_DISCONNECT_ERROR", key, error }); + }apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (1)
116-121: Verify account existence error handling is sufficient.The code throws a generic
Errorif account info is not found. Consider whether this error message provides enough context for debugging and whether it should be a more specific error type.Consider enriching the error message:
if (!accountInfo) { - throw new Error("Failed to get account info"); + throw new Error(`Failed to get account info for address: ${address}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/net/src/generated/netConfigData.tsis excluded by!**/generated/**
📒 Files selected for processing (4)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts(9 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(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/sync-signing-stargate-client/sync-signing-stargate-client.tsapps/api/src/billing/providers/signing-client.provider.tsapps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.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/lib/sync-signing-stargate-client/sync-signing-stargate-client.tsapps/api/src/billing/providers/signing-client.provider.tsapps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.tsapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts
🧬 Code graph analysis (3)
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(10-28)
apps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts (1)
apps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts (1)
SyncSigningStargateClient(10-28)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (2)
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(10-28)
⏰ 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). (11)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Validate local packages
🔇 Additional comments (8)
apps/api/src/billing/providers/signing-client.provider.ts (3)
31-31: Binding looks correct.The API change from
connectWithSignertoinitis properly applied with correct binding.
44-44: Binding looks correct.The API change is consistently applied across all three client registrations.
18-18: Incorrect timing characterization in review comment.The concern conflates container factory execution timing. Errors from
SyncSigningStargateClient.init()will not surface at "container resolution time" during app initialization. Instead, they surface during the first HTTP request that instantiates a service requiring the signing client (e.g., when TopUpToolsService is injected and used). The factory function inuseFactoryexecutes lazily on firstcontainer.resolve()call within the service instantiation chain, not at module load.The underlying point about error handling for synchronous initialization is worth considering, but the error surface timing as stated is incorrect.
Likely an incorrect or invalid review comment.
apps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts (1)
55-55: Binding correctly updated to use new API.The change from
connectWithSignertoinitis correct and consistent with the broader refactor.apps/api/src/billing/lib/sync-signing-stargate-client/sync-signing-stargate-client.ts (2)
2-5: Import changes are correct.The imports properly reflect the transport switch from HTTP to WebSocket: replaced
HttpEndpointandconnectCometwithWebsocketClientandComet38Client.
11-14: The review comment's core assumptions are incorrect—WebsocketClient uses lazy connection.WebsocketClient construction does not complete connection immediately; instead, it creates a client handle and connection is established asynchronously. The
connected()method returns a Promise that resolves when the WebSocket is ready.This means:
- The
init()method won't throw synchronously on invalid endpoints—errors occur on first use- The
disconnect()method properly closes the connection when called- No try-catch is needed in
init()for connection failuresThe review comment incorrectly assumes synchronous connection behavior. The code is sound as written.
Likely an incorrect or invalid review comment.
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (2)
30-30: Type definition correctly updated.The rename from
ConnectWithSignerFntoCreateWithSignerFnand the return type change fromPromise<SyncSigningStargateClient>toSyncSigningStargateClientcorrectly reflects the synchronous initialization pattern.
101-101: Direct client access is correct.Replacing
await this.getClient()calls with directthis.clientaccess is correct and eliminates unnecessary complexity. The removed caching layer was not needed with the new synchronous initialization.Also applies to: 131-133, 152-152, 155-155, 170-170, 210-210
| this.client = this.createWithSigner(this.config.get("RPC_NODE_ENDPOINT"), this.wallet, { | ||
| registry: this.registry | ||
| }); | ||
| this.init(); |
There was a problem hiding this comment.
Fix race condition: await the initialization.
The constructor calls this.init() without awaiting it, creating a race condition where this.chainId might be undefined when executeTx is first called. Line 134 uses this.chainId! (non-null assertion), which could fail if the transaction executes before initialization completes.
The constructor cannot be async, so you need an alternative initialization strategy. Consider one of these approaches:
Option 1: Make init synchronous by moving chainId fetch to first use (lazy initialization)
constructor(
private readonly config: BillingConfigService,
private readonly wallet: Wallet,
private readonly registry: Registry,
private readonly createWithSigner: CreateWithSignerFn,
private readonly chainErrorService: ChainErrorService,
private readonly loggerContext = BatchSigningClientService.name
) {
this.client = this.createWithSigner(this.config.get("RPC_NODE_ENDPOINT"), this.wallet, {
registry: this.registry
});
- this.init();
}
- private async init() {
- this.chainId = await this.client.getChainId();
- }
+ private async getChainId(): Promise<string> {
+ if (!this.chainId) {
+ this.chainId = await this.client.getChainId();
+ }
+ return this.chainId;
+ }Then update line 134:
await this.client.sign(accountInfo.address, messages, fee, "", {
accountNumber: accountInfo.accountNumber,
sequence: accountInfo.sequence + txIndex,
- chainId: this.chainId!
+ chainId: await this.getChainId()
})Option 2: Use a static factory method instead of constructor
+ public static async create(
+ config: BillingConfigService,
+ wallet: Wallet,
+ registry: Registry,
+ createWithSigner: CreateWithSignerFn,
+ chainErrorService: ChainErrorService,
+ loggerContext = BatchSigningClientService.name
+ ): Promise<BatchSigningClientService> {
+ const service = new BatchSigningClientService(config, wallet, registry, createWithSigner, chainErrorService, loggerContext);
+ await service.init();
+ return service;
+ }
- constructor(
+ private constructor(
...
) {
this.client = this.createWithSigner(this.config.get("RPC_NODE_ENDPOINT"), this.wallet, {
registry: this.registry
});
- this.init();
}Then update all instantiation sites to use await BatchSigningClientService.create(...).
Committable suggestion skipped: line range outside the PR's diff.
| async disconnect() { | ||
| this.client.disconnect(); | ||
| } | ||
|
|
||
| private incrementSequence(): void { | ||
| if (this.cachedAccountInfo) { | ||
| this.cachedAccountInfo.sequence++; | ||
| } | ||
| async dispose() { | ||
| await this.disconnect(); | ||
| } |
There was a problem hiding this comment.
Remove incorrect await keywords and add null-safety check.
Based on past review comments and CosmJS documentation:
disconnect()returnsvoid, not a Promise - theawaitkeywords are incorrect- The method should check if a client exists before disconnecting
Apply this diff:
async disconnect() {
- this.client.disconnect();
+ if (this.client) {
+ this.client.disconnect();
+ }
}
async dispose() {
- await this.disconnect();
+ this.disconnect();
}Note: Both methods can be made synchronous since disconnect() is synchronous:
- async disconnect() {
+ disconnect() {
if (this.client) {
this.client.disconnect();
}
}
- async dispose() {
+ dispose() {
this.disconnect();
}📝 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.
| async disconnect() { | |
| this.client.disconnect(); | |
| } | |
| private incrementSequence(): void { | |
| if (this.cachedAccountInfo) { | |
| this.cachedAccountInfo.sequence++; | |
| } | |
| async dispose() { | |
| await this.disconnect(); | |
| } | |
| async disconnect() { | |
| if (this.client) { | |
| this.client.disconnect(); | |
| } | |
| } | |
| async dispose() { | |
| this.disconnect(); | |
| } |
🤖 Prompt for AI Agents
In apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts
around lines 76 to 82, the disconnect()/dispose() implementations incorrectly
await a synchronous disconnect() and don't check for a null/undefined client;
change both methods to synchronous (remove await/async), add a null-safety check
like if (this.client) this.client.disconnect(), and optionally clear this.client
after disconnect to avoid repeated calls.
| if (!client.hasPendingTransactions && this.clientsByAddress.has(key)) { | ||
| this.logger.debug({ event: "DEDUPE_SIGNING_CLIENT_CLEAN_UP", key }); | ||
| this.clientsByAddress.delete(key); | ||
| await client.disconnect(); |
There was a problem hiding this comment.
Remove await from disconnect() call.
According to the past review comments and CosmJS documentation, SigningStargateClient.disconnect() returns void (not a Promise). The await keyword here has no effect but suggests async behavior that doesn't exist.
Apply this diff:
- await client.disconnect();
+ client.disconnect();📝 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.
| await client.disconnect(); | |
| client.disconnect(); |
🤖 Prompt for AI Agents
In
apps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts
around line 39, the call `await client.disconnect();` uses await but
SigningStargateClient.disconnect() is synchronous (returns void); remove the
`await` so the call becomes a plain `client.disconnect();`, ensuring the method
signature stays correct and no unnecessary async behavior is implied.
| this.cachedAccountInfo = undefined; | ||
| this.accountInfoPromise = undefined; | ||
| private async init() { | ||
| this.chainId = await this.client.getChainId(); |
There was a problem hiding this comment.
question(blocking): can this be done lazily? I assume it sends a remote request somewhere and if we do this without .catch it will crash the app when the remote service is down
2 participants
Summary by CodeRabbit
Bug Fixes
Chores
Refactor