feat: add liquidation margin collateral planner - #67
Conversation
Adds an execution-service helper that supplies margin collateral before liquidating and avoids duplicate collateral-enable planning for the liquidator sub-account.
LeonardEulerXYZ
left a comment
There was a problem hiding this comment.
Leonard Review — Changes requested
Reviewed head ee850dc2c04b9e27a6b9cd908861b43f87d7b085.
Walkthrough
This PR adds planLiquidationWithMarginCollateral, exports the new argument type, and adds execution-service tests around zero-margin fallback, approval/deposit ordering, wrapped-native handling, and duplicate collateral-enable suppression.
Verdict
Requesting changes for one correctness/API-safety issue in the exported planner. The helper currently suppresses the seized-collateral enable whenever the liquidator sub-account asks about args.collateral, even though the preceding deposit only enabled marginCollateral.vault. If those vaults differ, the generated liquidation batch can omit the required enable for the seized collateral vault.
Validation performed
git diff origin/main...HEAD --statand full changed-file review.- Prior comment lifecycle scan: inspected issue comments, formal reviews, and review comments; no prior Leonard comments or active bot/reviewer feedback found.
- Security/supply-chain sweep over the whole diff: no package/lock/workflow/script/dependency changes and no suspicious diff patterns found.
pnpm --filter @eulerxyz/euler-v2-sdk test -- test/executionService.test.ts→ pass (26files /347tests; repo script currently runs the package suite despite the path arg).pnpm --filter @eulerxyz/euler-v2-sdk typecheck→ pass.pnpm --filter @eulerxyz/euler-v2-sdk build→ pass.- Focused invariant probe for distinct margin-vault vs seized-collateral-vault liquidation planner → fails on this head: expected a second
enableCollateralfor the seized collateral vault, actual batch omits it. Temporary probe was removed afterward; worktree is clean.
Scalability / maintainability hygiene pass
I checked sibling execution planners and downstream Lite-style transaction call sites for similar readiness-gate / transaction-path patterns. The new helper is a narrow wrapper around existing planDeposit + planLiquidation, which is the right reusable shape. The missing piece is the abstraction boundary: the wrapper should model exactly what the preceding deposit made true (marginCollateral.vault enabled for the liquidator sub-account), not a broader assumption about the seized collateral vault. Add a focused regression test for the distinct-vault case, or enforce/document same-vault-only semantics at the API boundary if that is truly the intended invariant.
Screenshots: not applicable; SDK transaction-construction change, no user-visible UI captured.
comment lifecycle: none found / no cleanup needed.
| ) => { | ||
| if ( | ||
| getAddress(subAccount) === getAddress(liquidatorSubAccountAddress) && | ||
| getAddress(vault) === getAddress(collateral) |
There was a problem hiding this comment.
This wrapper makes the liquidation account report the seized collateral vault as enabled, but the preceding deposit only enabled marginCollateral.vault. When those two addresses differ, planLiquidation skips the enableCollateral call for the seized collateral vault, so the generated batch can be missing the collateral enable needed for the liquidation output. I reproduced it with a focused probe using distinct margin/seized vaults: the batch is [enableCollateral(margin), deposit, enableController, liquidate] instead of also appending enableCollateral(seized). Either compare this override against marginCollateral.vault (so only the actually-enabled vault is faked as enabled), or enforce a same-vault invariant in the exported API with a runtime guard + test.
Summary
Tests