fix(alert): properly calculates deployment escrow balance#1545
fix(alert): properly calculates deployment escrow balance#1545ygrishajev merged 1 commit intomainfrom
Conversation
WalkthroughLease pricing is now integrated into deployment balance calculations. The Changes
Sequence Diagram(s)sequenceDiagram
participant AlertService
participant DeploymentService
participant DeploymentHttpService
participant LeaseHttpService
AlertService->>DeploymentService: processSingleAlert(owner, dseq, block.height)
DeploymentService->>DeploymentHttpService: getDeploymentInfo(owner, dseq)
DeploymentService->>LeaseHttpService: list(owner, dseq)
DeploymentService->>DeploymentService: getPricePerBlock(leases)
DeploymentService->>DeploymentService: calculateBalance(escrow, pricePerBlock, settledAt, block.height)
DeploymentService-->>AlertService: return balance
Suggested reviewers
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! ✨ Finishing Touches
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1545 +/- ##
==========================================
+ Coverage 40.57% 40.59% +0.02%
==========================================
Files 872 872
Lines 21217 21228 +11
Branches 3864 3870 +6
==========================================
+ Hits 8608 8618 +10
- Misses 11881 12289 +408
+ Partials 728 321 -407
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (7)
apps/notifications/src/modules/alert/providers/http-sdk.provider.ts(2 hunks)apps/notifications/src/modules/alert/services/deployment-balance-alerts/deployment-balance-alerts.service.ts(1 hunks)apps/notifications/src/modules/alert/services/deployment/deployment.service.spec.ts(5 hunks)apps/notifications/src/modules/alert/services/deployment/deployment.service.ts(3 hunks)apps/notifications/test/functional/balance-alert.spec.ts(3 hunks)apps/notifications/test/seeders/deployment-balance-response.seeder.ts(2 hunks)packages/http-sdk/src/lease/lease-http.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.spec.{ts,tsx}`: Use a `setup` function instead of `beforeEach` for test setup. The `setup` function must be at the bottom of the root `describe` block, create and return the ...
**/*.spec.{ts,tsx}: Use asetupfunction instead ofbeforeEachfor test setup. Thesetupfunction must be at the bottom of the rootdescribeblock, create and return the object under test, accept a single parameter with inline type definition, avoid shared state, and should not specify a return type.
apps/notifications/test/functional/balance-alert.spec.tsapps/notifications/src/modules/alert/services/deployment/deployment.service.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-api-build
- GitHub Check: validate-api
- GitHub Check: validate-notifications
- GitHub Check: test-deploy-web-build
- GitHub Check: validate-deploy-web
- GitHub Check: Validate local packages
🔇 Additional comments (16)
packages/http-sdk/src/lease/lease-http.service.ts (1)
5-5: LGTM! Proper type export for cross-module usage.Exporting the type enables type-safe usage of lease response data in other modules, which aligns with the integration of lease pricing into deployment balance calculations.
apps/notifications/test/seeders/deployment-balance-response.seeder.ts (1)
8-8: LGTM! Proper seeder extension for escrow settlement timing.The addition of the
settledAtparameter with appropriate default value generation and optional typing correctly supports the enhanced deployment balance calculation logic.Also applies to: 14-14, 28-29
apps/notifications/src/modules/alert/services/deployment-balance-alerts/deployment-balance-alerts.service.ts (1)
54-54: LGTM! Correctly passes block height for time-based balance calculation.The addition of
block.heightas a parameter enables the deployment service to calculate balance based on elapsed time since escrow settlement, which is essential for the lease pricing integration.apps/notifications/src/modules/alert/providers/http-sdk.provider.ts (1)
1-1: LGTM! Proper provider setup for LeaseHttpService.The addition follows the established pattern for HTTP service providers, correctly injecting ConfigService and using the same API endpoint configuration.
Also applies to: 16-23
apps/notifications/test/functional/balance-alert.spec.ts (1)
78-80: LGTM! Comprehensive test coverage for lease-integrated balance calculation.The test properly mocks lease data and validates the enhanced balance calculation. The expected value of 700000 correctly reflects the calculation: initial balance (800000) minus cost for elapsed blocks (100 blocks × 1000 per block = 100000).
Also applies to: 82-92, 107-113, 127-127
apps/notifications/src/modules/alert/services/deployment/deployment.service.spec.ts (6)
1-2: LGTM: Proper imports for lease functionalityThe imports correctly include the new
LeaseHttpServiceandRestAkashLeaseListResponsetype needed for the updated functionality.
17-18: LGTM: Test setup updated for new functionalityThe test description and setup correctly reflect the addition of lease pricing calculations and the new block height parameter.
27-34: LGTM: Mock data properly updatedThe escrow balance and funds amounts have been increased to accommodate the new calculation logic, and the
settled_atfield has been correctly added.
38-48: LGTM: Lease service mock properly configuredThe mock correctly returns lease data with price information needed for the balance calculation.
49-51: LGTM: Method call and assertion updated correctlyThe method call now includes the required block height parameter, and the expected balance (700000) matches the calculation logic:
(400000 + 400000) / 1000 - (1000 - 900) = 700 blocks * 1000 = 700000.
91-104: LGTM: Setup function follows coding guidelinesThe setup function correctly follows the coding guidelines by:
- Being placed at the bottom of the root describe block
- Creating and returning the object under test
- Avoiding shared state
- Not specifying a return type
apps/notifications/src/modules/alert/services/deployment/deployment.service.ts (5)
1-1: LGTM: Import statement updated correctlyThe import statement properly includes the new
LeaseHttpServicedependency.
22-23: LGTM: Type definition updatedThe
DeploymentInfotype correctly includes the newsettled_atfield needed for escrow settlement calculations.
30-30: LGTM: Service dependency injectionThe
LeaseHttpServiceis properly injected into the constructor.
36-36: LGTM: Method signature updatedThe method signature correctly adds the
blockparameter needed for balance calculations.
38-38: LGTM: Concurrent data fetchingUsing
Promise.allfor concurrent fetching of deployment info and price per block is an efficient approach.
apps/notifications/src/modules/alert/services/deployment/deployment.service.ts
Show resolved
Hide resolved
apps/notifications/src/modules/alert/services/deployment/deployment.service.ts
Show resolved
Hide resolved
apps/notifications/src/modules/alert/services/deployment/deployment.service.ts
Show resolved
Hide resolved
ygrishajev
force-pushed
the
feature/alert
branch
from
1c861f2 to
b1ec54b
Compare
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/notifications/src/modules/alert/providers/http-sdk.provider.ts(2 hunks)apps/notifications/src/modules/alert/services/deployment-balance-alerts/deployment-balance-alerts.service.ts(1 hunks)apps/notifications/src/modules/alert/services/deployment/deployment.service.spec.ts(5 hunks)apps/notifications/src/modules/alert/services/deployment/deployment.service.ts(3 hunks)apps/notifications/test/functional/balance-alert.spec.ts(3 hunks)apps/notifications/test/seeders/deployment-balance-response.seeder.ts(2 hunks)packages/http-sdk/src/lease/lease-http.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/http-sdk/src/lease/lease-http.service.ts
- apps/notifications/test/seeders/deployment-balance-response.seeder.ts
- apps/notifications/test/functional/balance-alert.spec.ts
- apps/notifications/src/modules/alert/providers/http-sdk.provider.ts
- apps/notifications/src/modules/alert/services/deployment-balance-alerts/deployment-balance-alerts.service.ts
- apps/notifications/src/modules/alert/services/deployment/deployment.service.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.spec.{ts,tsx}`: Use a `setup` function instead of `beforeEach` for test setup. The `setup` function must be at the bottom of the root `describe` block, create and return the ...
**/*.spec.{ts,tsx}: Use asetupfunction instead ofbeforeEachfor test setup. Thesetupfunction must be at the bottom of the rootdescribeblock, create and return the object under test, accept a single parameter with inline type definition, avoid shared state, and should not specify a return type.
apps/notifications/src/modules/alert/services/deployment/deployment.service.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: validate-notifications
- GitHub Check: validate-api
- GitHub Check: test-api-build
- GitHub Check: validate-deploy-web
- GitHub Check: test-deploy-web-build
- GitHub Check: Validate local packages
🔇 Additional comments (6)
apps/notifications/src/modules/alert/services/deployment/deployment.service.spec.ts (6)
1-2: LGTM - Import statements properly updated for lease integration.The new imports for
DeploymentInfo,RestAkashLeaseListResponse, andLeaseHttpServiceare correctly added to support the lease pricing integration functionality.
17-18: Test description and setup appropriately updated.The test description now accurately reflects the "calculated escrow balance" functionality, and the setup destructuring includes the new dependencies (
leaseHttpServiceandCURRENT_HEIGHT) needed for lease pricing calculations.
38-48: Lease mock data structure looks correct.The mock
leaseHttpService.listresponse properly follows theRestAkashLeaseListResponsestructure with lease price data needed for balance calculations.
49-51: Method signature and assertion updated correctly.The
getDeploymentBalancecall now includes theCURRENT_HEIGHTparameter as expected, and the assertion reflects the new calculated balance including lease pricing.
76-78: Closed deployment test case properly maintained.The test for closed deployments correctly includes the new lease service mock and height parameter while maintaining the existing error handling logic.
27-34: To locate and inspect the exact balance‐calculation logic, let’s pull in the service file and search around the key terms:#!/bin/bash # 1. Find the deployment.service.ts implementation service_file=$(fd -g "deployment.service.ts" apps/notifications/src/modules/alert/services/deployment) echo "Found service file at: $service_file" # 2. Show the first 200 lines to locate relevant methods echo "----- Top of $service_file -----" sed -n '1,200p' "$service_file" # 3. Search for balance/settle/current‐height references to spot the calculation block echo "----- Scanning for balance calculation references -----" rg -n "balance" -n "settle" -n "CURRENT_HEIGHT" -n "block" -n "price" "$service_file"
apps/notifications/src/modules/alert/services/deployment/deployment.service.spec.ts
Show resolved
Hide resolved
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores