[DEC-2196] deprecate non-linear margin requirements #836
Conversation
|
Warning Rate Limit Exceeded@jayy04 has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 11 minutes and 29 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe overall change involves the deprecation and removal of the Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- protocol/indexer/events/events.pb.go
- protocol/x/perpetuals/types/perpetual.pb.go
Files selected for processing (25)
- proto/dydxprotocol/indexer/events/events.proto (1 hunks)
- proto/dydxprotocol/perpetuals/perpetual.proto (1 hunks)
- protocol/indexer/events/liquidity_tier.go (1 hunks)
- protocol/indexer/events/liquidity_tier_test.go (1 hunks)
- protocol/mocks/PerpetualsKeeper.go (1 hunks)
- protocol/testing/e2e/gov/perpetuals_test.go (7 hunks)
- protocol/testutil/constants/perpetuals.go (2 hunks)
- protocol/testutil/keeper/perpetuals.go (2 hunks)
- protocol/testutil/liquidity_tier/liquidity_tier.go (2 hunks)
- protocol/testutil/sim_helpers/genesis_parameters.go (1 hunks)
- protocol/x/clob/e2e/liquidation_deleveraging_test.go (1 hunks)
- protocol/x/perpetuals/client/cli/query_perpetual_test.go (1 hunks)
- protocol/x/perpetuals/genesis.go (1 hunks)
- protocol/x/perpetuals/genesis_test.go (9 hunks)
- protocol/x/perpetuals/keeper/msg_server_set_liquidity_tier.go (1 hunks)
- protocol/x/perpetuals/keeper/msg_server_set_liquidity_tier_test.go (10 hunks)
- protocol/x/perpetuals/keeper/perpetual.go (5 hunks)
- protocol/x/perpetuals/keeper/perpetual_test.go (27 hunks)
- protocol/x/perpetuals/simulation/genesis.go (2 hunks)
- protocol/x/perpetuals/types/errors.go (1 hunks)
- protocol/x/perpetuals/types/genesis_test.go (10 hunks)
- protocol/x/perpetuals/types/liquidity_tier.go (3 hunks)
- protocol/x/perpetuals/types/liquidity_tier_test.go (5 hunks)
- protocol/x/perpetuals/types/message_set_liquidity_tier_test.go (4 hunks)
- protocol/x/perpetuals/types/types.go (1 hunks)
Additional comments: 54
proto/dydxprotocol/indexer/events/events.proto (1)
- 369-372: > 💡 NOTE
Codebase verification is a beta feature.
The deprecation of the
base_position_notionalfield is confirmed in bothperpetual.protoandevents.proto. Ensure that any code relying on this field is updated to reflect its deprecation.proto/dydxprotocol/perpetuals/perpetual.proto (1)
- 96-99: The deprecation of
base_position_notionalis consistent with the PR's objective to move away from non-linear margin requirements. Ensure that the deprecation is reflected in all relevant parts of the codebase, including documentation and any logic that previously depended on this field.protocol/indexer/events/liquidity_tier.go (2)
7-10: The function signature in the provided summary does not match the actual code. The summary indicates that the
idparameter is of typestring, but the code shows it asuint32. Please update the summary to reflect the correct type of theidparameter.7-16: The changes align with the PR objective to deprecate non-linear margin requirements by removing the
basePositionNotionalparameter. This simplifies the function signature and the event structure, which is consistent with the broader goal of the PR.protocol/indexer/events/liquidity_tier_test.go (1)
- 12-22: The changes align with the PR objective to deprecate non-linear margin requirements and the removal of
BasePositionNotionalis correctly reflected in the test functionTestNewLiquidityTierUpsertEvent_Success.protocol/mocks/PerpetualsKeeper.go (1)
- 230-246: The changes to the
SetLiquidityTiermethod in thePerpetualsKeepermock are consistent with the PR's objective to deprecate non-linear margin requirements. Ensure that corresponding test cases are updated to reflect the removal of thebasePositionNotionalparameter.protocol/testing/e2e/gov/perpetuals_test.go (7)
31-35: The removal of
BasePositionNotionalfromLiquidityTieris consistent with the PR's objective to deprecate non-linear margin requirements.354-359: The updates to the test cases removing references to
BasePositionNotionalare in line with the changes made to theLiquidityTierstruct and the PR's objective.367-372: The updates to the test cases removing references to
BasePositionNotionalare in line with the changes made to theLiquidityTierstruct and the PR's objective.381-386: The updates to the test cases removing references to
BasePositionNotionalare in line with the changes made to theLiquidityTierstruct and the PR's objective.394-399: The updates to the test cases removing references to
BasePositionNotionalare in line with the changes made to theLiquidityTierstruct and the PR's objective.407-412: The updates to the test cases removing references to
BasePositionNotionalare in line with the changes made to theLiquidityTierstruct and the PR's objective.420-425: The updates to the test cases removing references to
BasePositionNotionalare in line with the changes made to theLiquidityTierstruct and the PR's objective.protocol/testutil/constants/perpetuals.go (3)
23-90: The removal of the
BasePositionNotionalfield from theLiquidityTierdata structure aligns with the PR's objective to deprecate non-linear margin requirements.26-82: The
ImpactNotionalvalues have been adjusted for eachLiquidityTierinstance. Ensure that these changes are intentional and correctly reflect the new margin requirements.321-338: The changes to the
Perpetuals_DefaultGenesisStatevariable, which include the removal of theBasePositionNotionalfield fromLiquidityTiers, are consistent with the PR's objective to deprecate non-linear margin requirements.protocol/testutil/keeper/perpetuals.go (2)
3-11: The reorganization of the import block is a cosmetic change and does not impact the functionality of the code.
175-179: The removal of
l.BasePositionNotionalfrom theSetLiquidityTierfunction call aligns with the PR's objective to deprecate non-linear margin requirements.protocol/testutil/liquidity_tier/liquidity_tier.go (2)
30-35: The
WithImpactNotionalfunction is added to set theImpactNotionalfield of theLiquidityTierstruct. This change aligns with the PR's objective of simplifying margin requirements, as it does not involve the deprecatedbasePositionNotional.51-54: The default values for
LiquidityTierare set within theGenerateLiquidityTierfunction. This is a common pattern for creating test objects with predefined defaults, which can then be modified by the optionalLtModifierOptionfunctions passed toGenerateLiquidityTier.protocol/testutil/sim_helpers/genesis_parameters.go (1)
- 133-138: The changes to
MaxNumLiquidityTiersandMinFundingRateClampFactorPpmare consistent with the PR's objective of deprecating non-linear margin requirements and do not introduce any syntax or logical issues.protocol/x/clob/e2e/liquidation_deleveraging_test.go (1)
- 694-696: The summary provided earlier indicates that test cases related to non-linear margin requirements were removed. However, the provided hunks do not show any test cases being removed. Please verify if the summary is accurate or if there are other hunks not provided that include the removal of the mentioned test cases.
protocol/x/perpetuals/client/cli/query_perpetual_test.go (1)
- 53-58: The changes from lines 53 to 58 reflect the removal of the
BasePositionNotionalfield from theLiquidityTierstruct, which is consistent with the PR's objective to deprecate non-linear margin requirements. This change will require updates to any tests or logic that previously relied on theBasePositionNotionalfield.protocol/x/perpetuals/genesis.go (1)
- 25-29: The removal of
BasePositionNotionalfrom theSetLiquidityTiercall withinInitGenesisis consistent with the PR's objective to deprecate non-linear margin requirements. However, this specific change is not mentioned in the summary forprotocol/x/perpetuals/genesis.go. It's important to ensure that the summary accurately reflects all significant changes made to the code.protocol/x/perpetuals/genesis_test.go (9)
41-46: The removal of parameters from the test cases aligns with the PR's objective to deprecate non-linear margin requirements. Ensure that the logic of the tests still holds true after the removal of these parameters.
51-56: The removal of parameters from the test cases aligns with the PR's objective to deprecate non-linear margin requirements. Ensure that the logic of the tests still holds true after the removal of these parameters.
61-66: The removal of parameters from the test cases aligns with the PR's objective to deprecate non-linear margin requirements. Ensure that the logic of the tests still holds true after the removal of these parameters.
71-76: The removal of parameters from the test cases aligns with the PR's objective to deprecate non-linear margin requirements. Ensure that the logic of the tests still holds true after the removal of these parameters.
81-96: The removal of parameters from the test cases aligns with the PR's objective to deprecate non-linear margin requirements. Ensure that the logic of the tests still holds true after the removal of these parameters.
101-106: The removal of parameters from the test cases aligns with the PR's objective to deprecate non-linear margin requirements. Ensure that the logic of the tests still holds true after the removal of these parameters.
111-116: The removal of parameters from the test cases aligns with the PR's objective to deprecate non-linear margin requirements. Ensure that the logic of the tests still holds true after the removal of these parameters.
133-137: The removal of parameters from the test cases aligns with the PR's objective to deprecate non-linear margin requirements. Ensure that the logic of the tests still holds true after the removal of these parameters.
212-215: The test function
assertLiquidityTierUpsertEventsInIndexerBlockappears to be intact and unaffected by the removal of non-linear margin requirements. This suggests that the function's logic is not dependent on the deprecated fields.protocol/x/perpetuals/keeper/msg_server_set_liquidity_tier.go (1)
- 29-34: The changes align with the PR's objective to deprecate non-linear margin requirements by removing the
BasePositionNotionalparameter from theSetLiquidityTierfunction call. Ensure that all dependent code has been updated to reflect this change.protocol/x/perpetuals/keeper/msg_server_set_liquidity_tier_test.go (1)
- 1-10: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-130]
The changes in the test cases reflect the deprecation of non-linear margin requirements by removing references to
BasePositionNotional. The test cases have been updated to only include the fields that are relevant after the deprecation. This is consistent with the PR's objective and the summary provided.protocol/x/perpetuals/keeper/perpetual.go (2)
- 1295-1300: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1295-1333]
The summary indicates that the
basePositionNotionalparameter was removed from theSetLiquidityTierfunction, but the provided code does not reflect this change. Please verify if this change is accurate and if it is located in a different part of the codebase.
- 889-896: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [836-896]
The changes from lines 836 to 896 in the
GetMarginRequirementsfunction correctly reflect the PR's objective to simplify the initial margin requirement calculation by removing the adjustment based onmarginAdjustmentPpmand directly usingInitialMarginPpm. This aligns with the PR's goal to deprecate non-linear margin requirements.protocol/x/perpetuals/keeper/perpetual_test.go (2)
- 549-609: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [443-606]
The changes to the test cases in
perpetual_test.gocorrectly reflect the deprecation ofbasePositionNotionaland the updated margin calculation logic. The test cases have been adjusted to no longer usebasePositionNotionaland to use the new margin requirements. This is consistent with the PR objectives and the generated summaries.
- 650-655: The function call within the
TestGetMarginRequirements_Successfunction has been correctly updated to remove thebasePositionNotionalfield, aligning with the changes in the test cases and the overall PR objective.protocol/x/perpetuals/simulation/genesis.go (2)
91-96: The addition of the
calculateImpactNotionalfunction with a check forinitialMarginPpm == 0is a good safeguard against division by zero errors. This change aligns with the PR's objective to simplify margin requirements.156-167: The generation of
LiquidityTierinstances with the newImpactNotionalcalculation is consistent and does not introduce any new issues. It reflects the PR's objective to deprecate non-linear margin requirements.protocol/x/perpetuals/types/errors.go (1)
- 62-67: The changes from lines 62 to 67 are consistent with the PR's objective to deprecate non-linear margin requirements, as indicated by the removal of the error related to the base position notional being zero. The sequential error codes and the gap between 12 and 14 confirm the removal of the error with code 13, which is not present in the hunk. No other issues are observed in the provided hunk.
protocol/x/perpetuals/types/genesis_test.go (1)
- 36-40: The changes from lines 36 to 40, as well as similar changes in other hunks, correctly reflect the PR's objective to deprecate non-linear margin requirements by removing the
BasePositionNotionalfield. This is consistent with the PR summary and the user-provided facts.protocol/x/perpetuals/types/liquidity_tier.go (2)
- 10-15: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [10-25]
The changes in the
Validatemethod reflect the PR's objective to deprecate non-linear margin requirements by removing the check forbasePositionNotional. This aligns with the PR's goal and the provided summary.
- 66-76: The renaming of
GetAdjustedInitialMarginQuoteQuantumstoGetInitialMarginQuoteQuantumsand the simplification of its logic are in line with the PR's objective to simplify the margin requirement calculations. The logic appears to be correct and the method's new implementation is consistent with the PR's goal.protocol/x/perpetuals/types/liquidity_tier_test.go (5)
5-10: The changes in the import section are appropriate and align with the removal of deprecated fields or functions.
13-42: The test cases have been correctly updated to remove references to the deprecated
BasePositionNotionalfield.48-53: The
LiquidityTierstruct has been correctly updated to exclude theBasePositionNotionalfield.202-230: The test function
TestGetInitialMarginQuoteQuantumshas been updated to reflect the new function name and the removal of thebasePositionNotionalfield.241-248: The test cases have been correctly updated to use the new
GetInitialMarginQuoteQuantumsfunction.protocol/x/perpetuals/types/message_set_liquidity_tier_test.go (3)
29-32: The changes from lines 29 to 32... The code removal of
BasePositionNotionalis not visible in the provided hunks. If the field has indeed been removed, ensure that all references to it are also removed from the codebase and any tests that rely on it are updated or removed.29-32: The changes from lines 29 to 32... The test cases have been updated to reflect the removal of
BasePositionNotional. This is consistent with the PR objectives to deprecate non-linear margin requirements.73-78: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [68-76]
The changes from lines 68 to 76... The test case "Failure: impact notional is zero" is correctly retained as it tests a different field (
ImpactNotional) and is not related to the deprecatedBasePositionNotional.protocol/x/perpetuals/types/types.go (1)
- 91-96: The changes to the
CreatePerpetualmethod signature in thePerpetualsKeeperinterface reflect the PR's objective to deprecate non-linear margin requirements by removing thebasePositionNotionalparameter. Ensure that all implementations of this interface and calls toCreatePerpetualare updated accordingly to handle the new method signature.
| Name: "Large-Cap", | ||
| InitialMarginPpm: 500_000, | ||
| MaintenanceFractionPpm: 750_000, | ||
| BasePositionNotional: 100, | ||
| ImpactNotional: 1_000_000_000, | ||
| }, |
There was a problem hiding this comment.
💡 NOTE
Codebase verification is a beta feature.
The removal of the BasePositionNotional field from the TestGenesisState_Validate function has been noted. However, references to BasePositionNotional still exist in other parts of the codebase, specifically within perpetual.pb.go and events.pb.go. These references suggest that the field may still be in use for serialization/deserialization logic or other functionality. It is crucial to ensure that these remaining references do not lead to inconsistencies or errors due to the field's removal from the TestGenesisState_Validate function.
Affected locations:
protocol/x/perpetuals/types/perpetual.pb.goprotocol/indexer/events/events.pb.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- protocol/scripts/genesis/sample_pregenesis.json
Files selected for processing (2)
- protocol/testing/genesis.sh (6 hunks)
- protocol/x/perpetuals/module_test.go (2 hunks)
Additional comments: 3
protocol/testing/genesis.sh (1)
- 126-131: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [126-153]
The hunks provided show the configuration of various liquidity tiers using
daselcommands. However, there is no evidence in the hunks of the removal ofbase_position_notionalsettings as mentioned in the summary. It's important to ensure that the summary accurately reflects the changes made in the code. Ifbase_position_notionalwas indeed removed, it should be visible in the hunks, or the summary should be updated to reflect the actual changes.protocol/x/perpetuals/module_test.go (2)
320-324: The changes from lines 320 to 324 show that the
impact_notionalfield's value has been changed from an integer to a string. This is consistent with the bot-generated summary. However, the user-provided facts mention the deprecation of thebasePositionNotionalfield, which is not reflected in the provided hunks or the bot-generated summary. It's important to ensure that the summary accurately reflects the changes made in the code.361-365: The changes from lines 361 to 365 also show the
impact_notionalfield's value being changed from an integer to a string in the JSON output of theExportGenesisfunction. This change should be verified across the codebase to ensure that all instances whereimpact_notionalis used are expecting a string instead of an integer to avoid type-related errors.
jayy04
force-pushed
the
jy/deprecate-scaling-mr
branch
from
59f1ca0 to
a3c5a62
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/events/events.ts (2 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/perpetuals/perpetual.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/perpetuals/perpetual.ts
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/x/perpetuals/keeper/perpetual_test.go (27 hunks)
- protocol/x/perpetuals/module_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/perpetuals/module_test.go
Additional comments: 1
protocol/x/perpetuals/keeper/perpetual_test.go (1)
- 549-609: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [443-600]
The changes in the test cases within the
TestGetMarginRequirements_Successfunction correctly reflect the deprecation ofbasePositionNotionalas per the PR objective. The calculations and test case structures have been updated accordingly, and no issues are found in the provided hunks.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- protocol/indexer/events/events.pb.go
Files selected for processing (2)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/events/events.ts (2 hunks)
- proto/dydxprotocol/indexer/events/events.proto (1 hunks)
Additional comments: 1
proto/dydxprotocol/indexer/events/events.proto (1)
- 374-377: The deprecation of the
base_position_notionalfield aligns with the PR objective to move away from non-linear margin requirements. Ensure that all services consuming this field are updated to handle its deprecation appropriately.
| * the margin requirements increase at a rate of sqrt(size). | ||
| */ | ||
|
|
||
| /** @deprecated */ | ||
|
|
||
| basePositionNotional: Long; | ||
| } |
There was a problem hiding this comment.
💡 NOTE
Codebase verification is a beta feature.
The basePositionNotional field has been deprecated as per the PR objectives. The search results indicate that there are multiple references to basePositionNotional across various TypeScript files. These references need to be reviewed and updated to ensure they comply with the deprecation and removal of the basePositionNotional field.
Original review comment
The basePositionNotional field has been correctly marked as deprecated in the LiquidityTierUpsertEventV1 interface. Ensure that the rest of the codebase is updated to reflect this deprecation, including removing or replacing any logic that relies on this field.
| * the margin requirements increase at a rate of sqrt(size). | ||
| */ | ||
|
|
||
| /** @deprecated */ | ||
|
|
||
| base_position_notional: Long; |
There was a problem hiding this comment.
💡 NOTE
Codebase verification is a beta feature.
The base_position_notional field has been deprecated as per the PR objectives. The following files contain references to this field and should be reviewed to ensure they align with the deprecation:
indexer/packages/v4-protos/src/codegen/dydxprotocol/perpetuals/perpetual.tsindexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/events/events.ts
Original review comment
The base_position_notional field has been correctly marked as deprecated in the LiquidityTierUpsertEventV1SDKType interface. Ensure that the rest of the codebase is updated to reflect this deprecation, including removing or replacing any logic that relies on this field.
| bigQuoteQuantums: big.NewInt(123_456), | ||
| basePositionNotional: uint64(1_000_000), | ||
| initialMarginPpm: uint32(567_243), // 56.7243% | ||
| bigQuoteQuantums: big.NewInt(123_456), | ||
| // initial margin * margin adjustment * quote quantums | ||
| // = 56.7243% * 100% * 123_456 | ||
| // ~= 70029.5518 -> round up to 70030 |
There was a problem hiding this comment.
For my understanding, why is the resulting InitialMarginQuoteQuantums still the same after BasePositionNotional effect is is removed? Wouldn't we expect the return values in some test cases to change?
There was a problem hiding this comment.
are you taking about this test specifically? from the comment below - the adjustment was 100% so the margin requirements are the same before and after
Removed the comment since margin adjustment is no longer applicable, but pasted here as an FYI
// initial margin * margin adjustment * quote quantums
// = 56.7243% * 100% * 123_456
// ~= 70029.5518 -> round up to 70030
There are a few tests that got updated I believe.
| @@ -96,7 +96,7 @@ message LiquidityTier { | |||
| // The maximum position size at which the margin requirements are | |||
| // not increased over the default values. Above this position size, | |||
| // the margin requirements increase at a rate of sqrt(size). | |||
| uint64 base_position_notional = 5; | |||
There was a problem hiding this comment.
Is there an upgrade handler needed for this change? Or will the protocol just ignore this value after the upgrade?
There was a problem hiding this comment.
No, since the field is only marked as deprecated so it's backwards compatible.
| @@ -374,7 +374,7 @@ message LiquidityTierUpsertEventV1 { | |||
| // The maximum position size at which the margin requirements are | |||
There was a problem hiding this comment.
Might be helpful to explicitly comment this is deprecated since some version, similar to Cosmos SDK style
I think here we should say Deprecated since v3.x?
| @@ -694,134 +694,6 @@ func TestPlacePerpetualLiquidation_Deleveraging(t *testing.T) { | |||
| }, | |||
| }, | |||
| }, | |||
| `Can place and match liquidation order for subaccount with non-linear margin requirement`: { | |||
There was a problem hiding this comment.
It seems like we are mostly deleting unit tests. Do we have a test where a subaccount would've been liquidated under non-linear margin requirements, but is collateralized after this change?
There was a problem hiding this comment.
Hmm I kind of don't feel like this is really needed since we are deprecating a behavior so I wanted to wipe out traces of non linear margin requirement in our codebase to keep it lean.
Let me know if you disagree. I can update these deleted tests to have the correct expected values, but feels redundant since they are essentially the same as other tests in the test suite.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- protocol/indexer/events/events.pb.go
- protocol/x/perpetuals/types/perpetual.pb.go
Files selected for processing (4)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/events/events.ts (2 hunks)
- proto/dydxprotocol/indexer/events/events.proto (1 hunks)
- proto/dydxprotocol/perpetuals/perpetual.proto (1 hunks)
- protocol/x/perpetuals/types/liquidity_tier_test.go (4 hunks)
Additional comments: 7
indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/events/events.ts (2)
945-951: The deprecation of
basePositionNotionalinLiquidityTierUpsertEventV1is consistent with the PR objectives to remove non-linear margin requirements.978-983: The deprecation of
base_position_notionalinLiquidityTierUpsertEventV1SDKTypeis consistent with the PR objectives to remove non-linear margin requirements.proto/dydxprotocol/indexer/events/events.proto (1)
- 374-379: The deprecation comment for
base_position_notionalhas been correctly added as per the previous review suggestion.proto/dydxprotocol/perpetuals/perpetual.proto (1)
- 96-101: The deprecation of the
base_position_notionalfield in theLiquidityTiermessage is correctly annotated with thedeprecated = trueattribute. This change is consistent with the PR's objective to deprecate scaling margin requirements.protocol/x/perpetuals/types/liquidity_tier_test.go (3)
13-42: The test cases in
TestLiquidityTierValidatefunction have been updated to remove theBasePositionNotionalfield, aligning with the PR's objective to deprecate non-linear margin requirements. The remaining fields and error checks appear to be consistent with the expected validation logic.48-52: The instantiation of
LiquidityTierwithin the test functionTestLiquidityTierValidatecorrectly omits theBasePositionNotionalfield, which is consistent with the PR's objective and the summary provided.202-238: The test function
TestGetInitialMarginQuoteQuantumshas been updated to reflect the renaming ofGetAdjustedInitialMarginQuoteQuantumstoGetInitialMarginQuoteQuantums. The test cases have been adjusted accordingly, and the calculations within the test cases seem to be correct, with proper rounding logic applied to the expected results.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/events/events.ts (2 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/perpetuals/perpetual.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/events/events.ts
Additional comments: 2
indexer/packages/v4-protos/src/codegen/dydxprotocol/perpetuals/perpetual.ts (2)
198-207: The deprecation of the
basePositionNotionalfield in theLiquidityTierinterface is correctly annotated with a comment indicating its deprecation since v3.x.241-250: The deprecation of the
base_position_notionalfield in theLiquidityTierSDKTypeinterface is correctly annotated with a comment indicating its deprecation since v3.x.
Changelist
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.