refactor: pause deployments when overdrawn #2011
Conversation
WalkthroughAdds DeploymentKeeper.OnPauseGroup and MarketKeeper.OnGroupPaused hooks; changes OnEscrowAccountClosed to decide per-group target state (GroupPaused vs GroupClosed) and call pausable/closable validations and corresponding hooks; centralizes group resource closing into closeMarketResourcesForGroup; adds parameterized tests for escrow-account-closure outcomes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant EscrowHook as Escrow Hook
participant Hooks as Market Hooks
participant DeployK as DeploymentKeeper
participant MarketK as MarketKeeper
participant MarketIK as Market IKeeper
EscrowHook->>Hooks: OnEscrowAccountClosed(escrowAcct)
alt escrow state = StateOverdrawn
Hooks->>Hooks: target = GroupPaused
loop per group
Hooks->>DeployK: ValidatePausable(group)
DeployK-->>Hooks: ok / err
Hooks->>DeployK: OnPauseGroup(group)
DeployK-->>Hooks: ok / err
Hooks->>MarketK: OnGroupPaused(groupID)
MarketK-->>Hooks: ok / err
Hooks->>MarketIK: closeMarketResourcesForGroup(groupID, InsufficientFunds)
MarketIK-->>Hooks: ok / err
end
else escrow state != StateOverdrawn
Hooks->>Hooks: target = GroupClosed
loop per group
Hooks->>DeployK: ValidateClosable(group)
DeployK-->>Hooks: ok / err
Hooks->>DeployK: OnCloseGroup(group, target)
DeployK-->>Hooks: ok / err
Hooks->>MarketIK: OnGroupClosed(groupID) -> closeMarketResourcesForGroup(groupID, Owner)
MarketIK-->>Hooks: ok / err
end
Hooks->>Hooks: CloseDeployment(...) %% invoked in close branch
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ 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 |
|
Marked as stale; will be closed in five days. |
rick-a-lane-ii
force-pushed
the
rick/support-issue-368
branch
from
da4f6c2 to
7f72c03
Compare
|
Rebased against the latest |
rick-a-lane-ii
force-pushed
the
rick/support-issue-368
branch
from
7f72c03 to
e0713db
Compare
|
Added testing. It might be more optimal to change the deployment creation to mocks here, but leaving as-is for now. |
|
A few notes:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
x/market/hooks/hooks_test.go (1)
125-125: Static analysis false positive (integer overflow).The gosec warning about integer overflow from
inttouint32is a false positive here, sincegroupCountis 3 andi+1will never exceed uint32 range. However, if you prefer to silence the linter, you can make the loop variable explicitly typed:- for i := range groups { + for i := 0; i < groupCount; i++ { groups[i] = testutil.DeploymentGroup(t, did, uint32(i+1)) }This is optional and doesn't affect correctness.
Based on static analysis hints.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
x/market/hooks/external.go(1 hunks)x/market/hooks/hooks.go(1 hunks)x/market/hooks/hooks_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
x/market/hooks/hooks_test.go (3)
testutil/state/suite.go (1)
SetupTestSuite(63-65)x/market/hooks/external.go (2)
DeploymentKeeper(12-18)MarketKeeper(20-28)x/market/hooks/hooks.go (1)
New(23-28)
🪛 golangci-lint (2.5.0)
x/market/hooks/hooks_test.go
[high] 125-125: G115: integer overflow conversion int -> uint32
(gosec)
x/market/hooks/hooks.go
[error] 52-52: Error return value of h.dkeeper.CloseDeployment is not checked
(errcheck)
[error] 60-60: Error return value of h.mkeeper.OnGroupClosed is not checked
(errcheck)
[error] 65-65: Error return value of h.mkeeper.OnGroupClosed is not checked
(errcheck)
🔇 Additional comments (2)
x/market/hooks/external.go (1)
17-17: LGTM! Clean interface extension.The new
OnPauseGrouphook follows the same pattern as existing lifecycle hooks and provides the necessary API for pausing groups when escrow accounts become overdrawn.x/market/hooks/hooks_test.go (1)
52-106: LGTM! Comprehensive test coverage.The test effectively validates both scenarios:
- Overdrawn accounts result in active deployments with paused groups
- Accounts in good standing result in closed deployments with closed groups
The test structure is clear and assertions appropriately verify the expected state transitions.
rick-a-lane-ii
force-pushed
the
rick/support-issue-368
branch
2 times, most recently
from
8be2416 to
713928a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
x/market/hooks/hooks_test.go (2)
125-125: Optional: Rewrite conversion to silence static analysis.The current conversion is safe (i ranges 0-2), but rewriting as
uint32(i)+1is more idiomatic and avoids the gosec warning.Apply this diff:
- groups[i] = testutil.DeploymentGroup(t, did, uint32(i+1)) + groups[i] = testutil.DeploymentGroup(t, did, uint32(i)+1)
79-104: Consider adding tests for validation failure paths.The current tests assume all groups pass
ValidatePausableandValidateClosable. Consider adding test cases where some groups fail validation to ensure the hook handles these edge cases gracefully (though this may be deferred to a follow-up PR given the preference for small, incremental changes).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
x/market/hooks/external.go(1 hunks)x/market/hooks/hooks.go(1 hunks)x/market/hooks/hooks_test.go(1 hunks)x/market/keeper/keeper.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
x/market/keeper/keeper.go (1)
x/escrow/keeper/keeper.go (1)
Keeper(30-50)
x/market/hooks/hooks_test.go (3)
testutil/state/suite.go (1)
SetupTestSuite(63-65)x/market/hooks/external.go (2)
DeploymentKeeper(12-18)MarketKeeper(20-29)x/market/hooks/hooks.go (1)
New(23-28)
x/market/hooks/external.go (1)
x/deployment/query/types.go (1)
Group(71-71)
🪛 golangci-lint (2.5.0)
x/market/hooks/hooks_test.go
[high] 125-125: G115: integer overflow conversion int -> uint32
(gosec)
🔇 Additional comments (7)
x/market/hooks/external.go (1)
17-17: LGTM! Clean interface additions.The new
OnPauseGroupandOnGroupPausedhooks follow the established pattern and naming convention, providing a clear separation between pause and close semantics.Also applies to: 25-25
x/market/hooks/hooks.go (2)
45-53: LGTM! Clear state mapping logic.The switch statement correctly maps escrow states to group lifecycle states: overdrawn accounts pause groups while other states trigger full closure. The conditional deployment closure (line 52) ensures deployments remain active when groups are only paused.
55-68: LGTM! State-driven group lifecycle handling.The refactored loop correctly separates pause and close semantics:
- Paused groups: validated via
ValidatePausable, thenOnPauseGroup+OnGroupPaused- Closed groups: validated via
ValidateClosable, thenOnCloseGroup+OnGroupClosedThis addresses the previous concern about calling
OnGroupClosedfor paused groups—the appropriate hooks are now invoked for each state.x/market/keeper/keeper.go (3)
32-32: LGTM! Interface extension for pause semantics.The
OnGroupPausedmethod completes the pause/close separation at the keeper interface level.
350-363: Excellent refactoring that addresses previous review concerns.The separation of
OnGroupClosedandOnGroupPausedwith distinctLeaseClosedReasonvalues addresses the semantic inconsistency flagged in earlier reviews:
- Paused groups →
LeaseClosedReasonInsufficientFunds- Closed groups →
LeaseClosedReasonOwnerThe documentation (lines 358-361) clearly explains the current behavior and acknowledges future extensibility considerations.
366-415: LGTM! Clean consolidation of resource-closing logic.The
closeMarketResourcesForGrouphelper effectively centralizes order/bid/lease closure with a parameterized reason (line 374), enabling bothOnGroupClosedandOnGroupPausedto share the same workflow while maintaining correct semantic tracking.x/market/hooks/hooks_test.go (1)
52-106: LGTM! Comprehensive test coverage for the main scenarios.The parameterized test clearly validates both overdrawn and good-standing account closures, asserting the correct deployment and group states for each case. The test structure is clean and readable.
rick-a-lane-ii
force-pushed
the
rick/support-issue-368
branch
2 times, most recently
from
9390941 to
0963adc
Compare
rick-a-lane-ii
force-pushed
the
rick/support-issue-368
branch
from
0963adc to
7ff090b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
x/market/hooks/hooks_test.go (1)
187-187: Optional: Make the safe integer conversion explicit.The conversion
uint32(i)+1is flagged by static analysis becauseihas typeint. In practice, this is safe sincegroupCount=3meansi ∈ {0,1,2}, but making the range explicit would silence the warning.Apply this diff to make the conversion explicit:
- for i := range groups { - groups[i] = testutil.DeploymentGroup(t, did, uint32(i)+1) + for i := 0; i < groupCount; i++ { + groups[i] = testutil.DeploymentGroup(t, did, uint32(i+1)) groups[i].State = ti.groupState }This keeps the index within known bounds and makes the safe conversion clear to both static analyzers and code reviewers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
x/market/hooks/external.go(1 hunks)x/market/hooks/hooks.go(1 hunks)x/market/hooks/hooks_test.go(1 hunks)x/market/keeper/keeper.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
x/market/hooks/external.go (1)
x/deployment/query/types.go (1)
Group(71-71)
x/market/keeper/keeper.go (1)
x/escrow/keeper/keeper.go (1)
Keeper(30-50)
x/market/hooks/hooks_test.go (3)
x/market/keeper/keeper.go (2)
Keeper(49-56)IKeeper(18-46)testutil/state/suite.go (1)
SetupTestSuite(63-65)x/market/hooks/external.go (2)
DeploymentKeeper(12-18)MarketKeeper(20-29)
🪛 golangci-lint (2.5.0)
x/market/hooks/hooks_test.go
[high] 187-187: G115: integer overflow conversion int -> uint32
(gosec)
🔇 Additional comments (7)
x/market/hooks/external.go (1)
17-17: LGTM: Well-designed interface additions for pause lifecycle.The new
OnPauseGroupandOnGroupPausedmethods cleanly extend the existing lifecycle hooks to support pausing groups separately from closing them. The method signatures are consistent with the existingOnCloseGroupandOnGroupClosedpatterns, ensuring a coherent interface design.Also applies to: 25-25
x/market/hooks/hooks.go (2)
45-53: LGTM: Correct state-driven deployment handling.The logic correctly distinguishes between overdrawn accounts (which should pause the deployment) and other closure scenarios (which should close the deployment). The separation ensures that deployments remain recoverable when accounts are merely overdrawn rather than permanently closed.
55-68: LGTM: Well-structured per-group state transitions.The refactored group processing correctly:
- Validates group state before transitions (pausable vs. closable)
- Routes to appropriate hooks based on target state
- Maintains separation between pause and close semantics
This addresses the semantic inconsistency flagged in earlier reviews by invoking
OnGroupPausedfor paused groups andOnGroupClosedonly for closed groups.x/market/hooks/hooks_test.go (1)
58-168: LGTM: Comprehensive parameterized test coverage.The test suite thoroughly validates the escrow account closure behavior across multiple scenarios:
- Overdrawn vs. closed accounts
- Active vs. closed deployments
- Invalid account states
The table-driven approach makes it easy to verify all state combinations and ensures the pause vs. close logic works correctly in different contexts.
x/market/keeper/keeper.go (3)
32-32: LGTM: Interface addition completes the pause lifecycle.The
OnGroupPausedmethod properly extends theIKeeperinterface to handle paused groups, complementing the existingOnGroupClosedmethod.
350-363: LGTM: Clean separation of close and pause semantics.The refactoring correctly:
- Delegates to a shared helper that eliminates duplication
- Uses
LeaseClosedReasonOwnerfor group closures (deployment owner initiated)- Uses
LeaseClosedReasonInsufficientFundsfor group pauses (account overdrawn)The inline comments and TODO appropriately document the current hardcoded approach and flag potential future enhancements if multiple pause reasons are needed. This is a pragmatic solution that addresses the semantic inconsistency raised in earlier reviews.
365-415: LGTM: Well-refactored resource cleanup with proper parameterization.The
closeMarketResourcesForGrouphelper successfully consolidates the resource-closing logic and parameterizes it byLeaseClosedReason. This ensures that leases closed due to pausing are correctly tagged withLeaseClosedReasonInsufficientFunds, while leases closed due to normal group closure useLeaseClosedReasonOwner.Line 374 correctly applies the parameterized reason when closing leases, which was a key concern in earlier reviews about accurate state tracking.
1 participant
Description
Refactors the behavior of deployments and deployment groups when accounts are overdrawn.
Author Checklist
!to the type prefix if API or client breaking changeCHANGELOG.md