[TRA-507] Add tests for UpgradeIsolatedPerpetualToCross #2583
Conversation
WalkthroughThe changes introduce a new method 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (24)
protocol/x/listing/types/msg_upgrade_isolated_perpetual_to_cross.go (2)
13-16: Consider adding panic recovery for better robustness.While discarding the error is a common pattern here (as ValidateBasic handles validation), consider adding panic recovery for extra safety.
func (msg *MsgUpgradeIsolatedPerpetualToCross) GetSigners() []sdk.AccAddress { + defer func() { + if r := recover(); r != nil { + // Return empty slice on panic, as this is better than crashing + return []sdk.AccAddress{} + } + }() addr, _ := sdk.AccAddressFromBech32(msg.Authority) return []sdk.AccAddress{addr} }
18-33: Consider additional validations for PerpetualId.While the current validations are good, consider adding an upper bound check for PerpetualId to prevent unreasonably large values.
func (msg *MsgUpgradeIsolatedPerpetualToCross) ValidateBasic() error { if _, err := sdk.AccAddressFromBech32(msg.Authority); err != nil { return errorsmod.Wrap( ErrInvalidAuthority, fmt.Sprintf( "authority '%s' must be a valid bech32 address, but got error '%v'", msg.Authority, err.Error(), ), ) } if msg.PerpetualId == 0 { return ErrInvalidPerpetualId } + // Add an upper bound check to prevent unreasonable values + if msg.PerpetualId > MaxPerpetualId { + return errorsmod.Wrapf( + ErrInvalidPerpetualId, + "perpetual id %d exceeds maximum allowed value %d", + msg.PerpetualId, + MaxPerpetualId, + ) + } return nil }protocol/x/listing/keeper/msg_server_upgrade_isolated_perpetual_to_cross.go (1)
24-35: Consider adding logging and eventsWhile the implementation is correct, consider enhancing observability by:
- Adding logging for successful upgrades
- Emitting events to track these administrative actions on-chain
Here's a suggested implementation:
if err != nil { return nil, err } + + // Log successful upgrade + k.Logger(ctx).Info("Successfully upgraded isolated perpetual to cross", + "perpetual_id", msg.PerpetualId, + "authority", msg.Authority, + ) + + // Emit event + if err := ctx.EventManager().EmitTypedEvent( + &types.EventPerpetualUpgradeToIsolated{ + PerpetualId: msg.PerpetualId, + Authority: msg.Authority, + }); err != nil { + return nil, err + } return &types.MsgUpgradeIsolatedPerpetualToCrossResponse{}, nilprotocol/x/listing/types/errors.go (2)
43-47: Consider simplifying the error message.The error message "Authority is invalid" can be simplified to "invalid authority" to be more concise and consistent with other error messages in the module.
ErrInvalidAuthority = errorsmod.Register( ModuleName, 7, - "Authority is invalid", + "invalid authority", )
49-53: Fix capitalization in error message.The error message should use consistent capitalization. "ID" should be in uppercase as it's an acronym.
ErrInvalidPerpetualId = errorsmod.Register( ModuleName, 8, - "Invalid perpetual ID", + "invalid perpetual ID", )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"
- "Failure: Invaid authority": { + "Failure: Invalid authority": { - "Failure: Invaid perpetual ID": { + "Failure: Invalid perpetual ID": {Also applies to: 41-41
16-60: Consider adding more test cases for comprehensive validationWhile the current test cases cover basic validation, consider adding:
- Test case for maximum allowed perpetual ID value
- Test case for malformed bech32 address (currently only testing empty and invalid format)
- Consider grouping test cases by the field being tested (authority vs perpetual ID) for better organization
Example structure:
tests := map[string]struct { msg types.MsgUpgradeIsolatedPerpetualToCross expectedErr string }{ "Authority Validation": { "Success: Valid authority": {...}, "Failure: Empty authority": {...}, "Failure: Invalid authority format": {...}, "Failure: Malformed bech32 address": {...}, }, "Perpetual ID Validation": { "Success: Valid perpetual ID": {...}, "Failure: Zero perpetual ID": {...}, "Failure: Exceeds maximum value": {...}, }, }indexer/packages/v4-protos/src/codegen/dydxprotocol/listing/tx.rpc.msg.ts (2)
15-20: Enhance JSDoc documentation for better clarity.While the JSDoc comment is clear, it could be more informative by including:
- Parameters description
- Return value description
- Any authorization requirements
- Potential error cases
Consider expanding the JSDoc like this:
/** * UpgradeIsolatedPerpetualToCross upgrades a perpetual from isolated to cross * margin + * @param request - Contains perpetualId and authority for the upgrade + * @returns Response indicating the success of the upgrade operation + * @throws If the caller lacks necessary permissions or if the perpetual is invalid */
51-55: Consider adding explicit error handling.While the implementation follows the established pattern, consider adding explicit error handling to provide more detailed error information to callers.
Consider wrapping the implementation like this:
upgradeIsolatedPerpetualToCross(request: MsgUpgradeIsolatedPerpetualToCross): Promise<MsgUpgradeIsolatedPerpetualToCrossResponse> { + try { const data = MsgUpgradeIsolatedPerpetualToCross.encode(request).finish(); const promise = this.rpc.request("dydxprotocol.listing.Msg", "UpgradeIsolatedPerpetualToCross", data); return promise.then(data => MsgUpgradeIsolatedPerpetualToCrossResponse.decode(new _m0.Reader(data))); + } catch (error) { + throw new Error(`Failed to upgrade perpetual: ${error.message}`); + } }proto/dydxprotocol/listing/tx.proto (2)
26-29: Consider enhancing the RPC method documentation.While the current comment describes the basic purpose, it would be helpful to add more details about:
- The requirements and preconditions for upgrading
- The implications of upgrading from isolated to cross margin
- Any side effects or important considerations
- // UpgradeIsolatedPerpetualToCross upgrades a perpetual from isolated to cross - // margin + // UpgradeIsolatedPerpetualToCross upgrades a perpetual from isolated to cross margin. + // Requirements: + // - The perpetual must exist and be in isolated margin mode + // - Only the authority can perform this upgrade + // Effects: + // - Changes the perpetual's margin type from isolated to cross + // - Affects how margin requirements are calculated for positions
81-89: Consider adding field validation for perpetual_id.While the field type is correct, consider adding validation constraints to ensure the perpetual_id is within valid bounds.
// ID of the perpetual to be upgraded to CROSS - uint32 perpetual_id = 2; + uint32 perpetual_id = 2 [(gogoproto.moretags) = "validate:\"required,min=0\""];protocol/testutil/keeper/listing.go (1)
42-44: Consider renamingbankKeeper_outfor consistency.The suffix
_outinbankKeeper_outdoesn't follow typical Go naming conventions. Consider using a more descriptive name without the underscore, such asbankKeeperBaseor simplybankKeeper.protocol/x/listing/keeper/listing.go (2)
156-191: Consider wrapping errors for better contextThe error handling could be improved by wrapping errors with additional context about the operation being performed.
Consider applying this change:
func (k Keeper) UpgradeIsolatedPerpetualToCross( ctx sdk.Context, perpetualId uint32, ) error { perpetual, err := k.PerpetualsKeeper.GetPerpetual(ctx, perpetualId) if err != nil { - return err + return fmt.Errorf("failed to get perpetual %d: %w", perpetualId, err) } if perpetual.Params.GetMarketType() != perpetualtypes.PerpetualMarketType_PERPETUAL_MARKET_TYPE_ISOLATED { return fmt.Errorf("perpetual %d is not an isolated perpetual and cannot be upgraded to cross", perpetualId) } err = k.SubaccountsKeeper.TransferIsolatedInsuranceFundToCross(ctx, perpetualId) if err != nil { - return err + return fmt.Errorf("failed to transfer isolated insurance fund for perpetual %d: %w", perpetualId, err) } err = k.SubaccountsKeeper.TransferIsolatedCollateralToCross(ctx, perpetualId) if err != nil { - return err + return fmt.Errorf("failed to transfer isolated collateral for perpetual %d: %w", perpetualId, err) } _, err = k.PerpetualsKeeper.SetPerpetualMarketType( ctx, perpetualId, perpetualtypes.PerpetualMarketType_PERPETUAL_MARKET_TYPE_CROSS, ) if err != nil { - return err + return fmt.Errorf("failed to set perpetual %d market type to cross: %w", perpetualId, err) } return nil }
188-189: Address TODO comment for indexer changesThe TODO comment indicates that indexer changes need to be propagated. This should be addressed before the PR is merged to ensure data consistency.
Would you like me to help create a GitHub issue to track the indexer integration work?
protocol/x/subaccounts/keeper/transfer.go (2)
582-587: Add logging and metrics for transfer monitoring.Consider adding logging and metrics to track successful transfers during the upgrade process. This will help with monitoring and debugging.
+ // Log successful transfer + ctx.Logger().Info( + "Transferred isolated collateral to cross pool", + "perpetual_id", perpetualId, + "amount", isolatedCollateralPoolBalance.String(), + ) + + // Add metric for monitoring + metrics.AddSampleWithLabels( + metrics.IsolatedToCrossTransfer, + metrics.GetMetricValueFromString(isolatedCollateralPoolBalance.String()), + metrics.GetLabelForStringValue(metrics.TransferType, "collateral"), + metrics.GetLabelForIntValue(metrics.PerpetualId, int(perpetualId)), + ) + return k.bankKeeper.SendCoins( ctx, isolatedCollateralPoolAddr, crossCollateralPoolAddr, []sdk.Coin{isolatedCollateralPoolBalance}, )
508-588: Consider adding transaction boundary.Both transfer functions modify bank state independently. Consider wrapping them in a single transaction to ensure atomicity during the upgrade process.
protocol/x/listing/keeper/listing_test.go (3)
429-437: Consider adding more test cases for better coverage.Consider adding the following test cases:
- Test case for non-existent perpetual ID
- Test case verifying event emission if applicable
- Test case validating liquidity tier changes if the upgrade affects them
tests := map[string]struct { perpetualId uint32 isolatedInsuranceFundBalance *big.Int isolatedCollateralPoolBalance *big.Int crossInsuranceFundBalance *big.Int crossCollateralPoolBalance *big.Int expectedErr string }{ + "failure - non-existent perpetual": { + perpetualId: 999, + isolatedInsuranceFundBalance: big.NewInt(1), + isolatedCollateralPoolBalance: big.NewInt(1), + crossInsuranceFundBalance: big.NewInt(1), + crossCollateralPoolBalance: big.NewInt(1), + expectedErr: "perpetual 999 not found", + },
517-539: Consider refactoring repeated test setup code.The fund initialization code could be extracted into a helper function to improve readability and maintainability.
func fundAccounts( t *testing.T, ctx sdk.Context, bankKeeper types.BankKeeper, fundingData []struct { addr sdk.AccAddress amount *big.Int }, ) { for _, data := range fundingData { if data.amount.Cmp(big.NewInt(0)) != 0 { err := bank_testutil.FundAccount( ctx, data.addr, sdk.Coins{ sdk.NewCoin(constants.Usdc.Denom, sdkmath.NewIntFromBigInt(data.amount)), }, bankKeeper, ) require.NoError(t, err) } } }
559-578: Consider refactoring balance verification code.Similar to the funding code, the balance verification could be extracted into a helper function.
func verifyBalances( t *testing.T, ctx sdk.Context, bankKeeper types.BankKeeper, expectedBalances []struct { addr sdk.AccAddress amount *big.Int }, ) { for _, data := range expectedBalances { require.Equal( t, sdk.NewCoin( asstypes.AssetUsdc.Denom, sdkmath.NewIntFromBigInt(data.amount), ), bankKeeper.GetBalance(ctx, data.addr, asstypes.AssetUsdc.Denom), ) } }protocol/x/subaccounts/keeper/subaccount.go (2)
833-850: Consider adding input validation and consistent error handling.The function has several potential improvements:
- Add validation for
perpetualIdparameter- Consider returning an error instead of panicking when USDC asset is not found, for consistency with other error cases
- Add nil check before accessing
insuranceFundBalance.Amountto prevent potential nil pointer dereference-func (k Keeper) GetInsuranceFundBalance(ctx sdk.Context, perpetualId uint32) (balance *big.Int) { +func (k Keeper) GetInsuranceFundBalance(ctx sdk.Context, perpetualId uint32) (*big.Int, error) { usdcAsset, exists := k.assetsKeeper.GetAsset(ctx, assettypes.AssetUsdc.Id) if !exists { - panic("GetInsuranceFundBalance: Usdc asset not found in state") + return nil, fmt.Errorf("GetInsuranceFundBalance: Usdc asset not found in state") } insuranceFundAddr, err := k.perpetualsKeeper.GetInsuranceFundModuleAddress(ctx, perpetualId) if err != nil { - return nil + return nil, fmt.Errorf("GetInsuranceFundBalance: failed to get insurance fund module address: %w", err) } insuranceFundBalance := k.bankKeeper.GetBalance( ctx, insuranceFundAddr, usdcAsset.Denom, ) + if insuranceFundBalance == nil || insuranceFundBalance.Amount == nil { + return nil, fmt.Errorf("GetInsuranceFundBalance: nil balance returned from bank keeper") + } // Return as big.Int. - return insuranceFundBalance.Amount.BigInt() + return insuranceFundBalance.Amount.BigInt(), nil
852-865: Align error handling with GetInsuranceFundBalance.For consistency with the suggested improvements to
GetInsuranceFundBalance, consider applying similar error handling patterns here.-func (k Keeper) GetCrossInsuranceFundBalance(ctx sdk.Context) (balance *big.Int) { +func (k Keeper) GetCrossInsuranceFundBalance(ctx sdk.Context) (*big.Int, error) { usdcAsset, exists := k.assetsKeeper.GetAsset(ctx, assettypes.AssetUsdc.Id) if !exists { - panic("GetCrossInsuranceFundBalance: Usdc asset not found in state") + return nil, fmt.Errorf("GetCrossInsuranceFundBalance: Usdc asset not found in state") } insuranceFundBalance := k.bankKeeper.GetBalance( ctx, perptypes.InsuranceFundModuleAddress, usdcAsset.Denom, ) + if insuranceFundBalance == nil || insuranceFundBalance.Amount == nil { + return nil, fmt.Errorf("GetCrossInsuranceFundBalance: nil balance returned from bank keeper") + } // Return as big.Int. - return insuranceFundBalance.Amount.BigInt() + return insuranceFundBalance.Amount.BigInt(), nilprotocol/x/subaccounts/keeper/subaccount_test.go (3)
6137-6147: Consider adding test cases for additional edge cases.While the test coverage is good, consider adding test cases for:
- Insurance fund balance with negative values (if applicable)
- Behavior when perpetual ID doesn't exist
- Race conditions with concurrent balance updates
6194-6199: Consider improving error handling test cases.The panic test case could be enhanced by:
- Testing more error scenarios
- Using more specific error types instead of generic errors
- Adding test cases for other potential error conditions
"panics when asset not found in state": { assets: []asstypes.Asset{}, perpetualId: 0, - expectedError: errors.New("GetInsuranceFundBalance: Usdc asset not found in state"), + expectedError: asstypes.ErrAssetNotFound, },
6201-6259: Consider improving test helper functions and setup.The test setup could be improved by:
- Extracting common setup code into helper functions
- Adding cleanup to ensure test isolation
- Adding logging for better debugging
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
protocol/x/listing/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (29)
indexer/packages/v4-protos/src/codegen/dydxprotocol/listing/tx.rpc.msg.ts(4 hunks)indexer/packages/v4-protos/src/codegen/dydxprotocol/listing/tx.ts(2 hunks)proto/dydxprotocol/listing/tx.proto(2 hunks)protocol/app/app.go(1 hunks)protocol/app/msgs/all_msgs.go(1 hunks)protocol/app/msgs/internal_msgs.go(1 hunks)protocol/app/msgs/internal_msgs_test.go(1 hunks)protocol/lib/ante/internal_msg.go(1 hunks)protocol/mocks/ClobKeeper.go(0 hunks)protocol/testutil/keeper/listing.go(9 hunks)protocol/x/clob/keeper/deleveraging.go(1 hunks)protocol/x/clob/keeper/deleveraging_test.go(0 hunks)protocol/x/clob/keeper/keeper.go(2 hunks)protocol/x/clob/types/expected_keepers.go(1 hunks)protocol/x/clob/types/liquidations_keeper.go(0 hunks)protocol/x/listing/keeper/keeper.go(2 hunks)protocol/x/listing/keeper/listing.go(2 hunks)protocol/x/listing/keeper/listing_test.go(5 hunks)protocol/x/listing/keeper/msg_server_upgrade_isolated_perpetual_to_cross.go(1 hunks)protocol/x/listing/keeper/msg_server_upgrade_isolated_perpetual_to_cross_test.go(1 hunks)protocol/x/listing/types/errors.go(1 hunks)protocol/x/listing/types/expected_keepers.go(2 hunks)protocol/x/listing/types/msg_upgrade_isolated_perpetual_to_cross.go(1 hunks)protocol/x/perpetuals/keeper/perpetual.go(1 hunks)protocol/x/perpetuals/keeper/perpetual_test.go(2 hunks)protocol/x/subaccounts/keeper/subaccount.go(2 hunks)protocol/x/subaccounts/keeper/subaccount_test.go(2 hunks)protocol/x/subaccounts/keeper/transfer.go(2 hunks)protocol/x/subaccounts/types/expected_keepers.go(3 hunks)
💤 Files with no reviewable changes (3)
- protocol/mocks/ClobKeeper.go
- protocol/x/clob/keeper/deleveraging_test.go
- protocol/x/clob/types/liquidations_keeper.go
🧰 Additional context used
📓 Learnings (8)
protocol/x/clob/keeper/deleveraging.go (3)
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.
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.
protocol/x/clob/keeper/keeper.go (3)
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/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.
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.
protocol/x/clob/types/expected_keepers.go (3)
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.
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.
protocol/x/listing/types/expected_keepers.go (1)
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.
protocol/x/subaccounts/keeper/subaccount.go (3)
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.
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.
protocol/x/subaccounts/keeper/subaccount_test.go (2)
<retrieved_learning>
Learnt from: hwray
PR: #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.
</retrieved_learning>
<retrieved_learning>
Learnt from: hwray
PR: #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.
</retrieved_learning>
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.
protocol/x/subaccounts/types/expected_keepers.go (1)
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.
🪛 Biome
indexer/packages/v4-protos/src/codegen/dydxprotocol/listing/tx.ts
[error] 130-130: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 136-136: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
🔇 Additional comments (47)
protocol/x/listing/types/msg_upgrade_isolated_perpetual_to_cross.go (2)
1-9: LGTM! Clean package structure and imports.
The package declaration and imports are well-organized and follow Go conventions.
11-11: Verify the message struct definition exists.
While the interface implementation check is correct, we should verify the existence of the MsgUpgradeIsolatedPerpetualToCross struct definition.
✅ Verification successful
Message struct is properly defined
The MsgUpgradeIsolatedPerpetualToCross struct is correctly defined in:
- Proto definition:
proto/dydxprotocol/listing/tx.proto - Generated Go code:
protocol/x/listing/types/tx.pb.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the struct definition in proto files and generated Go code
rg -t proto -t go "message MsgUpgradeIsolatedPerpetualToCross|type MsgUpgradeIsolatedPerpetualToCross struct"
Length of output: 141
Script:
#!/bin/bash
# Search for the struct definition in all files
rg "message MsgUpgradeIsolatedPerpetualToCross|type MsgUpgradeIsolatedPerpetualToCross struct"
# Also search for proto file containing this message
fd ".*\.proto" --exec grep -l "MsgUpgradeIsolatedPerpetualToCross"
Length of output: 455
protocol/x/listing/keeper/msg_server_upgrade_isolated_perpetual_to_cross.go (2)
1-11: LGTM: Clean package declaration and imports
The imports are well-organized and all are actively used in the implementation.
12-23: LGTM: Proper authority validation
The authority check is implemented correctly with proper error handling and descriptive error messages.
protocol/x/listing/keeper/msg_server_upgrade_isolated_perpetual_to_cross_test.go (3)
1-9: LGTM: Package and imports are well-organized
The package name follows Go conventions for test files, and imports are logically organized.
11-14: LGTM: Test constants are well-defined
Good practice using a predefined test account from constants package with clear documentation.
16-60: LGTM: Well-structured test implementation
The test implementation follows good practices:
- Uses table-driven tests effectively
- Validates both success and failure cases
- Provides clear error message validation
- Uses require assertions appropriately
While some improvements were suggested above, the current implementation provides good test coverage for the basic validation of MsgUpgradeIsolatedPerpetualToCross.
protocol/x/listing/keeper/keeper.go (3)
16-24: LGTM: Clean addition of SubaccountsKeeper field
The addition of SubaccountsKeeper follows the established pattern and is properly typed. The field placement is logical, grouped with other keeper dependencies.
36-48: LGTM: Constructor properly updated for SubaccountsKeeper
The constructor changes are clean and maintain consistency with the struct definition.
Line range hint 1-65: Verify implementation of UpgradeIsolatedPerpetualToCross method
The AI summary mentions a new UpgradeIsolatedPerpetualToCross method, but it's not present in this file. Let's verify its implementation.
✅ Verification successful
Let me gather more information about the implementation of UpgradeIsolatedPerpetualToCross in the keeper.
Let me check the rest of the implementation and the test file to fully understand the functionality.
I have enough information to generate the final response. The implementation of UpgradeIsolatedPerpetualToCross is present in the codebase and properly implemented in the keeper.
The UpgradeIsolatedPerpetualToCross method is correctly implemented in the listing keeper
The method is implemented in protocol/x/listing/keeper/listing.go with the following functionality:
- Validates that the perpetual exists and is in isolated mode
- Transfers isolated insurance fund to cross via SubaccountsKeeper
- Transfers isolated collateral to cross via SubaccountsKeeper
- Updates the perpetual market type to cross via PerpetualsKeeper
The implementation is also well-tested in listing_test.go with various test cases including success scenarios and edge cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the UpgradeIsolatedPerpetualToCross implementation
rg -l "func.*UpgradeIsolatedPerpetualToCross"
# Search for any references to this method
rg "UpgradeIsolatedPerpetualToCross"
# Look for any interfaces defining this method
ast-grep --pattern 'interface {
$$$
UpgradeIsolatedPerpetualToCross($$$)
$$$
}'
Length of output: 19868
Script:
#!/bin/bash
# Look at the implementation in listing.go
rg -A 20 "func.*UpgradeIsolatedPerpetualToCross" protocol/x/listing/keeper/listing.go
Length of output: 788
Script:
#!/bin/bash
# Get the full implementation from listing.go
rg -A 30 "func.*UpgradeIsolatedPerpetualToCross" protocol/x/listing/keeper/listing.go
# Look at the test implementation
rg -A 30 "TestUpgradeIsolatedPerpetualToCross" protocol/x/listing/keeper/listing_test.go
Length of output: 2256
protocol/x/subaccounts/types/expected_keepers.go (2)
32-38: LGTM! Clean interface method addition.
The GetAsset method follows good practices with clear input parameters and return values.
102-102: Consider using sdk.Context instead of context.Context for consistency.
The GetBalance method uses context.Context while other methods in the interface use sdk.Context. This inconsistency might cause issues when implementing the interface.
Let's verify the context type usage in other bank keeper methods:
protocol/x/listing/types/expected_keepers.go (2)
67-76: LGTM! Well-designed interface methods for perpetual market upgrades.
The new methods GetPerpetual and SetPerpetualMarketType are well-structured with appropriate parameters and return types, maintaining consistency with the interface's existing patterns.
103-112: LGTM! Clean interface design for fund transfers.
The new SubaccountsKeeper interface is well-structured with focused methods for handling different types of transfers during perpetual market upgrades.
Let's verify the keeper implementation has proper validation and error handling:
✅ Verification successful
Implementation validates the interface design with proper safeguards
The keeper implementation confirms the interface is well-designed with appropriate validation and error handling:
- Both methods validate perpetual existence before proceeding
- Proper null checks (zero balance handling in insurance fund transfer)
- Clear error propagation from dependent keepers
- Panic conditions only for critical system invariants (USDC asset existence)
- Comprehensive documentation explaining the purpose and state modifications
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation of these transfer methods
ast-grep --pattern 'func ($keeper $_) TransferIsolatedInsuranceFundToCross(ctx $_.$_, perpetualId $_) error {
$$$
}'
ast-grep --pattern 'func ($keeper $_) TransferIsolatedCollateralToCross(ctx $_.$_, perpetualId $_) error {
$$$
}'
Length of output: 238
Script:
#!/bin/bash
# Let's try a broader search for the implementation using ripgrep
rg -A 10 "func.*TransferIsolatedInsuranceFundToCross" --type go
rg -A 10 "func.*TransferIsolatedCollateralToCross" --type go
# Also search for any test files that might contain these method names
rg "TransferIsolatedInsuranceFundToCross|TransferIsolatedCollateralToCross" --type go "*_test.go"
Length of output: 1934
Script:
#!/bin/bash
# Let's get the complete implementation of both methods
rg -B 5 -A 20 "func \(k Keeper\) TransferIsolatedInsuranceFundToCross" protocol/x/subaccounts/keeper/transfer.go
rg -B 5 -A 20 "func \(k Keeper\) TransferIsolatedCollateralToCross" protocol/x/subaccounts/keeper/transfer.go
Length of output: 1995
indexer/packages/v4-protos/src/codegen/dydxprotocol/listing/tx.rpc.msg.ts (3)
3-3: LGTM! Import statement follows existing patterns.
The new message types are properly imported and follow the established naming conventions.
30-30: LGTM! Method binding follows class pattern.
The method is properly bound in the constructor, maintaining consistency with other methods.
51-55: Verify test coverage for the new RPC method.
Since this PR focuses on adding tests for the UpgradeIsolatedPerpetualToCross functionality, ensure comprehensive test coverage for this RPC method.
proto/dydxprotocol/listing/tx.proto (1)
91-93: LGTM!
The response message follows the established pattern in the codebase and is appropriate for this operation.
protocol/lib/ante/internal_msg.go (1)
106-106: LGTM! Verify security implications of internal message classification.
The addition of MsgUpgradeIsolatedPerpetualToCross as an internal message follows the established pattern and is appropriately placed with other listing module messages.
Let's verify that this message type is properly secured in its implementation:
✅ Verification successful
Based on the search results, I can now verify the security implications of marking MsgUpgradeIsolatedPerpetualToCross as an internal message. The results show:
- The message handler in
protocol/x/listing/keeper/msg_server_upgrade_isolated_perpetual_to_cross.goproperly checks authority:
if !k.Keeper.HasAuthority(msg.Authority) {
return nil, errorsmod.Wrapf(
govtypes.ErrInvalidSigner,
"invalid authority %s",
msg.Authority,
)
}- The message type in
protocol/x/listing/types/msg_upgrade_isolated_perpetual_to_cross.govalidates the authority inValidateBasic():
if _, err := sdk.AccAddressFromBech32(msg.Authority); err != nil {
return errorsmod.Wrap(
ErrInvalidAuthority,
fmt.Sprintf(
"authority '%s' must be a valid bech32 address",
msg.Authority,
)
)
}- The message is properly registered as an internal message in both
protocol/app/msgs/internal_msgs.goandprotocol/lib/ante/internal_msg.go.
LGTM! Security checks for internal message authority are properly implemented.
The message has appropriate authority validation at both the message and handler level, and its classification as an internal message is consistent with other privileged operations in the listing module.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the security checks in the message handler and type definition
# Check the message handler implementation for authority checks
ast-grep --pattern 'func $_(ctx $_, msg *listing.MsgUpgradeIsolatedPerpetualToCross) $_ {
$$$
if !$hasAuth($authority) {
$$$
}
$$$
}'
# Check the message type definition for authority validation
ast-grep --pattern 'func (msg *MsgUpgradeIsolatedPerpetualToCross) ValidateBasic() error {
$$$
if !$validAuth($authority) {
$$$
}
$$$
}'
Length of output: 298
Script:
#!/bin/bash
# Let's try a different approach to verify the security checks
# Find the message type definition file
fd "msg.*\.go" --type f --exec rg -l "MsgUpgradeIsolatedPerpetualToCross"
# Search for authority checks in handler implementation
rg -A 5 -B 5 "MsgUpgradeIsolatedPerpetualToCross"
# Look for any authority or permission related constants/interfaces
rg "Authority|Permission" --type go
Length of output: 176869
protocol/x/clob/types/expected_keepers.go (2)
80-85: LGTM! Clean interface definition for GetInsuranceFundBalance.
The method signature is well-defined and follows Go conventions. Based on the learnings, this is part of a refactoring effort to consolidate insurance fund balance management in the SubaccountsKeeper.
86-90: LGTM! Clean interface definition for GetCrossInsuranceFundBalance.
The method signature is well-defined and follows Go conventions. This addition complements the GetInsuranceFundBalance method and maintains consistency in the insurance fund balance management interface.
protocol/testutil/keeper/listing.go (5)
14-14: LGTM: Import additions are appropriate.
The new imports for assets and subaccounts keepers are correctly added and necessary for the new dependencies.
Also applies to: 18-18
Line range hint 121-134: LGTM: Proper initialization of new keepers.
The initialization of assetsKeeper and subaccountsKeeper is correctly implemented with proper dependency ordering and all necessary parameters.
Also applies to: 141-146
Line range hint 159-181: LGTM: CLOB keeper dependencies are properly updated.
The new dependencies are correctly integrated into the CLOB keeper creation, maintaining proper initialization order.
189-189: LGTM: Return statement properly updated.
The return statement correctly includes all new keeper instances in the appropriate order.
Line range hint 200-221: Verify keeper constructor parameter alignment.
The changes to createListingKeeper look correct. Let's verify that the parameters match the actual keeper constructor.
✅ Verification successful
Parameters in createListingKeeper match the keeper constructor
Looking at the NewKeeper function in protocol/x/listing/keeper/keeper.go, the parameters match exactly with those in createListingKeeper:
cdcstoreKeyauthoritiespricesKeeperclobKeepermarketMapKeeperperpetualsKeepersubaccountsKeepervaultKeeper
The parameters are in the correct order and of the expected types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the keeper constructor parameters match
# Search for the NewKeeper function definition in the listing keeper
ast-grep --pattern 'func NewKeeper($$$) *keeper.Keeper {
$$$
}'
Length of output: 67
Script:
#!/bin/bash
# Let's try a different approach to find the keeper constructor
# First, let's locate the keeper package file
fd "keeper.*\.go" --type f --exec rg -l "NewKeeper"
# Then let's search for the NewKeeper function with context
rg "func NewKeeper" -A 15
Length of output: 22498
protocol/app/msgs/internal_msgs_test.go (1)
119-120: LGTM! Verify message registration.
The new message types are correctly added to the governance message list, maintaining alphabetical order within the "listing" section.
Let's verify that these messages are properly registered in the message registry:
✅ Verification successful
Message types are properly registered in the message registry ✅
The new message types are correctly registered in all_msgs.go, matching the test expectations:
/dydxprotocol.listing.MsgUpgradeIsolatedPerpetualToCross/dydxprotocol.listing.MsgUpgradeIsolatedPerpetualToCrossResponse
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the new message types are registered in the message registry.
# Check for message registration in the registry
rg -A 1 "MsgUpgradeIsolatedPerpetualToCross" protocol/app/msgs/all_msgs.go
Length of output: 223
protocol/x/listing/keeper/listing.go (1)
169-177: Verify atomic transaction handling
The method performs multiple state changes (insurance fund transfer, collateral transfer, market type update) that should be atomic. Need to verify that these operations are properly handled within a transaction.
✅ Verification successful
Based on the investigation of the codebase, I can now provide a final response:
Atomic transactions are handled by Cosmos SDK context
The operations are inherently atomic as they are executed within a Cosmos SDK transaction context. The bankKeeper.SendCoins operations used in both transfer methods are atomic by design, and the Cosmos SDK's transaction mechanism ensures that either all state changes succeed or none do. If any operation fails, all changes are rolled back automatically.
Key observations:
- Both transfer methods ultimately use
bankKeeper.SendCoinswhich is an atomic operation - The operations are executed sequentially within the same Cosmos SDK context
- Any error in any operation triggers a complete rollback through the SDK's transaction handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the keeper methods use transaction boundaries
# Look for transaction handling in the keeper implementation
rg -A 5 "Begin.*Transaction|Commit.*Transaction|Rollback.*Transaction" --type go
# Check if the called methods handle transactions
ast-grep --pattern 'func $_(ctx $_, perpetualId $_) error {
$$$
Begin$_Transaction($_)
$$$
}'
Length of output: 184
Script:
#!/bin/bash
# Let's check the implementation of the called methods to understand transaction handling
# Check SubaccountsKeeper's methods implementation
ast-grep --pattern 'func (k $_) TransferIsolatedInsuranceFundToCross(ctx $_, perpetualId $_) error {
$$$
}'
ast-grep --pattern 'func (k $_) TransferIsolatedCollateralToCross(ctx $_, perpetualId $_) error {
$$$
}'
# Also check if there's any transaction handling in the parent function
rg -B 5 -A 10 "TransferIsolatedInsuranceFundToCross" --type go
Length of output: 8905
Script:
#!/bin/bash
# Let's check the Cosmos SDK's bank keeper implementation and the context to understand transaction handling
# Check if the context contains transaction management
rg -B 2 -A 2 "type Context" --type go
# Look for bank keeper's SendCoins implementation
ast-grep --pattern 'func $_ SendCoins(ctx $_, from $_, to $_, amt $_) error {
$$$
}'
# Check if there's any transaction management in the parent function that calls these methods
rg -B 10 -A 5 "func.*SetPerpetualMarketType" --type go
Length of output: 3437
protocol/app/msgs/internal_msgs.go (2)
159-162: LGTM! Formatting changes maintain consistency.
The reformatting of existing message types maintains consistent alignment with other entries in the map.
163-164: LGTM! New message types align with PR objectives.
The addition of MsgUpgradeIsolatedPerpetualToCross and its response type is consistent with:
- The PR's objective to implement the upgrade functionality
- The existing pattern of message/response pairs in the map
- The module's message naming conventions
Let's verify the message type definitions exist:
✅ Verification successful
Message types are properly defined and integrated across the codebase
The verification confirms that:
- The message types are properly defined in
proto/dydxprotocol/listing/tx.proto - The implementation exists across multiple files:
- Generated code in
protocol/x/listing/types/ - Keeper implementation in
protocol/x/listing/keeper/ - Integration in message handling in
protocol/app/msgs/ - Indexer support in
indexer/packages/v4-protos/
- Generated code in
The message types are correctly structured with:
- Authority signer requirement
- Proper field definitions
- Response message type
- Integration with the Msg service
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new message types are properly defined in the proto files
# and their generated Go code exists.
# Check for message definitions in proto files
echo "Checking proto definitions..."
rg -l "message MsgUpgradeIsolatedPerpetualToCross" "proto/"
# Check for generated Go types
echo "Checking generated Go types..."
rg -l "type MsgUpgradeIsolatedPerpetualToCross struct" "x/listing/types/"
Length of output: 366
Script:
#!/bin/bash
# Let's verify the message types in a different way
# 1. First, let's find all possible locations of the generated types
echo "Searching for the message type across the codebase..."
rg -l "MsgUpgradeIsolatedPerpetualToCross"
# 2. Let's also check the proto definition content
echo -e "\nChecking proto definition content..."
cat proto/dydxprotocol/listing/tx.proto
Length of output: 4244
protocol/x/clob/keeper/keeper.go (2)
6-6: LGTM!
The math/big import is correctly placed and necessary for the new method's return type.
166-168: LGTM!
The implementation correctly delegates to the subaccountsKeeper. Based on the retrieved learnings from PR #2551, this follows the established pattern of similar functions that were moved earlier.
indexer/packages/v4-protos/src/codegen/dydxprotocol/listing/tx.ts (2)
103-136: LGTM! Interface definitions follow protocol standards.
The interfaces are well-structured and properly documented. The empty response interfaces are intentional and follow the established pattern for RPC responses without return values.
🧰 Tools
🪛 Biome
[error] 130-130: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 136-136: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
405-491: LGTM! Implementation follows protobuf standards.
The encode/decode implementation correctly handles all fields and follows the protobuf wire format specification.
Let's verify the field numbers match the proto definition:
✅ Verification successful
Field numbers in implementation match proto definition ✅
The implementation correctly matches the proto definition:
authorityfield number = 1perpetual_idfield number = 2 (matchesperpetualIdin the implementation)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify field numbers in proto definition match the implementation
# Expected: Field numbers should be 1 for authority and 2 for perpetual_id
rg -A 5 "message MsgUpgradeIsolatedPerpetualToCross" "*.proto"
Length of output: 112
Script:
#!/bin/bash
# Let's first find the proto files in the repository
fd -e proto | grep -i 'listing\|perpetual'
# Then we can search for the message definition in the found files
Length of output: 525
Script:
#!/bin/bash
# Search for the message definition in the listing tx proto file since the implementation is in the listing package
rg -A 5 "message MsgUpgradeIsolatedPerpetualToCross" "proto/dydxprotocol/listing/tx.proto"
Length of output: 372
Script:
#!/bin/bash
# Let's get more context to see the perpetual_id field definition
rg -B 2 -A 10 "message MsgUpgradeIsolatedPerpetualToCross" "proto/dydxprotocol/listing/tx.proto"
Length of output: 669
protocol/x/subaccounts/keeper/transfer.go (1)
541-546: Verify source account balance before transfer.
Consider checking if the source account has sufficient balance before attempting the transfer to provide better error messages.
protocol/x/listing/keeper/listing_test.go (3)
9-9: LGTM: Import additions are appropriate.
The new imports are necessary for bank testing utilities and math operations used in the new test function.
Also applies to: 16-16
67-67: LGTM: Keeper initialization updates are consistent.
The ListingKeepers initialization has been consistently updated across all test functions to include necessary dependencies for the new functionality.
Also applies to: 136-136, 222-222
428-582: LGTM: Well-structured test implementation for perpetual upgrade functionality.
The test function thoroughly covers the core functionality with appropriate test cases for successful upgrades and validation of fund transfers between isolated and cross pools.
protocol/app/msgs/all_msgs.go (1)
237-238: LGTM! New message types are correctly added.
The new message types for upgrading isolated perpetual markets to cross margin are properly added to the AllTypeMessages map following the established naming conventions.
Let's verify if these new message types need to be added to other maps:
✅ Verification successful
Based on the search results, I can see that:
- The new message types are already properly added to
InternalMsgSamplesAllinprotocol/app/msgs/internal_msgs.go:
"/dydxprotocol.listing.MsgUpgradeIsolatedPerpetualToCross": &listing.MsgUpgradeIsolatedPerpetualToCross{},
"/dydxprotocol.listing.MsgUpgradeIsolatedPerpetualToCrossResponse": nil,- The message is an internal message that can only be executed by the authority (governance), as evidenced by:
- Its presence in
InternalMsgSamplesAll - The authority check in the message handler
- The protobuf definition with
option (cosmos.msg.v1.signer) = "authority"
- The message doesn't need to be added to other maps because:
- It's already in the correct map (
InternalMsgSamplesAll) - Internal messages are not meant to be in
AllowMsgs(which equalsNormalMsgs) - It's not a nested or unsupported message type
No additional maps needed for these message types
The message types are correctly placed in InternalMsgSamplesAll and don't need to be added to other maps since they are governance-only messages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new message types need to be added to DisallowMsgs or AllowMsgs maps
# by searching for similar patterns in the codebase.
# Search for message type definitions
rg -A 5 "MsgUpgradeIsolatedPerpetualToCross"
# Search for message handling patterns
rg "NormalMsgs|AppInjectedMsgSamples|InternalMsgSamplesAll|NestedMsgSamples|UnsupportedMsgSamples" --type go
Length of output: 44439
protocol/x/clob/keeper/deleveraging.go (1)
244-244: 💡 Codebase verification
Method exists in implementation but interface needs updating
The GetInsuranceFundBalance method is implemented in protocol/x/subaccounts/keeper/subaccount.go but is not declared in any of the SubaccountsKeeper interfaces. This could lead to compilation errors as the interface contract is incomplete.
- Add
GetInsuranceFundBalance(ctx sdk.Context, perpetualId uint32) *big.Intto theSubaccountsKeeperinterface in:protocol/x/clob/types/expected_keepers.goprotocol/x/subaccounts/types/types.go
🔗 Analysis chain
LGTM! Verify SubaccountsKeeper interface.
The change correctly uses the centralized GetInsuranceFundBalance method from SubaccountsKeeper, which aligns with previous refactoring efforts.
Let's verify the SubaccountsKeeper interface includes this method with the correct signature:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for GetInsuranceFundBalance method in the SubaccountsKeeper interface
ast-grep --pattern 'type SubaccountsKeeper interface {
$$$
GetInsuranceFundBalance(ctx sdk.Context, perpetualId uint32) *big.Int
$$$
}'
Length of output: 143
Script:
#!/bin/bash
# Let's try a different approach to find the SubaccountsKeeper interface and its method
rg "type SubaccountsKeeper interface" -A 20
# Also search for the actual implementation to verify the method exists
rg "func.*GetInsuranceFundBalance.*Context.*uint32.*big\.Int" -A 2
Length of output: 6431
⛔ Skipped due to learnings
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.
protocol/x/subaccounts/keeper/subaccount.go (1)
829-865: LGTM! Functions moved with existing behavior preserved.
Based on the learnings, these functions were moved from another location and the current error handling patterns are preserved intentionally.
protocol/x/perpetuals/keeper/perpetual.go (2)
200-204: LGTM! Important validation for market type transition.
The error handling for preventing changes to CROSS market type is well implemented. This validation is crucial as it ensures:
- One-way transition from isolated to cross margin.
- Clear error messaging with context (perpetual ID and market type).
Line range hint 191-219: Verify the usage of SetPerpetualMarketType.
The method correctly handles market type transitions and state updates. Let's verify its integration with the new UpgradeIsolatedPerpetualToCross functionality.
✅ Verification successful
Let me gather more specific information about the integration between SetPerpetualMarketType and UpgradeIsolatedPerpetualToCross.
Let me check one final aspect to verify the complete flow and error handling.
Integration between SetPerpetualMarketType and UpgradeIsolatedPerpetualToCross is correctly implemented
The verification confirms:
SetPerpetualMarketTypecorrectly handles the transition from isolated to cross market typeUpgradeIsolatedPerpetualToCrossproperly validates the market type before transition- Test coverage includes both success and failure scenarios for market type transitions
- The implementation includes proper state updates and fund transfers during the upgrade process
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of SetPerpetualMarketType to ensure proper integration.
# Search for direct method calls
echo "Direct method calls:"
rg -p "SetPerpetualMarketType"
# Search for potential upgrade implementations
echo -e "\nPotential upgrade implementations:"
rg -p "UpgradeIsolatedPerpetualToCross"
Length of output: 12813
Script:
#!/bin/bash
# Check the implementation of UpgradeIsolatedPerpetualToCross in listing keeper
echo "Listing keeper implementation:"
rg -A 20 "func \(k Keeper\) UpgradeIsolatedPerpetualToCross" protocol/x/listing/keeper/listing.go
# Check test cases for market type transitions
echo -e "\nTest cases for market type transitions:"
rg -A 10 "TestUpgradeIsolatedPerpetualToCross" protocol/x/listing/keeper/listing_test.go
Length of output: 1386
Script:
#!/bin/bash
# Check the test implementation for SetPerpetualMarketType
echo "SetPerpetualMarketType test cases:"
rg -A 20 "TestSetPerpetualMarketType" protocol/x/perpetuals/keeper/perpetual_test.go
# Check the complete test cases for upgrade flow
echo -e "\nComplete upgrade test cases:"
cat protocol/x/listing/keeper/listing_test.go | grep -A 50 "TestUpgradeIsolatedPerpetualToCross"
Length of output: 3275
protocol/app/app.go (1)
1245-1246: LGTM: Dependencies properly injected into ListingKeeper
The addition of SubaccountsKeeper and VaultKeeper as dependencies to the ListingKeeper initialization is consistent with the PR objectives to support upgrading isolated perpetuals to cross margin functionality.
Let's verify the dependencies are properly used in the ListingKeeper:
✅ Verification successful
Based on the search results, I can now verify the review comment. The code shows that:
- The
ListingKeeperimplementation inprotocol/x/listing/keeper/listing.gohas theUpgradeIsolatedPerpetualToCrossfunction that uses bothSubaccountsKeeperandVaultKeeper. - The
ListingKeeperstruct inprotocol/x/listing/keeper/keeper.goproperly declares both dependencies:
type Keeper struct {
SubaccountsKeeper types.SubaccountsKeeper
VaultKeeper types.VaultKeeper
}- The dependencies are properly injected in
app.gowhen initializing theListingKeeper:
app.ListingKeeper = *listingmodulekeeper.NewKeeper(
appCodec,
app.SubaccountsKeeper,
app.VaultKeeper,
)LGTM: Dependencies properly injected and used in ListingKeeper
The addition of SubaccountsKeeper and VaultKeeper as dependencies to the ListingKeeper is correctly implemented. These dependencies are used in the UpgradeIsolatedPerpetualToCross function to support upgrading isolated perpetuals to cross margin functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ListingKeeper's usage of SubaccountsKeeper and VaultKeeper
# Check ListingKeeper's implementation for usage of the new dependencies
ast-grep --pattern $'type ListingKeeper struct {
$$$
SubaccountsKeeper $$$
VaultKeeper $$$
$$$
}'
# Check for methods using these dependencies
ast-grep --pattern $'func (k *ListingKeeper) UpgradeIsolatedPerpetualToCross($$$) {
$$$
}'
Length of output: 200
Script:
#!/bin/bash
# Let's try a different approach to verify the dependencies
# First, let's find the listing keeper file
fd "listing_keeper.go" --type f
# Then search for SubaccountsKeeper and VaultKeeper usage in the listing keeper
rg "SubaccountsKeeper|VaultKeeper" -A 3
# Also check for any upgrade-related functions in listing keeper
rg "func.*Upgrade.*Perpetual" -A 5
Length of output: 107129
protocol/x/perpetuals/keeper/perpetual_test.go (2)
326-335: LGTM: Test cases for market type transitions look good.
The new test cases appropriately verify the ability to:
- Set market type from unspecified to cross
- Set market type from isolated to cross
These test cases ensure the core functionality of upgrading perpetual markets to cross margin type works as expected.
348-357: LGTM: Error case for cross market type is well tested.
The test case properly verifies that a perpetual market already set to cross type cannot be modified, with appropriate error message validation.
protocol/x/subaccounts/keeper/subaccount_test.go (1)
6135-6260: Well structured test suite with comprehensive coverage.
The test suite demonstrates good practices with:
- Table-driven tests with clear case descriptions
- Thorough test coverage including edge cases
- Proper test setup and cleanup
- Clear assertions and validations
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (9)
protocol/x/subaccounts/keeper/transfer.go (5)
509-512: Enhance function documentation.The function documentation should include more details about:
- When this function is expected to be called during the upgrade process
- Whether this is a one-time operation or can be called multiple times
- Any prerequisites or post-conditions
546-551: Add metrics for tracking insurance fund transfers.Consider adding metrics to track the transfer amount for monitoring and debugging purposes.
Add before the return statement:
+ metrics.AddSampleWithLabels( + metrics.InsuranceFundTransfer, + metrics.GetMetricValueFromBigInt(isolatedInsuranceFundBalance), + metrics.GetLabelForStringValue(metrics.TransferType, "isolated_to_cross"), + metrics.GetLabelForIntValue(metrics.PerpetualId, int(perpetualId)), + )
573-573: Improve panic message for USDC asset.The panic message should include more context about why this is a critical error.
- panic("TransferIsolatedCollateralToCross: Usdc asset not found in state") + panic("TransferIsolatedCollateralToCross: Critical error - USDC asset must exist in state for collateral transfers")
554-558: Enhance function documentation.The function documentation should include more details about:
- When this function is expected to be called during the upgrade process
- Whether this is a one-time operation or can be called multiple times
- Any prerequisites or post-conditions
587-592: Add metrics for tracking collateral transfers.Consider adding metrics to track the transfer amount for monitoring and debugging purposes.
Add before the return statement:
+ metrics.AddSampleWithLabels( + metrics.CollateralTransfer, + metrics.GetMetricValueFromBigInt(isolatedCollateralPoolBalance.Amount), + metrics.GetLabelForStringValue(metrics.TransferType, "isolated_to_cross"), + metrics.GetLabelForIntValue(metrics.PerpetualId, int(perpetualId)), + )protocol/x/listing/keeper/listing_test.go (4)
67-67: Consider replacing unused keeper variables with underscores.For better readability and to follow Go conventions, consider replacing unused keeper variables in the return values with underscores. This makes it immediately clear which keepers are actually used in each test.
For example, in
TestCreateMarket, apply this change:-ctx, keeper, _, _, pricesKeeper, _, _, marketMapKeeper, _, _, _ := keepertest.ListingKeepers( +ctx, keeper, _, _, pricesKeeper, _, _, marketMapKeeper, _, _, _ := keepertest.ListingKeepers(Also applies to: 136-136, 222-223, 486-491
439-478: Consider adding more edge cases.While the current test cases cover the main scenarios, consider adding:
- Test case for non-existent perpetual ID
- Test case for perpetual ID 0 (boundary value)
519-541: Consider improving test data setup readability.The current setup using nested slices and type assertions could be made more maintainable.
Consider using a helper struct:
+type fundingTestData struct { + address sdk.AccAddress + amount *big.Int +} -fundingData := [][]interface{}{ - {isolatedInsuranceFundAddr, tc.isolatedInsuranceFundBalance}, - {crossInsuranceFundAddr, tc.crossInsuranceFundBalance}, +fundingData := []fundingTestData{ + {address: isolatedInsuranceFundAddr, amount: tc.isolatedInsuranceFundBalance}, + {address: crossInsuranceFundAddr, amount: tc.crossInsuranceFundBalance},
561-580: Consider extracting balance verification into a helper function.The balance verification logic could be made more reusable and cleaner by extracting it into a helper function.
Consider creating a helper:
func verifyBalance( t *testing.T, ctx sdk.Context, bankKeeper types.BankKeeper, addr sdk.AccAddress, expectedAmount *big.Int, ) { require.Equal( t, sdk.NewCoin( asstypes.AssetUsdc.Denom, sdkmath.NewIntFromBigInt(expectedAmount), ), bankKeeper.GetBalance(ctx, addr, asstypes.AssetUsdc.Denom), ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
protocol/testutil/keeper/listing.go(9 hunks)protocol/x/listing/keeper/listing_test.go(5 hunks)protocol/x/subaccounts/keeper/transfer.go(3 hunks)
🧰 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 (9)
protocol/testutil/keeper/listing.go (4)
14-14: LGTM: Import statements are properly organized.
The new imports for assetskeeper and subaccountskeeper are correctly added and follow the existing import grouping pattern.
Also applies to: 18-18
71-71: LGTM: Keeper initialization changes are well-structured.
The changes maintain proper initialization order and dependency injection:
- Assets keeper is initialized before its dependents
- Subaccounts keeper is initialized with all required dependencies
- Bank keeper renaming is consistently applied
Also applies to: 77-77, 121-127, 141-150, 159-159
201-201: LGTM: ListingKeeper creation properly updated.
The createListingKeeper function is correctly modified to:
- Accept the new
subaccountsKeeperparameter - Pass it to the
NewKeeperconstructor
Also applies to: 222-222
42-44: Verify the impact of bankKeeper renaming.
The addition of new keeper parameters looks good. However, let's verify that the renaming of bankKeeper to bankKeeper_out doesn't have unintended consequences.
Also applies to: 189-190
protocol/x/subaccounts/keeper/transfer.go (2)
525-528: 🛠️ Refactor suggestion
Standardize error handling for USDC asset.
The error handling for USDC asset existence is inconsistent with the codebase patterns. Other functions panic in this case, as seen in the TransferInsuranceFundPayments function.
Consider using the same error handling pattern:
- _, exists := k.assetsKeeper.GetAsset(ctx, assettypes.AssetUsdc.Id)
- if !exists {
- return fmt.Errorf("USDC asset not found in state")
- }
+ _, _, err = k.assetsKeeper.ConvertAssetToCoin(
+ ctx,
+ assettypes.AssetUsdc.Id,
+ isolatedInsuranceFundBalance,
+ )
+ if err != nil {
+ // Panic if USDC does not exist.
+ panic(err)
+ }Likely invalid or redundant comment.
569-569: Verify the hardcoded cross-collateral pool address.
The hardcoded types.ModuleAddress needs verification to ensure it's the correct address for the cross-collateral pool.
Also applies to: 570-570
✅ Verification successful
Let me gather more specific information about the subaccounts module address usage.
Based on the gathered information, I can now provide the final response:
The hardcoded types.ModuleAddress is correctly used as the cross-collateral pool address.
The ModuleAddress is properly defined in the subaccounts module as the module's main address (dydx1v88c3xv9xyv3eetdx0tvcmq7ung3dywp5upwc6) and is consistently used throughout the codebase for managing the cross-collateral pool. This is verified by:
- The address is derived from the module name using
authtypes.NewModuleAddress(ModuleName) - It's extensively tested in transfer and subaccount tests
- Its usage is consistent with the module's design for handling cross-perpetual collateral pools
- The implementation matches the comments describing cross-collateral pool functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of ModuleAddress constant
# Test: Search for other occurrences of ModuleAddress to confirm its usage
rg -A 5 "ModuleAddress.*=.*"
Length of output: 2316
Script:
#!/bin/bash
# Check the definition and usage of ModuleAddress in subaccounts types
rg -A 5 "ModuleName.*=.*" protocol/x/subaccounts/types/
Length of output: 384
Script:
#!/bin/bash
# Check where this ModuleAddress is used in the subaccounts module
rg "ModuleAddress" protocol/x/subaccounts/
Length of output: 10109
Script:
#!/bin/bash
# Check for any cross-collateral pool related constants or configurations
rg -i "cross.*collateral.*pool" protocol/x/subaccounts/
Length of output: 1266
protocol/x/listing/keeper/listing_test.go (3)
9-9: LGTM: Import additions are appropriate.
The new imports for bank testutil and sdkmath are necessary for the new test functionality.
Also applies to: 16-16
429-438: LGTM: Well-structured test setup.
The test structure is clear and the test cases are well-organized.
551-558: LGTM: Proper validation of market type change.
The test correctly validates that the perpetual's market type has been upgraded to cross.
|
closing, tests will go in the previous PR in this stack |
Changelist
UpgradeIsolatedPerpetualToCrossTest 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
Release Notes
New Features
SubaccountsKeeperinterface with methods for managing insurance fund balances.Bug Fixes
Tests
Documentation