fix(billing): refactor billing signing client#1771
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BatchSigningClientService
participant BlockchainClient
Client->>BatchSigningClientService: executeTx(messages, options)
BatchSigningClientService->>BatchSigningClientService: getCachedFirstAddress()
alt Cache miss
BatchSigningClientService->>BlockchainClient: fetchFirstAddress()
BlockchainClient-->>BatchSigningClientService: address
BatchSigningClientService->>BatchSigningClientService: cache address
end
BatchSigningClientService->>BatchSigningClientService: getCachedAccountInfo()
alt Cache miss
BatchSigningClientService->>BlockchainClient: fetchAccountInfo(address)
BlockchainClient-->>BatchSigningClientService: accountInfo
BatchSigningClientService->>BatchSigningClientService: cache accountInfo
end
BatchSigningClientService->>BlockchainClient: signAndBroadcast(messages, accountInfo)
BlockchainClient-->>BatchSigningClientService: txResult
BatchSigningClientService->>BatchSigningClientService: incrementSequence()
BatchSigningClientService-->>Client: txResult
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 2
🧹 Nitpick comments (1)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (1)
129-131: Consider also clearing the cached address for consistency.While the current implementation is correct, consider clearing both caches together to ensure complete cache invalidation:
private clearCachedAccountInfo(): void { this.cachedAccountInfo = undefined; + this.cachedFirstAddress = undefined; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts(7 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
⏰ 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 (8)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (8)
18-22: LGTM! Good addition to the interface.Adding the
addressfield toShortAccountInfoimproves data cohesion by keeping all account-related information together.
44-46: LGTM! Well-structured cache properties.The optional private properties for caching address and account info are appropriately typed and encapsulated.
66-68: LGTM! Clean implementation of pending transaction check.The getter provides a clear interface to check for pending transactions using the existing semaphore.
70-76: LGTM! Clean transaction execution with proper error handling.The method correctly uses the DataLoader for batching and includes appropriate error handling with a clear error message.
123-127: LGTM! Safe sequence increment with proper null check.The method correctly checks for cached account info before incrementing and is protected by the semaphore during batch execution.
146-148: LGTM! Proper cache invalidation on sequence mismatch.The implementation correctly clears the cached account info when a sequence mismatch occurs and uses the appropriate getter for logging the address.
167-180: LGTM! Efficient use of cached account info.The implementation properly uses cached account information for signing transactions and correctly increments the sequence after each sign operation. The safe navigation for the granter property prevents potential runtime errors.
218-220: LGTM! Consistent use of address caching.The simulate method now properly uses the cached address, maintaining consistency with the caching approach throughout the service.
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
stalniy
left a comment
stalniy
left a comment
There was a problem hiding this comment.
Nice! short an clean, I like it 👍
baktun14
force-pushed
the
fix/account-null
branch
from
b50de52 to
4a60d7a
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1771 +/- ##
==========================================
- Coverage 42.45% 42.23% -0.23%
==========================================
Files 934 929 -5
Lines 26273 26131 -142
Branches 6990 6981 -9
==========================================
- Hits 11154 11036 -118
+ Misses 13952 13928 -24
Partials 1167 1167
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
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 (1)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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
**/*.{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.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.spec.ts
🧠 Learnings (4)
📓 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.
📚 Learning: applies to **/*.spec.{ts,tsx} : don't use `jest.mock()` to mock dependencies in test files. instead,...
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.
Applied to files:
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts
📚 Learning: the getprovidershosturibyattributes method in apps/api/src/provider/repositories/provider/provider.r...
Learnt from: stalniy
PR: akash-network/console#1436
File: apps/api/src/provider/repositories/provider/provider.repository.ts:79-90
Timestamp: 2025-06-08T03:07:13.871Z
Learning: The getProvidersHostUriByAttributes method in apps/api/src/provider/repositories/provider/provider.repository.ts already has comprehensive test coverage in provider.repository.spec.ts, including tests for complex HAVING clause logic with COUNT(*) FILTER (WHERE ...) syntax, signature conditions (anyOf/allOf), and glob pattern matching.
Applied to files:
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts
📚 Learning: applies to **/*.spec.{ts,tsx} : don't use shared state in `setup` function...
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-07-21T08:25:07.474Z
Learning: Applies to **/*.spec.{ts,tsx} : Don't use shared state in `setup` function
Applied to files:
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts
⏰ 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 (9)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts (9)
31-48: LGTM! Well-structured concurrency test.This test effectively validates that concurrent calls to
getCachedFirstAddress()share the same underlying promise and prevent race conditions. The use ofsetTimeoutto control promise resolution timing is appropriate for testing concurrent behavior.
50-61: LGTM! Clean caching validation test.This test clearly demonstrates that the first address is cached after the initial call, preventing redundant wallet operations.
98-112: LGTM! Proper account info caching validation.This test effectively verifies that account information is cached and reused on subsequent calls.
114-124: LGTM! Cache clearing functionality well-tested.This test properly validates that the cache clearing mechanism works as expected, forcing fresh data retrieval.
126-139: LGTM! Sequence increment logic properly tested.This test thoroughly validates the sequence increment functionality, ensuring correct transaction ordering behavior.
141-151: LGTM! Proper error handling and retry logic tested.This test effectively validates that failed promise caches are cleared, allowing for proper retry behavior when address fetching fails.
153-163: LGTM! Account info error handling properly tested.This test validates that account info fetching errors are handled correctly with proper cache clearing and retry behavior.
165-173: LGTM! Account not found scenario properly handled.This test validates the specific case when an account doesn't exist, ensuring a descriptive error message is provided for better debugging.
175-229: LGTM! Setup function follows coding guidelines.The setup function is properly positioned at the bottom of the describe block, uses
jest-mock-extendedfor mocking as required, and maintains no shared state between tests.
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts
Outdated
Show resolved
Hide resolved
* fix(billing): refactor billing signing client * fix(billing): race condition fix * fix(billing): add tests * fix(billing): pr fix
2 participants
#1752
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Other Improvements