[TRA-621] Sweep bank funds of megavault into subaccount. #2293
Conversation
WalkthroughThe pull request introduces enhancements to the vault management system by integrating Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (3)
protocol/x/assets/types/types.go (2)
9-32: Consider adding documentation comments for theAssetsKeeperinterface and its methodsThe
AssetsKeeperinterface and its methods are exported and would benefit from GoDoc comments. Adding documentation will improve code readability and help other developers understand the purpose and usage of each method.Apply this diff to add documentation comments:
type AssetsKeeper interface { + // ConvertAssetToCoin converts a specified quantity of an asset into its coin representation. ConvertAssetToCoin(ctx sdk.Context, assetId uint32, quantums *big.Int) (*big.Int, sdk.Coin, error) + + // CreateAsset creates a new asset with the given parameters. CreateAsset( ctx sdk.Context, assetId uint32, symbol string, denom string, denomExponent int32, hasMarket bool, marketId uint32, atomicResolution int32, ) ( Asset, error, ) + + // GetAsset retrieves an asset by its ID. GetAsset(ctx sdk.Context, id uint32) (Asset, bool) + + // GetAllAssets retrieves all existing assets. GetAllAssets(ctx sdk.Context) []Asset + + // IsPositionUpdatable checks if a position can be updated for a given asset ID. IsPositionUpdatable(ctx sdk.Context, id uint32) (bool, error) + + // ModifyAsset updates an existing asset's market properties. ModifyAsset(ctx sdk.Context, id uint32, hasMarket bool, marketId uint32) (Asset, error) }
10-10: Ensure consistent naming for asset ID parametersThe parameter for asset IDs is inconsistently named (
assetIdvs.id). For clarity and consistency, consider using a uniform naming convention across all methods.Apply this diff to standardize the parameter names:
ConvertAssetToCoin(ctx sdk.Context, assetId uint32, quantums *big.Int) (*big.Int, sdk.Coin, error) CreateAsset( ctx sdk.Context, assetId uint32, // ... ) -GetAsset(ctx sdk.Context, id uint32) (Asset, bool) +GetAsset(ctx sdk.Context, assetId uint32) (Asset, bool) -IsPositionUpdatable(ctx sdk.Context, id uint32) (bool, error) +IsPositionUpdatable(ctx sdk.Context, assetId uint32) (bool, error) -ModifyAsset(ctx sdk.Context, id uint32, hasMarket bool, marketId uint32) (Asset, error) +ModifyAsset(ctx sdk.Context, assetId uint32, hasMarket bool, marketId uint32) (Asset, error)Also applies to: 13-13, 25-25, 29-29, 31-31
protocol/x/vault/keeper/sweep_funds.go (1)
10-11: Correct the function comment to match the function name and capitalize "USDC".The function comment refers to
SweepMainVaultBankBalances, but the actual function name isSweepMainVaultBankBalance. Additionally, "usdc" should be capitalized as "USDC" for consistency.Apply the following diff to correct the comment:
-// SweepMainVaultBankBalances deposits any usdc balance from the Megavault main vault bank balance +// SweepMainVaultBankBalance deposits any USDC balance from the Megavault main vault bank balance
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- protocol/app/app.go (1 hunks)
- protocol/mocks/AssetsKeeper.go (1 hunks)
- protocol/mocks/Makefile (1 hunks)
- protocol/testutil/keeper/vault.go (1 hunks)
- protocol/x/assets/types/types.go (1 hunks)
- protocol/x/vault/abci.go (1 hunks)
- protocol/x/vault/keeper/keeper.go (3 hunks)
- protocol/x/vault/keeper/sweep_funds.go (1 hunks)
- protocol/x/vault/keeper/sweep_funds_test.go (1 hunks)
- protocol/x/vault/types/expected_keepers.go (2 hunks)
- protocol/x/vault/types/megavault.go (1 hunks)
Files skipped from review due to trivial changes (2)
- protocol/mocks/AssetsKeeper.go
- protocol/x/vault/types/megavault.go
Additional comments not posted (10)
protocol/x/vault/abci.go (1)
39-39: Integration ofSweepMainVaultBankBalanceintoEndBlockeris appropriateThe addition of
keeper.SweepMainVaultBankBalance(ctx)afterkeeper.RefreshAllVaultOrders(ctx)correctly implements the sweeping of the megavault's main vault bank balance into the subaccount balance during end-of-block processing, aligning with the PR objectives.protocol/x/vault/keeper/sweep_funds.go (1)
12-48: Function implementation looks good.The
SweepMainVaultBankBalancefunction correctly transfers any USDC balance from the Megavault's main vault bank account to its main vault subaccount, with appropriate error handling.protocol/testutil/keeper/vault.go (1)
65-66: Ensuremocks.AssetsKeeperandmocks.BankKeeperare Properly MockedThe
&mocks.AssetsKeeper{}and&mocks.BankKeeper{}are instantiated as empty structs when creating the vault keeper. To prevent nil pointer dereferences or unintended behavior during testing, ensure that all methods expected to be called on these mocks are properly set up with the necessary mock behaviors.To identify which methods need to be mocked, run the following script to find all method calls to
AssetsKeeperandBankKeeperwithin the vault keeper:This script searches for method invocations on
assetsKeeperandbankKeeperwithin the vault keeper module. Verify that all these methods are appropriately mocked in your test setup.protocol/x/vault/types/expected_keepers.go (3)
4-4: Importing the"context"packageThe addition of the
"context"package is necessary for theBankKeeperinterface. This ensures that all context types are properly referenced.
17-25:AssetsKeeperinterface addition is well-definedThe new
AssetsKeeperinterface with theGetAssetmethod is correctly defined and follows the existing code patterns.
96-102:DepositFundsFromAccountToSubaccountmethod addition is appropriateThe addition of the
DepositFundsFromAccountToSubaccountmethod to theSubaccountsKeeperinterface is well-structured and consistent with existing interface definitions.protocol/app/app.go (1)
1181-1182: Verify the updated VaultKeeper initialization parametersThe
vaultmodulekeeper.NewKeeperfunction now includesapp.AssetsKeeperandapp.BankKeeperas parameters. Ensure that this change aligns with the updated function signature in thevaultmodulekeeperpackage and that all other instantiations ofNewKeeperare updated accordingly throughout the codebase.Run the following script to verify the function usage:
Verification successful
To gather more information and ensure a comprehensive verification, I'll run additional scripts to check the points mentioned in the analysis.
To address the unexpected results and locate the correct NewKeeper function for the vault module, I'll run an additional script to search for the relevant files and function definition.
Based on the analysis of the NewKeeper function definition and its usage, I can now provide a final response.
VaultKeeper initialization parameters are correct and consistent
The updated VaultKeeper initialization in app.go correctly includes the new parameters
app.AssetsKeeperandapp.BankKeeper. These changes align with theNewKeeperfunction signature in the vault module (protocol/x/vault/keeper/keeper.go). All required parameters, includingapp.ClobKeeper, are present in the call site.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `vaultmodulekeeper.NewKeeper` include the new parameters. # Test: Search for the function calls. Expect: All calls should have the new parameters. rg --type go 'vaultmodulekeeper\.NewKeeper\(' -A 5Length of output: 324
Script:
#!/bin/bash # Search for any other instances of NewKeeper in the codebase echo "Searching for other instances of NewKeeper:" rg --type go 'NewKeeper\(' -g '!app.go' # Examine the NewKeeper function definition in the vaultmodulekeeper package echo -e "\nExamining NewKeeper function definition:" rg --type go '^func NewKeeper\(' -g '**/vaultmodulekeeper/*.go' -A 10 # Search for references to VaultKeeper that might need updates echo -e "\nSearching for VaultKeeper references:" rg --type go 'VaultKeeper' -g '!app.go'Length of output: 10563
Script:
#!/bin/bash # Search for vault keeper files echo "Searching for vault keeper files:" fd -e go keeper protocol/x/vault # Search for NewKeeper function in vault module files echo -e "\nSearching for NewKeeper function in vault module:" rg --type go '^func NewKeeper\(' protocol/x/vault -A 10 # Check for any TODO comments related to VaultKeeper echo -e "\nChecking for TODO comments related to VaultKeeper:" rg --type go 'TODO.*VaultKeeper'Length of output: 1285
protocol/x/vault/keeper/keeper.go (3)
19-20: Addition ofassetsKeeperandbankKeeperto theKeeperstruct is appropriate.These new dependencies will enhance the functionality of the
Keeperby integrating asset management and banking capabilities.
35-36: UpdatedNewKeeperfunction signature to include new dependencies.Including
assetsKeeperandbankKeeperin the constructor ensures that theKeeperis properly initialized with the necessary modules.
49-50: Assignment of new dependencies in theKeeperinitialization is correct.The
assetsKeeperandbankKeeperare correctly assigned to their respective fields in theKeeperstruct.
| type BankKeeper interface { | ||
| GetBalance( | ||
| ctx context.Context, | ||
| addr sdk.AccAddress, | ||
| denom string, | ||
| ) sdk.Coin | ||
| } |
There was a problem hiding this comment.
Inconsistent use of context types in BankKeeper interface
The BankKeeper interface's GetBalance method uses context.Context instead of sdk.Context, which is inconsistent with other interfaces in this file that use sdk.Context. This inconsistency might lead to confusion or potential errors due to the different context implementations.
Apply this diff to align the context type:
type BankKeeper interface {
GetBalance(
- ctx context.Context,
+ ctx sdk.Context,
addr sdk.AccAddress,
denom string,
) sdk.Coin
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type BankKeeper interface { | |
| GetBalance( | |
| ctx context.Context, | |
| addr sdk.AccAddress, | |
| denom string, | |
| ) sdk.Coin | |
| } | |
| type BankKeeper interface { | |
| GetBalance( | |
| ctx sdk.Context, | |
| addr sdk.AccAddress, | |
| denom string, | |
| ) sdk.Coin | |
| } |
| @@ -63,3 +63,4 @@ mock-gen: | |||
| @go run github.com/vektra/mockery/v2 --name=OracleClient --dir=$(GOPATH)/pkg/mod/github.com/skip-mev/slinky@$(SLINKY_VERSION)/service/clients/oracle --recursive --output=./mocks | |||
| @go run github.com/vektra/mockery/v2 --name=ExtendVoteHandler --dir=$(GOPATH)/pkg/mod/github.com/dydxprotocol/cosmos-sdk@$(COSMOS_VERSION)/types --recursive --output=./mocks | |||
| @go run github.com/vektra/mockery/v2 --name=UpdateMarketPriceTxDecoder --dir=./app/process --recursive --output=./mocks | |||
| @go run github.com/vektra/mockery/v2 --name=AssetsKeeper --dir=./x//types --recursive --output=./mocks | |||
There was a problem hiding this comment.
Correct the directory path in the mock generation command
The --dir argument in the mock generation command contains an extra slash (//) which may cause issues when generating the mock for AssetsKeeper. It should point to the correct directory where AssetsKeeper is defined.
Apply this diff to fix the directory path:
- @go run github.com/vektra/mockery/v2 --name=AssetsKeeper --dir=./x//types --recursive --output=./mocks
+ @go run github.com/vektra/mockery/v2 --name=AssetsKeeper --dir=./x/assets/types --recursive --output=./mocksCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @go run github.com/vektra/mockery/v2 --name=AssetsKeeper --dir=./x//types --recursive --output=./mocks | |
| @go run github.com/vektra/mockery/v2 --name=AssetsKeeper --dir=./x/assets/types --recursive --output=./mocks |
| func TestSweepMainVaultBankBalance(t *testing.T) { | ||
| tests := map[string]struct { | ||
| // Bank balance of main vault | ||
| bankBalance int64 | ||
| // Subaccount balance of main vault | ||
| subaccountBalance *big.Int | ||
| // Expected bank balance of main vault | ||
| expectedBankBalance int64 | ||
| // Expected subaccount balance of main vault | ||
| expectedSubaccountBalance *big.Int | ||
| }{ | ||
| "Zero bank balance, zero subaccount balance": { | ||
| bankBalance: 0, | ||
| subaccountBalance: big.NewInt(0), | ||
| expectedBankBalance: 0, | ||
| expectedSubaccountBalance: big.NewInt(0), | ||
| }, | ||
| "100_000_000 quantums bank balance, zero subaccount balance": { | ||
| bankBalance: 100_000_000, | ||
| subaccountBalance: big.NewInt(0), | ||
| expectedBankBalance: 0, | ||
| expectedSubaccountBalance: big.NewInt(100_000_000), | ||
| }, | ||
| "100_000_000 quantums bank balance, 50_000_000 subaccount balance": { | ||
| bankBalance: 100_000_000, | ||
| subaccountBalance: big.NewInt(50_000_000), | ||
| expectedBankBalance: 0, | ||
| expectedSubaccountBalance: big.NewInt(150_000_000), | ||
| }, | ||
| } | ||
| for name, tc := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| // Initialize vaults with their equities. | ||
| tApp := testapp.NewTestAppBuilder(t).WithGenesisDocFn(func() (genesis types.GenesisDoc) { | ||
| genesis = testapp.DefaultGenesis() | ||
| testapp.UpdateGenesisDocWithAppStateForModule( | ||
| &genesis, | ||
| func(genesisState *satypes.GenesisState) { | ||
| genesisState.Subaccounts = []satypes.Subaccount{ | ||
| { | ||
| Id: &vaulttypes.MegavaultMainSubaccount, | ||
| AssetPositions: []*satypes.AssetPosition{ | ||
| testutil.CreateSingleAssetPosition( | ||
| assettypes.AssetUsdc.Id, | ||
| tc.subaccountBalance, | ||
| ), | ||
| }, | ||
| }, | ||
| } | ||
| }, | ||
| ) | ||
| testapp.UpdateGenesisDocWithAppStateForModule( | ||
| &genesis, | ||
| func(genesisState *banktypes.GenesisState) { | ||
| genesisState.Balances = append(genesisState.Balances, banktypes.Balance{ | ||
| Address: vaulttypes.MegavaultMainAddress.String(), | ||
| Coins: sdktypes.Coins{ | ||
| sdktypes.NewCoin(constants.Usdc.Denom, sdkmath.NewInt(tc.bankBalance)), | ||
| }, | ||
| }) | ||
| }, | ||
| ) | ||
| return genesis | ||
| }).Build() | ||
| ctx := tApp.InitChain() | ||
| k := tApp.App.VaultKeeper | ||
|
|
||
| k.SweepMainVaultBankBalance(ctx) | ||
|
|
||
| mainVaultSubaccount := tApp.App.SubaccountsKeeper.GetSubaccount(ctx, vaulttypes.MegavaultMainSubaccount) | ||
| require.Equal(t, tc.expectedSubaccountBalance, mainVaultSubaccount.AssetPositions[0].Quantums.BigInt()) | ||
| mainVaultBankBalance := tApp.App.BankKeeper.GetBalance( | ||
| ctx, | ||
| vaulttypes.MegavaultMainAddress, | ||
| constants.Usdc.Denom, | ||
| ).Amount | ||
| require.Equal(t, sdkmath.NewIntFromBigInt(big.NewInt(tc.expectedBankBalance)), mainVaultBankBalance) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider Refactoring Common Setup Code to Reduce Duplication
The TestSweepMainVaultBankBalance function contains setup code for initializing the test application and configuring the genesis state (lines 55-86). This setup is similar to the one in TestSweepMainVaultBankBalance_EndBlock. To improve maintainability and reduce code duplication, consider refactoring this setup code into shared helper functions or methods.
| }, | ||
| "100_000_000 quantums bank balance, zero subaccount balance": { | ||
| bankBalance: 100_000_000, | ||
| subaccountBalance: 0, | ||
| expectedBankBalance: 0, | ||
| expectedSubaccountBalance: big.NewInt(100_000_000), | ||
| }, | ||
| "100_000_000 quantums bank balance, 50_000_000 subaccount balance": { | ||
| bankBalance: 100_000_000, | ||
| subaccountBalance: 50_000_000, | ||
| expectedBankBalance: 0, | ||
| expectedSubaccountBalance: big.NewInt(150_000_000), | ||
| }, | ||
| } | ||
| for name, tc := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| tApp := testapp.NewTestAppBuilder(t).WithGenesisDocFn(func() (genesis types.GenesisDoc) { | ||
| genesis = testapp.DefaultGenesis() | ||
| testapp.UpdateGenesisDocWithAppStateForModule( | ||
| &genesis, | ||
| func(genesisState *satypes.GenesisState) { | ||
| genesisState.Subaccounts = []satypes.Subaccount{ | ||
| { | ||
| Id: &vaulttypes.MegavaultMainSubaccount, | ||
| AssetPositions: []*satypes.AssetPosition{ | ||
| testutil.CreateSingleAssetPosition( | ||
| assettypes.AssetUsdc.Id, | ||
| big.NewInt(0), | ||
| ), | ||
| }, | ||
| }, | ||
| } | ||
| }, | ||
| ) | ||
| return genesis | ||
| }).Build() | ||
|
|
||
| // Fund the subaccount and bank balance of megavault | ||
| ctx := tApp.AdvanceToBlock(2, testapp.AdvanceToBlockOptions{}) | ||
| if tc.subaccountBalance > 0 { | ||
| var msg proto.Message | ||
| depositMsg := sendingtypes.MsgDepositToSubaccount{ | ||
| Sender: constants.AliceAccAddress.String(), | ||
| Recipient: vaulttypes.MegavaultMainSubaccount, | ||
| AssetId: constants.Usdc.Id, | ||
| Quantums: tc.subaccountBalance, | ||
| } | ||
| msg = &depositMsg | ||
| for _, checkTx := range testapp.MustMakeCheckTxsWithSdkMsg( | ||
| ctx, | ||
| tApp.App, | ||
| testapp.MustMakeCheckTxOptions{ | ||
| AccAddressForSigning: constants.AliceAccAddress.String(), | ||
| Gas: 1000000, | ||
| FeeAmt: constants.TestFeeCoins_5Cents, | ||
| }, | ||
| msg, | ||
| ) { | ||
| resp := tApp.CheckTx(checkTx) | ||
| require.Condition(t, resp.IsOK, "Expected CheckTx to succeed. Response: %+v", resp) | ||
| } | ||
| } | ||
| if tc.bankBalance > 0 { | ||
| var msg proto.Message | ||
| bankSendMsg := banktypes.MsgSend{ | ||
| FromAddress: constants.AliceAccAddress.String(), | ||
| ToAddress: vaulttypes.MegavaultMainAddress.String(), | ||
| Amount: sdktypes.Coins{ | ||
| sdktypes.NewCoin(constants.Usdc.Denom, sdkmath.NewInt(tc.bankBalance)), | ||
| }, | ||
| } | ||
| msg = &bankSendMsg | ||
| for _, checkTx := range testapp.MustMakeCheckTxsWithSdkMsg( | ||
| ctx, | ||
| tApp.App, | ||
| testapp.MustMakeCheckTxOptions{ | ||
| AccAddressForSigning: constants.AliceAccAddress.String(), | ||
| Gas: 1000000, | ||
| FeeAmt: constants.TestFeeCoins_5Cents, | ||
| }, | ||
| msg, | ||
| ) { | ||
| resp := tApp.CheckTx(checkTx) | ||
| require.Condition(t, resp.IsOK, "Expected CheckTx to succeed. Response: %+v", resp) | ||
| } | ||
| } | ||
| // Advance block to execute EndBlocker | ||
| ctx = tApp.AdvanceToBlock(3, testapp.AdvanceToBlockOptions{}) | ||
|
|
||
| mainVaultSubaccount := tApp.App.SubaccountsKeeper.GetSubaccount(ctx, vaulttypes.MegavaultMainSubaccount) | ||
| require.Equal(t, tc.expectedSubaccountBalance, mainVaultSubaccount.AssetPositions[0].Quantums.BigInt()) | ||
| mainVaultBankBalance := tApp.App.BankKeeper.GetBalance( | ||
| ctx, | ||
| vaulttypes.MegavaultMainAddress, | ||
| constants.Usdc.Denom, | ||
| ).Amount | ||
| require.Equal(t, sdkmath.NewIntFromBigInt(big.NewInt(tc.expectedBankBalance)), mainVaultBankBalance) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Consolidate Duplicate Test Setup Code
In TestSweepMainVaultBankBalance_EndBlock, the initialization and genesis state configuration (lines 136-155) duplicate code from TestSweepMainVaultBankBalance. Refactoring this shared code into helper functions will enhance readability and make future maintenance easier.
| ctx := tApp.AdvanceToBlock(2, testapp.AdvanceToBlockOptions{}) | ||
| if tc.subaccountBalance > 0 { | ||
| var msg proto.Message | ||
| depositMsg := sendingtypes.MsgDepositToSubaccount{ | ||
| Sender: constants.AliceAccAddress.String(), | ||
| Recipient: vaulttypes.MegavaultMainSubaccount, | ||
| AssetId: constants.Usdc.Id, | ||
| Quantums: tc.subaccountBalance, | ||
| } | ||
| msg = &depositMsg | ||
| for _, checkTx := range testapp.MustMakeCheckTxsWithSdkMsg( | ||
| ctx, | ||
| tApp.App, | ||
| testapp.MustMakeCheckTxOptions{ | ||
| AccAddressForSigning: constants.AliceAccAddress.String(), | ||
| Gas: 1000000, | ||
| FeeAmt: constants.TestFeeCoins_5Cents, | ||
| }, | ||
| msg, | ||
| ) { | ||
| resp := tApp.CheckTx(checkTx) | ||
| require.Condition(t, resp.IsOK, "Expected CheckTx to succeed. Response: %+v", resp) | ||
| } | ||
| } | ||
| if tc.bankBalance > 0 { | ||
| var msg proto.Message | ||
| bankSendMsg := banktypes.MsgSend{ | ||
| FromAddress: constants.AliceAccAddress.String(), | ||
| ToAddress: vaulttypes.MegavaultMainAddress.String(), | ||
| Amount: sdktypes.Coins{ | ||
| sdktypes.NewCoin(constants.Usdc.Denom, sdkmath.NewInt(tc.bankBalance)), | ||
| }, | ||
| } | ||
| msg = &bankSendMsg | ||
| for _, checkTx := range testapp.MustMakeCheckTxsWithSdkMsg( | ||
| ctx, | ||
| tApp.App, | ||
| testapp.MustMakeCheckTxOptions{ | ||
| AccAddressForSigning: constants.AliceAccAddress.String(), | ||
| Gas: 1000000, | ||
| FeeAmt: constants.TestFeeCoins_5Cents, | ||
| }, | ||
| msg, | ||
| ) { | ||
| resp := tApp.CheckTx(checkTx) | ||
| require.Condition(t, resp.IsOK, "Expected CheckTx to succeed. Response: %+v", resp) | ||
| } | ||
| } |
There was a problem hiding this comment.
Simplify Funding Logic in Tests
The funding logic for the subaccount and bank balance of the Megavault (lines 158-205) involves repeated patterns for message creation and transaction execution. To improve clarity and reduce repetition, consider creating helper functions to handle the funding of subaccounts and bank balances within your tests.
| func (k Keeper) SweepMainVaultBankBalance( | ||
| ctx sdk.Context, | ||
| ) { | ||
| usdcAsset, exists := k.assetsKeeper.GetAsset(ctx, assettypes.AssetUsdc.Id) |
There was a problem hiding this comment.
nit: can also just use assetstypes.AssetUsdc to be simpler? don't need to pass in AssetsKeeper anymore
There was a problem hiding this comment.
Led to a flaky simulation test when I removed it, going to add it back in for now.
This reverts commit e907a8b.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
protocol/app/app.go (2)
Line range hint
1181-1192: The authority for conducting upgrades is correctly set.The
VaultKeeperis instantiated with the governance and delaymsg module accounts as the authority for conducting upgrades, which looks good.Consider refactoring if the
VaultKeeperis taking on too many responsibilities.The
VaultKeeperhas dependencies on several other keepers (AssetsKeeper,BankKeeper,ClobKeeper,DelayMsgKeeper,PerpetualsKeeper,PricesKeeper,SendingKeeper,SubaccountsKeeper), which might indicate tight coupling and potential violation of the single responsibility principle.In the future, consider evaluating if the
VaultKeeperis taking on too many responsibilities and if it can be refactored to have fewer dependencies. However, more context is needed to make a definitive assessment.
Line range hint
1181-1192: Add theFeeTiersKeeperto theVaultKeeperinstantiation for consistency.The
FeeTiersKeeperis set to theVaultKeeperin the line after this code segment:app.FeeTiersKeeper.SetVaultKeeper(app.VaultKeeper)For consistency and to avoid potential issues if the
VaultKeeperneeds to interact with theFeeTiersKeeper, consider adding theFeeTiersKeeperto theVaultKeeperinstantiation as well:app.VaultKeeper = *vaultmodulekeeper.NewKeeper( // ... app.FeeTiersKeeper, // ... )
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- protocol/app/app.go (1 hunks)
- protocol/testutil/keeper/vault.go (1 hunks)
- protocol/x/vault/keeper/keeper.go (3 hunks)
- protocol/x/vault/keeper/sweep_funds.go (1 hunks)
- protocol/x/vault/types/expected_keepers.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/vault/keeper/sweep_funds.go
Additional comments not posted (9)
protocol/testutil/keeper/vault.go (1)
65-66: LGTM!The addition of
mocks.AssetsKeeperandmocks.BankKeeperas dependencies in thecreateVaultKeeperfunction is a good practice for enhancing the testing capabilities of the vault keeper.By using mocks for these dependencies, you can:
- Isolate the testing of the vault keeper from the actual implementations of the assets and bank modules.
- Simulate various scenarios and edge cases without relying on the real modules.
- Improve the robustness and maintainability of the testing framework.
This change facilitates more comprehensive unit testing and provides a controlled environment for the vault keeper's operations.
protocol/x/vault/keeper/keeper.go (4)
19-19: LGTM!The addition of the
assetsKeeperfield to theKeeperstruct is a valid change that aligns with the provided summary. It integrates asset management capabilities into theKeeper.
20-20: LGTM!The addition of the
bankKeeperfield to theKeeperstruct is a valid change that aligns with the provided summary. It integrates banking capabilities into theKeeper.
35-36: LGTM!The addition of the
assetsKeeperandbankKeeperparameters to theNewKeeperconstructor function is a valid change that aligns with the provided summary. It allows for the initialization of the corresponding fields in theKeeperstruct, supporting the integration of asset management and banking capabilities.
49-50: LGTM!The initialization of the
assetsKeeperandbankKeeperfields in theNewKeeperconstructor function is a valid change that aligns with the provided summary. It ensures that theKeeperstruct is properly initialized with the provided asset management and banking capabilities.protocol/x/vault/types/expected_keepers.go (3)
17-25: LGTM!The
AssetsKeeperinterface and itsGetAssetmethod are well-defined, with consistent usage ofsdk.Contextand appropriate return types.
96-102: LGTM!The addition of the
DepositFundsFromAccountToSubaccountmethod to theSubaccountsKeeperinterface is well-implemented, with consistent usage ofsdk.Contextand other types. This method aligns with the PR objectives of facilitating the transfer of funds from a main account to a subaccount.
27-33: Inconsistent use of context types inBankKeeperinterfaceThe
BankKeeperinterface'sGetBalancemethod usescontext.Contextinstead ofsdk.Context, which is inconsistent with other interfaces in this file that usesdk.Context. This inconsistency might lead to confusion or potential errors due to the different context implementations.Apply this diff to align the context type:
type BankKeeper interface { GetBalance( - ctx context.Context, + ctx sdk.Context, addr sdk.AccAddress, denom string, ) sdk.Coin }Likely invalid or redundant comment.
protocol/app/app.go (1)
1181-1182: LGTM!The
VaultKeeperis correctly instantiated with dependencies on theAssetsKeeperandBankKeeper, which it likely needs to interact with.
Changelist
Add function to sweep funds from bank balance to subaccount balance of megavault main vault. Execute in endblocker after refreshing vault orders.
Test Plan
Unit / E2E test.
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
VaultKeeperfunctionality with integrated asset management and banking capabilities.AssetsKeeperfor improved testing of asset management functionalities.AssetsKeeperandBankKeeperto improve asset and account management.Bug Fixes
Tests
Documentation