feat(perp): MsgShiftPegMultiplier, MsgShiftSwapInvariant #1680
Conversation
Bumps [cosmossdk.io/math](https://github.com/cosmos/cosmos-sdk) from 1.1.2 to 1.2.0. - [Release notes](https://github.com/cosmos/cosmos-sdk/releases) - [Changelog](https://github.com/cosmos/cosmos-sdk/blob/main/CHANGELOG.md) - [Commits](cosmos/cosmos-sdk@math/v1.1.2...log/v1.2.0) --- updated-dependencies: - dependency-name: cosmossdk.io/math dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>
|
Warning Rate Limit Exceeded@Unique-Divine has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 14 minutes and 3 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe updates revolve around the introduction of new admin functions for adjusting the peg multiplier and swap invariant parameters in an AMM pool. This includes the addition of Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- x/perp/v2/types/tx.pb.go
Files selected for processing (23)
- CHANGELOG.md (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 (4 hunks)
- wasmbinding/exec_test.go (2 hunks)
- wasmbinding/message_plugin.go (1 hunks)
- x/perp/v2/integration/action/dnr.go (1 hunks)
- x/perp/v2/integration/action/market.go (2 hunks)
- x/perp/v2/keeper/admin.go (1 hunks)
- x/perp/v2/keeper/admin_test.go (12 hunks)
- x/perp/v2/keeper/amm.go (3 hunks)
- x/perp/v2/keeper/amm_test.go (4 hunks)
- x/perp/v2/keeper/fixture_test.go (1 hunks)
- x/perp/v2/keeper/msg_server.go (2 hunks)
- x/perp/v2/types/amm.go (1 hunks)
- x/perp/v2/types/amm_test.go (1 hunks)
- x/perp/v2/types/codec.go (2 hunks)
- x/perp/v2/types/codec_test.go (1 hunks)
- x/perp/v2/types/errors.go (2 hunks)
- x/perp/v2/types/msgs.go (12 hunks)
- x/perp/v2/types/msgs_test.go (4 hunks)
- x/sudo/keeper/keeper.go (1 hunks)
Files skipped from review due to trivial changes (6)
- wasmbinding/exec_test.go
- x/perp/v2/integration/action/dnr.go
- x/perp/v2/keeper/fixture_test.go
- x/perp/v2/types/amm.go
- x/perp/v2/types/codec_test.go
- x/perp/v2/types/errors.go
Additional comments: 41
CHANGELOG.md (1)
- 60-63: The new feature entries for
MsgShiftPegMultiplierandMsgShiftSwapInvariantare correctly placed under the "State Machine Breaking" section, as they likely introduce changes that could affect the state machine.proto/nibiru/perp/v2/tx.proto (3)
41-50: The addition of the
ShiftPegMultiplierandShiftSwapInvariantRPC methods with admin-only comments is clear and follows the existing code style.395-407: The
MsgShiftPegMultipliermessage structure is correctly defined with appropriate field types and non-nullable constraints.414-426: The
MsgShiftSwapInvariantmessage structure is correctly defined with appropriate field types and non-nullable constraints.wasmbinding/bindings/msg.go (2)
20-25: The removal of
PegShiftandDepthShifttypes from theNibiruMsgstruct aligns with the changes described in the summary. Ensure that any code that previously depended on these fields has been updated or removed accordingly.20-25: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-68]
Verify that the removal of
PegShiftandDepthShiftfrom theNibiruMsgstruct does not result in any orphaned code or unhandled cases elsewhere in the codebase.wasmbinding/exec_perp.go (2)
140-142: The
InsuranceFundWithdrawfunction lacks a sender parameter in its signature, which might be necessary for permission checks or logging. Please verify if this is intentional or an oversight.137-142: Ensure that all references to the removed
PegShiftandDepthShiftmethods have been updated or removed throughout the entire codebase to prevent any broken functionality.wasmbinding/exec_perp_test.go (4)
91-96: The changes in the test suite appear to be consistent with the removal of the
DoPegShiftTestandDoDepthShiftTestfunctions as described in the summary. The remaining tests are well-structured and there are no signs of orphaned code or logic issues.211-216: The test function
DoInsuranceFundWithdrawTestis correctly implemented and follows the expected test pattern. It's good to see that the test checks for the proper handling of module account funding before attempting the withdrawal.280-285: The
TestSadPaths_Nilfunction correctly tests for nil inputs to various executor methods, ensuring that the system behaves as expected when provided with invalid arguments.319-324: The
TestSadPaths_InvalidPairfunction is testing the system's behavior with an invalid trading pair. This is a good practice to ensure that the system can handle unexpected or malformed input gracefully.wasmbinding/message_plugin.go (1)
- 80-81: The changes in the
DispatchMsgfunction appear to be correct and handle different message types as expected.x/perp/v2/integration/action/market.go (2)
142-144: The
editPriceMultiplieraction now usesUnsafeShiftPegMultiplier. Ensure that the necessary validations are performed before this action is invoked to prevent unintended consequences due to the "unsafe" nature of the method.159-160: Similarly, the
editSwapInvariantaction usesUnsafeShiftSwapInvariant. It is crucial to verify that appropriate checks are in place before this method is called to maintain system integrity.x/perp/v2/keeper/admin.go (2)
187-190: The permission check using
SudoKeeperis correctly placed before the call toUnsafeShiftPegMultiplier, ensuring only authorized users can perform this action.202-205: The permission check using
SudoKeeperis correctly placed before the call toUnsafeShiftSwapInvariant, ensuring only authorized users can perform this action.x/perp/v2/keeper/admin_test.go (5)
11-11: The addition of the "github.com/stretchr/testify/suite" import is appropriate for the new test suite
TestSuiteAdmin.21-21: The renaming of the "types" package alias to "perptypes" is consistent and improves clarity.
352-486: The addition of the
TestSuiteAdmintest suite with setup, test, and helper functions is a good practice for organizing admin-related tests.410-486: The new test cases for admin permissions and admin functionality are well-structured and cover important scenarios.
348-486: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-486]
No issues found in the provided hunks regarding logic, security, performance, or consistency.
x/perp/v2/keeper/amm.go (4)
14-21: The function
UnsafeShiftPegMultiplierhas been renamed and updated correctly according to the summary. Ensure that all references to this function in the codebase are updated to reflect the new name and signature.51-56: The function
UnsafeShiftSwapInvarianthas been renamed and updated correctly according to the summary. Ensure that all references to this function in the codebase are updated to reflect the new name and signature.85-86: The signature of the function
handleMarketUpdateCosthas been updated correctly according to the summary. Ensure that all calls to this function are updated to match the new signature.90-90: The error handling for insufficient funds in the margin vault is intentionally omitted, as explained in the comment. This is a business logic decision, but it's important to ensure that this behavior is well-documented and understood by the team to avoid confusion.
x/perp/v2/keeper/amm_test.go (4)
188-193: The error message checks in the test cases correctly reflect the new error types returned by the
UnsafeShiftPegMultiplierfunction.214-215: The error message checks in the test cases correctly reflect the new error types returned by the
UnsafeShiftPegMultiplierfunction.254-259: The error message checks in the test cases correctly reflect the new error types returned by the
UnsafeShiftSwapInvariantfunction.280-285: The error message checks in the test cases correctly reflect the new error types returned by the
UnsafeShiftSwapInvariantfunction.x/perp/v2/keeper/msg_server.go (2)
113-118: The implementation of
SettlePositionappears to be correct and follows the standard pattern for gRPC service methods in the Cosmos SDK.192-218: The addition of
ShiftPegMultiplierandShiftSwapInvariantfunctions is consistent with the admin-only pattern for sensitive operations. Ensure that the permission checks for sudoers are properly implemented elsewhere in the codebase.x/perp/v2/types/amm_test.go (1)
- 589-593: The update from
types.ErrNegativeSwapInvarianttotypes.ErrNonPositiveSwapInvariantcorrectly reflects the broader condition where thenewSwapInvariantis not only negative but also when it is zero. This change should be verified across the codebase to ensure that all relevant error checks and associated tests are updated accordingly.x/perp/v2/types/codec.go (2)
16-20: The registration of new message types
MsgShiftPegMultiplierandMsgShiftSwapInvariantin the codec is correct and aligns with the summary provided.31-35: The registration of new message types
MsgShiftPegMultiplierandMsgShiftSwapInvariantin the interface registry is correct and aligns with the summary provided.x/perp/v2/types/msgs_test.go (5)
375-394: The test cases for
MsgShiftPegMultipliercorrectly check for an invalid pair and a nonpositive peg multiplier. This aligns with the expected validation logic for the message.397-414: The test cases for
MsgShiftSwapInvariantcorrectly check for an invalid pair and a nonpositive swap invariant. This aligns with the expected validation logic for the message.441-445: The
MsgShiftPegMultiplierandMsgShiftSwapInvariantmessages have been correctly added to themsgValidSenderListfor testing theGetSignersmethod with a valid sender.456-457: The
MsgShiftPegMultiplierandMsgShiftSwapInvariantmessages have been correctly added to themsgInvalidSenderListfor testing theGetSignersmethod with an invalid sender.595-600: The
MsgShiftPegMultiplierandMsgShiftSwapInvariantmessages have been correctly added to the test cases for theGetSignBytesmethod, ensuring that the serialization of these messages is consistent and correct.x/sudo/keeper/keeper.go (1)
- 141-147: The use of
%wfor error wrapping in theCheckPermissionsfunction is correct and allows for better error handling by enabling errors to be unwrapped later. This is a good practice in Go error handling.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 60-63: The addition of
MsgShiftPegMultiplierandMsgShiftSwapInvariantto the perpetual contracts module is a significant update that introduces new admin-only transaction messages for adjusting financial parameters. Ensure that the necessary documentation, user guides, and admin operational procedures are updated to reflect these new features.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1680 +/- ##
==========================================
+ Coverage 73.92% 74.17% +0.25%
==========================================
Files 192 192
Lines 15398 15451 +53
==========================================
+ Hits 11383 11461 +78
+ Misses 3355 3323 -32
- Partials 660 667 +7
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (5)
- x/perp/v2/keeper/admin.go (1 hunks)
- x/perp/v2/keeper/admin_test.go (12 hunks)
- x/perp/v2/keeper/amm.go (3 hunks)
- x/perp/v2/keeper/amm_test.go (6 hunks)
- x/perp/v2/types/msgs.go (12 hunks)
Additional comments: 11
x/perp/v2/keeper/admin.go (1)
- 178-206: The new functions
ShiftPegMultiplierandShiftSwapInvariantare correctly checking permissions before proceeding with the operation. Ensure that these functions are not exposed to the MsgServer, are not called from other methods in the x/perp module, and are only callable from nibiru/wasmbinding via sudo contracts as intended.x/perp/v2/keeper/amm.go (3)
14-21: The function
UnsafeShiftPegMultiplierhas been correctly renamed and the comments have been updated to reflect the new functionality. The additional parameterpair asset.Pairhas been added to the function signature as described in the summary.51-56: The function
UnsafeShiftSwapInvarianthas been correctly renamed and the comments have been updated to reflect the new functionality. The additional parameterpair asset.Pairhas been added to the function signature as described in the summary.85-86: The signature of the
handleMarketUpdateCostfunction has been correctly updated to include the additional parametercostAmt sdkmath.Int, aligning with the summary.x/perp/v2/types/msgs.go (7)
22-23: The addition of the new message types
MsgShiftPegMultiplierandMsgShiftSwapInvariantis confirmed and the interface assertions are correctly in place.254-255: The renaming of the
MsgPartialClosetype string to "partial_close_msg" is correct and aligns with the summary.285-285: The renaming of the
MsgChangeCollateralDenomtype string to "change_collateral_denom_msg" is correct and aligns with the summary.366-367: The validation in
MsgShiftPegMultipliercorrectly checks for a positiveNewPegMult, which aligns with the summary.393-394: The validation in
MsgShiftSwapInvariantcorrectly checks for a positiveNewSwapInvariant, which aligns with the summary.372-382: The
GetSignBytesandGetSignersmethods forMsgShiftPegMultiplierare correctly implemented.399-409: The
GetSignBytesandGetSignersmethods forMsgShiftSwapInvariantare correctly implemented.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- x/perp/v2/types/event.pb.go
Files selected for processing (7)
- proto/nibiru/perp/v2/event.proto (1 hunks)
- x/perp/v2/integration/action/market.go (2 hunks)
- x/perp/v2/keeper/admin.go (1 hunks)
- x/perp/v2/keeper/amm.go (6 hunks)
- x/perp/v2/keeper/amm_test.go (12 hunks)
- x/perp/v2/keeper/clearing_house_test.go (9 hunks)
- x/perp/v2/keeper/msg_server.go (2 hunks)
Files skipped from review due to trivial changes (1)
- x/perp/v2/keeper/clearing_house_test.go
Additional comments: 5
x/perp/v2/keeper/admin.go (1)
- 178-205: The code does not show any logic for checking or transferring funds from the perp fund to cover the adjustments made by
ShiftPegMultiplierandShiftSwapInvariant. This is mentioned in the summary as an important aspect of these operations. Please ensure that the necessary logic for handling funds is implemented or clarify if it's handled elsewhere.x/perp/v2/keeper/amm.go (4)
11-21: The function
UnsafeShiftPegMultiplierhas been updated to include a new parameternewPriceMultiplierand to emit an event after updating the price multiplier. Ensure that all calls to this function are updated to pass the new parameter and that the event emission is handled correctly.56-62: The function
UnsafeShiftSwapInvarianthas been updated to include a new parameternewSwapInvariantand to emit an event after updating the swap invariant. Ensure that all calls to this function are updated to pass the new parameter and that the event emission is handled correctly.93-95: The
handleMarketUpdateCostfunction now returnscostPaidalong with any error. Verify that the callers of this function handle the new return value correctly.129-146: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [121-141]
The logic for handling negative costs in the
handleMarketUpdateCostfunction does not treat a lack of funds in the vault as an error. This is an unusual approach and could potentially lead to bad debt in the system. It is recommended to verify this business logic decision with the stakeholders to ensure it aligns with the intended behavior of the system.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files selected for processing (3)
- x/perp/v2/keeper/amm.go (6 hunks)
- x/perp/v2/keeper/amm_test.go (12 hunks)
- x/perp/v2/types/errors.go (2 hunks)
Additional comments: 4
x/perp/v2/keeper/amm.go (3)
- 24-32: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [14-53]
The changes made to the
UnsafeShiftPegMultiplierfunction, including the renaming, addition of theoldPriceMultparameter, early return optimization, and event emission, are consistent with the summary and appear to be correctly implemented.
56-90: The changes made to the
UnsafeShiftSwapInvariantfunction, including the renaming and event emission, are consistent with the summary and appear to be correctly implemented.129-146: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [93-143]
The
handleMarketUpdateCostfunction has been updated to returncostPaid sdk.Coinand includes detailed error handling and comments explaining the logic for handling negative costs. These changes are consistent with the summary and appear to be correctly implemented.x/perp/v2/types/errors.go (1)
- 38-42: The renaming of
ErrNegativeSwapInvarianttoErrNonPositiveSwapInvariantand the updated error message forErrNilSwapInvariantare consistent with the summary. Verify that all references to these errors in the codebase have been updated accordingly.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- x/perp/v2/integration/action/market.go (1 hunks)
- x/perp/v2/keeper/admin.go (1 hunks)
- x/perp/v2/keeper/amm.go (6 hunks)
- x/perp/v2/keeper/amm_test.go (18 hunks)
- x/perp/v2/keeper/clearing_house_test.go (9 hunks)
Additional comments: 12
x/perp/v2/integration/action/market.go (1)
- 137-168: The changes to the
shiftPegMultiplierandshiftSwapInvariantstructs and their associatedDomethods, as well as the functions to create these actions, are consistent with the summary provided and appear to be correctly implemented.x/perp/v2/keeper/amm.go (10)
11-21: The function
UnsafeShiftPegMultiplierhas been correctly renamed and updated to include the new logic for handling the peg multiplier change.24-32: The early return when the new price multiplier is the same as the old one is a good practice to avoid unnecessary computation and state changes.
37-43: The error handling in
UnsafeShiftPegMultiplieris consistent and ensures that any error from calculating the cost or handling the market update cost is returned immediately.46-53: The event emission in
UnsafeShiftPegMultiplierhas been updated to include the old and new peg multipliers as well as the cost paid, which aligns with the summary of changes.61-62: The addition of the
newSwapInvariantparameter to theUnsafeShiftSwapInvariantfunction is consistent with the summary and allows for the necessary functionality to adjust the swap invariant.69-77: The error handling in
UnsafeShiftSwapInvariantis consistent and ensures that any error from calculating the cost or handling the market update cost is returned immediately.79-90: The event emission in
UnsafeShiftSwapInvarianthas been updated to include the old and new swap invariants as well as the cost paid, which aligns with the summary of changes.93-95: The
handleMarketUpdateCostfunction has been updated to return thecostPaidas part of its return values, which is necessary for the updated event emissions in theUnsafeShiftPegMultiplierandUnsafeShiftSwapInvariantfunctions.110-117: The error handling in
handleMarketUpdateCostwhen there's not enough funds in the perp EF to pay for a positive cost is clear and provides detailed information about the shortfall.129-141: The handling of a negative cost in
handleMarketUpdateCostis unconventional as it does not return an error if there are insufficient funds in the vault. This behavior should be verified to ensure it aligns with the system's requirements and risk management strategy.x/perp/v2/keeper/clearing_house_test.go (1)
- 2040-2048: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1956-2125]
The test cases provided for the
ShiftSwapInvariantfunction appear to be comprehensive and well-structured, covering a variety of scenarios including only long positions, only short positions, and both with increasing and decreasing swap invariants. The assertions after the swap invariant changes, such asAMMShouldBeEqualandModuleBalanceShouldBeEqualTo, are correctly used to verify the expected state of the system. The tests also include cleanup checks to ensure that positions are closed and module balances are as expected at the end of the tests. Overall, the test cases seem to be maintainable and easy to understand, which is good for future test enhancements and debugging.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- x/perp/v2/types/event.pb.go
- x/perp/v2/types/tx.pb.go
Files selected for processing (11)
- proto/nibiru/perp/v2/event.proto (1 hunks)
- proto/nibiru/perp/v2/tx.proto (2 hunks)
- x/common/testutil/testapp/testapp.go (2 hunks)
- x/perp/v2/integration/action/dnr.go (5 hunks)
- x/perp/v2/integration/action/market.go (2 hunks)
- x/perp/v2/integration/action/position.go (3 hunks)
- x/perp/v2/keeper/admin.go (1 hunks)
- x/perp/v2/keeper/amm.go (3 hunks)
- x/perp/v2/keeper/amm_test.go (18 hunks)
- x/perp/v2/keeper/msg_server.go (4 hunks)
- x/perp/v2/keeper/msg_server_test.go (3 hunks)
Files skipped from review due to trivial changes (1)
- x/perp/v2/keeper/msg_server_test.go
Additional comments: 29
proto/nibiru/perp/v2/tx.proto (2)
43-51: The addition of the
ShiftPegMultiplierandShiftSwapInvariantRPC methods aligns with the summary and pull request details, providing admin-level management of market parameters. Ensure that appropriate access controls are implemented to restrict these operations to admin users only.395-429: The message definitions for
MsgShiftPegMultiplierandMsgShiftSwapInvariantare consistent with the summary and pull request details. The use ofgithub.com/cosmos/cosmos-sdk/types.Decandgithub.com/cosmos/cosmos-sdk/types.Intfor the new peg multiplier and swap invariant ensures that the values are handled with the correct precision and type.x/common/testutil/testapp/testapp.go (5)
21-21: The import of
sudotypesis correctly added and used within the file.40-40: The
app.SudoKeeper.Sudoers.Setmethod call is correctly added and uses theDefaultSudoersfunction to set the initial sudoers state.46-50: The
NewContextfunction is correctly implemented and used to create a freshsdk.Contextfor the givenNibiruApp.54-60: The
DefaultSudoersfunction is correctly implemented, providing asudotypes.Sudoerstype with the root address set for both the root and contracts fields.63-65: The
DefaultSudoRootfunction is correctly implemented, returning ansdk.AccAddressin the correct Bech32 format.x/perp/v2/integration/action/dnr.go (1)
- 217-236: In the
fundDnREpochDomethod, a temporary account is funded and then used to callAllocateEpochRebates. It's important to ensure that this temporary account is securely handled and that its usage here does not introduce any security risks, such as the potential for unauthorized access to funds.x/perp/v2/integration/action/market.go (9)
9-12: The addition of the
testutil/testapppackage is appropriate as it is used within the newShiftPegMultiplierandShiftSwapInvariantfunctions.138-140: The new type
shiftPegMultiplieris correctly defined to encapsulate the parameters for theShiftPegMultiplierfunction.157-159: The new type
shiftSwapInvariantis correctly defined to encapsulate the parameters for theShiftSwapInvariantfunction.143-147: The
ShiftPegMultiplierfunction is correctly implemented to use thePerpKeeperV2.Admin.ShiftPegMultipliermethod with the provided parameters.162-166: The
ShiftSwapInvariantfunction is correctly implemented to use thePerpKeeperV2.Admin.ShiftSwapInvariantmethod with the provided parameters.138-140: The fields of the
shiftPegMultiplierstruct are unexported, which is suitable for encapsulation within the package.157-159: The fields of the
shiftSwapInvariantstruct are unexported, which is suitable for encapsulation within the package.143-147: The
Domethod ofshiftPegMultipliercorrectly calls theShiftPegMultiplieradmin function and returns the expected values.162-166: The
Domethod ofshiftSwapInvariantcorrectly calls theShiftSwapInvariantadmin function and returns the expected values.x/perp/v2/integration/action/position.go (2)
13-17: The addition of the
perpkeeperimport is appropriate for the new usage ofperpkeeper.NewMsgServerImplin thepartialCloseandpartialCloseFailsfunctions.346-357: The refactoring of the
partialClosefunction to use theMsgPartialClosetype and thePartialClosemethod onperpkeeper.MsgServerImplis consistent with the summary.x/perp/v2/keeper/admin.go (2)
174-175: The change to
UnsafeChangeCollateralDenomto set the collateral denom is correct and aligns with the summary.178-262: The new functions
ShiftPegMultiplierandShiftSwapInvariantare implemented correctly. They include permission checks, error handling, and event emission. Ensure that all dependent code and documentation are updated to reflect these new admin capabilities.x/perp/v2/keeper/amm.go (2)
- 50-67: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [14-64]
The
handleMarketUpdateCostfunction now assigns acostPaidvalue even in error scenarios. Ensure that this behavior is well-documented and understood by the team, as it represents a significant change in the function's contract. The comments provide some context, but it's crucial that all stakeholders are aware of this change and its implications.
- 52-52: The use of
nolint:staticchecksuppresses lint warnings. Confirm that this is the best approach and that no potential issues are being masked by this directive. It's important to ensure that code quality tools are used effectively and that exceptions are made with careful consideration.x/perp/v2/keeper/amm_test.go (6)
24-27: The test function name
TestShiftPegMultipliercorrectly reflects the updated functionality.35-35: The test case is correctly using the
ShiftPegMultiplierfunction to test the new functionality.192-192: The
ShiftPegMultiplierfunction call within theTestShiftPegMultiplier_Failtest case correctly includes theadminAddrparameter.265-265: The
ShiftSwapInvariantfunction call within theTestShiftSwapInvariant_Failtest case correctly replaces the deprecatedEditSwapInvariant.314-314: The test case is correctly using the
ShiftSwapInvariantfunction to test the new functionality.358-358: The test case is correctly using the
ShiftSwapInvariantfunction to test the new functionality.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (7)
- x/common/constants.go (1 hunks)
- x/common/testutil/const.go (1 hunks)
- x/common/testutil/testapp/testapp.go (2 hunks)
- x/perp/v2/integration/action/market.go (4 hunks)
- x/perp/v2/module/genesis_test.go (1 hunks)
- x/spot/genesis_test.go (1 hunks)
- x/spot/keeper/balances_test.go (1 hunks)
Files skipped from review due to trivial changes (2)
- x/common/testutil/const.go
- x/perp/v2/module/genesis_test.go
Additional comments: 13
x/common/constants.go (2)
10-10: The change from
NibiruTeamtoNIBIRU_TEAMaligns with Go's naming conventions for constants, which is a good practice for code readability and maintainability.13-14: The function
NibiruTeamAddrcorrectly uses the updated constantNIBIRU_TEAM. Ensure that all references to this constant throughout the codebase have been updated accordingly.x/common/testutil/testapp/testapp.go (4)
15-22: The import statements for the new packages
github.com/NibiruChain/nibiru/x/common/testutilandsudotypeshave been added correctly.54-65: The new functions
DefaultSudoersandDefaultSudoRoothave been added as per the summary and appear to be correctly implemented.38-43: The
NewNibiruTestAppAndContextfunction has been updated to include setting up the oracle prices and default sudoers, which aligns with the summary.67-69: The
NewNibiruTestAppAndContextAtTimefunction is a new utility function for testing purposes, allowing the creation of an app and context with a specified block time.x/perp/v2/integration/action/market.go (5)
9-12: The import statement for "github.com/NibiruChain/nibiru/x/common/testutil/testapp" has been added, ensure that it is used appropriately in the new functions and there are no unused imports.
138-154: The new type "shiftPegMultiplier" and its associated function "ShiftPegMultiplier" have been added. Verify that the new action is correctly integrated into the system and that the function is being used as intended.
157-173: The new type "shiftSwapInvariant" and its associated function "ShiftSwapInvariant" have been added. Verify that the new action is correctly integrated into the system and that the function is being used as intended.
207-210: The "setCollateral" struct has been modified to use "common.NIBIRU_TEAM" as the root sudoer. Ensure that this change is consistent with the intended use of the sudoers in the system and that it does not introduce any security or permission issues.
221-224: The "SetCollateral" function has been modified to use "common.NIBIRU_TEAM" as the sender. Ensure that this change is consistent with the intended use of the sender in the system and that it does not introduce any security or permission issues.
x/spot/genesis_test.go (1)
- 16-16: The addition of
testapp.EnsureNibiruPrefix()is noted. Ensure that this function is necessary for the test setup and does not introduce side effects that could affect the test outcomes.x/spot/keeper/balances_test.go (1)
- 55-55: The addition of
testapp.EnsureNibiruPrefix()is a good practice to ensure that the address prefix is correctly set for the test environment. This helps in maintaining consistency across tests and avoids potential issues with address formatting.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- app/keepers.go (8 hunks)
- x/common/testutil/testapp/testapp.go (2 hunks)
- x/devgas/v1/ante/ante_test.go (1 hunks)
- x/inflation/keeper/inflation_test.go (1 hunks)
- x/oracle/keeper/hooks_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- x/oracle/keeper/hooks_test.go
Additional comments: 11
app/keepers.go (1)
- 241-241: The introduction of the
govModuleAddrvariable to store the result ofauthtypes.NewModuleAddress(govtypes.ModuleName).String()and its subsequent use in the initialization of various keepers is a good practice. It avoids repeated function calls, which is a performance optimization.x/common/testutil/testapp/testapp.go (6)
29-29: The call to
EnsureNibiruPrefix()is correctly placed to set the address prefix before any test logic that depends on Bech32 addresses is executed.42-45: The code correctly sets default values for the
OracleKeeperandSudoKeeperwithin theNewNibiruTestAppAndContextfunction, ensuring that the test app is initialized with a consistent state.50-55: The
NewContextfunction is well-defined and returns a freshsdk.Contextwith a height of 1 and the current UTC time, which is consistent with the typical setup for a new blockchain context in tests.58-64: The
DefaultSudoersfunction correctly initializes thesudotypes.Sudoersstruct with the default sudo root address, which is necessary for setting up thex/sudomodule state in tests.67-68: The
DefaultSudoRootfunction correctly retrieves the default sudo root address from thetestutilpackage, which is a common pattern for setting up test addresses.73-73: The function
NewNibiruTestAppAndContextAtTimeis not mentioned in the summary. It appears to be a refactoring to use theNewContextfunction, which should be noted for clarity.x/devgas/v1/ante/ante_test.go (2)
27-30: The addition of
testapp.EnsureNibiruPrefix()in theTestAnteSuitefunction is confirmed and should ensure the correct setup for theAnteTestSuite.25-31: No further issues or changes detected in the provided hunk.
x/inflation/keeper/inflation_test.go (2)
72-82: The removal of the conditional block around the
SudoKeepermethod call is not reflected in the provided hunk. The test still contains logic that checkstc.rootAccountfor an empty string before setting the strategic reserve balance. If the conditional block was indeed removed, this could lead to unintended behavior whentc.rootAccountis an empty string. Please verify the intended behavior and update the test accordingly.76-77: The direct assignment of
tc.rootAccountto theRootfield in theSudoersstruct has been changed to use the value without conversion. Ensure thattc.rootAccountis provided in the correct format expected by theSudoersstruct, as this could potentially lead to issues if the format is not as expected.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (9)
- CHANGELOG.md (1 hunks)
- app/genesis.go (1 hunks)
- app/ibc_test.go (1 hunks)
- app/keepers.go (8 hunks)
- wasmbinding/wasmbin_test.go (1 hunks)
- x/perp/v2/integration/action/position.go (4 hunks)
- x/perp/v2/keeper/msg_server.go (3 hunks)
- x/perp/v2/keeper/msg_server_test.go (4 hunks)
- x/perp/v2/module/genesis_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- app/ibc_test.go
Additional comments: 23
CHANGELOG.md (1)
- 74-77: The changelog correctly documents the introduction of
MsgShiftPegMultiplierandMsgShiftSwapInvariantas state machine breaking changes, which aligns with the summary provided. The entries are clear and specify the pull request numbers for reference.app/keepers.go (10)
241-249: The change to store the result of
authtypes.NewModuleAddress(govtypes.ModuleName).String()ingovModuleAddrand use it in theConsensusParamsKeeperinitialization is correctly implemented.275-286: The change to store the result of
authtypes.NewModuleAddress(govtypes.ModuleName).String()ingovModuleAddrand use it in theBankKeeperinitialization is correctly implemented.287-293: The change to store the result of
authtypes.NewModuleAddress(govtypes.ModuleName).String()ingovModuleAddrand use it in thestakingKeeperinitialization is correctly implemented.298-304: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [294-302]
The change to store the result of
authtypes.NewModuleAddress(govtypes.ModuleName).String()ingovModuleAddrand use it in theDistrKeeperinitialization is correctly implemented.
308-314: The change to store the result of
authtypes.NewModuleAddress(govtypes.ModuleName).String()ingovModuleAddrand use it in thecrisisKeeperinitialization is correctly implemented.327-331: The change to store the result of
authtypes.NewModuleAddress(govtypes.ModuleName).String()ingovModuleAddrand use it in theupgradeKeeperinitialization is correctly implemented.337-341: The change to store the result of
authtypes.NewModuleAddress(govtypes.ModuleName).String()ingovModuleAddrand use it in theslashingKeeperinitialization is correctly implemented.443-448: The change to store the result of
authtypes.NewModuleAddress(govtypes.ModuleName).String()ingovModuleAddrand use it in theWasmKeeperinitialization is correctly implemented.451-453: The change to store the result of
authtypes.NewModuleAddress(govtypes.ModuleName).String()ingovModuleAddrand use it in theDevGasKeeperinitialization is correctly implemented.553-557: The change to store the result of
authtypes.NewModuleAddress(govtypes.ModuleName).String()ingovModuleAddrand use it in theGovKeeperinitialization is correctly implemented.wasmbinding/wasmbin_test.go (2)
21-23: The addition of the
initfunction to set a specific prefix is a critical change. Verify that this does not interfere with the initialization sequence of other packages or the global state.25-27: The
TestSetupContractsfunction appears to be correctly setting up the contracts for testing. Ensure that the unchanged status of this function is consistent with the intended behavior and that the function interacts correctly with the newinitfunction.x/perp/v2/integration/action/position.go (4)
13-18: The addition of the "perpkeeper" import aligns with the summary.
344-357: The refactoring of the
partialClosemethod to useMsgPartialCloseandPartialClosemethod onperpkeeper.MsgServerImplis consistent with the summary.376-390: The refactoring of the
partialCloseFailsmethod to useMsgPartialCloseandPartialClosemethod onperpkeeper.MsgServerImplis consistent with the summary.396-403: The correction of the spelling of the "expectedErr" parameter in the
PartialCloseFailsfunction signature is consistent with the summary.x/perp/v2/keeper/msg_server.go (1)
- 197-218: Verify that proper access control is implemented for the new admin-only functions
ShiftPegMultiplierandShiftSwapInvariantto ensure that only sudoers can call them.x/perp/v2/keeper/msg_server_test.go (4)
17-22: The import alias change from
keepertoperpkeeperis correctly reflected in the code.324-330: The usage of the new alias
perpkeeperinTestFailMsgServeris correct and consistent with the changes.372-377: The usage of the new alias
perpkeeperinTestMsgChangeCollateralDenomis correct and consistent with the changes.447-452: The usage of the new alias
perpkeeperinTestAllocateEpochRebatesis correct and consistent with the changes.x/perp/v2/module/genesis_test.go (1)
- 87-90: The change from
common.NibiruTeamtocommon.NIBIRU_TEAMshould be verified across the entire codebase to ensure that all references to this constant are updated accordingly, and that the change does not break any functionality that relies on the specific case of the variable value.
| @@ -78,6 +78,7 @@ func NewDefaultGenesisState(cdc codec.JSONCodec) GenesisState { | |||
| gen := ModuleBasics.DefaultGenesis(cdc) | |||
|
|
|||
| authGenesis := new(authtypes.GenesisState) | |||
| authtypes.DefaultGenesisState() | |||
There was a problem hiding this comment.
The call to authtypes.DefaultGenesisState() is not being used. If the intention was to set the default genesis state for the auth module, it should be assigned to authGenesis:
- authtypes.DefaultGenesisState()
+ *authGenesis = authtypes.DefaultGenesisState()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.
| authtypes.DefaultGenesisState() | |
| *authGenesis = authtypes.DefaultGenesisState() |
Purpose
CosmosMsg::StargateMsg#1580Summary by CodeRabbit
New Features
MsgShiftPegMultiplierto adjust peg multiplier parameters for perpetual contracts.MsgShiftSwapInvariantfor modifying swap invariant parameters in perpetual contracts.Improvements
Bug Fixes
ErrNegativeSwapInvarianttoErrNonPositiveSwapInvariantfor clarity.Refactor
PegShiftandDepthShiftfrom various modules.Tests
Documentation