feat(perp): MsgWithdrawFromPerpFund sudo call as part of #1642 #1734
Conversation
WalkthroughThe updates primarily revolve around the introduction of a new perpetual fund feature, including the ability to donate and withdraw from this fund. Changes reflect a shift in terminology from "Insurance Fund" to "Perp Fund" and the transition of certain administrative functions to be accessible via a "Sudo" permission level. The oracle module has been enhanced with more events to track validator performance, and there are general dependency updates across the system. Changes
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? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- x/perp/v2/types/tx.pb.go
Files selected for processing (30)
- CHANGELOG.md (1 hunks)
- app/keepers.go (1 hunks)
- proto/nibiru/perp/v2/tx.proto (2 hunks)
- wasmbinding/bindings/msg.go (2 hunks)
- wasmbinding/exec_perp.go (1 hunks)
- wasmbinding/exec_perp_test.go (3 hunks)
- wasmbinding/exec_test.go (1 hunks)
- wasmbinding/message_plugin.go (1 hunks)
- x/oracle/keeper/hooks.go (1 hunks)
- x/oracle/keeper/hooks_test.go (1 hunks)
- x/perp/v2/keeper/admin.go (1 hunks)
- x/perp/v2/keeper/admin_test.go (4 hunks)
- x/perp/v2/keeper/amm.go (2 hunks)
- x/perp/v2/keeper/amm_test.go (12 hunks)
- x/perp/v2/keeper/clearing_house.go (1 hunks)
- x/perp/v2/keeper/clearing_house_test.go (16 hunks)
- x/perp/v2/keeper/dnr_test.go (8 hunks)
- x/perp/v2/keeper/grpc_query_test.go (2 hunks)
- x/perp/v2/keeper/liquidate.go (1 hunks)
- x/perp/v2/keeper/liquidate_test.go (10 hunks)
- x/perp/v2/keeper/margin_test.go (6 hunks)
- x/perp/v2/keeper/msg_server.go (2 hunks)
- x/perp/v2/keeper/msg_server_test.go (3 hunks)
- x/perp/v2/keeper/withdraw.go (2 hunks)
- x/perp/v2/keeper/withdraw_test.go (3 hunks)
- x/perp/v2/module/module.go (1 hunks)
- x/perp/v2/types/errors.go (1 hunks)
- x/perp/v2/types/keys.go (2 hunks)
- x/perp/v2/types/msgs.go (2 hunks)
- x/perp/v2/types/msgs_test.go (6 hunks)
Files skipped from review due to trivial changes (3)
- wasmbinding/exec_test.go
- x/perp/v2/keeper/clearing_house_test.go
- x/perp/v2/keeper/liquidate_test.go
Additional comments: 89
x/perp/v2/types/keys.go (2)
8-8: Renaming the constant
PerpEFModuleAccounttoPerpFundModuleAccountis consistent with the PR's objective to shift from an insurance fund model to a perpetual fund model.29-29: The inclusion of
PerpFundModuleAccountin theModuleAccountsslice ensures that the renamed module account is recognized as a valid module account within the system.x/oracle/keeper/hooks.go (1)
- 60-60: The update to use
perptypes.PerpFundModuleAccountas the destination module account aligns with the PR's goal of transitioning to a perpetual fund model.wasmbinding/exec_perp.go (1)
- 23-23: The comment indicates a future action to rename the function to
CloseMarket, which suggests an upcoming change in functionality or clarification of the function's purpose.x/oracle/keeper/hooks_test.go (1)
- 76-76: The test has been correctly updated to use
perptypes.PerpFundModuleAccountin line with the changes made to the module account naming.x/perp/v2/types/errors.go (1)
- 53-53: The addition of
ErrGenericprovides a generic error type for the perpetual module. It's important to ensure that this error is used appropriately and does not mask more specific errors that could be more informative for debugging.Verification successful
The usage of
ErrGenericin the codebase appears to be appropriate. It is used inmsgs.gowith additional context to inform about a specific requirement for a message amount, and inmsgs_test.goto likely ensure the correct error is thrown in tests.
- The error is not overused or hiding specific errors based on the current usage.
- The addition of specific context when
ErrGenericis used inmsgs.goaligns with best practices in error handling.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of ErrGeneric to ensure it's not overused or hiding specific errors. ast-grep --lang go --pattern $'ErrGeneric'Length of output: 320
wasmbinding/bindings/msg.go (1)
- 17-18: The removal of insurance fund-related types and the update to
SetMarketEnabledandCreateMarkettypes in theNibiruMsgstruct align with the PR's shift from an insurance fund to a perpetual fund model.wasmbinding/message_plugin.go (1)
- 71-72: The removal of the
InsuranceFundWithdrawrelated code fromDispatchMsgis consistent with the PR's objective to remove insurance fund functionality.x/perp/v2/keeper/amm.go (2)
31-31: The change to send funds from
types.PerpFundModuleAccounttotypes.VaultModuleAccountwhen the cost amount is positive is consistent with the new perpetual fund model.49-49: The change to send funds from
types.VaultModuleAccounttotypes.PerpFundModuleAccountwhen the cost amount is negative aligns with the new perpetual fund model.x/perp/v2/keeper/withdraw_test.go (4)
47-47: The renaming of
PerpEFModuleAccounttoPerpFundModuleAccountis consistent with the PR objectives and the AI-generated summary. Ensure that all references to the old account name are updated across the entire codebase to maintain consistency.55-55: The renaming of
PerpEFModuleAccounttoPerpFundModuleAccountis consistent with the PR objectives and the AI-generated summary. Ensure that all references to the old account name are updated across the entire codebase to maintain consistency.72-72: The renaming of
PerpEFModuleAccounttoPerpFundModuleAccountis consistent with the PR objectives and the AI-generated summary. Ensure that all references to the old account name are updated across the entire codebase to maintain consistency.89-89: The renaming of
PerpEFModuleAccounttoPerpFundModuleAccountis consistent with the PR objectives and the AI-generated summary. Ensure that all references to the old account name are updated across the entire codebase to maintain consistency.x/perp/v2/keeper/withdraw.go (2)
65-65: The renaming of
PerpEFModuleAccounttoPerpFundModuleAccountin theWithdrawFromVaultfunction is consistent with the PR objectives and the AI-generated summary. Ensure that all references to the old account name are updated across the entire codebase to maintain consistency.128-128: The renaming of
PerpEFModuleAccounttoPerpFundModuleAccountin therealizeBadDebtfunction is consistent with the PR objectives and the AI-generated summary. Ensure that all references to the old account name are updated across the entire codebase to maintain consistency.x/perp/v2/module/module.go (1)
- 145-145: The renaming of
PerpEFModuleAccounttoPerpFundModuleAccountin theInitGenesismethod is consistent with the PR objectives and the AI-generated summary. Ensure that all references to the old account name are updated across the entire codebase to maintain consistency.wasmbinding/exec_perp_test.go (4)
134-142: The removal of
DoInsuranceFundWithdrawTestfrom theTestPerpExecutorHappyfunction is consistent with the PR objectives and the AI-generated summary, which indicates a shift away from the insurance fund model.177-177: The removal of
DoInsuranceFundWithdrawTestfrom theDoCreateMarketTestWithParamsfunction is consistent with the PR objectives and the AI-generated summary, which indicates a shift away from the insurance fund model.195-195: The removal of
DoInsuranceFundWithdrawTestfrom theDoSetMarketEnabledTestfunction is consistent with the PR objectives and the AI-generated summary, which indicates a shift away from the insurance fund model.198-200: The removal of
TestSadPaths_InvalidPairfunction is not mentioned in the PR objectives or the AI-generated summary. If this removal is intentional and related to the shift from the insurance fund model to the perpetual fund model, ensure that it is documented in the PR description or comments for clarity.x/perp/v2/keeper/admin.go (2)
26-64: The changes to
WithdrawFromPerpFundfunction correctly reflect the renaming and the addition of thesenderparameter. The logic to handle thedenomparameter is also sound, defaulting to the state's collateral denom if none is provided. The event name update is consistent with the function's new purpose.36-36: Ensure that the
CheckPermissionsmethod fromSudoKeeperis robust and correctly validates thesenderas an admin. This is crucial for the security of admin-only functions.x/perp/v2/keeper/msg_server.go (2)
131-137: The update to
DonateToEcosystemFundcorrectly reflects the renaming of the module account fromPerpEFModuleAccounttoPerpFundModuleAccount.222-233: The addition of the
WithdrawFromPerpFundfunction to themsgServeris consistent with the PR's objectives to allow admin withdrawal from the perpetual fund. The function uses theWithdrawFromPerpFundmethod from theadminstruct, which is appropriate for the intended admin-only functionality.x/perp/v2/keeper/dnr_test.go (8)
41-41: The update to
FundModulein theTestUserVolumesfunction correctly reflects the renaming of the module account toPerpFundModuleAccount.62-62: The update to
FundModulein theTestUserVolumesfunction correctly reflects the renaming of the module account toPerpFundModuleAccount.86-86: The update to
FundModulein theTestUserVolumesfunction correctly reflects the renaming of the module account toPerpFundModuleAccount.129-129: The update to
FundModulein theTestDiscountfunction correctly reflects the renaming of the module account toPerpFundModuleAccount.150-150: The update to
FundModulein theTestDiscountfunction correctly reflects the renaming of the module account toPerpFundModuleAccount.175-175: The update to
FundModulein theTestDiscountfunction correctly reflects the renaming of the module account toPerpFundModuleAccount.200-200: The update to
FundModulein theTestDiscountfunction correctly reflects the renaming of the module account toPerpFundModuleAccount.238-238: The update to
FundModulein theTestRebatesfunction correctly reflects the renaming of the module account toPerpFundModuleAccount.x/perp/v2/types/msgs.go (4)
24-24: The addition of
MsgWithdrawFromPerpFundto the list ofsdk.Msgimplementations is correct and follows the Cosmos SDK pattern for message types.414-429: The
ValidateBasicmethod forMsgWithdrawFromPerpFundcorrectly checks for a valid sender address, a positive amount, and a valid recipient address. It uses thesdkerrors.Wrapfmethod to wrap the underlying error, which is a good practice for error handling in the Cosmos SDK.432-437: The
GetSignersmethod forMsgWithdrawFromPerpFundcorrectly returns the sender's address as the required signer. The use ofpanicin case of an error is consistent with Cosmos SDK practices, as this method should not be called with an invalid message that has not passedValidateBasic.440-442: The
GetSignBytesmethod forMsgWithdrawFromPerpFundcorrectly marshals the message into JSON using the module's codec, which is then sorted to ensure consistent serialization for signing. This is standard practice for Cosmos SDK messages.x/perp/v2/keeper/liquidate.go (1)
- 421-421: The change in the
distributeLiquidateRewardsfunction correctly updates the destination module account fromPerpEFModuleAccounttoPerpFundModuleAccount. This aligns with the PR's objective to shift from an insurance fund model to a perpetual fund model. Ensure that thePerpFundModuleAccountis properly initialized and has the necessary permissions to receive funds.Verification successful
The search results indicate that
PerpFundModuleAccountis referenced in multiple files and contexts within the codebase. It appears in tests, module account funding, balance checks, and various keeper functions. This suggests that the account is properly initialized and used throughout the module, which is a good sign that it has been integrated into the system's logic.However, the search results do not explicitly confirm that
PerpFundModuleAccounthas the necessary permissions to receive funds. Permissions are typically set during module account creation or initialization, which might not be directly visible in the search results.To verify that
PerpFundModuleAccounthas the necessary permissions, we would need to look at the account initialization code, which is likely to be in the module'sinitorstartfunctions, or where the module accounts are set up.Let's generate a script to check the initialization of
PerpFundModuleAccountto ensure it has the necessary permissions.
The attempt to find the initialization of
PerpFundModuleAccountby searching for its occurrence near theNewKeeperfunction did not yield any results. This could mean that the account's permissions are not set up in the proximity of theNewKeeperfunction or that the account is initialized with permissions elsewhere in the codebase.Given the absence of direct evidence from the search results, we cannot conclusively verify that
PerpFundModuleAccounthas the necessary permissions to receive funds. The permissions could be set up in a different part of the codebase that was not captured by the search parameters used.To proceed, we should look for the module account initialization code, which might be in the genesis initialization or in a specific setup function for module accounts. Let's generate a new script to search for the initialization of module accounts in the genesis setup or any other relevant initialization function.
The search for the
InitGenesisfunction across various modules has provided us with a list of files where this function is defined. However, the output does not directly show theInitGenesisfunction for theperpmodule, which is where we would expect to find the initialization ofPerpFundModuleAccount.Since the
InitGenesisfunction for theperpmodule is likely to be in thex/perp/v2/module/module.gofile, as indicated by the search results, we should examine this file to see ifPerpFundModuleAccountis initialized with the necessary permissions there.Let's generate a script to look into the
InitGenesisfunction within thex/perp/v2/module/module.gofile to verify the setup ofPerpFundModuleAccount.
The output from the
InitGenesisfunction in thex/perp/v2/module/module.gofile shows that thePerpFundModuleAccountis being retrieved using theGetModuleAccountmethod from the account keeper (am.ak). This is a common pattern in Cosmos SDK-based applications, where module accounts are retrieved or created during the genesis initialization process.The presence of
am.ak.GetModuleAccount(ctx, types.PerpFundModuleAccount)in theInitGenesisfunction suggests that thePerpFundModuleAccountis indeed initialized at genesis. However, the output does not explicitly show the permissions being set. In the Cosmos SDK, permissions for module accounts are typically set when the module account is first created, which may not be shown in theInitGenesisfunction.Given that the
PerpFundModuleAccountis being retrieved in theInitGenesisfunction, it is reasonable to infer that the account is expected to exist and be operational at the start of the blockchain. This would imply that the account has been set up with the necessary permissions elsewhere in the code, likely during the module's initialization or setup phase.Since the output does not provide details on the specific permissions, we cannot conclusively verify that
PerpFundModuleAccounthas the necessary permissions to receive funds based solely on this output. However, the consistent use ofPerpFundModuleAccountthroughout the codebase and its retrieval during genesis initialization strongly suggests that it is set up correctly.If further confirmation is needed, we would need to inspect the code where the module accounts are created, which is typically done in the module's
keeperpackage or during the application's initialization process.For now, based on the available information, it seems appropriate to conclude:
- The
PerpFundModuleAccountis initialized during the genesis process.- The account is used consistently throughout the codebase, indicating it is a functional part of the module's logic.
Further manual verification by the development team or maintainers would be required to confirm the specific permissions of the
PerpFundModuleAccount.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the PerpFundModuleAccount is set up with the necessary permissions. rg --type go "PerpFundModuleAccount"Length of output: 12651
Script:
#!/bin/bash # Check the initialization of PerpFundModuleAccount to ensure it has the necessary permissions. rg --type go -A 10 "func .NewKeeper" | rg "PerpFundModuleAccount"Length of output: 65
Script:
#!/bin/bash # Search for the initialization of module accounts in the genesis setup or any other relevant initialization function. rg --type go "func \(.*\) InitGenesis"Length of output: 1345
Script:
#!/bin/bash # Examine the InitGenesis function in the x/perp/v2/module/module.go file for the setup of PerpFundModuleAccount. rg --type go -A 20 "func \(.*\) InitGenesis" x/perp/v2/module/module.goLength of output: 903
proto/nibiru/perp/v2/tx.proto (2)
53-56: The addition of the
WithdrawFromPerpFundgRPC transaction message is consistent with the PR's objective to allow admin withdrawal from the perp fund module account. Ensure that the necessary authorization checks are implemented server-side to restrict this action to admin users only.442-450: The
MsgWithdrawFromPerpFundmessage fields are correctly defined with appropriate data types. Theamountfield uses a custom type forIntwhich should ensure proper handling of large numbers. Thedenomfield is a string, which is standard for currency denominations. Theto_addrfield is also a string, which should be validated to ensure it contains a valid address format.x/perp/v2/keeper/margin_test.go (6)
74-74: The update to use
PerpFundModuleAccountin theTestAddMarginfunction aligns with the PR's objective to shift from an insurance fund model to a perpetual fund model. This change should be reflected consistently across all tests and the actual implementation.124-124: The change to
PerpFundModuleAccountis consistently applied in theTestAddMarginfunction. This should be part of a global update across the entire test suite and implementation to ensure consistency.204-204: The update to
PerpFundModuleAccountin theTestRemoveMarginfunction is consistent with the renaming of the module account. This change should be verified across all references to ensure that the old account name is no longer used.231-231: The replacement of
PerpEFModuleAccountwithPerpFundModuleAccountin theTestRemoveMarginfunction is correct and aligns with the PR's objectives. Ensure that all associated logic and tests are updated accordingly.279-279: The change to
PerpFundModuleAccountin theTestRemoveMarginfunction for the short position scenario is consistent with the changes made for the long position scenario. This consistency is necessary for the correctness of the test suite.306-306: The update to
PerpFundModuleAccountin theTestRemoveMarginfunction for the failure case is correctly applied. It's important to ensure that all failure cases are also updated to reflect the new module account name.x/perp/v2/keeper/grpc_query_test.go (2)
350-350: The change from a hardcoded string to a constant for the module account is a good practice for maintainability and readability.
367-370: Using the
types.PerpFundModuleAccountconstant instead of a hardcoded string ensures consistency and reduces the risk of typos or mismatches in the module account name across the test suite.x/perp/v2/keeper/admin_test.go (4)
31-90: The refactoring of
TestAdmin_WithdrawFromPerpFundappears to be comprehensive, with the removal of thesetupfunction and integration of its functionality into the test cases. ThefundModulefunction has been simplified to take one less argument, and the test cases now use thesreceiver for the test suite, which is a standard practice in Go'ssuitepackage. The use ofs.NoErrorands.EqualValuesassertions is appropriate for checking conditions without stopping the test (asrequirewould). The addition ofMsgWithdrawFromPerpFundto theHandleMsgfunction aligns with the new feature being tested. Overall, the changes are consistent with the PR objectives and the AI-generated summary.408-409: The addition of the
MsgWithdrawFromPerpFundcase in theHandleMsgfunction is consistent with the new administrative feature allowing withdrawals from the perpetual fund. This change is necessary to ensure that the message is properly handled by the message server.436-441: The test case within
TestCheckPermissionsforMsgWithdrawFromPerpFundis correctly checking for unauthorized access, which is crucial for admin-only operations. This ensures that only authorized users can perform administrative actions on the perpetual fund.472-484: The
DoWithdrawFromPerpFundTestfunction is a new addition that tests the withdrawal functionality from the perpetual fund. It is important to ensure that this function is called in various test scenarios to cover different edge cases and potential error conditions. The test setup usingFundModuleAccountand the subsequent withdrawal attempt is a good practice for testing the new feature.x/perp/v2/types/msgs_test.go (6)
7-7: The addition of
sdkerrorsimport is appropriate given the new error checks introduced in the test cases.397-400: The renaming of the test case from
MsgDonateToEcosystemFundtoMsgShiftSwapInvariantshould reflect changes in the functionality being tested. Confirm that the new name accurately describes the test's purpose in the context of the PR's objectives.429-462: New test cases for
MsgWithdrawFromPerpFundhave been added. Ensure that these tests cover all expected error scenarios and that the error messages are accurate and informative for debugging purposes.493-493: The inclusion of
MsgWithdrawFromPerpFundin themsgValidSenderListis correct and ensures that the new message type is tested for valid sender scenarios.506-506: The inclusion of
MsgWithdrawFromPerpFundin themsgInvalidSenderListis correct and ensures that the new message type is tested for invalid sender scenarios.652-653: The addition of
MsgWithdrawFromPerpFundto theGetSignBytestest cases is appropriate to verify that the serialization of the message for signing is correct.x/perp/v2/keeper/msg_server_test.go (3)
246-251: The test case
TestMsgServerDonateToPerpEfhas been updated to reflect the renaming ofPerpEFModuleAccounttoPerpFundModuleAccount. Ensure that the corresponding implementation ofMsgServerDonateToPerpEfhas been updated to use the new account name as well.276-282: The test case
TestMsgServerMultiLiquidatehas been updated to check the balance ofPerpFundModuleAccountafter a partial liquidation. This change is consistent with the PR's objective to shift from an insurance fund model to a perpetual fund model.309-315: The test case
TestMsgServerMultiLiquidatehas been updated to check the balance ofPerpFundModuleAccountafter a full liquidation. This change is consistent with the PR's objective to shift from an insurance fund model to a perpetual fund model.x/perp/v2/keeper/amm_test.go (24)
32-32: The replacement of
PerpEFModuleAccountwithPerpFundModuleAccountin the test setup is consistent with the PR's objective to rename the module account. This change should not affect the logic of the tests as long as the new account name is correctly set up elsewhere in the codebase.39-39: The assertion for the module balance using the new
PerpFundModuleAccountis correctly updated. Ensure that the balance checking logic inModuleBalanceEqualis compatible with the renamed account.55-55: The funding of the
PerpFundModuleAccountwithin the test setup is updated to reflect the new account naming. This change is necessary for the tests to run correctly with the updated module account names.62-62: The assertion for the module balance using the new
PerpFundModuleAccountis correctly updated. It is important to verify that the test logic correctly interprets the new account name.75-75: The funding of the
PerpFundModuleAccountwithin the test setup is updated to reflect the new account naming. This change is necessary for the tests to run correctly with the updated module account names.82-82: The assertion for the module balance using the new
PerpFundModuleAccountis correctly updated. It is important to verify that the test logic correctly interprets the new account name.95-95: The funding of the
PerpFundModuleAccountwithin the test setup is updated to reflect the new account naming. This change is necessary for the tests to run correctly with the updated module account names.102-102: The assertion for the module balance using the new
PerpFundModuleAccountis correctly updated. It is important to verify that the test logic correctly interprets the new account name.115-115: The funding of the
PerpFundModuleAccountwithin the test setup is updated to reflect the new account naming. This change is necessary for the tests to run correctly with the updated module account names.122-122: The assertion for the module balance using the new
PerpFundModuleAccountis correctly updated. It is important to verify that the test logic correctly interprets the new account name.135-135: The funding of the
PerpFundModuleAccountwithin the test setup is updated to reflect the new account naming. This change is necessary for the tests to run correctly with the updated module account names.142-142: The assertion for the module balance using the new
PerpFundModuleAccountis correctly updated. It is important to verify that the test logic correctly interprets the new account name.311-311: The funding of the
PerpFundModuleAccountwithin the test setup is updated to reflect the new account naming. This change is necessary for the tests to run correctly with the updated module account names.318-318: The assertion for the module balance using the new
PerpFundModuleAccountis correctly updated. It is important to verify that the test logic correctly interprets the new account name.335-335: The funding of the
PerpFundModuleAccountwithin the test setup is updated to reflect the new account naming. This change is necessary for the tests to run correctly with the updated module account names.342-342: The assertion for the module balance using the new
PerpFundModuleAccountis correctly updated. It is important to verify that the test logic correctly interprets the new account name.355-355: The funding of the
PerpFundModuleAccountwithin the test setup is updated to reflect the new account naming. This change is necessary for the tests to run correctly with the updated module account names.362-362: The assertion for the module balance using the new
PerpFundModuleAccountis correctly updated. It is important to verify that the test logic correctly interprets the new account name.375-375: The funding of the
PerpFundModuleAccountwithin the test setup is updated to reflect the new account naming. This change is necessary for the tests to run correctly with the updated module account names.382-382: The assertion for the module balance using the new
PerpFundModuleAccountis correctly updated. It is important to verify that the test logic correctly interprets the new account name.395-395: The funding of the
PerpFundModuleAccountwithin the test setup is updated to reflect the new account naming. This change is necessary for the tests to run correctly with the updated module account names.402-402: The assertion for the module balance using the new
PerpFundModuleAccountis correctly updated. It is important to verify that the test logic correctly interprets the new account name.415-415: The funding of the
PerpFundModuleAccountwithin the test setup is updated to reflect the new account naming. This change is necessary for the tests to run correctly with the updated module account names.422-422: The assertion for the module balance using the new
PerpFundModuleAccountis correctly updated. It is important to verify that the test logic correctly interprets the new account name.x/perp/v2/keeper/clearing_house.go (2)
- 685-691: The change from
types.PerpEFModuleAccounttotypes.PerpFundModuleAccountin thetransferFeefunction is noted. Ensure that all references to the old module account name across the entire codebase have been updated to maintain consistency.Verification successful
The search for the term
PerpEFModuleAccountdid not yield any results in the codebase. This suggests that all references to the old module account name may have been successfully updated toPerpFundModuleAccount.* 685-691: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-691]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old module account name. rg --type go 'PerpEFModuleAccount'Length of output: 34
Review the entire
clearing_house.gofile to ensure that the renaming of the module account fromPerpEFModuleAccounttoPerpFundModuleAccountis reflected everywhere it is used. This includes checking for any logic that might be tied to the old account name that would require updates.Verification successful
The search for the term
PerpEFModuleAccountdid not yield any results in the codebase. This suggests that the renaming of the module account toPerpFundModuleAccounthas been fully applied and there are no remaining references to the old name that would require updates.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any logic that might be tied to the old module account name. rg --type go 'PerpEFModuleAccount'Length of output: 34
app/keepers.go (1)
- 808-808: The renaming of
PerpEFModuleAccounttoPerpFundModuleAccountin theModuleAccPermsfunction is consistent with the PR's objectives to rebrand the module account for the perpetual fund. Ensure that all references to the old account name are updated across the entire codebase to maintain consistency.Verification successful
The search for the old module account name
PerpEFModuleAccountacross all file types did not yield any results. This suggests that the renaming toPerpFundModuleAccounthas been thoroughly applied throughout the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining occurrences of the old module account name. rg --type go 'PerpEFModuleAccount'Length of output: 34
Script:
#!/bin/bash # Search for any remaining occurrences of the old module account name in all file types. rg 'PerpEFModuleAccount'Length of output: 24
CHANGELOG.md (1)
- 69-69: The changelog entry for the new feature
MsgDonateToPerpFundis correctly formatted and appropriately placed under the "Dapp modules: perp, spot, etc" section.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1734 +/- ##
==========================================
- Coverage 73.46% 73.46% -0.01%
==========================================
Files 195 194 -1
Lines 15449 15475 +26
==========================================
+ Hits 11350 11369 +19
- Misses 3437 3444 +7
Partials 662 662
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- app/keepers.go (1 hunks)
- x/perp/v2/client/cli/cli_test.go (2 hunks)
- x/perp/v2/keeper/admin_test.go (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- x/perp/v2/keeper/admin_test.go
Additional comments: 3
x/perp/v2/client/cli/cli_test.go (2)
11-11: The import of
authtypesis added to utilizeNewModuleAddressfor obtaining module account addresses. Ensure that the usage ofauthtypes.NewModuleAddressis consistent with the intended logic across the test suite.719-732: The test case is using
authtypes.NewModuleAddressto obtain the module account address for thePerpFundModuleAccount. It's important to ensure that the module account name used here matches the one defined in the actual module to avoid discrepancies.- moduleAccPerpFund := authtypes.NewModuleAddress(types.PerpFundModuleAccount).String() + moduleAccPerpFund := authtypes.NewModuleAddress(types.PerpFundModuleName).String()Also, the assertion of the coin values should accurately reflect the expected state of the module account after the donation transaction.
app/keepers.go (1)
- 808-808: The renaming of
PerpEFModuleAccounttoPerpFundModuleAccountin theModuleAccPermsfunction is consistent with the PR objectives and the AI-generated summary. Ensure that all references to this module account are updated across the entire codebase to maintain consistency.Verification successful
The renaming of
PerpEFModuleAccounttoPerpFundModuleAccountappears to have been applied consistently throughout the codebase, as there are no remaining references to the old name and multiple references to the new name across various files.
- The change is consistent with the PR objectives and the AI-generated summary.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining occurrences of the old module account name. rg --type go 'PerpEFModuleAccount' # Ensure the new module account name is used consistently. rg --type go 'PerpFundModuleAccount'Length of output: 12711
| ) (err error) { | ||
| collateral, err := k.Collateral.Get(ctx) | ||
| if err != nil { | ||
| if err := k.SudoKeeper.CheckPermissions(sender, ctx); err != nil { |
There was a problem hiding this comment.
I have some opinions related to checking permissions here. I created some PR here: #1751
There was a problem hiding this comment.
This is a stateful validation, reading from the KV store of another module, so it's not really for the MsgServer
There was a problem hiding this comment.
Yeah. It does not change that principle if this call is still in the message server. MsgServer delegates validation of the request to the SudoKeeper (that he really is the one reading the KVStore, not the MsgServer), and if it is correct, it calls the Admin function.
What I don't believe is in the admin function to validate the permissions. It is a thin line and a matter of convention, but I can show you some benefits of this slight separation.
There was a problem hiding this comment.
Checking a whitelist is not a message field validation. If the keeper method doesn't have the permission check, other modules that import the methods can sidestep the sudo, defeating the purpose for the check.
It's not just a difference by convention. Switching SudoKeeper.CheckPermissions to happen in the MsgServer changes the security of the keeper method itself.
There was a problem hiding this comment.
The message field validation should be done by default out of the message server as long as you implement ValidateBasic(). Validating any field in the message server is duplicating computing cycles. What, then, is the use case of the MsgServer if we remove the ValidateBasic?
There are two paradigms here: the MsgServer as orchestrator of keepers (my ideal) or the Keepers as holders of all the logic and being pure defensive (your ideal; this removes the need for MsgServer in my opinion as it becomes only a wrapper of the keeper).
It is a matter of convention from the moment you agree on what it means to share some Keeper method. Is there any use case at this stage for CloseMarket to be public in the keeper? Try to avoid the what-if thing.
I am open to anything as long as it makes sense, and we will follow the convention from now on.
Unique-Divine
changed the title
| (gogoproto.nullable) = false | ||
| ]; | ||
| string denom = 3; | ||
| string to_addr = 4; |
There was a problem hiding this comment.
(nit) it might be safer to just withdraw to whoever sends the message, i.e. the message sender.
I can foresee possible human error in the future where somebody sends the message but accidentally sets the to_addr different from the message signer/sender.
There was a problem hiding this comment.
I think so too. The problem we might encounter here is when the claim goes to a smart contract that doesn't expose a function to withdraw its balance, or if there's routing happening between multiple contracts like the ones used in Mars Protocol or in future Nibiru-developed contracts.
sudo_contract_calls_msg -> withdraw_to_contract_addr -> No fn to withdraw -> funds are lost
I'm trying to avoid this ^ deadlock.
Adding an explicit to_addr makes it less error prone in a different way, by making the caller pass in the location they want the funds to end up.
Another option is to make that field nullable, so that empty means, to_addr := sender. Would you prefer that?
There was a problem hiding this comment.
Wow, I didn't even consider that scenario. In that case, then I think it's better to be explicit.
Thinking about it more, in the general case, we'll likely be sending these WithdrawFromPerpFund messages from the CW3 multisig. Having the ability to target a recipient with the message would be more efficient, instead of withdrawing to the CW3 multisig and then doing a bank transfer.
A more sophisticated design could be to only execute the message if the to_addr is in the sudo sudoers list. It would prevent the funds from going to a CosmWasm smart contract that doesn't have the withdraw functionality, because we would be the ones whitelisting the possible recipients. But that could be addressed in a separate PR. What's implemented looks good to me.
CHANGELOG.md
Outdated
| @@ -66,6 +66,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
| * [#1573](https://github.com/NibiruChain/nibiru/pull/1573) - feat(perp): Close markets and compute settlement price | |||
| * [#1705](https://github.com/NibiruChain/nibiru/pull/1705) - feat(perp): Add oracle pair to market object | |||
| * [#1718](https://github.com/NibiruChain/nibiru/pull/1718) - fix: fees does not require additional funds | |||
| * [#1734](https://github.com/NibiruChain/nibiru/pull/1734) - feat(perp): MsgDonateToPerpFund | |||
There was a problem hiding this comment.
A little bit of a more descriptive message like the one in the title would be nice. You will appreciate it when you search for this in the changelog in four years!
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (2 hunks)
Additional comments: 2
CHANGELOG.md (2)
66-70: The entry for PR feat(perp): MsgWithdrawFromPerpFund sudo call as part of #1642 #1734 is correctly placed under the "Dapp modules: perp, spot, etc" section, which is appropriate given the context of the PR related to the perpetual futures module.
86-91: The dependency bumps are listed under the correct section "Dependencies" and include the necessary links to the pull requests for traceability.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- x/perp/v2/types/tx.pb.go
Files selected for processing (12)
- proto/nibiru/perp/v2/tx.proto (6 hunks)
- wasmbinding/exec_perp.go (3 hunks)
- x/perp/v2/integration/action/market.go (4 hunks)
- x/perp/v2/integration/action/settlement.go (2 hunks)
- x/perp/v2/keeper/amm_test.go (22 hunks)
- x/perp/v2/keeper/keeper.go (3 hunks)
- x/perp/v2/keeper/msg_server.go (4 hunks)
- x/perp/v2/keeper/sudo.go (7 hunks)
- x/perp/v2/keeper/sudo_test.go (5 hunks)
- x/perp/v2/module/abci_test.go (2 hunks)
- x/perp/v2/module/genesis.go (1 hunks)
- x/perp/v2/module/genesis_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- x/perp/v2/keeper/amm_test.go
Additional comments: 37
wasmbinding/exec_perp.go (2)
36-36: The change from
AdmintoSudoin theSetMarketEnabledmethod correctly reflects the shift in access control mechanisms. Ensure that the necessary permissions and checks are in place for the use ofSudo.74-77: The change from
AdmintoSudoin theCreateMarketmethod is consistent with the new access control strategy. Verify that theSudomethod enforces the appropriate authorization checks.x/perp/v2/integration/action/settlement.go (2)
20-20: The update to use
Sudo().CloseMarketin thecloseMarkettype'sDofunction is in line with the new administrative control changes. Confirm that theSudofunctionality is properly secured.38-38: The use of
Sudo().CloseMarketin thecloseMarketShouldFailtype'sDofunction is consistent with the access control changes. Ensure that theSudomethod is correctly handling permissions.x/perp/v2/module/abci_test.go (2)
37-37: The replacement of
Admin.CreateMarketwithSudo().CreateMarketin theTestSnapshotUpdatesfunction is correct. Verify that the test still passes and that theSudomethod is being tested for the correct behavior.122-122: The change to
Sudo().CreateMarketin theTestEndBlockerfunction aligns with the updated access control. Confirm that the test coverage is adequate and that theSudomethod is functioning as expected.x/perp/v2/module/genesis.go (1)
- 77-77: The update to use
Sudo().UnsafeChangeCollateralDenomin theInitGenesisfunction is consistent with the shift towards usingSudofor administrative tasks. Ensure that this change is accompanied by the necessary permission checks.x/perp/v2/integration/action/market.go (4)
130-130: The change to
Sudo().ShiftPegMultiplierin theshiftPegMultipliertype'sDofunction correctly reflects the new access control strategy. Verify that theSudomethod is properly secured and tested.149-149: The update to
Sudo().ShiftSwapInvariantin theshiftSwapInvarianttype'sDofunction is in line with the access control changes. Confirm that theSudomethod is correctly handling permissions and that the functionality is tested.169-169: The use of
Sudo().CreateMarketin thecreatePooltype'sDofunction aligns with the updated access control. Ensure that theSudomethod is functioning as expected and that the change is covered by tests.203-203: The change to
Sudo().ChangeCollateralDenomin thesetCollateraltype'sDofunction is consistent with the new administrative control strategy. Verify that theSudomethod enforces the appropriate authorization checks.x/perp/v2/keeper/keeper.go (2)
28-29: The addition of
SudoKeeperto theKeeperstruct aligns with the shift fromAdmintoSudofunctionality.63-66: The updated
NewKeeperfunction correctly initializes theKeeperstruct without setting anAdminfield, reflecting the move toSudofunctionality.x/perp/v2/module/genesis_test.go (1)
- 94-95: The
ChangeCollateralDenomfunction call has been correctly updated to useSudo()instead ofAdmin. Ensure that all references to the oldAdminfunctionality have been updated throughout the codebase.Verification successful
The search for references to
Admin.ChangeCollateralDenomin Go files did not yield any results. This suggests that there are no remaining references to the oldAdminfunctionality for changing the collateral denomination in the codebase, at least not with the exact method nameChangeCollateralDenom.Given this information, it appears that the change from
AdmintoSudofor theChangeCollateralDenomfunction call has been correctly implemented without any leftover references to the old method.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old Admin functionality. rg --type go 'Admin\.ChangeCollateralDenom'Length of output: 45
x/perp/v2/keeper/sudo.go (6)
14-26: The addition of the
Sudofunction andsudoExtensiontype is a good encapsulation of sudo functionality, making it clear when sudo-level operations are being used.28-61: The
WithdrawFromPerpFundfunction has been added with adenomparameter, allowing for withdrawals in different denominations. This is a flexible design that accommodates multiple collateral types.161-165: The
ChangeCollateralDenomfunction has been correctly updated to be callable only by sudoers, aligning with the shift toSudofunctionality.175-178: The
UnsafeChangeCollateralDenomfunction is a necessary addition for genesis operations where sudo permissions are not required.189-192: The
ShiftPegMultiplierfunction has been updated to be callable only by sudoers, which is appropriate for operations that can significantly impact the system's economics.235-238: The
ShiftSwapInvariantfunction has been updated to be callable only by sudoers, ensuring that sensitive operations that can affect market liquidity are properly gated.x/perp/v2/keeper/msg_server.go (5)
- 134-137: The function
DonateToEcosystemFundhas been updated to usetypes.PerpFundModuleAccountinstead oftypes.PerpEFModuleAccount. Ensure that all references to the module account are consistent across the entire codebase.Verification successful
The references to
PerpFundModuleAccountare consistent across the codebase, and there are no remaining occurrences ofPerpEFModuleAccount. The change appears to have been applied correctly.* 156-156: The `ChangeCollateralDenom` function now uses the `Sudo` permission. Ensure that the necessary access control checks are in place to restrict this function to sudoers only.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the PerpFundModuleAccount is consistently used across the codebase. rg --type go 'PerpEFModuleAccount' rg --type go 'PerpFundModuleAccount'Length of output: 12707
Verification successful
The output from the script provides the necessary context to verify the implementation of the
Sudopermission within theChangeCollateralDenommethod. The method performs a basic validation check on the incoming message and then proceeds to callm.k.Sudo().ChangeCollateralDenomwith the provided parameters.However, the output does not show the internal implementation of the
Sudomethod itself, which is where the actual permission check should be enforced. To confirm that theSudomethod is properly restricting access to sudoers only, we would need to inspect the implementation of theSudomethod.Since the
Sudomethod is likely part of the keeper's functionality and not directly visible within theChangeCollateralDenommethod, we would need to verify theSudomethod separately. Given the current context, we can conclude that:
- The
ChangeCollateralDenommethod is indeed using theSudopermission as indicated in the PR summary and review comment.- The actual access control checks for the
Sudopermission need to be verified within the implementation of theSudomethod.To proceed, we should generate a new script to inspect the
Sudomethod's implementation in the keeper package.
The output from the script shows the
Sudomethod in thekeeperpackage, which returns asudoExtensiontype. ThissudoExtensiontype is a struct that embeds theKeepertype. The comment above theWithdrawFromPerpFundmethod within thesudoExtensionstruct indicates that it is intended for sending funds from the Perp Fund to a specified address and that the sender must be an admin address registered inx/sudo.This suggests that the
Sudomethod is designed to extend theKeeperwith additional functionality that is restricted to administrative use. However, the output does not provide the specific implementation details of how the access control is enforced, such as checking if the sender is indeed an admin.To fully confirm that the
Sudopermission is correctly implemented to restrict access to sudoers only, we would need to see the implementation details of thesudoExtensionmethods, particularly any method that performs the access control check.Given the current context, we can conclude that:
- The
Sudomethod is part of thekeeperpackage and is intended for administrative functions.- The
sudoExtensionstruct is used to extend theKeeperwithSudo-specific methods.- The access control mechanism to ensure that only sudoers can invoke these methods needs to be verified by inspecting the implementation of the
sudoExtensionmethods.To proceed, we should generate a new script to inspect the implementation details of the
sudoExtensionmethods, focusing on access control checks.
The output from the script provides the declaration of the
sudoExtensionstruct and the beginning of theWithdrawFromPerpFundmethod within thesudoExtension. The comment above the method indicates that it is used to send funds from the Perp Fund to a specified "to" address and that the sender must be an admin address registered inx/sudo.However, the output does not show the actual implementation of the
WithdrawFromPerpFundmethod, specifically the part where it would perform the access control check to ensure that the sender is indeed an admin. To confirm that theSudopermission is correctly implemented to restrict access to sudoers only, we need to see the body of theWithdrawFromPerpFundmethod where the access control check is expected to be enforced.To proceed, we should generate a new script to extract the full implementation of the
WithdrawFromPerpFundmethod to verify the access control mechanism.
The output from the script provides the implementation of the
WithdrawFromPerpFundmethod within thesudoExtension. The method includes an access control check that callsk.SudoKeeper.CheckPermissions(sender, ctx). If the sender does not have the appropriate permissions, an error is returned, preventing further execution of the method.This confirms that the
Sudopermission is being enforced within theWithdrawFromPerpFundmethod by checking the permissions of the sender against theSudoKeeper. The method proceeds to prepare a coin transfer only if the permission check passes.Based on the provided context, we can conclude that:
- The
ChangeCollateralDenommethod uses theSudopermission as indicated in the PR summary and review comment.- The
Sudopermission is correctly implemented within theWithdrawFromPerpFundmethod to restrict access to sudoers only, as evidenced by theCheckPermissionscall.Therefore, the review comment concerning the
ChangeCollateralDenomfunction using theSudopermission and ensuring that the necessary access control checks are in place to restrict this function to sudoers only is consistent with the codebase.* 206-206: The `ShiftPegMultiplier` function has been updated to use the `Sudo` permission. Confirm that the permission checks are correctly implemented.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the Sudo permission is properly checked before executing the ChangeCollateralDenom function. ast-grep --lang go --pattern $'func (m msgServer) ChangeCollateralDenom($_, $_) { m.k.Sudo().ChangeCollateralDenom($$$) }'Length of output: 125
Script:
#!/bin/bash # Search for the ChangeCollateralDenom function to verify its existence and usage of Sudo permission. ast-grep --lang go --pattern $'func (m msgServer) ChangeCollateralDenom($_, $_) { $$$ }'Length of output: 91
Script:
#!/bin/bash # Search for any Go file containing the ChangeCollateralDenom method. rg --type go 'func \(.*msgServer\) ChangeCollateralDenom'Length of output: 132
Script:
#!/bin/bash # Extract the contents of the ChangeCollateralDenom method to verify the Sudo permission implementation. rg --type go -A 10 'func \(m msgServer\) ChangeCollateralDenom' x/perp/v2/keeper/msg_server.goLength of output: 425
Script:
#!/bin/bash # Extract more of the ChangeCollateralDenom method to verify the Sudo permission implementation. rg --type go -A 20 'func \(m msgServer\) ChangeCollateralDenom' x/perp/v2/keeper/msg_server.goLength of output: 769
Script:
#!/bin/bash # Inspect the implementation of the Sudo method in the keeper package. rg --type go -A 10 'func \(.*Keeper\) Sudo' x/perp/v2/keeper/Length of output: 706
Script:
#!/bin/bash # Inspect the implementation details of the sudoExtension methods, focusing on access control checks. rg --type go -A 10 'type sudoExtension struct' x/perp/v2/keeper/sudo.goLength of output: 499
Script:
#!/bin/bash # Extract the full implementation of the WithdrawFromPerpFund method to verify the access control mechanism. rg --type go -A 20 'func \(k sudoExtension\) WithdrawFromPerpFund' x/perp/v2/keeper/sudo.goLength of output: 663
- 218-218: The
ShiftSwapInvariantfunction has been updated to use theSudopermission. Confirm that the permission checks are correctly implemented.Verification successful
The script output provides the implementation details for the
ShiftSwapInvariantfunction within thesudo.gofile. The relevant lines of code show that there is a permission check implemented as follows:func (k sudoExtension) ShiftSwapInvariant( ctx sdk.Context, pair asset.Pair, newSwapInvariant sdkmath.Int, sender sdk.AccAddress, ) error { if err := k.SudoKeeper.CheckPermissions(sender, ctx); err != nil { return err } // ... rest of the function implementation }This code snippet confirms that the
ShiftSwapInvariantfunction does indeed perform a permission check usingk.SudoKeeper.CheckPermissions(sender, ctx). If this check fails, an error is returned, preventing the function from executing further.Based on this information, the review comment regarding the implementation of the
Sudopermission check for theShiftSwapInvariantfunction is verified to be correctly implemented.* 222-234: A new function `WithdrawFromPerpFund` has been added, which allows withdrawing from the perpetual funding pool and is restricted to the `SUDO` permission. Ensure that the function is correctly implemented and that the permission checks are in place.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the Sudo permission is properly checked before executing the ShiftSwapInvariant function. ast-grep --lang go --pattern $'func (m msgServer) ShiftSwapInvariant($_, $_) { m.k.Sudo().ShiftSwapInvariant($$$) }'Length of output: 119
Script:
#!/bin/bash # Search for the definition and usage of ShiftSwapInvariant function to verify the Sudo permission check. rg "func .*ShiftSwapInvariant"Length of output: 5538
Script:
#!/bin/bash # Extract the relevant section of the sudo.go file to verify the Sudo permission check implementation for ShiftSwapInvariant. rg -C 10 "func \(k sudoExtension\) ShiftSwapInvariant" x/perp/v2/keeper/sudo.goLength of output: 780
proto/nibiru/perp/v2/tx.proto (6)
- 33-33: The permission comment for
ChangeCollateralDenomhas been updated to "[SUDO]". Ensure that the documentation and implementation are consistent with this permission level.Verification successful
The search results indicate that the
ChangeCollateralDenommessage is present in theproto/nibiru/perp/v2/tx.protofile. However, the output does not show the permission level comment directly. To verify that the documentation forChangeCollateralDenomreflects the "SUDO" permission level, we need to view the surrounding comments of theChangeCollateralDenommessage definition in thetx.protofile.
The output from the script confirms that the
MsgChangeCollateralDenommessage in theproto/nibiru/perp/v2/tx.protofile has a comment indicating "[SUDO] Only callable by sudoers." This aligns with the review comment that the permission forChangeCollateralDenomhas been updated to "[SUDO]". Therefore, the documentation forChangeCollateralDenomis consistent with the permission level as per the provided code context.* 44-44: The permission comment for `ShiftPegMultiplier` has been updated to "[SUDO]". Ensure that the documentation and implementation are consistent with this permission level.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the documentation for ChangeCollateralDenom reflects the SUDO permission level. rg --type proto 'ChangeCollateralDenom'Length of output: 69
Script:
#!/bin/bash # Search for the ChangeCollateralDenom message in .proto files without specifying the file type. rg 'ChangeCollateralDenom' -g '*.proto'Length of output: 666
Script:
#!/bin/bash # Extract the surrounding comments for the ChangeCollateralDenom message to verify the SUDO permission level. rg -C 3 'message MsgChangeCollateralDenom {' -g 'proto/nibiru/perp/v2/tx.proto'Length of output: 211
Script:
#!/bin/bash # Correct the regex pattern by escaping the curly brace and extract the surrounding comments for the ChangeCollateralDenom message. rg -C 3 'message MsgChangeCollateralDenom \{' -g 'proto/nibiru/perp/v2/tx.proto'Length of output: 483
Verification successful
The review comment has been verified successfully. The permission comment for
ShiftPegMultiplierin theproto/nibiru/perp/v2/tx.protofile has been updated to include "[SUDO] Only callable by sudoers," which is consistent with the review comment.
* 49-49: The permission comment for `ShiftSwapInvariant` has been updated to "[SUDO]". Ensure that the documentation and implementation are consistent with this permission level.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the documentation for ShiftPegMultiplier reflects the SUDO permission level. rg --type proto 'ShiftPegMultiplier'Length of output: 66
Script:
#!/bin/bash # Search for occurrences of ShiftPegMultiplier in all text files, as the proto file type is not recognized by ripgrep by default. rg 'ShiftPegMultiplier'Length of output: 13359
Script:
#!/bin/bash # Extract the relevant lines from the tx.proto file to verify the permission comment for ShiftPegMultiplier. rg -C 3 'ShiftPegMultiplier' proto/nibiru/perp/v2/tx.protoLength of output: 914
Verification successful
The search results indicate that the
ShiftSwapInvariantmessage is present in theproto/nibiru/perp/v2/tx.protofile, and there are comments associated with it. However, the output does not show the actual content of the comments to verify if the permission level "[SUDO]" is correctly documented.To ensure that the documentation for
ShiftSwapInvariantreflects the "[SUDO]" permission level, I will need to view the content of the comments around theShiftSwapInvariantmessage in thetx.protofile.
The output from the script confirms that the
MsgShiftSwapInvariantmessage in theproto/nibiru/perp/v2/tx.protofile is documented with the comment// [SUDO] Only callable sudoers.This indicates that the documentation is consistent with the permission level "[SUDO]" as mentioned in the review comment.Based on the provided information, the review comment regarding the permission comment for
ShiftSwapInvariantbeing updated to "[SUDO]" is correct and consistent with the implementation in the codebase.* 53-56: A new service message `WithdrawFromPerpFund` has been added with "SUDO" permissions. Confirm that the new message is properly documented and implemented.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the documentation for ShiftSwapInvariant reflects the SUDO permission level. rg --type proto 'ShiftSwapInvariant'Length of output: 66
Script:
#!/bin/bash # Search for the ShiftSwapInvariant message in .proto files without specifying the file type. rg 'ShiftSwapInvariant' -g '*.proto'Length of output: 814
Script:
#!/bin/bash # Extract the comments and the message definition for ShiftSwapInvariant from the tx.proto file. rg -A 5 -B 5 'message MsgShiftSwapInvariant ' proto/nibiru/perp/v2/tx.protoLength of output: 455
Verification successful
The
WithdrawFromPerpFundmessage has been found in thetx.protofile with the "SUDO" permission level documented. It is also present in various implementation and test files, indicating that it has been integrated into the codebase. The implementation details are beyond the scope of the provided context, but based on the available information, the documentation and presence of the message in the codebase are confirmed.* 444-444: Consider the suggestion by `k-yang` to use a `sdk.Coin` field instead of separate `amount` and `denom` fields for simplicity. However, the reasoning provided by `Unique-Divine` for keeping them separate is valid. Ensure that the implementation accounts for the possibility of a blank `denom` defaulting to the collateral denom.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the documentation for WithdrawFromPerpFund reflects the SUDO permission level and that the message is properly implemented. rg --type proto 'WithdrawFromPerpFund'Length of output: 68
Script:
#!/bin/bash # Search for occurrences of WithdrawFromPerpFund in the repository to verify its documentation and implementation. rg 'WithdrawFromPerpFund'Length of output: 9875
- 450-450: The concern raised by
k-yangabout the potential for human error with theto_addrfield is valid. The response fromUnique-Divineaddresses this concern by explaining the rationale for including an explicitto_addr. Ensure that the implementation mitigates the risk of funds being lost due to contract routing issues.x/perp/v2/keeper/sudo_test.go (6)
31-87: The refactoring of
TestAdmin_WithdrawFromInsuranceFundtoTestAdmin_WithdrawFromPerpFundaligns with the PR's objective to update the withdrawal process to use theSudofunctionality. The test cases within this function are well-structured and seem to cover both successful withdrawal and error handling when attempting to withdraw more than the available balance. However, it's important to ensure that theamountToFundandamountToWithdraware tested with various edge cases to ensure robustness.90-97: The
TestCreateMarketfunction has been updated to use theSudofunctionality. It's crucial to verify that theCreateMarketmethod behaves correctly when provided with invalid parameters and that the error messages are clear and informative. The test cases appear to cover different failure scenarios, which is good practice.338-340: The
TestAdmin_ChangeCollateralDenomfunction now uses theSudofunctionality to change the collateral denomination. It's important to ensure that theChangeCollateralDenommethod is thoroughly tested with various denominations, including normal, token factory, IBC, and invalid denominations, to validate the error handling and success paths.407-408: The addition of the
MsgWithdrawFromPerpFundcase in theHandleMsgfunction is consistent with the PR's objective to introduce a new sudo call for withdrawing from the perpetual fund. It's important to ensure that this new message type is handled correctly and that appropriate tests are added to cover its functionality.435-440: In the
TestCheckPermissionsfunction, new test cases forMsgWithdrawFromPerpFundhave been added to verify that the expected admin functions fail without sudo permissions. It's important to ensure that these tests cover all necessary permission scenarios and that the error messages are clear for unauthorized access.471-487: The
DoWithdrawFromPerpFundTestfunction is a new addition that tests the withdrawal functionality. It's important to ensure that this function is called with various parameters to test different scenarios, including edge cases and failure modes. Additionally, the test should verify that the funds are correctly withdrawn from the module account and received by the intended recipient.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- CHANGELOG.md (2 hunks)
- x/perp/v2/module/module.go (1 hunks)
Files skipped from review due to trivial changes (1)
- CHANGELOG.md
Additional comments: 1
x/perp/v2/module/module.go (1)
- 148-150: The renaming from
PerpEFModuleAccounttoPerpFundModuleAccountin theGetModuleAccountcalls should be checked for consistency across the entire codebase to ensure that all references to the module account are updated accordingly.Verification successful
The review comment has been verified:
- No remaining references to
PerpEFModuleAccountwere found, suggesting that it has been completely replaced.- The new
PerpFundModuleAccountis used consistently across the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old `PerpEFModuleAccount` to ensure they've been updated. rg --type go 'PerpEFModuleAccount' # Confirm that the new `PerpFundModuleAccount` is used consistently. rg --type go 'PerpFundModuleAccount'Length of output: 12707
Purpose / Abstract
Summary by CodeRabbit
New Features
WithdrawFromPerpFundfunctionality for the perpetual fund module account.Improvements
Refactor
PerpEFModuleAccounttoPerpFundModuleAccountacross various modules.AdmintoSudoto align with access control policies.Bug Fixes
Documentation
Tests
WithdrawFromPerpFundfunctionality.Chores
Revert