fix: handle "tx already in cache" gracefully#1728
fix: handle "tx already in cache" gracefully#1728baktun14 merged 2 commits intoakash-network:mainfrom
Conversation
|
""" WalkthroughThis update refactors the transaction broadcasting logic in the batch signing client service to handle "transaction already exists in cache" errors gracefully. It introduces a new test suite that mocks dependencies and verifies the service's behavior when duplicate transaction errors occur, ensuring correct transaction hash handling and robust error recovery. Additionally, the service now accepts a Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant BatchSigningClientService
participant MockedStargateClient
TestSuite->>BatchSigningClientService: executeTxBatch(transactions)
loop For each transaction except last
BatchSigningClientService->>MockedStargateClient: tmBroadcastTxSync(tx)
alt Error: tx already in cache
BatchSigningClientService->>BatchSigningClientService: Compute tx hash from bytes
else Success
BatchSigningClientService->>BatchSigningClientService: Collect tx hash from response
end
end
BatchSigningClientService->>MockedStargateClient: broadcastTx(lastTx)
alt Error: tx already in cache
BatchSigningClientService->>BatchSigningClientService: Compute tx hash from bytes
else Success
BatchSigningClientService->>BatchSigningClientService: Collect tx hash from response
end
loop For each tx hash
BatchSigningClientService->>MockedStargateClient: getTx(hash)
alt Not found
BatchSigningClientService->>BatchSigningClientService: Return placeholder IndexedTx
else Found
BatchSigningClientService->>BatchSigningClientService: Return IndexedTx from client
end
end
BatchSigningClientService-->>TestSuite: Array of IndexedTx (with hashes)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (1)
192-192: Unnecessary filtering of non-null entries.The filter condition
tx => tx !== nullis unnecessary since the map function always returns an IndexedTx object (either from getTx or the fallback placeholder). This filtering will never remove any entries.-return txs.filter(tx => tx !== null); +return txs;apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts (1)
23-86: Setup function follows guidelines well with minor improvements needed.The setup function is properly positioned and creates comprehensive mocks. However, consider these improvements:
- The setup function should accept a parameter even if not used, as per guidelines
- The static method override approach works but could be more explicit
- function setup() { + function setup(overrides: {} = {}) {The static method mocking is functional but consider documenting why this approach is necessary for the SyncSigningStargateClient.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
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(2 hunks)
📓 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/lib/batch-signing-client/batch-signing-client.service.spec.tsapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/general.mdc)
**/*.{js,jsx,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.spec.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)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts (2)
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/no-jest-mock.mdc:0-0
Timestamp: 2025-07-21T08:24:24.269Z
Learning: Applies to **/*.spec.{ts,tsx} : Don't use jest.mock() to mock dependencies in test files. Instead, use jest-mock-extended to create mocks and pass mocks as dependencies to the service under test.
Learnt from: jzsfkzm
PR: #1372
File: apps/api/src/dashboard/services/stats/stats.service.ts:0-0
Timestamp: 2025-05-28T20:42:58.200Z
Learning: The user successfully implemented CosmosHttpService with retry logic using axiosRetry, exponential backoff, and proper error handling for Cosmos API calls, replacing direct axios usage in StatsService.
🧰 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/lib/batch-signing-client/batch-signing-client.service.spec.tsapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/general.mdc)
**/*.{js,jsx,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.spec.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)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts (2)
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/no-jest-mock.mdc:0-0
Timestamp: 2025-07-21T08:24:24.269Z
Learning: Applies to **/*.spec.{ts,tsx} : Don't use jest.mock() to mock dependencies in test files. Instead, use jest-mock-extended to create mocks and pass mocks as dependencies to the service under test.
Learnt from: jzsfkzm
PR: #1372
File: apps/api/src/dashboard/services/stats/stats.service.ts:0-0
Timestamp: 2025-05-28T20:42:58.200Z
Learning: The user successfully implemented CosmosHttpService with retry logic using axiosRetry, exponential backoff, and proper error handling for Cosmos API calls, replacing direct axios usage in StatsService.
⏰ 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 (7)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (3)
2-2: Good addition for hash computation.The sha256 import is correctly added to support computing transaction hashes when handling cache errors.
147-168: Excellent error handling for duplicate transactions.The refactored broadcasting logic properly handles "tx already exists in cache" errors by computing the transaction hash from the encoded bytes. The individual transaction processing with try-catch blocks ensures robust error recovery.
177-189: Well-structured fallback IndexedTx object.The placeholder IndexedTx object includes all required fields with appropriate default values. The use of BigInt for gas fields and proper typing for events and msgResponses arrays is correct.
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts (4)
1-11: Proper use of jest-mock-extended for mocking.Good adherence to coding guidelines by using
jest-mock-extendedinstead ofjest.mock()for creating mocks.
13-21: Well-focused test case for error handling.The test effectively verifies that the service handles duplicate transaction errors gracefully and returns the correct transaction hash. Direct access to the private method is appropriate for unit testing.
70-77: Excellent error simulation for cache scenarios.The mock implementations properly simulate the "tx already exists in cache" error for both
tmBroadcastTxSyncandbroadcastTxmethods, ensuring comprehensive testing of the error handling logic.
32-68: Comprehensive dependency mocking.The setup includes thorough mocking of all service dependencies with realistic return values. The configuration mock covers all required keys, and the client mock includes all necessary methods for the test scenario.
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts
Outdated
Show resolved
Hide resolved
jzsfkzm
force-pushed
the
bugfixes/1692-handle-tx-in-cache-gracefully
branch
4 times, most recently
from
10d40e2 to
dc7d219
Compare
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.spec.ts
Outdated
Show resolved
Hide resolved
| Object.defineProperty(SyncSigningStargateClient, "connectWithSigner", { | ||
| value: jest.fn().mockResolvedValue(mockClient) | ||
| }); |
There was a problem hiding this comment.
suggestion(non-blocking): would be awesome to extract SyncSigningStargateClient.connectWithSigner into dependency and pass it into BatchSigningClientService
There was a problem hiding this comment.
@stalniy Do you mean we should register SyncSigningStargateClient.connectWithSigner in the container? But BatchSigningClientService is not @Injectable at the moment, all the places where we instantiate it, we do it manually at the moment. Should we change that too?
|
@ygrishajev need your review for this PR |
jzsfkzm
force-pushed
the
bugfixes/1692-handle-tx-in-cache-gracefully
branch
from
dc7d219 to
9088f43
Compare
jzsfkzm
force-pushed
the
bugfixes/1692-handle-tx-in-cache-gracefully
branch
from
9088f43 to
035a75e
Compare
| return tx; | ||
| } | ||
|
|
||
| return { |
There was a problem hiding this comment.
I think these changes makes sense. Just one question, why are we returning default values instead of filtering them out? I don't remember in details why we did in the first place.
There was a problem hiding this comment.
I wasn't sure about that part either, maybe @ygrishajev you can have a second opinion?
My logic was before this we were just filtering out every nullish tx-es, but now we'll have a hash for a tx that is not just in the mempool yet. Questions here are (if not more):
- should we return mempool tx-es at all?
- if so, should we fill with defaults?
- mark such a tx in a way?
* fix: handle "tx already in cache" gracefully closes #1692 * fix: make changes as per PR suggestions
3 participants
closes #1692
Summary by CodeRabbit
Bug Fixes
Tests