Check Leverage on Withdrawals and Updates#3153
Conversation
WalkthroughThis PR introduces leverage management capabilities to subaccounts with margin requirement validation. It adds methods to get, set, and update leverage per perpetual, refactors margin calculations to be leverage-aware, validates that leverage updates do not violate margin requirements, and includes end-to-end tests covering withdrawal and leverage update scenarios. Changes
Sequence DiagramsequenceDiagram
participant User
participant Keeper as Subaccounts Keeper
participant Store as State Store
participant Calc as Margin Calculator
User->>Keeper: UpdateLeverage(subaccountId, leverageMap)
Keeper->>Store: GetLeverage(subaccountId)
Store-->>Keeper: currentLeverage
Keeper->>Keeper: Merge new leverage into current
Keeper->>Keeper: checkNewLeverageAgainstMarginRequirements()
rect rgb(240, 248, 255)
note over Keeper,Calc: Validation Phase
loop For each perpetual in leverageMap
Keeper->>Calc: GetNetCollateralAndMarginRequirementsWithLeverage(emptyUpdate, mergedLeverage)
Calc-->>Keeper: margin.Risk
alt Initial Collateralization < 0
Keeper-->>User: ErrLeverageViolatesMarginRequirements
end
end
end
rect rgb(144, 238, 144)
note over Keeper,Store: Persistence Phase
Keeper->>Store: SetLeverage(subaccountId, mergedLeverage)
Store-->>Keeper: OK
Keeper-->>User: Success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes involve new leverage management APIs and refactoring of margin calculations across multiple files. While the logic is conceptually straightforward (validation before state update, wrapper pattern for backward compatibility), the changes require careful review of margin requirement checks, leverage-aware calculation flows, and test coverage for leverage constraints. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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 |
8e868f2 to
8cbdff9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
protocol/x/clob/keeper/leverage_e2e_test.go (1)
291-293: Critical: Fix assertion to expect IMF in ppm, not leverage multiplier.Line 293 expects
uint32(5)butGetLeveragereturns the Initial Margin Fraction in parts-per-million, not the leverage multiplier. For 5x leverage (CustomImfPpm: 200_000), you should expectuint32(200_000).This issue was flagged in previous reviews and remains unfixed.
Apply this diff:
// Verify leverage was set immediately (ante handler sets it) aliceLeverageMap, found := tApp.App.SubaccountsKeeper.GetLeverage(ctx, &constants.Alice_Num0) require.True(t, found, "Alice's leverage should be set") - require.Equal(t, uint32(5), aliceLeverageMap[0], "Alice's leverage for perpetual 0 should be 5") + require.Equal(t, uint32(200_000), aliceLeverageMap[0], "Alice's CustomImfPpm for perpetual 0 should be 200_000 (5x leverage)")
🧹 Nitpick comments (1)
protocol/x/clob/keeper/leverage_e2e_test.go (1)
450-450: Clarify or remove arbitrary gas limit multiplication.Line 450 multiplies the gas limit by 10 (
constants.TestGasLimit * 10) without explanation. If this is necessary for Alice's withdrawal to fail correctly, add a comment explaining why; otherwise, use the standardconstants.TestGasLimitas done for Bob's withdrawal.Apply this diff if the multiplication is unnecessary:
testapp.MustMakeCheckTxOptions{ AccAddressForSigning: constants.Alice_Num0.Owner, - Gas: constants.TestGasLimit * 10, + Gas: constants.TestGasLimit, FeeAmt: constants.TestFeeCoins_5Cents, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
protocol/x/clob/keeper/leverage_e2e_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-06T20:00:48.549Z
Learnt from: anmolagrawal345
PR: dydxprotocol/v4-chain#3079
File: protocol/x/clob/types/leverage.go:10-56
Timestamp: 2025-10-06T20:00:48.549Z
Learning: In the leverage update flow for protocol/x/clob, the `ValidateUpdateLeverageMsg` function performs basic validation (non-nil checks, non-zero leverage, unique IDs, valid clob pair existence), while maximum leverage validation against `GetMaxLeverageForPerpetual` is intentionally deferred to the `UpdateLeverage` keeper method.
Applied to files:
protocol/x/clob/keeper/leverage_e2e_test.go
🧬 Code graph analysis (1)
protocol/x/clob/keeper/leverage_e2e_test.go (5)
protocol/testutil/app/app.go (5)
NewTestAppBuilder(335-350)MustMakeCheckTxsWithClobMsg(1352-1393)AdvanceToBlockOptions(125-152)MustMakeCheckTxsWithSdkMsg(1397-1408)MustMakeCheckTxOptions(100-109)protocol/x/clob/types/expected_keepers.go (1)
SubaccountsKeeper(20-128)protocol/x/perpetuals/lib/lib.go (1)
GetNetCollateralAndMarginRequirements(71-90)protocol/testutil/constants/addresses.go (2)
BobAccAddress(11-11)AliceAccAddress(10-10)protocol/testutil/constants/assets.go (3)
Usdc(49-57)TestGasLimit(19-19)TestFeeCoins_5Cents(26-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: build
- GitHub Check: benchmark
- GitHub Check: golangci-lint
- GitHub Check: liveness-test
- GitHub Check: test-coverage-upload
- GitHub Check: test-race
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: build-and-push-testnet
- GitHub Check: check-sample-pregenesis-up-to-date
- GitHub Check: build-and-push-mainnet
- GitHub Check: container-tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (2)
protocol/x/clob/keeper/leverage_e2e_test.go (2)
460-543: LGTM! Test correctly validates leverage update rejection with existing positions.This test properly verifies that updating leverage to a more restrictive configuration (2x leverage = 500_000 ppm IMF) fails when the subaccount already holds a position that would violate the new Initial Margin Requirements. The test flow is clear:
- Places orders for Bob and Alice (no leverage configured yet)
- Advances block to match orders and create positions
- Attempts to update Alice's leverage to a more conservative setting
- Correctly asserts that the leverage update fails via CheckTx
269-458: Approve test logic: effectively validates leverage impact on withdrawals.The
TestWithdrawalWithLeveragetest provides valuable coverage by:
- Configuring leverage for Alice (5x = 200_000 ppm IMF) via CheckTx/ante handler
- Creating identical positions for Alice and Bob (Bob uses default 20x leverage)
- Computing per-subaccount risk (NC, IMR) and verifying Alice's IMR is higher
- Demonstrating that Bob can withdraw more collateral than Alice due to lower margin requirements
This correctly validates that the leverage-aware risk calculation restricts withdrawals appropriately.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
protocol/x/clob/keeper/leverage_e2e_test.go (4)
407-409: Consider making the leverage comparison more explicit.The comment correctly states Alice has lower leverage than Bob, but it could be clearer by mentioning Bob's default leverage value from the liquidity tier (20x).
- // Alice's IMR should be higher due to lower leverage (more conservative) + // Alice's IMR should be higher due to lower leverage (5x vs Bob's default 20x from liquidity tier)
447-447: Consider clarifying the test intent.The comment could be more explicit that this withdrawal amount is intentionally chosen to test a failure case for Alice.
- Quantums: bobAvailable.Uint64(), // using bob's available collateral which is greater than alice's + Quantums: bobAvailable.Uint64(), // Intentionally using Bob's higher available amount to trigger Alice's withdrawal failure
455-455: Inconsistent gas limit for failed withdrawal.Alice's withdrawal uses
TestGasLimit * 10while Bob's usesTestGasLimit. Since Alice's withdrawal is expected to fail validation, it shouldn't require significantly more gas.testapp.MustMakeCheckTxOptions{ AccAddressForSigning: constants.Alice_Num0.Owner, - Gas: constants.TestGasLimit * 10, + Gas: constants.TestGasLimit, FeeAmt: constants.TestFeeCoins_5Cents, },
529-530: Consider explaining why the leverage update should fail.The comment states the update should fail but doesn't explain the mechanism. Adding clarity about the margin requirement increase would help future readers understand the test intent.
- // Configure leverage for Alice with an existing bitcoin position - // This should make the account fail against the IMR check + // Configure leverage for Alice with an existing bitcoin position + // Updating to 2x leverage (from default ~20x) increases IMR significantly, + // so the existing position fails the margin requirement check
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
protocol/x/clob/keeper/leverage_e2e_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-06T20:00:48.549Z
Learnt from: anmolagrawal345
PR: dydxprotocol/v4-chain#3079
File: protocol/x/clob/types/leverage.go:10-56
Timestamp: 2025-10-06T20:00:48.549Z
Learning: In the leverage update flow for protocol/x/clob, the `ValidateUpdateLeverageMsg` function performs basic validation (non-nil checks, non-zero leverage, unique IDs, valid clob pair existence), while maximum leverage validation against `GetMaxLeverageForPerpetual` is intentionally deferred to the `UpdateLeverage` keeper method.
Applied to files:
protocol/x/clob/keeper/leverage_e2e_test.go
🧬 Code graph analysis (1)
protocol/x/clob/keeper/leverage_e2e_test.go (6)
protocol/testutil/app/app.go (5)
NewTestAppBuilder(335-350)MustMakeCheckTxsWithClobMsg(1352-1393)AdvanceToBlockOptions(125-152)MustMakeCheckTxsWithSdkMsg(1397-1408)MustMakeCheckTxOptions(100-109)protocol/x/clob/types/expected_keepers.go (1)
SubaccountsKeeper(20-128)protocol/x/perpetuals/lib/lib.go (1)
GetNetCollateralAndMarginRequirements(71-90)protocol/x/subaccounts/types/update.go (1)
Update(79-86)protocol/testutil/constants/addresses.go (2)
BobAccAddress(11-11)AliceAccAddress(10-10)protocol/testutil/constants/assets.go (3)
Usdc(49-57)TestGasLimit(19-19)TestFeeCoins_5Cents(26-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: container-tests
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: benchmark
- GitHub Check: liveness-test
- GitHub Check: test-race
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: test-coverage-upload
- GitHub Check: build-and-push-mainnet
- GitHub Check: build
- GitHub Check: golangci-lint
- GitHub Check: check-sample-pregenesis-up-to-date
- GitHub Check: build-and-push-testnet
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Summary
🔇 Additional comments (2)
protocol/x/clob/keeper/leverage_e2e_test.go (2)
1-61: LGTM! Helper functions are well-structured.The addition of
sendingtypesimport and the helper functions are appropriate for the new withdrawal and leverage update tests.
63-267: LGTM! Existing tests look solid.The leverage configuration tests properly use
CustomImfPpmwith correct values, and the test logic effectively validates leverage behavior on order placement.
|
https://github.com/Mergifyio backport release/protocol/v9.x |
✅ Backports have been createdDetails
|
(cherry picked from commit 70ac841) # Conflicts: # protocol/x/clob/keeper/leverage_e2e_test.go # protocol/x/subaccounts/keeper/leverage.go # protocol/x/subaccounts/types/errors.go
Changelist
Now that we use leverage in our margining calculations - we should also ensure the same collateral checks are being applied in other flows. The 2 main ones here are validating that withdrawals and incoming leverage updates respect this.
For withdrawals, we get the check for free since it lives in the core risk calculation. The test creates 2 identical positions in 2 identical subaccounts, except one has a lower leverage set, and ensure that the withdrawable balance is lower than one without leverage set.
For incoming leverage updates, we take the new configuration and run it against the same risk checks as order placement, but with empty position updates. This will use the new leverage map to check if the position passes initial collateralization checks. If it does, we update the leverage map with the new configuration.
Test Plan
Added tests in leverage_e2e_test to test both these cases.
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
Bug Fixes
Tests