-
Notifications
You must be signed in to change notification settings - Fork 77
refactor(billing): extract balance validation and improve accuracy #2430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughExposes Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ManagedSigner as ManagedSigner Service
participant Balances as Balances Service
participant RPC as Blockchain RPC
Client->>ManagedSigner: executeDecodedTxByUserWallet(userWallet, messages)
rect rgba(200,230,255,0.6)
Note over ManagedSigner: Consolidated balance check
ManagedSigner->>ManagedSigner: `#validateBalances`(userWallet, messages)
end
alt cache miss or deployment present
ManagedSigner->>Balances: retrieveAndCalcFeeLimit(userWallet)
Balances->>RPC: fetch live balance & fee data
RPC-->>Balances: balance/fee info
Balances-->>ManagedSigner: fee/deploy allowances
else use cached allowances
ManagedSigner->>ManagedSigner: use cached allowances
end
alt insufficient funds
ManagedSigner-->>Client: 402 Payment Required
else sufficient funds
ManagedSigner->>ManagedSigner: continue validations & sign/execute tx
ManagedSigner-->>Client: Transaction result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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). (3)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/billing/services/balances/balances.service.tsapps/api/src/billing/services/managed-signer/managed-signer.service.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{ts,tsx,js}: Never use typeanyor cast to typeany. Always define the proper TypeScript types.
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.ts
🧬 Code graph analysis (2)
apps/api/src/billing/services/balances/balances.service.ts (2)
apps/api/src/billing/services/managed-signer/managed-signer.service.ts (1)
userWallet(169-180)apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
UserWalletOutput(18-22)
apps/api/src/billing/services/managed-signer/managed-signer.service.ts (1)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
UserWalletOutput(18-22)
⏰ 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). (3)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/api/src/billing/services/balances/balances.service.ts (1)
63-72: LGTM! Method visibility change enables the new validation pattern.Making
retrieveAndCalcFeeLimitpublic is necessary to support the refactored balance validation in the managed-signer service. The method implementation remains sound and the exposure is appropriate for inter-service communication within the billing module.apps/api/src/billing/services/managed-signer/managed-signer.service.ts (1)
112-112: LGTM! Clean refactoring that extracts balance validation.The single call to
#validateBalancesreplaces inline balance checks, improving code organization and maintainability while preserving the validation logic.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2430 +/- ##
==========================================
- Coverage 51.25% 50.94% -0.31%
==========================================
Files 1072 1061 -11
Lines 29393 28933 -460
Branches 6472 6393 -79
==========================================
- Hits 15065 14741 -324
+ Misses 13998 13784 -214
- Partials 330 408 +78
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
c76142b to
f9f573f
Compare
Extract balance validation logic from executeDecodedTxByUserWallet into a dedicated #validateBalances method. The new method fetches balances from the chain when cached values are 0 to ensure accuracy, rather than relying solely on cached values. Use Promise.all for parallel fetching of fee and deployment allowances. Also update insufficient funds error status codes from 403 to 402 (Payment Required) for better semantic accuracy. closes #2338
9564572 to
868ae8e
Compare
Extract balance validation logic from executeDecodedTxByUserWallet into a dedicated #validateBalances method. The new method fetches balances from the chain when cached values are 0 to ensure accuracy, rather than relying solely on cached values. Use Promise.all for parallel fetching of fee and deployment allowances.
Also update insufficient funds error status codes from 403 to 402 (Payment Required) for better semantic accuracy.
closes #2338
Summary by CodeRabbit
Refactor
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.