Check Leverage on Withdrawals and Updates#3153
Conversation
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
had a problem deploying
to
mainnet
GitHub Actions
Failure
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
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
mainnet
GitHub Actions
Inactive
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 |
anmolagrawal345
force-pushed
the
anmol/leverage-orders
branch
from
8e868f2 to
8cbdff9
Compare
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
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
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
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
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
protocol/x/subaccounts/lib/updates.go (1)
339-347: Update function docs to reflectleverageMapand zero-is-default semantics.The comment references an “update” param and doesn’t mention leverage. Please refresh docs to match the new signature and behavior.
Suggested doc edit:
-// GetRiskForSubaccount returns the risk value of the `Subaccount` after updates are applied. -// It is used to get information about speculative changes to the `Subaccount`. -// The input subaccount must be settled. -// -// The provided update can also be "zeroed" in order to get information about -// the current state of the subaccount (i.e. with no changes). +// GetRiskForSubaccount returns the risk for a settled `Subaccount`. +// Optional `leverageMap` (perpetualId -> custom_imf_ppm) applies user-selected IMF overrides. +// Pass nil or omit an entry to use the liquidity tier default. A value of 0 means “use default.”protocol/x/perpetuals/types/liquidity_tier.go (1)
76-87: Add nil guard and clamp custom_imf_ppm to 1_000_000 ppm.The function lacks protection against nil
custom_imf_ppmand does not enforce the IMF ≤ 100% (1_000_000 ppm) invariant. Currently, the clob module validatesCustomImfPpm ∈ (0, 1_000_000], but the subaccounts path (which calls this function) has no upper-bound check, allowing values exceeding 1_000_000 ppm to inflate initial margin above 100%. Apply the suggested diff to add the nil guard and clamp.Optional: rename
custom_imf_ppmtocustomImfPpm(Go naming), and expand the function doc to state: "IfcustomImfPpm> 0, IM PPM = max(OI-adjusted IMF, min(customImfPpm, 1_000_000)); nil or ≤ 0 means no override."func (liquidityTier LiquidityTier) GetInitialMarginQuoteQuantums( quoteQuantums *big.Int, oiQuoteQuantums *big.Int, - custom_imf_ppm *big.Int, + custom_imf_ppm *big.Int, ) *big.Int { totalImfPpm := liquidityTier.GetAdjustedInitialMarginPpm(oiQuoteQuantums) - if custom_imf_ppm.Sign() > 0 { - // use the configured IMF if it is greater than the OI scaled IMF - totalImfPpm = lib.BigMax(totalImfPpm, custom_imf_ppm) - } + if custom_imf_ppm != nil && custom_imf_ppm.Sign() > 0 { + // Clamp to [0, 1_000_000] and take the max with OI-scaled IMF. + custom := new(big.Int).Set(custom_imf_ppm) + if custom.Cmp(lib.BigIntOneMillion()) > 0 { + custom = lib.BigIntOneMillion() + } + totalImfPpm = lib.BigMax(totalImfPpm, custom) + } return lib.BigMulPpm(quoteQuantums, totalImfPpm, true) // Round up initial margin. }
♻️ Duplicate comments (3)
protocol/x/clob/keeper/leverage_e2e_test.go (1)
190-199: Fix comment/value mismatch for leverage.Comment says "1x on BTC perpetual" but
CustomImfPpmis set to 1_000_000 (100% IMF = 1x leverage, which is correct). However, this appears to be testing a restrictive leverage scenario. If the intent is to test 1x leverage (no leverage), this is correct. If testing higher leverage (e.g., 2x), setCustomImfPpm: 500_000.Note: This duplicates an existing review comment about the comment/value alignment.
protocol/x/subaccounts/keeper/subaccount.go (1)
268-296: Leverage ignored for asset-only updates (withdraw/transfer/collat checks). Fetch leverage unconditionally.Currently leverage is fetched only when
PerpetualUpdatesexist OR perpetual positions exist (lines 273-279). Withdrawals or pure asset updates from subaccounts with no current perpetual positions will bypass leverage fetching, violating the PR's stated objective of "leverage-aware collateral checks for withdrawals."For example: a user configures 5x leverage, opens a position, closes it completely (no perpetual positions remain), then attempts to withdraw. The current logic won't fetch leverage, allowing the withdrawal to bypass leverage-aware margin checks.
Note: This duplicates a previous review comment on the same lines. The fix is to remove the conditional guards and always call
k.GetLeverage(ctx, &u.SubaccountId)when initializing leverageMap.Based on learnings
protocol/x/subaccounts/keeper/leverage.go (1)
104-124: Critical: Validate the full merged leverage configuration, not just the delta.Line 116 passes only
perpetualLeverage(the newly updated perpetuals) to margin checks, ignoring the existing leverage on other perpetuals. This allows unsafe updates where the combined risk of all perpetuals is not validated.Apply this diff to validate the complete merged leverage state:
+ // Check if the new leverage values break margin requirements - err := k.checkNewLeverageAgainstMarginRequirements(ctx, subaccountId, perpetualLeverage) + err := k.checkNewLeverageAgainstMarginRequirements(ctx, subaccountId, existingLeverage) if err != nil { return err }Based on learnings
🧹 Nitpick comments (11)
protocol/x/perpetuals/lib/lib.go (1)
114-165: Clarify comment about leverage scaling.The comment at lines 116-117 states "If leverage > 0, scales the margin requirements" but the actual scaling logic appears to be delegated to
liquidityTier.GetInitialMarginQuoteQuantums()(called at lines 146-149 and 158-162). Consider updating the comment to clarify that the scaling is performed by the liquidity tier's IMF calculation, or thatcustom_imf_ppmis used as an override when non-zero.protocol/x/clob/client/cli/tx_update_leverage.go (2)
42-48: Prefer numeric sort over lexicographic for CLOB pair IDs.String sort makes "10" come before "2". Sort numerically to avoid surprises and keep determinism.
- // Sort the keys to ensure deterministic ordering - keys := make([]string, 0, len(leverageMap)) - for clobPairIdStr := range leverageMap { - keys = append(keys, clobPairIdStr) - } - sort.Strings(keys) + // Sort CLOB pair IDs numerically for deterministic ordering + ids := make([]uint32, 0, len(leverageMap)) + for clobPairIdStr := range leverageMap { + id, err := strconv.ParseUint(clobPairIdStr, 10, 32) + if err != nil { + return fmt.Errorf("invalid clob pair ID %s: %w", clobPairIdStr, err) + } + ids = append(ids, uint32(id)) + } + sort.Slice(ids, func(i, j int) bool { return ids[i] < ids[j] }) + if len(ids) == 0 { + return fmt.Errorf("leverage map cannot be empty") + }
49-60: Preallocate slice and reuse parsed IDs for efficiency/clarity.Minor ergonomics: preallocate capacity; reuse numeric IDs from the sort step to avoid re-parsing and map lookups by string.
- var clobPairLeverage []*types.LeverageEntry - for _, clobPairIdStr := range keys { - clobPairId, err := strconv.ParseUint(clobPairIdStr, 10, 32) - if err != nil { - return fmt.Errorf("invalid clob pair ID %s: %w", clobPairIdStr, err) - } - - clobPairLeverage = append(clobPairLeverage, &types.LeverageEntry{ - ClobPairId: uint32(clobPairId), - CustomImfPpm: leverageMap[clobPairIdStr], - }) - } + clobPairLeverage := make([]*types.LeverageEntry, 0, len(ids)) + for _, id := range ids { + clobPairLeverage = append(clobPairLeverage, &types.LeverageEntry{ + ClobPairId: id, + CustomImfPpm: leverageMap[strconv.FormatUint(uint64(id), 10)], + }) + }protocol/x/clob/keeper/leverage.go (1)
14-14: Delegation looks good.Simple pass-through to subaccounts keeper keeps leverage logic in one place. Consider aligning param naming with callers (“leverageMap”) for consistency across the codebase.
protocol/x/clob/types/expected_keepers.go (1)
109-127: Add brief doc comments for new leverage APIs (units/semantics).Clarify that values are IMF in ppm and note expected idempotent behavior.
- // Leverage methods + // Leverage methods + // Values are IMF overrides in parts-per-million (ppm). + // Implementations should be idempotent (reapplying the same map is a no-op).proto/dydxprotocol/clob/tx.proto (1)
222-224: Clarify units/semantics and align validation forcustom_imf_ppm.Rename looks good and keeps tag=2. Please:
- Document units and default: “parts‑per‑million; 0 means use liquidity tier default.”
- Verify server‑side validation accepts 0 (previous flow disallowed zero leverage). Based on learnings.
Suggested proto comment tweak:
- // The user selected imf. + // User-selected initial margin factor in parts-per-million (ppm). + // 0 means: use the liquidity tier default (no custom override). uint32 custom_imf_ppm = 2;protocol/x/clob/keeper/grpc_query_leverage.go (2)
27-33: Consider using the accessor to decouple from struct field.Prefer
k.GetSubaccountsKeeper().GetLeverage(...)over directk.subaccountsKeeperfor consistency with the interface and mocks.
35-56: Deterministic ordering LGTM; improve error context.Sorting keys for stable output is great. When mapping to ClobPairId fails, include the
perpetualIdin the error for easier debugging.Apply:
- return nil, status.Error(codes.Internal, errorsmod.Wrap(err, "failed to get clob pair id for perpetual").Error()) + return nil, status.Error( + codes.Internal, + errorsmod.Wrapf(err, "failed to get clob pair id for perpetual %d", perpetualId).Error(), + )protocol/x/subaccounts/lib/updates.go (1)
371-375: Add clarifying comment on override semantics.Verification confirms leverage is correctly threaded through all callsites (subaccount.go:703, 780; updates.go:401; sub_task_runner.go:341 with intentional nil for liquidation). The inline comment documenting that
0means "use liquidity tier default" remains a good clarity improvement for future maintainers.- custom_imf_ppm := uint32(0) + // 0 means: use liquidity tier default (no custom override). + custom_imf_ppm := uint32(0) if leverageMap != nil { custom_imf_ppm = leverageMap[pos.PerpetualId] }Apply the same comment at line 382–383 if the pattern repeats there.
protocol/x/perpetuals/types/liquidity_tier_test.go (2)
238-241: Benchmark also a non-zero custom IMF path.Add a second loop with a non‑zero
customImfto benchmark the new branch.Example:
customImf := big.NewInt(300_000) // 30% b.Run("with_custom_imf", func(b *testing.B) { b.ResetTimer() for i := 0; i < b.N; i++ { _ = liquidityTier.GetInitialMarginQuoteQuantums(bigQuoteQuantums, oiLower, customImf) _ = liquidityTier.GetInitialMarginQuoteQuantums(bigQuoteQuantums, oiUpper, customImf) _ = liquidityTier.GetInitialMarginQuoteQuantums(bigQuoteQuantums, oiMiddle, customImf) } })
360-364: Add unit tests for custom IMF override behavior (>, <, =, and >1e6).Current cases pass
0. Please add explicit tests to lock the new semantics.Example new test (add as a separate function):
func TestGetInitialMarginQuoteQuantums_WithCustomIMF(t *testing.T) { lt := &types.LiquidityTier{ InitialMarginPpm: 200_000, // 20% OpenInterestLowerCap: 1_000_000, OpenInterestUpperCap: 2_000_000, } q := big.NewInt(1_000_000) // OI mid ⇒ adjusted IMF ≈ 60% oiMid := big.NewInt(1_500_000) // custom < adjusted ⇒ use adjusted (60%) got := lt.GetInitialMarginQuoteQuantums(q, oiMid, big.NewInt(300_000)) require.Equal(t, big.NewInt(600_000), got) // custom == adjusted ⇒ unchanged got = lt.GetInitialMarginQuoteQuantums(q, oiMid, big.NewInt(600_000)) require.Equal(t, big.NewInt(600_000), got) // custom > adjusted ⇒ use custom got = lt.GetInitialMarginQuoteQuantums(q, oiMid, big.NewInt(700_000)) require.Equal(t, big.NewInt(700_000), got) // custom > 1_000_000 ⇒ clamped to 1_000_000 got = lt.GetInitialMarginQuoteQuantums(q, oiMid, big.NewInt(1_500_000)) require.Equal(t, big.NewInt(1_000_000), got) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 400661fe865204d719ea5cc1bd1677694107ccce and 918f237b19023000b85a5617ab488810c0f09f7f.
⛔ Files ignored due to path filters (3)
protocol/x/clob/types/query.pb.gois excluded by!**/*.pb.goprotocol/x/clob/types/tx.pb.gois excluded by!**/*.pb.goprotocol/x/subaccounts/types/leverage.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (36)
indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts(4 hunks)indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts(5 hunks)indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/tx.ts(5 hunks)indexer/packages/v4-protos/src/codegen/dydxprotocol/subaccounts/leverage.ts(1 hunks)indexer/packages/v4-protos/src/codegen/gogoproto/bundle.ts(1 hunks)indexer/packages/v4-protos/src/codegen/google/bundle.ts(1 hunks)proto/dydxprotocol/clob/query.proto(1 hunks)proto/dydxprotocol/clob/tx.proto(1 hunks)proto/dydxprotocol/subaccounts/leverage.proto(1 hunks)protocol/daemons/liquidation/client/sub_task_runner.go(1 hunks)protocol/mocks/ClobKeeper.go(4 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/client/cli/tx_update_leverage.go(2 hunks)protocol/x/clob/e2e/app_test.go(1 hunks)protocol/x/clob/keeper/grpc_query_leverage.go(2 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/clob/types/leverage.go(2 hunks)protocol/x/perpetuals/lib/lib.go(7 hunks)protocol/x/perpetuals/lib/lib_test.go(4 hunks)protocol/x/perpetuals/types/liquidity_tier.go(1 hunks)protocol/x/perpetuals/types/liquidity_tier_test.go(2 hunks)protocol/x/subaccounts/keeper/leverage.go(1 hunks)protocol/x/subaccounts/keeper/margining.go(2 hunks)protocol/x/subaccounts/keeper/subaccount.go(9 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)
🚧 Files skipped from review as they are similar to previous changes (14)
- protocol/x/perpetuals/lib/lib_test.go
- protocol/x/subaccounts/lib/updates_test.go
- protocol/daemons/liquidation/client/sub_task_runner.go
- protocol/x/subaccounts/types/keys.go
- protocol/testutil/app/app.go
- protocol/testutil/keeper/listing.go
- protocol/x/subaccounts/types/settled_update.go
- protocol/x/clob/types/clob_keeper.go
- protocol/x/subaccounts/keeper/margining.go
- protocol/x/clob/e2e/app_test.go
- protocol/x/clob/keeper/liquidations.go
- protocol/x/subaccounts/types/errors.go
- proto/dydxprotocol/subaccounts/leverage.proto
- indexer/packages/v4-protos/src/codegen/gogoproto/bundle.ts
🧰 Additional context used
🧠 Learnings (4)
📚 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/types/leverage.goprotocol/x/clob/keeper/leverage.goprotocol/mocks/ClobKeeper.goprotocol/x/clob/ante/clob.goprotocol/x/clob/types/expected_keepers.goprotocol/x/clob/client/cli/tx_update_leverage.goprotocol/x/clob/keeper/grpc_query_leverage.goprotocol/x/clob/keeper/leverage_e2e_test.goprotocol/x/subaccounts/keeper/subaccount.goprotocol/x/subaccounts/keeper/leverage.goproto/dydxprotocol/clob/tx.proto
📚 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/x/clob/types/expected_keepers.goprotocol/x/clob/keeper/keeper.goprotocol/x/subaccounts/lib/updates.goprotocol/x/subaccounts/keeper/subaccount.goprotocol/x/subaccounts/keeper/leverage.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/clob/keeper/keeper.goprotocol/x/subaccounts/keeper/subaccount.goprotocol/x/subaccounts/keeper/leverage.go
📚 Learning: 2024-11-15T16:17:29.092Z
Learnt from: hwray
PR: dydxprotocol/v4-chain#2551
File: protocol/x/clob/types/expected_keepers.go:86-90
Timestamp: 2024-11-15T16:17:29.092Z
Learning: The function `GetCrossInsuranceFundBalance` in `protocol/x/clob/types/expected_keepers.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/clob/keeper/keeper.go
🧬 Code graph analysis (13)
protocol/x/clob/types/leverage.go (1)
protocol/x/clob/types/errors.go (1)
ErrInvalidLeverage(564-568)
protocol/x/perpetuals/types/liquidity_tier.go (1)
protocol/lib/big_math.go (2)
BigMax(105-115)BigMulPpm(20-28)
protocol/x/perpetuals/lib/lib.go (1)
protocol/lib/big_math.go (1)
BigU(10-12)
protocol/x/clob/ante/clob.go (1)
protocol/x/clob/types/leverage.go (1)
ValidateAndConstructPerpetualLeverageMap(58-75)
protocol/x/clob/client/cli/tx_update_leverage.go (2)
indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/tx.ts (2)
LeverageEntry(387-393)LeverageEntry(1395-1441)protocol/x/clob/types/tx.pb.go (3)
LeverageEntry(1095-1100)LeverageEntry(1104-1104)LeverageEntry(1105-1107)
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)
protocol/x/clob/keeper/grpc_query_leverage.go (2)
indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts (4)
ClobPairLeverageInfo(299-305)ClobPairLeverageInfo(1466-1512)QueryLeverageResponse(287-290)QueryLeverageResponse(1420-1457)protocol/x/clob/types/query.pb.go (6)
ClobPairLeverageInfo(954-959)ClobPairLeverageInfo(963-963)ClobPairLeverageInfo(964-966)QueryLeverageResponse(908-911)QueryLeverageResponse(915-915)QueryLeverageResponse(916-918)
protocol/x/clob/keeper/leverage_e2e_test.go (6)
protocol/testutil/app/app.go (6)
TestApp(600-622)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/assets/types/genesis.go (1)
AssetUsdc(12-19)protocol/dtypes/serializable_int.go (1)
NewIntFromBigInt(43-49)protocol/x/subaccounts/types/update.go (4)
Update(79-86)AssetUpdate(88-93)PerpetualUpdate(102-110)CollatCheck(133-133)protocol/x/perpetuals/lib/lib.go (1)
GetNetCollateralAndMarginRequirements(71-90)
indexer/packages/v4-protos/src/codegen/dydxprotocol/subaccounts/leverage.ts (1)
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/x/subaccounts/lib/updates.go (4)
protocol/x/perpetuals/lib/lib.go (1)
GetNetCollateralAndMarginRequirements(71-90)protocol/x/subaccounts/types/settled_update.go (1)
SettledUpdate(7-17)protocol/x/perpetuals/types/perpinfo.go (1)
PerpInfos(16-16)protocol/lib/margin/risk.go (1)
Risk(9-13)
protocol/x/subaccounts/keeper/subaccount.go (4)
protocol/x/subaccounts/types/settled_update.go (1)
SettledUpdate(7-17)protocol/x/subaccounts/lib/updates.go (2)
GetSettledSubaccountWithPerpetuals(20-82)GetRiskForSettledUpdate(393-402)protocol/x/subaccounts/types/update.go (1)
Update(79-86)protocol/lib/margin/risk.go (1)
Risk(9-13)
protocol/x/subaccounts/keeper/leverage.go (6)
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 (4)
ErrInvalidLeverage(83-83)ErrLeverageExceedsMaximum(84-84)ErrLeverageViolatesMarginRequirements(86-86)ErrInitialMarginPpmIsZero(85-85)protocol/x/subaccounts/types/update.go (2)
Update(79-86)PerpetualUpdate(102-110)
indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (2)
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.lcd.ts (1)
vault(31-34)indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.rpc.Query.ts (2)
vault(53-57)vault(108-110)
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (24)
indexer/packages/v4-protos/src/codegen/google/bundle.ts (1)
1-16: LGTM! Auto-generated bundle reorganization.The import reindexing and export reordering correctly incorporates new protobuf modules (duration, timestamp, any) into the public surface, aligning with the broader leverage subsystem changes.
protocol/x/perpetuals/lib/lib.go (1)
41-67: LGTM! Custom IMF parameter correctly threaded through margin calculations.The
custom_imf_ppmparameter is properly propagated to enable per-subaccount leverage overrides. The 0 value convention for using default margins is clear and consistent.indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (1)
114-417: LGTM! Auto-generated bundle correctly incorporates leverage module.The import additions (line 114:
subaccounts/leverage) and export reshuffling properly integrate the new leverage-related protobuf types into the public dydxprotocol namespace. All module indices are consistently updated.protocol/x/clob/types/leverage.go (1)
10-75: LGTM! Field rename fromLeveragetoCustomImfPpmcorrectly applied.The validation logic properly enforces
0 < CustomImfPpm ≤ 1,000,000(representing 0% to 100% IMF), and the perpetual leverage map construction (line 71) correctly uses the new field name. Error messages are clear and accurate.proto/dydxprotocol/clob/query.proto (1)
198-204: LGTM! Protobuf field rename aligns with codebase refactoring.The field rename from
leveragetocustom_imf_ppm(lines 202-203) better represents the semantic meaning (custom initial margin fraction in PPM). While this is a breaking API change, it's coordinated across the protocol, indexer, and client surfaces.protocol/x/clob/keeper/keeper.go (1)
167-169: LGTM! New getter enables leverage delegation to Subaccounts keeper.The
GetSubaccountsKeeper()method exposes the subaccounts keeper to support the architectural change where leverage operations are delegated from the CLOB module to the Subaccounts module, as seen in related changes (e.g.,protocol/x/clob/ante/clob.goandprotocol/x/clob/keeper/leverage.go).protocol/x/clob/keeper/leverage_e2e_test.go (1)
273-286: UseCustomImfPpminstead of deprecatedLeveragefield.Line 279 uses the old field name
Leverage: 5instead of the newCustomImfPpmfield. This is inconsistent with the rest of the codebase changes where the field was renamed fromLeveragetoCustomImfPpm.Apply this diff:
leverageMsg := &clobtypes.MsgUpdateLeverage{ SubaccountId: &constants.Alice_Num0, ClobPairLeverage: []*clobtypes.LeverageEntry{ { ClobPairId: 0, - Leverage: 5, + CustomImfPpm: 200_000, // 20% IMF = 5x leverage }, }, }⛔ Skipped due to learnings
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.protocol/x/clob/ante/clob.go (2)
196-204: Good: centralizing validation via ValidateAndConstruct + subaccounts keeper.This ensures a single source of truth for IMF/leverage validation and persistence. Nice.
203-208: AddIsReCheckTx guard to UpdateLeverage call in ante handler or defer state-dependent validation to DeliverTx.The issue is valid.
UpdateLeveragedepends on the current subaccount state (margin requirements validation viacheckNewLeverageAgainstMarginRequirements). During ReCheckTx, the subaccount state may have changed from CheckTx due to DeliverTx execution—positions could be closed, collateral updated—causing the margin check to fail on replay even though it passed initially. This breaks idempotency.The codebase already uses
IsReCheckTx()guards for other state-dependent operations (e.g., short-term order processing in clob ante at lines 85, 112, 158). UpdateLeverage should either:
- Skip execution on ReCheckTx in the ante handler, or
- Defer state-dependent margin validation to the message handler (DeliverTx only).
indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts (1)
300-305: Proto rename wired correctly (customImfPpm).Encode/decode/fromPartial keep tag 2 for the renamed field; aligns with tx.ts LeverageEntry. Looks consistent.
Also applies to: 309-314, 1460-1463, 1472-1474, 1493-1494, 1506-1509
protocol/x/subaccounts/lib/updates.go (1)
390-402: New helper is clean and focused.
GetRiskForSettledUpdatecomposes the updated subaccount and defers to the leverage-aware path. LGTM.indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/tx.ts (1)
389-393: Codegen matches proto rename; surfaces are consistent.Interfaces, SDK types, and encode/decode paths correctly use
customImfPpm. No further action.Also applies to: 399-402, 1389-1392, 1401-1403, 1422-1438
protocol/mocks/ClobKeeper.go (2)
627-645: Accessor added for subaccounts keeper — good.Matches interface usage. Ensure tests set an expectation for
GetSubaccountsKeeper()when needed. Also confirm there’s no remaining dependency on a direct field.
1035-1083: AffiliateParameters type alias change — consistent.Signature and typed callbacks updated coherently. LGTM.
indexer/packages/v4-protos/src/codegen/dydxprotocol/subaccounts/leverage.ts (4)
1-14: LGTM!The imports and interface definitions follow standard protobuf patterns and align with the Go proto definitions.
20-38: LGTM!The SDK type definitions and LeverageData interfaces follow protobuf conventions with appropriate snake_case naming for wire format compatibility.
40-93: LGTM!The encode/decode implementation correctly uses protobuf wire format with proper field tags and gracefully handles unknown fields.
95-138: LGTM!The nested message encoding correctly uses fork/ldelim pattern and properly delegates to PerpetualLeverageEntry encode/decode methods.
protocol/x/subaccounts/keeper/leverage.go (6)
14-22: LGTM!The prefix store setup and key generation follow standard keeper patterns and properly namespace leverage data per subaccount.
24-48: LGTM!The protobuf binary serialization correctly addresses the previous review concern about JSON encoding. Sorting entries ensures deterministic state storage.
50-69: LGTM!The retrieval logic correctly deserializes protobuf data and reconstructs the leverage map with proper nil handling.
71-102: LGTM!The per-perpetual IMF validation correctly checks against minimum allowed values and provides clear error messages.
126-157: LGTM with upstream dependency.The margin validation logic correctly constructs zero-delta updates and checks initial collateralization. This will validate the complete risk once the upstream fix at line 116 is applied.
159-174: LGTM!The function correctly retrieves the minimum IMF from the liquidity tier with appropriate zero-check for InitialMarginPpm.
| // Configure leverage for Alice with an existing bitcoin position | ||
| // This should make the account fail against the IMR check | ||
| aliceLeverage := &clobtypes.MsgUpdateLeverage{ | ||
| SubaccountId: &constants.Alice_Num0, | ||
| ClobPairLeverage: []*clobtypes.LeverageEntry{ | ||
| { | ||
| ClobPairId: 0, | ||
| Leverage: 2, | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Use CustomImfPpm instead of deprecated Leverage field.
Line 531 uses the old field name Leverage: 2 instead of the new CustomImfPpm field, inconsistent with the broader refactoring.
Apply this diff:
aliceLeverage := &clobtypes.MsgUpdateLeverage{
SubaccountId: &constants.Alice_Num0,
ClobPairLeverage: []*clobtypes.LeverageEntry{
{
ClobPairId: 0,
- Leverage: 2,
+ CustomImfPpm: 500_000, // 50% IMF = 2x leverage
},
},
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Configure leverage for Alice with an existing bitcoin position | |
| // This should make the account fail against the IMR check | |
| aliceLeverage := &clobtypes.MsgUpdateLeverage{ | |
| SubaccountId: &constants.Alice_Num0, | |
| ClobPairLeverage: []*clobtypes.LeverageEntry{ | |
| { | |
| ClobPairId: 0, | |
| Leverage: 2, | |
| }, | |
| }, | |
| } | |
| // Configure leverage for Alice with an existing bitcoin position | |
| // This should make the account fail against the IMR check | |
| aliceLeverage := &clobtypes.MsgUpdateLeverage{ | |
| SubaccountId: &constants.Alice_Num0, | |
| ClobPairLeverage: []*clobtypes.LeverageEntry{ | |
| { | |
| ClobPairId: 0, | |
| CustomImfPpm: 500_000, // 50% IMF = 2x leverage | |
| }, | |
| }, | |
| } |
🤖 Prompt for AI Agents
In protocol/x/clob/keeper/leverage_e2e_test.go around lines 524 to 534, the test
uses the deprecated Leverage field (Leverage: 2); replace that with the new
CustomImfPpm field and set it to the equivalent parts‑per‑million value for the
same 2x leverage (e.g., CustomImfPpm: 2000000) so the test uses the refactored
field name and preserves the same intent.
| // Leverage methods | ||
| SetLeverage( | ||
| ctx sdk.Context, | ||
| subaccountId *satypes.SubaccountId, | ||
| leverageMap map[uint32]uint32, | ||
| ) | ||
| GetLeverage( | ||
| ctx sdk.Context, | ||
| subaccountId *satypes.SubaccountId, | ||
| ) (map[uint32]uint32, bool) | ||
| UpdateLeverage( | ||
| ctx sdk.Context, | ||
| subaccountId *satypes.SubaccountId, | ||
| perpetualLeverage map[uint32]uint32, | ||
| ) error | ||
| GetMinImfForPerpetual( | ||
| ctx sdk.Context, | ||
| perpetualId uint32, | ||
| ) (uint32, error) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify mocks and interface implementers updated.
Interface expansion can break builds if mocks/keepers lag behind. Please confirm all implementers were updated.
🏁 Script executed:
#!/bin/bash
# Find all types implementing SubaccountsKeeper and ensure new methods exist.
set -euo pipefail
echo "Search for SubaccountsKeeper implementations (mocks, real keepers):"
rg -n 'type .*SubaccountsKeeper' -C2
echo
echo "Check for new methods in mocks/keepers:"
for m in SetLeverage GetLeverage UpdateLeverage GetMinImfForPerpetual; do
echo "== $m =="
rg -n "$m\\(" -C2
doneLength of output: 23330
#!/bin/bash
# Check if SubaccountsKeeper mock has all four methods
echo "Checking protocol/mocks/SubaccountsKeeper.go for method implementations:"
cat protocol/mocks/SubaccountsKeeper.go | grep -E "func.*\(.*SubaccountsKeeper\)" | head -20
echo
echo "Checking for the four specific methods:"
for method in SetLeverage GetLeverage UpdateLeverage GetMinImfForPerpetual; do
echo "=== $method ==="
grep -n "func.*$method" protocol/mocks/SubaccountsKeeper.go 2>/dev/null || echo "NOT FOUND"
done[/run_scripts]
anmolagrawal345
force-pushed
the
anmol/leverage-orders
branch
from
918f237 to
b4dff41
Compare
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
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
protocol/x/clob/keeper/leverage_e2e_test.go (1)
526-533: Compile blocker: deprecated Leverage field; use CustomImfPpm for 2x.2x → 50% IMF → 500,000 ppm. Based on learnings.
aliceLeverage := &clobtypes.MsgUpdateLeverage{ SubaccountId: &constants.Alice_Num0, ClobPairLeverage: []*clobtypes.LeverageEntry{ { ClobPairId: 0, - Leverage: 2, + CustomImfPpm: 500_000, // 2x }, }, }protocol/x/subaccounts/keeper/leverage.go (1)
111-116: Validate the merged leverage, not just the delta.Current check passes only the delta map, underestimating IMR and allowing unsafe updates when existing perps are configured.
- // Check if the new leverage values break margin requirements - err := k.checkNewLeverageAgainstMarginRequirements(ctx, subaccountId, perpetualLeverage) + // Check if the merged leverage values break margin requirements + err := k.checkNewLeverageAgainstMarginRequirements(ctx, subaccountId, existingLeverage) if err != nil { return err }Based on learnings
🧹 Nitpick comments (3)
protocol/x/clob/keeper/leverage_e2e_test.go (2)
144-144: Logging shows ppm as “x”.Log the ratio, not raw ppm.
- t.Logf("✅ Successfully configured and verified %dx leverage for subaccount", custom_imf_ppm) + t.Logf("✅ Successfully configured and verified %.0fx leverage for subaccount", float64(1_000_000)/float64(custom_imf_ppm)) - t.Logf("✅ Successfully configured %dx leverage", tc.custom_imf_ppm) + t.Logf("✅ Successfully configured %.0fx leverage", float64(1_000_000)/float64(tc.custom_imf_ppm))Also applies to: 176-176
410-413: Defensive guard before Uint64 conversion.Avoid modulo wrap on negative big.Int; assert positivity before casting.
- // Verify Bob has more available collateral than Alice due to lower leverage requirements + // Verify Bob has more available collateral than Alice due to lower leverage requirements require.True(t, bobAvailable.Cmp(aliceAvailable) > 0, "Bob's available collateral (%s) should be greater than Alice's (%s) due to lower IMR from higher leverage", bobAvailable.String(), aliceAvailable.String()) + // Guard: must be non-negative before converting to uint64 + require.True(t, bobAvailable.Sign() >= 0, "Bob's available collateral must be non-negative")Also applies to: 417-423
protocol/x/subaccounts/keeper/leverage.go (1)
122-153: Reduce N calls to risk; build one aggregate update.Single-shot risk check is cheaper and simpler.
-func (k Keeper) checkNewLeverageAgainstMarginRequirements( +func (k Keeper) checkNewLeverageAgainstMarginRequirements( ctx sdk.Context, subaccountId *types.SubaccountId, - leverageMap map[uint32]uint32, + leverageMap map[uint32]uint32, ) (err error) { - for perpetualId := range leverageMap { - update := types.Update{ - SubaccountId: *subaccountId, - PerpetualUpdates: []types.PerpetualUpdate{ - { PerpetualId: perpetualId, BigQuantumsDelta: big.NewInt(0) }, - }, - } - risk, err := k.GetNetCollateralAndMarginRequirementsWithLeverage(ctx, update, leverageMap) - if err != nil { - return err - } - if !risk.IsInitialCollateralized() { - return errorsmod.Wrapf( - types.ErrLeverageViolatesMarginRequirements, - "subaccount %s violates margin requirements with new leverage", - subaccountId.String(), - ) - } - } - return nil + // Build a single zero-delta update covering all configured perps. + updates := make([]types.PerpetualUpdate, 0, len(leverageMap)) + for perpetualId := range leverageMap { + updates = append(updates, types.PerpetualUpdate{ PerpetualId: perpetualId, BigQuantumsDelta: big.NewInt(0) }) + } + update := types.Update{ SubaccountId: *subaccountId, PerpetualUpdates: updates } + risk, err := k.GetNetCollateralAndMarginRequirementsWithLeverage(ctx, update, leverageMap) + if err != nil { + return err + } + if !risk.IsInitialCollateralized() { + return errorsmod.Wrapf( + types.ErrLeverageViolatesMarginRequirements, + "subaccount %s violates margin requirements with new leverage", + subaccountId.String(), + ) + } + return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 918f237b19023000b85a5617ab488810c0f09f7f and b4dff41.
📒 Files selected for processing (4)
protocol/x/clob/keeper/leverage_e2e_test.go(2 hunks)protocol/x/subaccounts/keeper/leverage.go(2 hunks)protocol/x/subaccounts/keeper/subaccount.go(2 hunks)protocol/x/subaccounts/types/errors.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- protocol/x/subaccounts/types/errors.go
🧰 Additional context used
🧠 Learnings (3)
📚 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.goprotocol/x/subaccounts/keeper/leverage.goprotocol/x/subaccounts/keeper/subaccount.go
📚 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/x/subaccounts/keeper/leverage.goprotocol/x/subaccounts/keeper/subaccount.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/keeper/leverage.goprotocol/x/subaccounts/keeper/subaccount.go
🧬 Code graph analysis (3)
protocol/x/clob/keeper/leverage_e2e_test.go (4)
protocol/testutil/app/app.go (4)
NewTestAppBuilder(335-350)MustMakeCheckTxsWithClobMsg(1352-1393)MustMakeCheckTxsWithSdkMsg(1397-1408)MustMakeCheckTxOptions(100-109)protocol/x/clob/types/expected_keepers.go (1)
SubaccountsKeeper(20-128)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)
protocol/x/subaccounts/keeper/leverage.go (2)
protocol/x/subaccounts/types/update.go (2)
Update(79-86)PerpetualUpdate(102-110)protocol/x/subaccounts/types/errors.go (1)
ErrLeverageViolatesMarginRequirements(86-86)
protocol/x/subaccounts/keeper/subaccount.go (4)
protocol/x/subaccounts/keeper/keeper.go (1)
Keeper(17-26)protocol/x/perpetuals/lib/lib.go (1)
GetNetCollateralAndMarginRequirements(71-90)protocol/x/subaccounts/types/update.go (1)
Update(79-86)protocol/lib/margin/risk.go (1)
Risk(9-13)
⏰ 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: (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-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-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: (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-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-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: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: golangci-lint
- GitHub Check: build
- GitHub Check: benchmark
- GitHub Check: check-sample-pregenesis-up-to-date
- 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-and-push-testnet
- GitHub Check: container-tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (4)
protocol/x/subaccounts/keeper/subaccount.go (2)
731-745: Wrapper reads leverage once and delegates — LGTM.Good API to keep callers leverage-aware without duplicating fetch logic.
758-761: WithLeverage API is clear and consistent.Signature and threading of leverageMap into risk are correct.
protocol/x/subaccounts/keeper/leverage.go (2)
24-44: Proto-based storage (deterministic, gas‑efficient) — nice.Using LeverageData with codec is the right choice versus JSON.
47-65: GetLeverage decode path — LGTM.Decodes to map with sorted entries upstream; straightforward and deterministic.
| leverageMsg := &clobtypes.MsgUpdateLeverage{ | ||
| SubaccountId: &constants.Alice_Num0, | ||
| ClobPairLeverage: []*clobtypes.LeverageEntry{ | ||
| { | ||
| ClobPairId: 0, | ||
| Leverage: 5, | ||
| }, | ||
| }, | ||
| } | ||
| for _, checkTx := range testapp.MustMakeCheckTxsWithClobMsg(ctx, tApp.App, *leverageMsg) { |
There was a problem hiding this comment.
Compile blocker: replace deprecated Leverage with CustomImfPpm (ppm).
Use CustomImfPpm for 5x (IMF 20% → 200,000 ppm).
leverageMsg := &clobtypes.MsgUpdateLeverage{
SubaccountId: &constants.Alice_Num0,
ClobPairLeverage: []*clobtypes.LeverageEntry{
{
ClobPairId: 0,
- Leverage: 5,
+ CustomImfPpm: 200_000, // 5x
},
},
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| leverageMsg := &clobtypes.MsgUpdateLeverage{ | |
| SubaccountId: &constants.Alice_Num0, | |
| ClobPairLeverage: []*clobtypes.LeverageEntry{ | |
| { | |
| ClobPairId: 0, | |
| Leverage: 5, | |
| }, | |
| }, | |
| } | |
| for _, checkTx := range testapp.MustMakeCheckTxsWithClobMsg(ctx, tApp.App, *leverageMsg) { | |
| leverageMsg := &clobtypes.MsgUpdateLeverage{ | |
| SubaccountId: &constants.Alice_Num0, | |
| ClobPairLeverage: []*clobtypes.LeverageEntry{ | |
| { | |
| ClobPairId: 0, | |
| CustomImfPpm: 200_000, // 5x | |
| }, | |
| }, | |
| } | |
| for _, checkTx := range testapp.MustMakeCheckTxsWithClobMsg(ctx, tApp.App, *leverageMsg) { |
🤖 Prompt for AI Agents
In protocol/x/clob/keeper/leverage_e2e_test.go around lines 274 to 283, the test
is using the deprecated Leverage field on LeverageEntry; replace it with
CustomImfPpm set to 200000 (which corresponds to 5x / IMF 20% → 200,000 ppm) and
remove the Leverage field; keep ClobPairId as-is and ensure the struct literal
uses the CustomImfPpm numeric value of 200000 (matching the expected type).
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
|
2 participants
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