[TRA-507] Add new gov message for upgrading markets: UpgradeIsolatedPerpetualToCross #2551
Conversation
WalkthroughThe changes introduce new functionality to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
You might run into a circular dependency since collateral pools reside in x/subaccounts which is dependent on x/perpetuals. I think you have 2 options:
|
protocol/x/listing/keeper/msg_server_upgrade_isolated_perpetual_to_cross_test.go
Show resolved
Hide resolved
protocol/x/listing/keeper/msg_upgrade_isolated_perpetual_to_cross.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
protocol/x/listing/types/msg_upgrade_isolated_perpetual_to_cross.go (1)
13-16: Consider handling AccAddressFromBech32 error in GetSigners.The error from
AccAddressFromBech32is discarded, which could mask invalid addresses ifValidateBasichasn't been called. While this is a common pattern in Cosmos SDK, it's worth documenting why the error is intentionally ignored.Consider adding a comment explaining the error handling strategy:
func (msg *MsgUpgradeIsolatedPerpetualToCross) GetSigners() []sdk.AccAddress { + // Error is intentionally ignored as validation is handled in ValidateBasic addr, _ := sdk.AccAddressFromBech32(msg.Authority) return []sdk.AccAddress{addr} }protocol/x/listing/keeper/msg_server_upgrade_isolated_perpetual_to_cross_test.go (2)
34-34: Fix typos in test case namesThere are typos in the test case names:
- "Invaid authority" should be "Invalid authority"
- "Invaid perpetual ID" should be "Invalid perpetual ID"
Also applies to: 41-41
16-48: Consider adding more edge cases for perpetual IDWhile the current test cases cover basic scenarios, consider adding these additional cases for better coverage:
- Maximum allowed perpetual ID
- Negative perpetual ID
"Failure: Invalid perpetual ID": { msg: types.MsgUpgradeIsolatedPerpetualToCross{ Authority: validAuthority, PerpetualId: 0, }, expectedErr: "Invalid perpetual ID", }, + "Failure: Negative perpetual ID": { + msg: types.MsgUpgradeIsolatedPerpetualToCross{ + Authority: validAuthority, + PerpetualId: -1, + }, + expectedErr: "Invalid perpetual ID", + }, + "Success: Maximum perpetual ID": { + msg: types.MsgUpgradeIsolatedPerpetualToCross{ + Authority: validAuthority, + PerpetualId: uint32(^uint32(0)), + }, + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
protocol/x/listing/keeper/msg_server_upgrade_isolated_perpetual_to_cross_test.go(1 hunks)protocol/x/listing/types/msg_upgrade_isolated_perpetual_to_cross.go(1 hunks)
🔇 Additional comments (7)
protocol/x/listing/types/msg_upgrade_isolated_perpetual_to_cross.go (3)
1-9: LGTM: Package and imports are well-organized.
The imports are correctly structured and include all necessary dependencies.
11-11: LGTM: Proper interface compliance check.
The compile-time check ensures MsgUpgradeIsolatedPerpetualToCross implements the sdk.Msg interface.
18-33: Consider additional validations for perpetual upgrade.
The basic validations look good, but given this is for upgrading isolated perpetuals to cross-margin, consider adding these checks:
- Verify if the perpetual exists
- Verify if the perpetual is currently in isolated mode
- Add upper bound check for PerpetualId
Let's check how these validations are handled in the keeper:
protocol/x/listing/keeper/msg_server_upgrade_isolated_perpetual_to_cross_test.go (4)
1-9: LGTM!
The package name and imports are appropriate for a test file.
11-14: LGTM!
The validAuthority variable is well-documented and appropriately uses a test constant.
50-60: LGTM!
The test execution logic is well-structured and follows best practices for table-driven tests.
1-60: Verify module dependencies for potential circular issues
Given the concerns about circular dependencies mentioned in the PR comments, let's verify that the test file's location in the listing module aligns with the architectural decisions.
✅ Verification successful
Based on the search results, I can see that the test file msg_server_upgrade_isolated_perpetual_to_cross_test.go is part of the listing module and has dependencies only on standard testing packages and internal constants. The file tests the basic validation of the MsgUpgradeIsolatedPerpetualToCross message type.
Looking at the imports and module dependencies:
- The listing module is imported by perpetuals and subaccounts modules for expected keeper interfaces
- The test file itself only imports from the same module and test utilities
- There are no circular dependencies visible in the import graph
No circular dependency concerns found
The test file is appropriately placed in the listing module since it tests a message handler specific to that module. The imports are clean and follow the expected module boundaries.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential circular dependencies between modules
# Look for imports between relevant modules
echo "Checking imports between modules:"
rg -l "github.com/dydxprotocol/v4-chain/protocol/x/(perpetuals|subaccounts|listing)" protocol/x/{perpetuals,subaccounts,listing}
# Look for specific type usage
echo "Checking for collateral pool usage:"
rg "CollateralPool" protocol/x/{perpetuals,subaccounts,listing}
Length of output: 36338
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
protocol/app/msgs/unsupported_msgs.go (1)
34-37: Remove "WIP" comment and consider message organization
- The "WIP" comment should be removed or replaced with a more descriptive comment explaining why these messages are unsupported.
- Consider grouping these messages with other similar functionality or adding a "listing" section header for better organization.
- // WIP + // listing - these messages are unsupported to prevent direct usage outside of governance "/dydxprotocol.listing.MsgUpgradeIsolatedPerpetualToCross": &listingtypes.MsgUpgradeIsolatedPerpetualToCross{}, "/dydxprotocol.listing.MsgUpgradeIsolatedPerpetualToCrossResponse": nil,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
protocol/app/msgs/unsupported_msgs.go(2 hunks)
🔇 Additional comments (2)
protocol/app/msgs/unsupported_msgs.go (2)
8-8: LGTM: Import addition follows existing patterns
The new import for listingtypes is appropriately placed and necessary for the new message types.
34-36: Verify if these messages should be marked as unsupported
According to the PR objectives, these messages are intended to be active governance messages for upgrading isolated perpetuals. However, they are currently listed in UnsupportedMsgSamples. Please verify if this is intentional or if these messages should be supported.
✅ Verification successful
Based on the gathered information, I can now provide the final response:
These messages should be marked as unsupported
The messages are correctly placed in UnsupportedMsgSamples. While there is a handler implementation in the listing module (msg_server_upgrade_isolated_perpetual_to_cross.go), the code appears to be work-in-progress ("WIP") and intentionally disabled. This is evidenced by:
- The message is properly registered in the module's services but listed in
UnsupportedMsgSamples - The implementation exists but is marked with "WIP"
- The message requires authority validation, suggesting it's a governance-related message that's not yet ready for use
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if these messages are used in governance handlers or other supported locations
# Search for message handler implementations
echo "Searching for message handlers..."
rg -A 5 "MsgUpgradeIsolatedPerpetualToCross"
# Check for any governance-related usage
echo "Checking governance-related usage..."
rg -l "MsgUpgradeIsolatedPerpetualToCross" protocol/x/gov/
Length of output: 35725
Script:
#!/bin/bash
# Let's check if this message is meant to be unsupported by looking at its implementation and usage
# Check if there's a handler implementation
echo "Checking for handler implementation..."
rg -A 10 "func.*UpgradeIsolatedPerpetualToCross" protocol/x/listing/keeper/
# Check if it's registered in the module's message server
echo "Checking message server registration..."
rg "RegisterServices" protocol/x/listing/module.go
# Check if it's meant to be a governance proposal
echo "Checking governance proposal registration..."
rg "RegisterProposalType" protocol/x/listing/
Length of output: 3881
Script:
#!/bin/bash
# Let's check if this message is actually registered in the module's message server
# and if it's currently marked as unsupported in other places
# Check module registration
echo "Checking module registration..."
rg -A 5 "RegisterServices.*Configurator" protocol/x/listing/module.go
# Check if it's registered in the keeper's message server
echo "Checking keeper message server..."
rg -A 10 "type msgServer struct" protocol/x/listing/keeper/
# Check if it's marked as unsupported in other places
echo "Checking other unsupported references..."
rg "UnsupportedMsgSamples" protocol/app/msgs/
Length of output: 2354
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
protocol/app/msgs/unsupported_msgs.go(2 hunks)protocol/app/msgs/unsupported_msgs_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- protocol/app/msgs/unsupported_msgs.go
🔇 Additional comments (1)
protocol/app/msgs/unsupported_msgs_test.go (1)
22-24: Remove or address the "WIP" comment before merging.
The "WIP" comment suggests this implementation might be incomplete. Please either remove the comment if the implementation is complete, or clarify what remaining work needs to be done.
protocol/x/listing/types/msg_upgrade_isolated_perpetual_to_cross.go
Outdated
Show resolved
Hide resolved
protocol/x/listing/keeper/msg_server_upgrade_isolated_perpetual_to_cross_test.go
Outdated
Show resolved
Hide resolved
protocol/x/listing/keeper/msg_server_upgrade_isolated_perpetual_to_cross_test.go
Outdated
Show resolved
Hide resolved
protocol/x/listing/keeper/msg_server_upgrade_isolated_perpetual_to_cross_test.go
Show resolved
Hide resolved
protocol/x/listing/types/msg_upgrade_isolated_perpetual_to_cross.go
Outdated
Show resolved
Hide resolved
protocol/x/listing/types/msg_upgrade_isolated_perpetual_to_cross.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
protocol/x/listing/keeper/msg_server_upgrade_isolated_perpetual_to_cross_test.go (2)
51-145: Consider adding edge cases for perpetual ID validation.While the test cases cover most scenarios, consider adding:
- Test case for negative perpetual ID
- Test case for zero perpetual ID
tests := map[string]struct { // ... existing fields ... }{ + "Failure: Negative perpetual ID": { + msg: &types.MsgUpgradeIsolatedPerpetualToCross{ + Authority: validAuthority, + PerpetualId: -1, + }, + expectedErr: "perpetual ID must be positive", + }, + "Failure: Zero perpetual ID": { + msg: &types.MsgUpgradeIsolatedPerpetualToCross{ + Authority: validAuthority, + PerpetualId: 0, + }, + expectedErr: "perpetual ID must be positive", + }, // ... existing test cases ... }
147-211: Consider extracting setup logic into helper functions.The test setup is quite lengthy and could be more maintainable if split into helper functions:
+func setupTestState( + t *testing.T, + ctx sdk.Context, + perpetualId uint32, + balances map[string]*big.Int, +) ( + *listingkeeper.Keeper, + *perpetualskeeper.Keeper, + *bankkeeper.Keeper, +) { + mockIndexerEventManager := &mocks.IndexerEventManager{} + keeper, _, _, pricesKeeper, perpetualsKeeper, _, _, assetsKeeper, + bankKeeper, subaccountsKeeper := keepertest.ListingKeepers( + t, + &mocks.BankKeeper{}, + mockIndexerEventManager, + ) + + // Create test state + keepertest.CreateTestMarkets(t, ctx, pricesKeeper) + keepertest.CreateTestLiquidityTiers(t, ctx, perpetualsKeeper) + require.NoError(t, keepertest.CreateUsdcAsset(ctx, assetsKeeper)) + keepertest.CreateTestPerpetuals(t, ctx, perpetualsKeeper) + + // Setup balances + setupBalances(t, ctx, perpetualId, balances, perpetualsKeeper, subaccountsKeeper, bankKeeper) + + return keeper, perpetualsKeeper, bankKeeper +}protocol/x/listing/keeper/listing_test.go (2)
65-65: Document the purpose of new keeper parametersWhile the change to add new keeper parameters is consistent with the reorganization mentioned in the PR objectives, it would be helpful to document the purpose of these new parameters, even if they're not used in this test.
220-221: Maintain consistent line wrapping styleThe line wrapping style here differs from the other
ListingKeeperscalls in the file. Consider maintaining a consistent style across all occurrences for better readability.- ctx, keeper, _, _, pricesKeeper, perpetualsKeeper, clobKeeper, marketMapKeeper, - _, _, _ := keepertest.ListingKeepers( + ctx, keeper, _, _, pricesKeeper, perpetualsKeeper, clobKeeper, marketMapKeeper, _, _, _ := keepertest.ListingKeepers(protocol/x/subaccounts/keeper/transfer.go (2)
509-552: Consider adding metrics for monitoring transfers.The function implementation looks good with proper validations and error handling. Consider adding metrics to track the transfer amounts and success/failure rates for monitoring purposes.
Add metrics similar to those used in the
DistributeFeesfunction:+ // Emit transfer metrics + metrics.AddSampleWithLabels( + metrics.InsuranceFundTransfer, + metrics.GetMetricValueFromBigInt(isolatedInsuranceFundBalance), + metrics.GetLabelForStringValue(metrics.TransferType, "isolated_to_cross"), + metrics.GetLabelForIntValue(metrics.PerpetualId, int(perpetualId)), + )
554-593: Consider adding metrics for monitoring collateral transfers.The function implementation looks good but would benefit from metrics tracking for monitoring purposes.
Add metrics similar to those used in the
DistributeFeesfunction:+ // Emit transfer metrics + metrics.AddSampleWithLabels( + metrics.CollateralTransfer, + metrics.GetMetricValueFromBigInt(isolatedCollateralPoolBalance.Amount), + metrics.GetLabelForStringValue(metrics.TransferType, "isolated_to_cross"), + metrics.GetLabelForIntValue(metrics.PerpetualId, int(perpetualId)), + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
protocol/testutil/keeper/listing.go(9 hunks)protocol/x/listing/keeper/listing_test.go(3 hunks)protocol/x/listing/keeper/msg_server_upgrade_isolated_perpetual_to_cross_test.go(1 hunks)protocol/x/subaccounts/keeper/transfer.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- protocol/testutil/keeper/listing.go
🧰 Additional context used
📓 Learnings (1)
protocol/x/subaccounts/keeper/transfer.go (2)
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.
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.
🔇 Additional comments (3)
protocol/x/listing/keeper/msg_server_upgrade_isolated_perpetual_to_cross_test.go (1)
1-28: LGTM! Package structure and imports are well-organized.
The test file follows Go conventions with appropriate package naming and imports. The validAuthority constant is properly defined using the governance module address.
protocol/x/listing/keeper/listing_test.go (2)
134-134: Consider adding test cases for cross-margin upgrade scenarios
Since this PR introduces functionality to upgrade isolated perpetuals to cross-margin, consider adding test cases that verify:
- The initial creation of an isolated perpetual (current coverage)
- The successful upgrade of an isolated perpetual to cross-margin
- Edge cases and validation during the upgrade process
Let's verify the existing test coverage for perpetual market types:
Line range hint 1-424: Consider architectural implications of module dependencies
Based on the PR objectives and the comment from shrenujb regarding circular dependencies, the addition of new keeper parameters might be addressing these concerns. However, we should carefully consider:
- Whether the current approach of adding new keeper parameters is the best solution
- If moving the collateral pool functionality to
x/perpetualsor creating the governance message inx/subaccountswould be cleaner alternatives
This architectural decision could impact future maintainability and module coupling.
Let's analyze the current module dependencies:
Changelist
GetInsuranceFundBalance,GetCrossInsuranceFundBalance) around to avoid cyclic dependenciesTest Plan
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