feat: Pause deployments when overdrawn #2013
Conversation
Signed-off-by: Chris Marslender <chrismarslender@gmail.com>
Signed-off-by: Chris Marslender <chrismarslender@gmail.com>
Signed-off-by: Chris Marslender <chrismarslender@gmail.com>
…to pause all deployment groups Signed-off-by: Chris Marslender <chrismarslender@gmail.com>
…vs closed state Signed-off-by: Chris Marslender <chrismarslender@gmail.com>
Signed-off-by: Chris Marslender <chrismarslender@gmail.com>
Signed-off-by: Chris Marslender <chrismarslender@gmail.com>
Signed-off-by: Chris Marslender <chrismarslender@gmail.com>
WalkthroughThis PR adds an "account paused" hook path: escrow accounts entering StateOverdrawn now invoke OnEscrowAccountPaused hooks; the market module resolves deployments and calls DeploymentKeeper.OnPauseGroup for pausable groups, and groups are persisted with state Paused while the deployment remains Active. Changes
Sequence Diagram(s)sequenceDiagram
participant EscrowKeeper as Escrow Keeper
participant Account as Escrow Account
participant HooksReg as Escrow Hooks Registry
participant MarketHooks as Market Hooks
participant Deployment as Deployment Keeper
Note over Account,EscrowKeeper: Account transitions to StateOverdrawn
EscrowKeeper->>Account: set state = Overdrawn
EscrowKeeper->>HooksReg: invoke onAccountPaused hooks
HooksReg->>MarketHooks: OnEscrowAccountPaused(ctx, account)
MarketHooks->>Deployment: resolve deployment by escrow ID
Deployment-->>MarketHooks: deployment + groups
loop per pausable group
MarketHooks->>Deployment: OnPauseGroup(ctx, group)
Deployment->>Deployment: update group.state = Paused
Deployment->>Deployment: recompute group store key for Paused
Deployment->>Deployment: persist group under new key
Deployment-->>MarketHooks: success
end
Note over Deployment: deployment.state remains Active
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
[pre_merge_check_pass] The PR successfully implements the core coding requirements from issue #368. It prevents deployments from being closed when overdrawn by introducing OnEscrowAccountPaused hooks that pause groups instead [all files]. The new OnPauseGroup method ensures groups transition to paused state rather than closed state, allowing deployments to remain active for future lease creation after funding [x/deployment/keeper/keeper.go, x/market/hooks/hooks.go]. Tests verify that OnEscrowAccountPaused is invoked for overdraw scenarios and groups transition to the paused state [x/deployment/keeper/keeper_test.go, x/escrow/keeper/keeper_test.go]. The implementation aligns with the stated objectives to pause groups while keeping deployments open for resumption after account restoration [#368]. | ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
|
|
||
| key, err := GroupKey(GroupStateToPrefix(group.State), group.ID) | ||
| if err != nil { | ||
| return fmt.Errorf("%s: failed to encode group key", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
Looking at OnCloseGroup, it seems like the new key with the updated prefix should have been used here or else it gets saved to the wrong part of the KVStore.
It does look like findGroup was checking all possible variants of a key when looking for a group, so it probably could have been found anyways, but this should still be a more accurate state/key for the Group
| return err | ||
| } | ||
|
|
||
| store.Set(key, k.cdc.MustMarshal(&group)) |
There was a problem hiding this comment.
This was already called a few lines above
![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
🧹 Nitpick comments (1)
x/escrow/keeper/keeper.go (1)
779-789: Consider using a switch statement for better maintainability.The current if-else chain works correctly, but static analysis suggests using a tagged switch statement for improved clarity and extensibility, especially if more states are added in the future.
- if obj.State.State == etypes.StateClosed { + switch obj.State.State { + case etypes.StateClosed: // call hooks for _, hook := range k.hooks.onAccountClosed { hook(ctx, obj.Account) } - } else if obj.State.State == etypes.StateOverdrawn { + case etypes.StateOverdrawn: // call hooks for _, hook := range k.hooks.onAccountPaused { hook(ctx, obj.Account) } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.md(1 hunks)app/types/app.go(1 hunks)x/deployment/keeper/keeper.go(1 hunks)x/deployment/keeper/keeper_test.go(2 hunks)x/escrow/keeper/keeper.go(5 hunks)x/escrow/keeper/keeper_test.go(2 hunks)x/market/hooks/external.go(1 hunks)x/market/hooks/hooks.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
app/types/app.go (1)
testutil/state/suite.go (1)
Keepers(50-59)
x/market/hooks/external.go (1)
x/deployment/query/types.go (1)
Group(71-71)
x/deployment/keeper/keeper.go (1)
x/deployment/keeper/key.go (2)
GroupKey(66-89)GroupStateToPrefix(144-158)
🪛 golangci-lint (2.5.0)
x/escrow/keeper/keeper.go
[error] 779-779: QF1003: could use tagged switch on obj.State.State
(staticcheck)
x/deployment/keeper/keeper_test.go
[error] 9-9: ST1019: package "pkg.akt.dev/go/node/deployment/v1" is being imported more than once
(staticcheck)
🔇 Additional comments (8)
app/types/app.go (1)
470-470: LGTM!The new hook registration follows the established pattern and is correctly positioned with other account-related hooks.
x/market/hooks/external.go (1)
17-17: LGTM!The method signature is consistent with the existing
OnCloseGroupmethod and appropriately extends the interface to support the pause functionality.x/deployment/keeper/keeper.go (1)
304-332: LGTM! Critical fix for state consistency.The reordering ensures the group is persisted with the correct state key (Paused) rather than its previous key. This matches the pattern used in
OnCloseGroupandOnStartGroup, maintaining consistency across state transitions.x/escrow/keeper/keeper_test.go (1)
182-247: LGTM! Comprehensive test coverage.The test properly verifies the new pause behavior by:
- Tracking hook invocations with clear state variables
- Asserting OnAccountPaused is called with correct account state
- Confirming OnAccountClosed is NOT called for overdrawn accounts
- Using well-structured subtests for clear assertions
x/escrow/keeper/keeper.go (3)
44-44: LGTM!The new
AddOnAccountPausedHookmethod properly extends the Keeper interface to support pause hooks.
324-324: LGTM! Critical fix for account recovery.Allowing deposits when
StateOverdrawnenables accounts to be refunded and resume operations, which is essential for the pause-and-resume workflow.
589-592: LGTM!The implementation follows the same pattern as
AddOnAccountClosedHookand correctly appends hooks to the collection.x/deployment/keeper/keeper_test.go (1)
121-163: LGTM! Test properly validates pause behavior.The renamed test correctly verifies that:
- Deployments remain active when accounts are overdrawn
- Groups transition to paused state (not closed)
- Multiple groups are handled correctly
The test structure with subtests provides clear, isolated assertions for each state verification.
| ## [Unreleased] | ||
|
|
||
| * (ibc-go) Use ibc v4.4.0 https://pkg.akt.dev/node/pull/1825 | ||
| * Pause deployment groups when account is overdrawn |
There was a problem hiding this comment.
Format the changelog entry per guidelines.
The changelog entry should follow the format specified in lines 15-19: * (<tag>) \#<issue-number> message. It should also be placed under an appropriate stanza (e.g., "Features" or "Improvements") rather than directly under [Unreleased].
Apply this diff to fix the formatting:
## [Unreleased]
* (ibc-go) Use ibc v4.4.0 https://pkg.akt.dev/node/pull/1825
-* Pause deployment groups when account is overdrawn
### Improvements
+* (escrow) \#368 Pause deployment groups when account is overdrawn
* (sdk) Bump Cosmos SDK version to [v0.38.3](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.38.3)🤖 Prompt for AI Agents
In CHANGELOG.md around line 42, the entry "* Pause deployment groups when
account is overdrawn" is not formatted per the project's changelog guideline and
sits directly under [Unreleased]; change it to the prescribed format and place
it under the correct stanza: convert the line to something like "* (improvement)
#<issue-number> Pause deployment groups when account is overdrawn" (replace
<issue-number> with the real issue/PR number and pick the correct tag, e.g.,
"improvement" or "feature"), and move that formatted bullet into the appropriate
section header (for example under "## Improvements" or "## Features") instead of
directly beneath [Unreleased].
| func (h *hooks) OnEscrowAccountPaused(ctx sdk.Context, obj etypes.Account) { | ||
| id, err := dv1.DeploymentIDFromEscrowID(obj.ID) | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| deployment, found := h.dkeeper.GetDeployment(ctx, id) | ||
| if !found { | ||
| return | ||
| } | ||
|
|
||
| if deployment.State != dv1.DeploymentActive { | ||
| return | ||
| } | ||
|
|
||
| for _, group := range h.dkeeper.GetGroups(ctx, deployment.ID) { | ||
| if group.ValidatePausable() == nil { | ||
| _ = h.dkeeper.OnPauseGroup(ctx, group) | ||
| _ = h.mkeeper.OnGroupClosed(ctx, group.ID) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify OnGroupClosed is correct when pausing groups.
Line 78 calls OnGroupClosed after pausing the group. While this mirrors the pattern in OnEscrowAccountClosed (line 55), please confirm this is intentional. When a group is paused (not closed), should market resources (orders, bids, leases) be closed?
If pausing should preserve market state for resumption, consider whether a different market keeper method (e.g., OnGroupPaused) might be more appropriate.
🏁 Script executed:
#!/bin/bash
# Verify OnGroupClosed behavior and check if OnGroupPaused exists
# Check if OnGroupPaused method exists in MarketKeeper interface
ast-grep --pattern $'OnGroupPaused($$$) $$$'
# Check OnGroupClosed implementation to understand what it does
ast-grep --pattern $'func ($$$) OnGroupClosed(ctx $_, id $_) error {
$$$
}'Length of output: 2724
---
🏁 Script executed:
```shell
#!/bin/bash
# Check if paused deployments can be resumed and explore pause/unpause semantics
# Search for resume or unpause functionality
rg -i "unpause|resume" -A 2 -B 2 --type go
# Check deployment state transitions
rg "DeploymentPaused\|DeploymentActive" -C 2 --type go
# Check what happens when escrow account is resumed/unpaused
rg "OnEscrowAccountResumed\|OnEscrowAccountUnpaused" -A 5 --type go
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Investigate deployment pause semantics more directly
# Check deployment state enum values
rg "DeploymentPaused|DeploymentClosed|type.*State" -B 2 -A 2 --type go | head -50
# Look at OnEscrowAccountPaused implementation to understand pattern
rg -A 20 "func.*OnEscrowAccountPaused" --type go
# Check if paused deployments have any restrictions or special handling
rg "State.*Paused\|Paused.*State" -C 3 --type goLength of output: 4967
🏁 Script executed:
#!/bin/bash
# Find OnPauseGroup implementation and examine the test
# Search for OnPauseGroup method
rg -A 15 "func.*OnPauseGroup" --type go
# Get the full test case to understand intended behavior
rg -A 50 "func Test_OnEscrowAccountPaused_overdrawn" --type goLength of output: 4348
🏁 Script executed:
#!/bin/bash
# Check if paused groups/deployments can resume or transition back to active state
# Search for transitions FROM GroupPaused or DeploymentPaused to active states
rg "GroupPaused|DeploymentPaused" -B 3 -A 3 --type go | grep -A 3 -B 3 "GroupOpen\|DeploymentActive\|Active\|resume"
# Check if there's any method that resumes or unpauses a group
rg "GroupOpen" --type go | grep -i "paused\|resume"
# Look for state transition logic
rg "group.State\s*=" -B 2 -A 2 --type go | head -80Length of output: 1930
🏁 Script executed:
#!/bin/bash
# Find the method that transitions group.State to GroupOpen
# Search for methods containing the state transition to GroupOpen
rg -B 30 "group.State = types.GroupOpen" --type go | grep -A 30 "^x/deployment"Length of output: 1551
Correct the hook to not close market resources on pause—groups can be resumed.
The review comment is valid. OnPauseGroup transitions the group state to GroupPaused, and there exists an OnStartGroup method that transitions paused groups back to GroupOpen. This means groups are designed to be resumable.
However, OnGroupClosed permanently closes orders, bids, and leases. Calling OnGroupClosed during pause means market resources are destroyed and cannot be restored if the group is later resumed via OnStartGroup. This violates pause semantics—pausing should be temporary.
The line 78 call to h.mkeeper.OnGroupClosed(ctx, group.ID) should be removed or replaced with a method like OnGroupPaused (if one is needed for market-side pause handling) to preserve market state for potential resumption.
🤖 Prompt for AI Agents
In x/market/hooks/hooks.go around lines 60 to 81, the hook currently calls
h.mkeeper.OnGroupClosed during an escrow pause which permanently destroys market
resources; remove the h.mkeeper.OnGroupClosed(ctx, group.ID) call (or replace it
with a non-destructive handler such as h.mkeeper.OnGroupPaused(ctx, group.ID) if
the market keeper exposes an explicit pause method) so that pausing only
transitions the group to GroupPaused and preserves orders/bids/leases for
potential resumption.
There was a problem hiding this comment.
May be useful to get some feedback here from someone more familiar with desired state. I was initially under the impression that on re-funding and resuming, new orders, etc would have to be created, though maybe this is not actually the case and was a misunderstanding on my side
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check OnStartGroup implementation to see if it recreates market resources
ast-grep --pattern $'func ($$$) OnStartGroup(ctx $_, $_, $_) error {
$$$
}'
# Search for OnStartGroup usage and implementation
rg -A 20 "func.*OnStartGroup" --type go
# Check what OnGroupClosed does in the market keeper
rg -A 25 "func.*OnGroupClosed" --type go | head -100
Length of output: 4237
🏁 Script executed:
#!/bin/bash
# Look for order/bid/lease creation logic that might happen on resume
rg "CreateOrder|NewOrder|create.*order" -i -C 3 --type go | head -80
# Check if there's any logic that recreates market state
rg "resume.*order|unpause.*order|reactivate" -i -C 3 --type go
Length of output: 4934
🏁 Script executed:
#!/bin/bash
# Check the market keeper interface to understand available methods
cat x/market/hooks/external.go
# Look for any pause-related market keeper methods
rg "OnGroup" --type go -g "x/market/**/*.go" | grep -i "interface\|func"
Length of output: 1282
🏁 Script executed:
#!/bin/bash
# Check tests related to pause/resume to understand intended behavior
rg -A 30 "pause.*resume|resume.*pause" -i --type go | head -100
# Look at deployment handler to see what happens when groups are started
rg -A 20 "MsgStartGroup|HandleMsgStartGroup" --type go
Length of output: 2716
@cmmarslender Your initial understanding was actually correct! Based on the code investigation, here's what happens:
Current Design Pattern:
-
On Pause (when overdrawn):
- Groups transition to
GroupPausedstate OnGroupClosedis called, which permanently closes all market resources (orders, bids, leases, and escrow payments)
- Groups transition to
-
On Resume (via
MsgStartGroup):- Group transitions from
GroupPausedtoGroupOpen - The handler explicitly creates NEW orders via
ms.market.CreateOrder(ctx, group.ID, group.GroupSpec)(seex/deployment/handler/server.goline ~73)
- Group transitions from
So the current implementation in this PR is consistent with that pattern—calling OnGroupClosed during pause is appropriate because:
- Market resources are destroyed when paused
- Fresh orders are created when the group is resumed
- The pause/resume cycle intentionally goes through a full teardown and recreation of market state
My initial review concern was incorrect. The code is working as designed. Calling OnGroupClosed ensures clean teardown of the marketplace side before resumption, which will trigger new order creation.
If this is the intended behavior for handling overdrawn accounts, the current implementation is correct. 👍
🧬 Code Graph Analysis Results
-
File: x/deployment/handler/keepers.go
Line ranges: 19-22MarketKeeper interface { CreateOrder(ctx sdk.Context, id types.GroupID, spec v1beta4.GroupSpec) (mtypes.Order, error) OnGroupClosed(ctx sdk.Context, id types.GroupID) error } -
File: x/market/handler/keepers.go
Line ranges: 43-47DeploymentKeeper interface { GetGroup(ctx sdk.Context, id dtypes.GroupID) (dbeta.Group, bool) OnBidClosed(ctx sdk.Context, id dtypes.GroupID) error OnLeaseClosed(ctx sdk.Context, id dtypes.GroupID) (dbeta.Group, error) } -
File: x/market/hooks/external.go
Line ranges: 12-18DeploymentKeeper interface { GetDeployment(ctx sdk.Context, id dv1.DeploymentID) (dv1.Deployment, bool) GetGroups(ctx sdk.Context, id dv1.DeploymentID) dtypes.Groups CloseDeployment(ctx sdk.Context, deployment dv1.Deployment) error OnCloseGroup(ctx sdk.Context, group dtypes.Group, state dtypes.Group_State) error OnPauseGroup(ctx sdk.Context, group dtypes.Group) error } -
File: x/market/hooks/external.go
Line ranges: 20-28MarketKeeper interface { GetOrder(ctx sdk.Context, id mv1.OrderID) (mtypes.Order, bool) GetBid(ctx sdk.Context, id mv1.BidID) (mtypes.Bid, bool) GetLease(ctx sdk.Context, id mv1.LeaseID) (mv1.Lease, bool) OnGroupClosed(ctx sdk.Context, id dv1.GroupID) error OnOrderClosed(ctx sdk.Context, order mtypes.Order) error OnBidClosed(ctx sdk.Context, bid mtypes.Bid) error OnLeaseClosed(ctx sdk.Context, lease mv1.Lease, state mv1.Lease_State, reason mv1.LeaseClosedReason) error }
Signed-off-by: Chris Marslender <chrismarslender@gmail.com>
Signed-off-by: Chris Marslender <chrismarslender@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
x/deployment/keeper/keeper_test.go (1)
142-146: Fix grammatical error in comment.The comment contains a grammatical error: "by calling pausing all groups" should be corrected for clarity.
Apply this diff to fix the comment:
- // Simulate escrow account becoming overdrawn by calling pausing all groups + // Simulate escrow account becoming overdrawn by pausing all groups for _, group := range groups { err := keeper.OnPauseGroup(ctx, group) require.NoError(t, err) }
🧹 Nitpick comments (1)
x/deployment/keeper/keeper_test.go (1)
148-162: State assertions correctly verify the new pausing behavior.The test properly verifies that when groups are paused due to an overdrawn account, the deployment remains active (DeploymentActive) while groups transition to GroupPaused. This aligns perfectly with the PR objective to prevent deployments from closing when funds are exhausted.
Optional enhancement: Consider verifying event emission.
The test could optionally verify that the appropriate events are emitted when groups are paused, similar to the event assertion in
Test_Create(line 28). This would provide more comprehensive coverage:// After the pause loop, before final assertions: events := ctx.EventManager().Events() assert.NotEmpty(t, events, "Expected events to be emitted when groups are paused")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/deployment/keeper/keeper_test.go(1 hunks)
🔇 Additional comments (2)
x/deployment/keeper/keeper_test.go (2)
120-122: LGTM! Test name accurately reflects the new pausing behavior.The renamed test function clearly indicates it's testing the overdrawn scenario where groups are paused rather than closed, aligning with the PR objectives.
126-140: Well-structured initial state verification.The subtests clearly verify the baseline state before simulating the overdrawn scenario, making the test easier to understand and debug.
1 participant
Description
Closes: akash-network/support#368
Updates overdrawn account handling to pause all deployment groups and leave the deployment active, instead of closing out the deployment so that the groups may be resumed once the account is funded again.
I did note that unpausing a group doesn't currently seem to do any balance checks as part of that process. It might be worth doing a balance check there before unpausing or else the groups will just end up paused again by this logic.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.md