fix(alert): disable deployment alerts before intentional close#2845
fix(alert): disable deployment alerts before intentional close#2845
Conversation
When a user intentionally closes a deployment via managed wallet, the chain event fires and triggers unwanted alert notifications. This disables alerts before the close transaction so the chain event finds no enabled alerts. Handles both the direct close endpoint and generic transaction paths.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughManagedSignerService now detects Changes
Sequence DiagramsequenceDiagram
participant MS as ManagedSignerService
participant NS as NotificationService
participant API as Notifications API
MS->>MS: scan tx messages for MsgCloseDeployment
activate MS
MS->>NS: disableDeploymentAlerts(userId, walletAddress, dseq)
deactivate MS
activate NS
NS->>API: GET /deployment-alerts?userId=...&wallet=...
activate API
API-->>NS: current alerts
deactivate API
NS->>NS: determine which alerts to disable
alt alerts need update
NS->>API: UPSERT /deployment-alerts (disable payload) with backoff
activate API
API-->>NS: success
deactivate API
else no update needed or suppressed
NS->>NS: skip upsert
end
NS-->>MS: return void
deactivate NS
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![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
🤖 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/billing/services/managed-signer/managed-signer.service.ts`:
- Around line 117-127: The code currently uses messages.find to get only the
first MsgCloseDeployment (closeDeploymentMessage) so other close messages in the
same transaction are ignored; change this to iterate over all matching messages
(e.g., use messages.filter or a for loop over messages.filter(m =>
m.typeUrl.endsWith(".MsgCloseDeployment"))), and for each matching message where
userWallet.userId exists call this.notificationService.disableDeploymentAlerts
with the same payload (userId: userWallet.userId, walletAddress:
userWallet.address!, dseq: message.value.id!.dseq.toString()) so every
MsgCloseDeployment in the transaction is processed.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/api/src/billing/services/managed-signer/managed-signer.service.tsapps/api/src/notifications/services/notification/notification.service.spec.tsapps/api/src/notifications/services/notification/notification.service.ts
apps/api/src/billing/services/managed-signer/managed-signer.service.ts
Outdated
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2845 +/- ##
==========================================
- Coverage 54.25% 53.46% -0.80%
==========================================
Files 1020 985 -35
Lines 23635 22831 -804
Branches 5789 5701 -88
==========================================
- Hits 12824 12206 -618
+ Misses 9423 9246 -177
+ Partials 1388 1379 -9
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
Process all MsgCloseDeployment messages in a transaction batch instead of only the first one. Adds tests for single, multiple, and absent close deployment messages in ManagedSignerService.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/billing/services/managed-signer/managed-signer.service.ts`:
- Around line 117-129: The closeDeploymentMessages handling can throw when
message.value.id or dseq is missing and may call disableDeploymentAlerts
multiple times for the same dseq; update the block that builds
closeDeploymentMessages to first filter messages to those with a present id and
dseq (e.g. message.value?.id?.dseq != null), map to a normalized dseq string
(message.value.id.dseq.toString()), deduplicate that list (e.g. Set) and then
call this.notificationService.disableDeploymentAlerts once per unique dseq using
userWallet.userId and userWallet.address; reference the closeDeploymentMessages
variable, the notificationService.disableDeploymentAlerts call, and userWallet
fields to locate and change the code.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/api/src/billing/services/managed-signer/managed-signer.service.spec.tsapps/api/src/billing/services/managed-signer/managed-signer.service.ts
| await this.notificationService.disableDeploymentAlerts({ | ||
| userId: userWallet.userId, | ||
| walletAddress: userWallet.address!, | ||
| dseq | ||
| }); |
There was a problem hiding this comment.
question: does it make sense to keep this logic on notification service? for example, when it receives a block or event (not sure how it does it) then it can check whether there is a MsgCloseDeployment message for that dseq and if so, it means user intentionally closed it and it can skip notification.
There was a problem hiding this comment.
This may be a cleaner approach indeed. Some logic to identify the event source would be needed here. Here's what the trigger event looks like:
[
{
"type": "akash.deployment.v1.EventDeploymentClosed",
"attributes": [
{
"key": "id",
"value": "{\"owner\":\"akash1sjds2vaquyax6zf88jhl9kt4enp3tqdn4r9erv\",\"dseq\":\"2330094\"}"
},
{
"key": "msg_index",
"value": "0"
}
]
}
]
|
I think from awareness standpoint, it's good to receive this notification even if it was initiated by user. Because he receives confirmation that he won't be billed for that deployment anymore. Also from audit trail standpoint it may be useful, since blockchain is abstracted from managed wallet user. So, probably it makes sense to make it configurable via "notify on explicit close" checkbox, which can be disabled by default |
There was a problem hiding this comment.
Although I understand the purpose of this PR I'd like to throw in some concern:
We currently do not support SASS like product organization. Meaning if any user wants to use akash as an organization they are forced to share credentials and use the same account. This said I as a user may want to receive notifications if other users close deployments intentionally.
| await this.notificationService.disableDeploymentAlerts({ | ||
| userId: userWallet.userId, | ||
| walletAddress: userWallet.address!, | ||
| dseq | ||
| }); |
There was a problem hiding this comment.
This may be a cleaner approach indeed. Some logic to identify the event source would be needed here. Here's what the trigger event looks like:
[
{
"type": "akash.deployment.v1.EventDeploymentClosed",
"attributes": [
{
"key": "id",
"value": "{\"owner\":\"akash1sjds2vaquyax6zf88jhl9kt4enp3tqdn4r9erv\",\"dseq\":\"2330094\"}"
},
{
"key": "msg_index",
"value": "0"
}
]
}
]3 participants
Why
When a user intentionally closes a deployment via managed wallet, the
deployment-closedchain event fires and triggers an unwanted alert notification. This is annoying for users since they deliberately closed the deployment.What
Disable deployment alerts before the close transaction is signed, so the chain event finds no enabled alerts to trigger.
disableDeploymentAlertsmethod toNotificationServicethat fetches current alerts and upserts them withenabled: false(best-effort, errors are caught and logged)getDeploymentAlertsprivate helper toNotificationServiceManagedSignerService.executeDecodedTxByUserWallet()to detectMsgCloseDeploymentand disable alerts before signing — this covers both the direct close endpoint (DELETE /v1/deployments/{dseq}) and the generic transaction path (POST /v1/tx)disableDeploymentAlertscovering: both alerts enabled, already disabled, suppressed by system, no alerts, error handling, partial disableSummary by CodeRabbit
New Features
Updates
Tests