Check Leverage On Order Placement#3141
Conversation
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
WalkthroughLeverage ownership moved from the CLOB keeper into the Subaccounts module; per-subaccount leverage persisted and threaded as custom_imf_ppm through risk/margin APIs, protos/TS types updated, CLOB exposes SubaccountsKeeper, mocks/CLI/GRPC/tests/e2e adapted. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Ante as CLOB AnteHandler
participant CLOB as CLOB Keeper
participant SA as Subaccounts Keeper
participant Perp as Perpetuals Keeper
rect rgba(220,240,255,0.35)
Note over User,SA: MsgUpdateLeverage delegation
User->>Ante: MsgUpdateLeverage {SubaccountId, leverageMap}
Ante->>CLOB: GetSubaccountsKeeper()
CLOB-->>Ante: SubaccountsKeeper
Ante->>SA: UpdateLeverage(SubaccountId, leverageMap)
SA->>Perp: GetPerpetualAndMarketPriceAndLiquidityTier(perpId)
Perp-->>SA: {perpetual, liquidityTier}
SA-->>Ante: ok / error
end
sequenceDiagram
autonumber
participant Keeper as Subaccounts Keeper
participant Lib as Subaccounts Lib
participant PerpLib as Perpetuals Lib
rect rgba(235,255,235,0.35)
Note over Keeper,PerpLib: Leverage-aware risk calculation
Keeper->>Keeper: getSettledUpdates(... fetch LeverageMap)
Keeper->>Lib: GetRiskForSettledUpdate(settledUpdate, perpInfos)
Lib->>PerpLib: GetNetCollateralAndMarginRequirements(..., custom_imf_ppm)
PerpLib-->>Lib: Margin & risk
Lib-->>Keeper: Risk
Keeper-->>Keeper: accept/reject updates
end
sequenceDiagram
autonumber
participant Liquid as Liquidations
participant PerpLib as Perpetuals Lib
rect rgba(255,245,220,0.35)
Note over Liquid,PerpLib: Liquidation pricing using default IMF
Liquid->>PerpLib: GetPositionNetNotionalValueAndMarginRequirements(..., custom_imf_ppm=0)
PerpLib-->>Liquid: Risk (default IMR/MMR)
Liquid-->>Liquid: Compute bankruptcy / fillable price
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
![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
🧹 Nitpick comments (3)
protocol/x/clob/keeper/leverage.go (1)
8-30: Consider removing deprecated delegation wrappers.Since all leverage operations now delegate directly to
SubaccountsKeeper, these wrapper methods inClobKeeperadd unnecessary indirection. The deprecation comments and TODOs suggest they should be unused or migrated.Consider:
- Audit all call sites in the clob module to use
GetSubaccountsKeeper()directly- Remove these wrapper methods to eliminate the technical debt
Example migration:
-k.UpdateLeverage(ctx, subaccountId, leverageMap) +k.GetSubaccountsKeeper().UpdateLeverage(ctx, subaccountId, leverageMap)protocol/x/subaccounts/keeper/subaccount.go (1)
308-385: Verify collateral sufficiency before allocation.The
applyLeverageAwareCollateralAllocationmethod moves collateral from the main USDC balance to perpetual positions based on leverage-scaled IMR. However, there's no explicit check that the subaccount has sufficient USDC balance to satisfy the allocation before modifying the updates.Scenario: If a subaccount opens a leveraged position but has insufficient USDC, the allocation could drive the USDC balance negative (lines 365-367 or 373), which may or may not be caught by downstream collateral checks depending on the overall risk calculation.
Question: Is the collateral sufficiency guaranteed by the risk checks in
internalCanUpdateSubaccountsWithLeverage(line 766), which callsGetRiskForSettledUpdateon the modified settled update? If so, a comment explaining this would be helpful.Consider adding a comment like:
// Note: Collateral sufficiency is validated by downstream risk checks in // internalCanUpdateSubaccountsWithLeverage, which will reject the update if // the allocated collateral exceeds available USDC balance.Or, if validation should occur here, add an explicit check before allocation.
protocol/x/subaccounts/keeper/leverage.go (1)
37-52: Document panic behavior in GetLeverage/SetLeverage. Add doc comments noting these methods intentionally panic on JSON (un)marshal errors to clarify crash-on-corruption behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 8eb3563 and 7d6eac2cb4082d00f169dae293e8ce06348667d1.
📒 Files selected for processing (20)
protocol/daemons/liquidation/client/sub_task_runner.go(1 hunks)protocol/mocks/ClobKeeper.go(1 hunks)protocol/testutil/app/app.go(2 hunks)protocol/testutil/keeper/listing.go(2 hunks)protocol/x/clob/ante/clob.go(1 hunks)protocol/x/clob/keeper/keeper.go(1 hunks)protocol/x/clob/keeper/leverage.go(1 hunks)protocol/x/clob/keeper/leverage_e2e_test.go(1 hunks)protocol/x/clob/keeper/liquidations.go(2 hunks)protocol/x/clob/types/clob_keeper.go(1 hunks)protocol/x/clob/types/expected_keepers.go(1 hunks)protocol/x/perpetuals/lib/lib.go(6 hunks)protocol/x/perpetuals/lib/lib_test.go(4 hunks)protocol/x/subaccounts/keeper/leverage.go(1 hunks)protocol/x/subaccounts/keeper/subaccount.go(8 hunks)protocol/x/subaccounts/lib/updates.go(2 hunks)protocol/x/subaccounts/lib/updates_test.go(2 hunks)protocol/x/subaccounts/types/errors.go(1 hunks)protocol/x/subaccounts/types/keys.go(1 hunks)protocol/x/subaccounts/types/settled_update.go(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-11-15T16:00:11.304Z
Learnt from: hwray
PR: dydxprotocol/v4-chain#2551
File: protocol/x/subaccounts/keeper/subaccount.go:852-865
Timestamp: 2024-11-15T16:00:11.304Z
Learning: The function `GetCrossInsuranceFundBalance` in `protocol/x/subaccounts/keeper/subaccount.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.
Applied to files:
protocol/mocks/ClobKeeper.goprotocol/x/subaccounts/lib/updates_test.goprotocol/x/clob/types/clob_keeper.goprotocol/daemons/liquidation/client/sub_task_runner.goprotocol/x/subaccounts/keeper/subaccount.goprotocol/testutil/keeper/listing.goprotocol/x/subaccounts/keeper/leverage.goprotocol/x/clob/keeper/keeper.go
📚 Learning: 2024-11-15T15:59:28.095Z
Learnt from: hwray
PR: dydxprotocol/v4-chain#2551
File: protocol/x/subaccounts/keeper/subaccount.go:833-850
Timestamp: 2024-11-15T15:59:28.095Z
Learning: The function `GetInsuranceFundBalance` in `protocol/x/subaccounts/keeper/subaccount.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.
Applied to files:
protocol/x/subaccounts/lib/updates_test.goprotocol/x/clob/types/clob_keeper.goprotocol/daemons/liquidation/client/sub_task_runner.goprotocol/testutil/keeper/listing.goprotocol/x/clob/keeper/keeper.go
📚 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.goprotocol/x/perpetuals/lib/lib.goprotocol/x/clob/keeper/leverage_e2e_test.goprotocol/x/clob/types/expected_keepers.goprotocol/testutil/app/app.goprotocol/x/subaccounts/keeper/subaccount.goprotocol/x/clob/ante/clob.goprotocol/x/subaccounts/keeper/leverage.go
🧬 Code graph analysis (14)
protocol/mocks/ClobKeeper.go (3)
protocol/x/clob/types/clob_keeper.go (1)
ClobKeeper(13-165)protocol/x/subaccounts/types/types.go (1)
SubaccountsKeeper(11-80)protocol/mocks/SubaccountsKeeper.go (1)
SubaccountsKeeper(19-21)
protocol/x/perpetuals/lib/lib_test.go (1)
protocol/x/perpetuals/lib/lib.go (1)
GetNetCollateralAndMarginRequirements(71-90)
protocol/x/subaccounts/lib/updates_test.go (1)
protocol/x/subaccounts/lib/updates.go (1)
GetRiskForSubaccount(343-388)
protocol/x/subaccounts/types/errors.go (1)
protocol/x/subaccounts/types/keys.go (1)
ModuleName(6-6)
protocol/x/clob/types/clob_keeper.go (2)
protocol/x/clob/types/expected_keepers.go (1)
SubaccountsKeeper(20-128)protocol/x/subaccounts/types/types.go (1)
SubaccountsKeeper(11-80)
protocol/x/clob/keeper/leverage.go (1)
protocol/x/clob/keeper/keeper.go (1)
Keeper(29-71)
protocol/x/clob/keeper/leverage_e2e_test.go (9)
protocol/testutil/app/app.go (2)
TestApp(600-622)MustMakeCheckTxsWithClobMsg(1352-1393)protocol/x/clob/types/clob_keeper.go (1)
ClobKeeper(13-165)protocol/x/assets/types/genesis.go (1)
AssetUsdc(12-19)protocol/x/clob/types/expected_keepers.go (1)
SubaccountsKeeper(20-128)protocol/x/subaccounts/types/update.go (3)
Update(79-86)AssetUpdate(88-93)PerpetualUpdate(102-110)protocol/x/clob/types/order_id.go (1)
OrderIdFlags_LongTerm(15-15)protocol/x/clob/types/order.pb.go (1)
Order_SIDE_BUY(38-38)protocol/x/clob/types/orderbook.go (1)
Subticks(11-11)protocol/x/clob/types/message_place_order.go (1)
NewMsgPlaceOrder(33-37)
protocol/testutil/app/app.go (2)
indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/tx.ts (2)
MsgUpdateLeverage(408-414)MsgUpdateLeverage(1450-1496)protocol/x/clob/types/tx.pb.go (3)
MsgUpdateLeverage(1151-1156)MsgUpdateLeverage(1160-1160)MsgUpdateLeverage(1161-1163)
protocol/x/subaccounts/keeper/subaccount.go (4)
protocol/x/subaccounts/types/settled_update.go (1)
SettledUpdate(7-17)protocol/x/subaccounts/lib/updates.go (4)
GetSettledSubaccountWithPerpetuals(20-82)CalculateLeverageAwareCollateralRequirement(408-428)GetRiskForSettledUpdate(393-402)GetRiskForSubaccount(343-388)protocol/x/subaccounts/types/update.go (2)
AssetUpdate(88-93)PerpetualUpdate(102-110)protocol/x/assets/types/genesis.go (1)
AssetUsdc(12-19)
protocol/x/clob/ante/clob.go (1)
protocol/x/clob/types/leverage.go (1)
ValidateAndConstructPerpetualLeverageMap(58-75)
protocol/x/subaccounts/lib/updates.go (5)
protocol/x/perpetuals/lib/lib.go (2)
GetNetCollateralAndMarginRequirements(71-90)GetMarginRequirementsInQuoteQuantums(117-181)protocol/x/subaccounts/types/settled_update.go (1)
SettledUpdate(7-17)protocol/x/perpetuals/types/perpinfo.go (2)
PerpInfos(16-16)PerpInfo(9-13)protocol/lib/margin/risk.go (1)
Risk(9-13)protocol/x/subaccounts/types/update.go (1)
PerpetualUpdate(102-110)
protocol/x/clob/keeper/liquidations.go (1)
protocol/x/perpetuals/lib/lib.go (1)
GetPositionNetNotionalValueAndMarginRequirements(41-67)
protocol/x/subaccounts/keeper/leverage.go (3)
protocol/x/clob/keeper/keeper.go (1)
Keeper(29-71)protocol/x/subaccounts/types/keys.go (1)
LeverageKeyPrefix(30-30)protocol/x/subaccounts/types/errors.go (3)
ErrInvalidLeverage(83-83)ErrLeverageExceedsMaximum(84-84)ErrInitialMarginPpmIsZero(85-85)
protocol/x/clob/keeper/keeper.go (2)
protocol/x/clob/types/expected_keepers.go (1)
SubaccountsKeeper(20-128)protocol/x/subaccounts/types/types.go (1)
SubaccountsKeeper(11-80)
⏰ 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). (29)
- GitHub Check: liveness-test
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: test-coverage-upload
- GitHub Check: test-race
- 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-bazooka-lambda / (bazooka) Build and Push Lambda
- 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-vulcan / (vulcan) 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-ender / (ender) 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-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-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-comlink / (comlink) 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: check-sample-pregenesis-up-to-date
- GitHub Check: exchange-tests
- GitHub Check: benchmark
- GitHub Check: build
- GitHub Check: build-and-push-testnet
- GitHub Check: golangci-lint
- GitHub Check: container-tests
- GitHub Check: build-and-push-mainnet
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (33)
protocol/daemons/liquidation/client/sub_task_runner.go (1)
341-345: LGTM!Correctly passes
nilfor the leverage parameter in liquidation calculations. The comment clearly explains the rationale: leverage only affects initial margin requirements (IMR) during order placement, while liquidation checks use maintenance margin requirements (MMR) which remain unchanged by leverage.protocol/x/clob/types/clob_keeper.go (1)
164-164: LGTM!Clean interface addition that exposes the SubaccountsKeeper to enable delegation of leverage-related operations. The method signature follows existing conventions.
protocol/x/clob/keeper/keeper.go (1)
167-169: LGTM!Straightforward implementation of the GetSubaccountsKeeper accessor declared in the ClobKeeper interface.
protocol/x/subaccounts/types/keys.go (1)
29-30: LGTM!Clean addition of a store key prefix constant for leverage data storage. The constant naming and placement follow existing conventions in the file.
protocol/testutil/keeper/listing.go (1)
144-177: LGTM!Clean refactoring that reflects the architectural shift where leverage is now managed directly by the subaccounts keeper rather than a separate leverage keeper. The initialization order change (creating subaccounts keeper before clob keeper) is appropriate, and the comments clearly document the reasoning.
protocol/x/clob/ante/clob.go (1)
196-216: LGTM!Clean delegation pattern where the clob ante handler validates the UpdateLeverage message and constructs the perpetual leverage map, then delegates the actual storage and validation to the SubaccountsKeeper. This improves separation of concerns while maintaining consistent error handling.
protocol/x/perpetuals/lib/lib_test.go (5)
207-213: LGTM!Tests correctly updated to pass
0for the new leverage parameter, indicating default margin requirements should be used. This is appropriate for tests that aren't specifically testing leverage functionality.
214-221: LGTM!Consistent with the pattern above - passing
0for leverage to use default margin requirements.
314-321: LGTM!Benchmark updated to pass
0for the leverage parameter, maintaining default behavior.
400-409: LGTM!Test correctly updated with the
0leverage argument.
683-689: LGTM!Final test case updated consistently with the new function signature.
protocol/x/subaccounts/types/settled_update.go (1)
14-16: LGTM!Clean addition of the LeverageMap field to carry per-perpetual leverage configuration through the settled update flow. The documentation clearly explains the nil semantics (use default margin requirements when not configured).
protocol/x/clob/keeper/liquidations.go (2)
454-467: LGTM: Correct leverage handling for liquidations.Passing
0for leverage in liquidation calculations is correct, as liquidations should always use the default margin requirements from the liquidity tier rather than user-configured leverage. The inline comments clearly document this intent.
546-552: LGTM: Consistent leverage handling in fillable price calculation.The change correctly passes
0for leverage in the fillable price calculation, maintaining consistency with the bankruptcy price calculation and ensuring liquidations use default margin requirements.protocol/x/subaccounts/lib/updates_test.go (2)
163-163: LGTM: Correct adaptation to new function signature.The test correctly passes
nilfor the newleverageMapparameter, which means "use default margin requirements". This is appropriate for tests that are not specifically testing leverage functionality.
186-186: LGTM: Consistent test adaptation.The panic test case correctly includes the
nilleverage parameter, maintaining consistency with the other test cases.protocol/mocks/ClobKeeper.go (1)
509-527: LGTM: Standard mock implementation.The
GetSubaccountsKeepermock method follows the established mockery pattern used throughout this file, with proper return value validation and type handling.protocol/testutil/app/app.go (1)
1352-1377: LGTM: Consistent extension for new message type.The addition of
clobtypes.MsgUpdateLeveragesupport follows the established pattern used for other CLOB message types, with correct signer address extraction and message handling.protocol/x/subaccounts/types/errors.go (1)
82-85: LGTM: Well-defined leverage errors.The new leverage-related error variables follow the established patterns in this file, with appropriate error codes (700-799 range) and clear, concise error messages.
protocol/x/clob/keeper/leverage_e2e_test.go (2)
194-202: Verify leverage=1 configuration is intentional.The test configures Alice with
Leverage: 1(1x leverage), which represents the maximum restriction on leverage. With this configuration, Alice should indeed have more stringent collateral requirements than Bob (who uses default margin requirements).Please confirm this is the intended test behavior. If testing maximum restriction is the goal, consider adding a comment explaining that leverage=1 means 1x (no leverage), to make the test's intent clearer.
67-123: LGTM: Comprehensive keeper wiring test.The test effectively verifies that the SubaccountsKeeper leverage subsystem is properly wired by:
- Configuring leverage
- Verifying storage
- Testing leverage-aware operations (
CanUpdateSubaccounts)This provides good coverage of the integration points.
protocol/x/subaccounts/lib/updates.go (3)
343-388: LGTM: Robust leverage integration in risk calculation.The updated
GetRiskForSubaccountfunction correctly:
- Accepts an optional
leverageMapparameter- Defaults to
leverage = 0(use default margins) when not configured- Passes per-perpetual leverage to margin calculations
The nil-check on
leverageMap(line 372) prevents panics and ensures safe operation when leverage is not configured.
390-402: LGTM: Convenient helper for settled update risk calculation.The
GetRiskForSettledUpdatehelper appropriately:
- Calculates the updated subaccount state
- Extracts leverage from the
SettledUpdate- Delegates to
GetRiskForSubaccountThis provides a clean abstraction for the common pattern of calculating risk for settled updates with embedded leverage data.
404-428: LGTM: Clear collateral requirement calculation.The
CalculateLeverageAwareCollateralRequirementhelper correctly:
- Returns zero when leverage is not configured or position size is zero
- Uses absolute value for margin calculation (line 423)
- Calculates IMR based on the configured leverage
The early returns (lines 414-416) optimize for common cases and improve readability.
protocol/x/perpetuals/lib/lib.go (2)
41-67: LGTM!The leverage parameter is cleanly threaded through to the margin calculation functions.
71-90: LGTM!The comment clarifies the default behavior when leverage is 0.
protocol/x/clob/types/expected_keepers.go (1)
108-128: LGTM!The leverage management methods are cleanly added to the SubaccountsKeeper interface. The method signatures align with the implementations in
protocol/x/subaccounts/keeper/leverage.go.protocol/x/subaccounts/keeper/leverage.go (3)
54-97: LGTM!The validation logic correctly checks leverage against the maximum allowed value before merging and storing. The error wrapping provides clear context for debugging.
99-116: Good: Returns error instead of panicking.Unlike the panic in
protocol/x/perpetuals/lib/lib.goline 166, this function correctly returns an error whenInitialMarginPpm == 0. This is the safer approach for handling invalid configurations.
24-34: Keep panic here — consistent with repository conventionsThe codebase intentionally panics on marshal/unmarshal/invariant failures (see protocol/x/subaccounts/keeper/leverage.go and many other keeper/test files); returning an error would be inconsistent with existing patterns.
Likely an incorrect or invalid review comment.
protocol/x/subaccounts/keeper/subaccount.go (3)
253-302: Leverage caching and propagation looks correct.The implementation efficiently caches leverage data per subaccount (line 254, 279) and propagates it through
SettledUpdate.LeverageMap(line 290). The conditional application of leverage-aware collateral allocation (lines 294-296) ensures it only runs when leverage is configured.
468-473: LGTM: Leverage-aware validation path integrated.The changes correctly route both
UpdateSubaccountsandCanUpdateSubaccountsthrough the leverage-aware validation path (internalCanUpdateSubaccountsWithLeverage). The settled updates already contain the leverage data (viaLeverageMap), which is propagated to risk calculations.Also applies to: 616-617
765-789: Leverage correctly propagated to risk calculations.The leverage map is correctly fetched and passed to risk calculation functions (
GetRiskForSettledUpdateat line 766 andGetRiskForSubaccountat lines 785-788, 851-854). This ensures that margin requirements are computed with leverage awareness.Also applies to: 845-856
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
protocol/x/subaccounts/keeper/leverage.go (2)
65-96: Validation loop correctly ensures deterministic error handling.The sorted iteration (line 72) ensures all nodes validate perpetuals in the same order and return consistent errors. The validation logic correctly enforces that
custom_imf_ppm >= minImfPpm.Minor: The error message at line 81 says "failed to get max leverage" but the function is
GetMinImfForPerpetual. Consider: "failed to get min IMF for perpetual %d: %v".
98-112: Consider sorting the update loop for code consistency.While the unsorted iteration (lines 105-107) doesn't affect consensus safety (since
SetLeveragehandles deterministic storage), sorting here would maintain consistency with the validation loop above and improve code readability.Apply this diff for consistency:
// Update with new leverage values - for perpetualId, custom_imf_ppm := range perpetualLeverage { + for _, perpetualId := range sortedPerpIds { + custom_imf_ppm := perpetualLeverage[perpetualId] existingLeverage[perpetualId] = custom_imf_ppm }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
protocol/x/subaccounts/keeper/leverage.go(1 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/subaccounts/keeper/leverage.go
🧬 Code graph analysis (1)
protocol/x/subaccounts/keeper/leverage.go (6)
protocol/x/clob/keeper/keeper.go (1)
Keeper(29-71)protocol/x/subaccounts/types/keys.go (1)
LeverageKeyPrefix(30-30)protocol/x/subaccounts/types/leverage.pb.go (6)
PerpetualLeverageEntry(27-32)PerpetualLeverageEntry(36-36)PerpetualLeverageEntry(37-39)LeverageData(82-85)LeverageData(89-89)LeverageData(90-92)protocol/lib/collections.go (1)
GetSortedKeys(57-67)protocol/lib/sortable.go (1)
Sortable(9-9)protocol/x/subaccounts/types/errors.go (3)
ErrInvalidLeverage(83-83)ErrLeverageExceedsMaximum(84-84)ErrInitialMarginPpmIsZero(85-85)
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: Summary
🔇 Additional comments (3)
protocol/x/subaccounts/keeper/leverage.go (3)
11-19: LGTM: Standard storage helper functions.The prefix store pattern and key generation using
ToStateKey()follow Cosmos SDK conventions correctly.
21-42: LGTM: Deterministic storage implementation.The function correctly sorts perpetual IDs before building the entry slice, ensuring deterministic protobuf serialization across all nodes.
44-63: LGTM: Clean retrieval logic.The conversion from the deterministic stored slice back to a map is appropriate for read operations.
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
protocol/x/subaccounts/keeper/leverage.go (4)
16-19: Document/guard nil subaccountId.Add a brief precondition comment (or a nil check) to avoid a less-informative NPE if callers pass nil.
// leverageKey returns the store key to retrieve leverage data for a subaccount. func leverageKey(subaccountId *types.SubaccountId) []byte { + // Precondition: subaccountId must be non-nil. + // (Callers should validate upstream; this helps future-proof against panics.) + if subaccountId == nil { + panic("leverageKey: subaccountId is nil") + } return subaccountId.ToStateKey() }
26-34: Deterministic build is good; preallocate and (optionally) drop empty sets.
- Preallocate entries capacity to reduce allocs.
- If leverageMap is empty, delete the key to save storage and make GetLeverage() return exists=false.
func (k Keeper) SetLeverage(ctx sdk.Context, subaccountId *types.SubaccountId, leverageMap map[uint32]uint32) { store := k.getLeverageStore(ctx) key := leverageKey(subaccountId) - var entries []*types.PerpetualLeverageEntry + if len(leverageMap) == 0 { + store.Delete(key) + return + } + entries := make([]*types.PerpetualLeverageEntry, 0, len(leverageMap)) sortedPerpIds := lib.GetSortedKeys[lib.Sortable[uint32]](leverageMap) for _, perpetualId := range sortedPerpIds { customImfPpm := leverageMap[perpetualId] entries = append(entries, &types.PerpetualLeverageEntry{ PerpetualId: perpetualId, CustomImfPpm: customImfPpm, }) }
104-107: Optional: reuse the sorted keys for consistency.Not required for determinism (SetLeverage sorts), but iterating the same sorted slice improves readability and consistency.
-// Update with new leverage values -for perpetualId, custom_imf_ppm := range perpetualLeverage { - existingLeverage[perpetualId] = custom_imf_ppm -} +// Update with new leverage values (reuse deterministic order) +for _, perpetualId := range sortedPerpIds { + existingLeverage[perpetualId] = perpetualLeverage[perpetualId] +}
114-128: LGTM; consider a more explicit name.Implementation and docs are clear. If you want extra precision: GetMinImfPpmForPerpetual.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
protocol/x/subaccounts/keeper/leverage.go(1 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/subaccounts/keeper/leverage.go
🧬 Code graph analysis (1)
protocol/x/subaccounts/keeper/leverage.go (5)
protocol/x/subaccounts/types/keys.go (1)
LeverageKeyPrefix(30-30)protocol/x/subaccounts/types/leverage.pb.go (6)
PerpetualLeverageEntry(27-32)PerpetualLeverageEntry(36-36)PerpetualLeverageEntry(37-39)LeverageData(82-85)LeverageData(89-89)LeverageData(90-92)protocol/lib/collections.go (1)
GetSortedKeys(57-67)protocol/lib/sortable.go (1)
Sortable(9-9)protocol/x/subaccounts/types/errors.go (3)
ErrInvalidLeverage(83-83)ErrLeverageExceedsMaximum(84-84)ErrInitialMarginPpmIsZero(85-85)
⏰ 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). (39)
- GitHub Check: liveness-test
- GitHub Check: test-race
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: test-coverage-upload
- GitHub Check: container-tests
- GitHub Check: build
- 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-vulcan / (vulcan) 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-roundtable / (roundtable) 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: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: check-build-bazooka
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- 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-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- 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-vulcan / (vulcan) Build and Push
- GitHub Check: check-build-auxo
- GitHub Check: test / run_command
- GitHub Check: golangci-lint
- GitHub Check: exchange-tests
- GitHub Check: check-sample-pregenesis-up-to-date
- GitHub Check: build-and-push-testnet
- GitHub Check: benchmark
- GitHub Check: build-and-push-mainnet
- GitHub Check: run_command
- GitHub Check: lint
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (3)
protocol/x/subaccounts/keeper/leverage.go (3)
11-14: LGTM on namespaced store.Prefix store + LeverageKeyPrefix is correct and consistent with module patterns.
40-42: LGTM on protobuf marshal/store.Correct codec usage and write path.
49-63: LGTM on decode and map reconstruction.Clear exists=false contract when no key; map reconstruction is straightforward.
| // Sort the perpetual IDs to ensure deterministic ordering | ||
| sortedPerpIds := lib.GetSortedKeys[lib.Sortable[uint32]](perpetualLeverage) | ||
|
|
||
| // Validate leverage against maximum allowed leverage for each perpetual | ||
| for _, perpetualId := range sortedPerpIds { | ||
| custom_imf_ppm := perpetualLeverage[perpetualId] | ||
| minImfPpm, err := k.GetMinImfForPerpetual(ctx, perpetualId) | ||
| if err != nil { | ||
| return errorsmod.Wrapf( | ||
| types.ErrInvalidLeverage, | ||
| "failed to get max leverage for perpetual %d: %v", | ||
| perpetualId, | ||
| err, | ||
| ) | ||
| } | ||
|
|
||
| if custom_imf_ppm < minImfPpm { | ||
| return errorsmod.Wrapf( | ||
| types.ErrLeverageExceedsMaximum, | ||
| "%d is less than minimum allowed imf (%d) for perpetual %d resulting in higher than allowed leverage", | ||
| custom_imf_ppm, | ||
| minImfPpm, | ||
| perpetualId, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix wording + unify var naming.
- Comment and error message mention “max leverage” but you’re querying minimum IMF; adjust for clarity.
- Use Go‑idiomatic variable naming (customImfPpm).
-// Validate leverage against maximum allowed leverage for each perpetual
+// Validate custom IMF against the minimum allowed IMF (lower IMF => higher leverage) for each perpetual
for _, perpetualId := range sortedPerpIds {
- custom_imf_ppm := perpetualLeverage[perpetualId]
+ customImfPpm := perpetualLeverage[perpetualId]
minImfPpm, err := k.GetMinImfForPerpetual(ctx, perpetualId)
if err != nil {
return errorsmod.Wrapf(
types.ErrInvalidLeverage,
- "failed to get max leverage for perpetual %d: %v",
+ "failed to get minimum IMF for perpetual %d: %v",
perpetualId,
err,
)
}
- if custom_imf_ppm < minImfPpm {
+ if customImfPpm < minImfPpm {
return errorsmod.Wrapf(
types.ErrLeverageExceedsMaximum,
"%d is less than minimum allowed imf (%d) for perpetual %d resulting in higher than allowed leverage",
- custom_imf_ppm,
+ customImfPpm,
minImfPpm,
perpetualId,
)
}
}🤖 Prompt for AI Agents
In protocol/x/subaccounts/keeper/leverage.go around lines 71 to 96, the comment
and one error message refer to “max leverage” while the code fetches a minimum
IMF; rename the comment to state “minimum IMF” and update the error text to say
“failed to get minimum IMF” (or similar accurate wording). Also rename the Go
variable custom_imf_ppm to idiomatic customImfPpm (update all uses) to follow Go
naming conventions, and ensure the wrapped error messages reference the correct
variables/phrasing (minimum IMF) for clarity.
|
@Mergifyio backport release/protocol/v9.x |
✅ Backports have been createdDetails
|
(cherry picked from commit d5af215) # Conflicts: # indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts # indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/tx.ts # proto/dydxprotocol/clob/query.proto # proto/dydxprotocol/clob/tx.proto # protocol/x/clob/ante/clob.go # protocol/x/clob/client/cli/tx_update_leverage.go # protocol/x/clob/e2e/app_test.go # protocol/x/clob/keeper/grpc_query_leverage.go # protocol/x/clob/keeper/leverage.go # protocol/x/clob/types/clob_keeper.go # protocol/x/clob/types/leverage.go # protocol/x/clob/types/query.pb.go # protocol/x/clob/types/tx.pb.go
4 participants
Changelist
Using the configured leverage for a subaccount, validate that the user has enough collateral on order placement to satisfy the scaled IMR requirements. Remember that the MMR, or liquidation level, does not change with leverage, so all that matters here is scaling the IMR to ensure that the net collateral is sufficient.
Test Plan
Added leverage_e2e_test to check a case where Alice and Bob place the exact same trade, but Alice has a lower leverage configured and thus the order placement fails at the collateral check phase.
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
New Features
Improvements
Tests