feat: add ManagedDeploymentLeaseCreated event and handler#2728
Merged
feat: add ManagedDeploymentLeaseCreated event and handler#2728
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a ManagedDeploymentLeaseCreated domain event, publishes it from managed-signer, registers a new handler that calls NotificationService.autoEnableDeploymentAlert, and extends NotificationService with user, deployment, and logger dependencies plus the auto-enable alert flow. Changes
Sequence DiagramsequenceDiagram
participant ManagedSigner as ManagedSigner Service
participant EventBus as Event Bus
participant Handler as ManagedDeploymentLeaseCreated Handler
participant UserRepo as User Repository
participant DeploymentHTTP as Deployment HTTP Service
participant NotificationsAPI as Notifications API Client
ManagedSigner->>EventBus: publish(ManagedDeploymentLeaseCreated)
EventBus->>Handler: handle(event)
Handler->>UserRepo: findById(userId)
UserRepo-->>Handler: user (email?)
Handler->>NotificationsAPI: ensureDefaultChannel(userId)
NotificationsAPI-->>Handler: channelId
Handler->>DeploymentHTTP: fetchDeployment(walletAddress, dseq)
DeploymentHTTP-->>Handler: deployment (escrow balance)
Handler->>NotificationsAPI: upsertDeploymentAlert(channelId, threshold)
NotificationsAPI-->>Handler: upsert result
Handler-->>EventBus: complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2728 +/- ##
==========================================
+ Coverage 51.96% 52.08% +0.11%
==========================================
Files 1041 1043 +2
Lines 27397 27454 +57
Branches 6332 6341 +9
==========================================
+ Hits 14238 14300 +62
+ Misses 12695 12681 -14
- Partials 464 473 +9
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@apps/api/src/app/services/managed-deployment-lease-created/managed-deployment-lease-created.handler.spec.ts`:
- Around line 29-39: Update the setup function to accept a single typed
parameter (e.g., params?: { notificationService?: Partial<NotificationService>;
logger?: Partial<LoggerService> }) and use that parameter to allow overrides
while still creating default mocked services; keep creating the handler with new
ManagedDeploymentLeaseCreatedHandler(params.notificationService ||
defaultMockNotificationService, params.logger || defaultMockLogger) and ensure
the default mock for notificationService still stubs autoEnableDeploymentAlert
as jest.fn().mockResolvedValue(undefined). This preserves existing behavior but
satisfies the "single typed parameter" setup signature rule.
In
`@apps/api/src/notifications/services/notification/notification.service.spec.ts`:
- Around line 276-283: The setup function currently has no parameters; change it
to accept a single inline-typed parameter (e.g., overrides?: { api?:
NotificationsApiClient; userRepository?: UserRepository; deploymentHttpService?:
DeploymentHttpService; logger?: LoggerService }) and create defaults using
mockDeep/mock for api, mock for userRepository, deploymentHttpService, and
logger, then merge any provided overrides into those defaults before
instantiating NotificationService; keep returned object shape { service, api,
userRepository, deploymentHttpService, logger } so tests can override individual
dependencies when calling setup({ api, logger }) while preserving strong typing
in the single parameter.
- Around line 173-274: Tests for autoEnableDeploymentAlert use "as any" casts
(e.g. on userRepository.findById return, api.v1 responses, and
deploymentHttpService.findByOwnerAndDseq) which violate the no-any rule; replace
these casts with proper typed fixtures or mocked return types by using the
actual interfaces/models or Jest's helpers (e.g. Partial<YourType> or
jest.Mocked<...>) so each mockResolvedValue returns a correctly typed object for
userRepository.findById, api.v1.createDefaultChannel,
api.v1.getNotificationChannels, deploymentHttpService.findByOwnerAndDseq, and
api.v1.upsertDeploymentAlert, updating the test setup to import/declare the
appropriate types and constructing minimal valid objects rather than casting to
any.
In `@apps/api/src/notifications/services/notification/notification.service.ts`:
- Around line 77-118: The new method autoEnableDeploymentAlert uses `as any` in
two API calls; remove those casts and replace them with the correct OpenAPI/SDK
operation parameter types: type the getNotificationChannels call using the
generated operation type for that endpoint (e.g.
operations['getNotificationChannels']['parameters'] or the client-specific
Parameters type) and construct the header/query object to match it, and likewise
type the upsertDeploymentAlert parameters using the upsertDeploymentAlert
operation type (e.g. operations['upsertDeploymentAlert']['parameters'] and its
request body type) so headers ("x-user-id", "x-owner-address"), path (dseq) and
body match the proper interfaces; update imports to bring in the generated
operation types if needed and adjust the createDefaultChannel call site only if
its signature requires typed input (createDefaultChannel,
notificationsApi.v1.getNotificationChannels,
notificationsApi.v1.upsertDeploymentAlert, and autoEnableDeploymentAlert are the
symbols to modify).
...p/services/managed-deployment-lease-created/managed-deployment-lease-created.handler.spec.ts
Outdated
Show resolved
Hide resolved
apps/api/src/notifications/services/notification/notification.service.spec.ts
Show resolved
Hide resolved
apps/api/src/notifications/services/notification/notification.service.spec.ts
Outdated
Show resolved
Hide resolved
apps/api/src/notifications/services/notification/notification.service.ts
Outdated
Show resolved
Hide resolved
stalniy
previously approved these changes
Feb 17, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/src/notifications/services/notification/notification.service.ts (1)
104-118: Guard against floating-point precision for escrow balances.
parseFloatcan lose precision on very large blockchain amounts (e.g., "500000.000000000000000000"). While the current pattern matches existing code indraining-deployment-rpc.service.ts, consider usingBigIntinteger math for the calculation before converting back tonumberfor the API, which requires a numeric type. Include aNumber.isSafeInteger()check to handle amounts that exceed JavaScript's safe range.Possible approach with integer math
- const escrowBalance = deployment.escrow_account.state.funds.reduce((sum, { amount }) => sum + parseFloat(amount), 0); - const threshold = Math.ceil(0.3 * escrowBalance); - if (threshold === 0) return; + const escrowBalance = deployment.escrow_account.state.funds.reduce((sum, { amount }) => sum + BigInt(Math.floor(parseFloat(amount))), 0n); + const threshold = (escrowBalance * 3n + 9n) / 10n; // ceil(0.3 * balance) + if (threshold === 0n) return; + const thresholdNumber = Number(threshold); + if (!Number.isSafeInteger(thresholdNumber)) { + this.logger.warn({ event: "SKIP_AUTO_ENABLE_ALERT", reason: "Threshold exceeds safe integer range", dseq: input.dseq }); + return; + }- deploymentBalance: { notificationChannelId: channelId, enabled: true, threshold } + deploymentBalance: { notificationChannelId: channelId, enabled: true, threshold: thresholdNumber }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/notifications/services/notification/notification.service.ts` around lines 104 - 118, The current sum uses parseFloat and can lose precision; replace the reduce that computes escrowBalance with BigInt integer math: parse each string amount into a fixed-decimal BigInt (strip the decimal point assuming the chain’s known scale, e.g., 18 decimals, or determine scale consistently), sum into totalBigInt, compute thresholdBigInt as the ceiling of 30% via integer arithmetic (e.g., thresholdBigInt = (totalBigInt * 3n + 9n) / 10n), then before calling notificationsApi.v1.upsertDeploymentAlert convert thresholdBigInt back to a JavaScript number only after checking Number.isSafeInteger(thresholdNum) (or handle overflow by using Number.MAX_SAFE_INTEGER or failing early with a logged error); update references around escrowBalance/threshold computation in the block that feeds upsertDeploymentAlert so the API receives a safe numeric threshold.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/api/src/notifications/services/notification/notification.service.ts`:
- Around line 104-118: The current sum uses parseFloat and can lose precision;
replace the reduce that computes escrowBalance with BigInt integer math: parse
each string amount into a fixed-decimal BigInt (strip the decimal point assuming
the chain’s known scale, e.g., 18 decimals, or determine scale consistently),
sum into totalBigInt, compute thresholdBigInt as the ceiling of 30% via integer
arithmetic (e.g., thresholdBigInt = (totalBigInt * 3n + 9n) / 10n), then before
calling notificationsApi.v1.upsertDeploymentAlert convert thresholdBigInt back
to a JavaScript number only after checking Number.isSafeInteger(thresholdNum)
(or handle overflow by using Number.MAX_SAFE_INTEGER or failing early with a
logged error); update references around escrowBalance/threshold computation in
the block that feeds upsertDeploymentAlert so the API receives a safe numeric
threshold.
baktun14
force-pushed
the
feat/auto-enable-alert
branch
from
a004c56 to
a5a7607
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/src/notifications/services/notification/notification.service.ts (1)
86-98: Avoid unconditional default-channel creation to prevent failures.
createDefaultChannelruns before checking existing channels. If the API isn’t idempotent (or rejects duplicates), this will throw and abort alert enablement for existing users. Consider fetching channels first and only creating when none are found (or explicitly ignoring “already exists” errors).Proposed refactor
- await this.createDefaultChannel({ id: input.userId, email: user.email }); - - const channelsResult = await this.notificationsApi.v1.getNotificationChannels({ - parameters: { - header: { "x-user-id": input.userId }, - query: { page: 1, limit: 1 } - } as operations["getNotificationChannels"]["parameters"] & { header: { "x-user-id": string } } - }); - const channelId = channelsResult?.data?.data?.[0]?.id; + const channelsParams = + { + header: { "x-user-id": input.userId }, + query: { page: 1, limit: 1 } + } as operations["getNotificationChannels"]["parameters"] & { header: { "x-user-id": string } }; + + let channelsResult = await this.notificationsApi.v1.getNotificationChannels({ parameters: channelsParams }); + let channelId = channelsResult?.data?.data?.[0]?.id; + + if (!channelId) { + await this.createDefaultChannel({ id: input.userId, email: user.email }); + channelsResult = await this.notificationsApi.v1.getNotificationChannels({ parameters: channelsParams }); + channelId = channelsResult?.data?.data?.[0]?.id; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/notifications/services/notification/notification.service.ts` around lines 86 - 98, The code currently calls createDefaultChannel(...) before checking existing channels which can fail on duplicate creation; change the flow in notification.service.ts to first call this.notificationsApi.v1.getNotificationChannels(...) (as already used) and only call this.createDefaultChannel({ id: input.userId, email: user.email }) when channelsResult.data.data is empty (no channelId), or wrap createDefaultChannel in a try/catch that swallows/ignores a deterministic "already exists" error but rethrows other errors; update logic around channelId (derived from channelsResult?.data?.data?.[0]?.id) to reflect this new order so you only create when necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/notifications/services/notification/notification.service.ts`:
- Around line 106-108: The escrow balance calculation in notification.service.ts
uses parseFloat directly in the reducer and should mirror
deployment-reader.service.ts by applying fixed 18-decimal normalization after
the sum; change the computation of escrowBalance (the variable named
escrowBalance) to first sum amounts (e.g., total =
deployment.escrow_account.state.funds.reduce(..., 0)), then call
total.toFixed(18) and convert that to a Number/parseFloat for escrowBalance
before computing threshold (still using
DEPLOYMENT_BALANCE_ALERT_THRESHOLD_RATIO).
---
Nitpick comments:
In `@apps/api/src/notifications/services/notification/notification.service.ts`:
- Around line 86-98: The code currently calls createDefaultChannel(...) before
checking existing channels which can fail on duplicate creation; change the flow
in notification.service.ts to first call
this.notificationsApi.v1.getNotificationChannels(...) (as already used) and only
call this.createDefaultChannel({ id: input.userId, email: user.email }) when
channelsResult.data.data is empty (no channelId), or wrap createDefaultChannel
in a try/catch that swallows/ignores a deterministic "already exists" error but
rethrows other errors; update logic around channelId (derived from
channelsResult?.data?.data?.[0]?.id) to reflect this new order so you only
create when necessary.
apps/api/src/notifications/services/notification/notification.service.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/notifications/services/notification/notification.service.ts`:
- Around line 106-108: The current computation of escrowBalance and threshold
can result in NaN or negative values if any parsed amount is non-finite; update
the logic around escrowBalance, threshold and the use of
DEPLOYMENT_BALANCE_ALERT_THRESHOLD_RATIO to validate inputs: when reducing
deployment.escrow_account.state.funds, coerce each amount safely (e.g.,
parseFloat with fallback to 0) and compute escrowBalance, then check
Number.isFinite(escrowBalance) and escrowBalance > 0 before computing threshold;
after computing threshold also guard with Number.isFinite(threshold) and
threshold > 0 and return early if those checks fail so no invalid threshold is
sent to the API (refer to escrowBalance, threshold,
DEPLOYMENT_BALANCE_ALERT_THRESHOLD_RATIO).
- Around line 88-124: The external calls getNotificationChannels,
deploymentHttpService.findByOwnerAndDseq, and
notificationsApi.v1.upsertDeploymentAlert need retry/error handling: wrap each
call in the same backOff retry helper used by
createNotification/createDefaultChannel (or add equivalent try/catch with
retries) so transient errors are retried and failures are logged via
this.logger.warn/error with context (event, dseq, userId, walletAddress) and
cause graceful degradation (skip auto-enable flow when still failing).
Specifically, replace direct calls to
this.notificationsApi.v1.getNotificationChannels,
this.deploymentHttpService.findByOwnerAndDseq, and
this.notificationsApi.v1.upsertDeploymentAlert with backOff(() => /* call */)
and add catch blocks that log the error and return early where appropriate to
preserve current control flow.
- Around line 86-93: The call to createDefaultChannel is unconditional and can
throw on conflict; change autoEnableDeploymentAlert (or the function containing
the shown snippet) to first call notificationsApi.v1.getNotificationChannels (as
already available) and check if any channel exists for the userId before calling
createDefaultChannel({ id: input.userId, email: user.email }); alternatively
wrap createDefaultChannel in a try/catch that specifically handles conflict
responses (HTTP 409) by swallowing or logging them and only rethrowing
unexpected errors; reference createDefaultChannel and
notificationsApi.v1.getNotificationChannels when making the change.
apps/api/src/notifications/services/notification/notification.service.ts
Outdated
Show resolved
Hide resolved
apps/api/src/notifications/services/notification/notification.service.ts
Outdated
Show resolved
Hide resolved
apps/api/src/notifications/services/notification/notification.service.ts
Outdated
Show resolved
Hide resolved
Contributor
|
please make sure to follow pull request template and describe the changes |
ygrishajev
reviewed
Feb 18, 2026
apps/api/src/notifications/services/notification/notification.service.ts
Outdated
Show resolved
Hide resolved
apps/api/src/notifications/services/notification/notification.service.ts
Outdated
Show resolved
Hide resolved
apps/api/src/billing/events/managed-deployment-lease-created.ts
Outdated
Show resolved
Hide resolved
…els and improve error handling
baktun14
force-pushed
the
feat/auto-enable-alert
branch
from
ca20355 to
93eef13
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Author
Tested locally and both are enabled by default when creating a deployment
|
ygrishajev
approved these changes
Feb 18, 2026
ygrishajev
approved these changes
Feb 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Tests