chore(wasmbinding): delete CustomQuerier since we have QueryRequest::Stargate now #1687
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]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
WalkthroughThe changes revolve around the upgrade of a blockchain application, with a focus on enhancing the perpetual futures module (perp). Key updates include the addition of handlers for peg multiplier and swap invariant adjustments, removal of outdated querier and message types, and the introduction of new admin functions and RPC methods. The test suite has been updated accordingly, and there's a general shift towards using more descriptive and standardized naming conventions across the codebase. Changes
TipsChat with CodeRabbit Bot (
|
|
@coderabbitai pause |
![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 (2)
- x/perp/v2/types/event.pb.go
- x/perp/v2/types/tx.pb.go
Files selected for processing (42)
- CHANGELOG.md (2 hunks)
- app/genesis.go (1 hunks)
- app/ibc_test.go (1 hunks)
- app/keepers.go (8 hunks)
- proto/nibiru/perp/v2/event.proto (1 hunks)
- proto/nibiru/perp/v2/tx.proto (2 hunks)
- wasmbinding/bindings/marshalling_test.go (3 hunks)
- wasmbinding/bindings/msg.go (2 hunks)
- wasmbinding/exec_perp.go (1 hunks)
- wasmbinding/exec_perp_test.go (7 hunks)
- wasmbinding/exec_test.go (3 hunks)
- wasmbinding/message_plugin.go (1 hunks)
- wasmbinding/wasm.go (1 hunks)
- wasmbinding/wasmbin_test.go (1 hunks)
- x/common/constants.go (1 hunks)
- x/common/testutil/const.go (1 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)
- x/perp/v2/integration/action/dnr.go (4 hunks)
- x/perp/v2/integration/action/market.go (4 hunks)
- x/perp/v2/integration/action/position.go (4 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 (18 hunks)
- x/perp/v2/keeper/clearing_house_test.go (9 hunks)
- x/perp/v2/keeper/fixture_test.go (1 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)
- 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/spot/genesis_test.go (1 hunks)
- x/spot/keeper/balances_test.go (1 hunks)
- x/sudo/keeper/keeper.go (1 hunks)
Files skipped from review due to trivial changes (4)
- x/common/constants.go
- x/common/testutil/const.go
- x/perp/v2/keeper/msg_server_test.go
- x/perp/v2/types/amm_test.go
Additional comments: 79
CHANGELOG.md (2)
47-51: The changes in the State Machine Breaking section of the changelog correctly reflect the addition of the upgrade handler for v1.1.0, the deletion of CustomQuerier, and the addition of new features MsgShiftPegMultiplier and MsgShiftSwapInvariant in the perp module.
75-79: The changes in the State Machine Breaking section of the changelog correctly reflect the addition of MsgShiftPegMultiplier and MsgShiftSwapInvariant in the perp module.
app/ibc_test.go (1)
- 18-19: The addition of
testapp.EnsureNibiruPrefix()in theinitfunction could affect the initialization sequence. Verify that this does not introduce any side effects or alter the behavior of the package.app/keepers.go (2)
241-249: The use of
govModuleAddrvariable instead of string literals is consistent and improves maintainability by centralizing the module address definition.275-295: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [238-557]
The changes to the keepers' initialization using
govModuleAddrare correctly implemented. Ensure that all dependent modules that rely on theNewKeeperfunction signatures are updated accordingly to handle the new parameter.proto/nibiru/perp/v2/event.proto (1)
- 238-261: The addition of
EventShiftPegMultiplierandEventShiftSwapInvariantmessages is consistent with the summary and follows protobuf best practices. Ensure that the corresponding handling logic for these new events is implemented in the codebase.proto/nibiru/perp/v2/tx.proto (3)
29-52: The addition of the new RPC methods
ChangeCollateralDenom,ShiftPegMultiplier, andShiftSwapInvariantis correctly implemented and clearly documented as admin-only functionality. Ensure that appropriate access controls are in place to enforce these restrictions.397-410: The message
MsgShiftPegMultipliercorrectly includes the sender, pair, and new peg multiplier fields. Ensure that the new peg multiplier value is validated before use to prevent any invalid or malicious input.416-429: The message
MsgShiftSwapInvariantcorrectly includes the sender, pair, and new swap invariant fields. As withMsgShiftPegMultiplier, ensure that the new swap invariant value is validated before use to prevent any invalid or malicious input.wasmbinding/bindings/marshalling_test.go (1)
- 52-56: The removal of several strings from the
testCaseMapmay affect test coverage. Verify that the removed test cases are either no longer relevant or have been replaced by equivalent tests to maintain thorough testing.wasmbinding/bindings/msg.go (1)
- 13-21: The summary indicates that structs such as
MarketOrder,ClosePosition,AddMargin,RemoveMargin,PegShift,DepthShiftwere removed, but they are not present in the provided code. If they were indeed removed, ensure that any code that depended on these structs has been updated accordingly.wasmbinding/exec_perp.go (3)
20-21: The implementation of the
MsgServerfunction appears correct and aligns with the expected behavior of returning a newMsgServerImplinstance.23-25: The
InsuranceFundWithdrawfunction is present without any changes in this hunk. Assuming error handling is managed upstream, there are no issues to address here.23-25: The
CreateMarketfunction is present without any changes in this hunk. Assuming error handling is managed upstream, there are no issues to address here.wasmbinding/exec_perp_test.go (8)
16-23: The new imports
oracletypesandgenesisare correctly added as per the summary.42-70: The
SetExchangeRatesfunction correctly sets exchange rates and asserts no errors occur during the process.72-86: The
ExampleFieldsstruct andGetHappyFieldsfunction are correctly implemented to provide example data for tests.88-92: The
SetupPerpGenesisfunction is correctly implemented to set up the Perp Genesis state.131-139: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [134-142]
The
TestPerpExecutorHappyfunction is correctly renamed and includes assertions to validate the test outcomes.
131-132: The
ratesMapfield is correctly set using theSetExchangeRatesfunction in theOnSetupEndmethod.199-202: The
TestSadPaths_Nilfunction correctly tests the sad path for a nil argument passed toInsuranceFundWithdraw.229-236: The
TestSadPaths_InvalidPairfunction correctly tests the sad path for invalid asset pairs and includes assertions to validate the test outcomes.wasmbinding/exec_test.go (1)
- 127-132: The summary indicates that test functions related to market order execution, margin addition and removal, position closing, peg shifting, and depth shifting have been removed. However, the provided file does not show these removals. Please verify if the summary is accurate or if the file provided is incomplete.
wasmbinding/message_plugin.go (1)
- 66-67: The summary indicates that permission checks for certain operations have been removed, but the code still contains permission checks with
messenger.Sudo.CheckPermissions. Please verify if the permission checks are indeed supposed to be removed or if the summary is incorrect.wasmbinding/wasm.go (1)
- 20-26: The changes appear to be correctly implemented in the hunk. However, ensure that the removal of
wasmQueryPluginand its usage inCustomQuerierdoes not negatively impact any dependent code that relies on the custom querying functionality previously provided bywasmQueryPlugin.wasmbinding/wasmbin_test.go (2)
21-23: The addition of the
initfunction to set a specific prefix usingtestapp.EnsureNibiruPrefix()is a significant change that may impact the initialization sequence of the package and any global state it relies on. Ensure that this change does not have unintended side effects on the package's behavior.25-27: The
TestSetupContractsfunction remains unchanged as per the summary, which is consistent with the hunk provided.x/common/testutil/testapp/testapp.go (4)
28-29: The addition of
EnsureNibiruPrefix()inNewNibiruTestAppAndContextis a good practice to ensure that the Bech32 prefix is set correctly for tests.42-45: Setting defaults for modules in
NewNibiruTestAppAndContextis a good practice to ensure a consistent test environment. However, it's important to ensure that the hardcoded values likesdk.NewDec(20000)andtypes.TestingCollateralDenomNUSDare appropriate for all tests that will use this function.58-64: The addition of
DefaultSudoers()andDefaultSudoRoot()functions is a good practice to provide default state and addresses for thex/sudomodule in tests. Ensure that the default values are suitable for all tests that will use these functions.67-68: The
DefaultSudoRoot()function usessdk.MustAccAddressFromBech32(testutil.ADDR_SUDO_ROOT), which is a good practice to ensure that the address is valid. However, it's important to ensure thattestutil.ADDR_SUDO_ROOTis correctly set to a valid Bech32 address.x/devgas/v1/ante/ante_test.go (1)
- 27-30: The addition of
testapp.EnsureNibiruPrefix()inTestAnteSuiteshould be verified to ensure it does not introduce any unintended side effects or dependencies that could affect the tests. Confirm that this change is compatible with the existing test setup and does not require additional changes elsewhere.x/inflation/keeper/inflation_test.go (1)
- 72-82: The summary indicates that a conditional block checking for
tc.rootAccounthas been removed, but the hunk does not show this change. If the conditional logic was indeed removed, it could lead to incorrect behavior whentc.rootAccountis empty, as seen in the test case "pass - no root account". Please verify if the conditional logic is handled elsewhere or if its removal was intentional.x/oracle/keeper/hooks_test.go (1)
- 17-19: The addition of the
initfunction to calltestapp.EnsureNibiruPrefix()is a good practice for ensuring consistent test environment setup. However, verify that this does not interfere with the initialization sequence of other packages or global state.x/perp/v2/integration/action/dnr.go (5)
12-17: The import of "perpkeeper" from "github.com/NibiruChain/nibiru/x/perp/v2/keeper" has been added correctly.
219-220: The field "amt" has been renamed to "funds" in the struct "fundDnREpoch" as per the summary.
229-233: The call to "AllocateEpochRebates" has been correctly updated to use "perpkeeper.NewMsgServerImpl(app.PerpKeeperV2)".
269-273: The method call to "WithdrawEpochRebates" in "dnrRebateIsAction" has been updated to use "perpkeeper.NewMsgServerImpl(app.PerpKeeperV2)".
304-311: The method signature of "dnrRebateFailsAction" has been updated to match the new method signature of "WithdrawEpochRebates". Ensure that the handling of the response is also correct and consistent with the new method's expected behavior.
x/perp/v2/integration/action/market.go (4)
9-12: The addition of the
testapppackage import is consistent with the summary.138-173: The addition of
shiftPegMultiplierandshiftSwapInvarianttypes and their corresponding functions is consistent with the summary.207-210: The change to use
common.NIBIRU_TEAMas theRootin thesetCollateralstruct'sDomethod is not mentioned in the summary. This could potentially impact the behavior of theSetCollateralaction and should be verified for correctness.221-224: The
SetCollateralfunction now usescommon.NIBIRU_TEAMas theSender, which is not mentioned in the summary. This could affect the behavior of theSetCollateralaction and should be verified for correctness.x/perp/v2/integration/action/position.go (4)
13-18: The addition of the "perpkeeper" import is consistent with the changes made in the file, where the
perpkeeper.NewMsgServerImplis used.346-357: The change from a direct keeper call to using
MsgPartialClosealigns with the summary and is a good practice for encapsulating the message creation and handling logic.378-390: The update to use
MsgPartialClosein thepartialCloseFailsmethod is consistent with the changes in thepartialClosemethod and the summary.396-402: The summary mentions a correction in the spelling of the "expectedErr" parameter in the
PartialCloseFailsfunction signature, but the hunk does not show the function signature. Please verify if the signature change is reflected elsewhere in the code.x/perp/v2/keeper/admin.go (3)
174-176: The changes to the
UnsafeChangeCollateralDenomfunction are correct and follow the intended use case for genesis or controlled scenarios.178-221: The
ShiftPegMultiplierfunction correctly checks permissions, validates the new price multiplier, calculates the cost of re-pegging, handles the market update cost, updates the price multiplier, and emits an event. Ensure that theCalcRepegCostandhandleMarketUpdateCostfunctions are implemented correctly and tested.224-262: The
ShiftSwapInvariantfunction correctly checks permissions, calculates the cost of updating the swap invariant, handles the market update cost, updates the swap invariant, and emits an event. Ensure that theCalcUpdateSwapInvariantCost,handleMarketUpdateCost, andUpdateSwapInvariantfunctions are implemented correctly and tested.x/perp/v2/keeper/admin_test.go (1)
- 348-482: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-472]
The changes in the test file are consistent with the summary and the pull request description. The updated references to
perptypesare correctly used throughout the tests, and the new test suiteTestSuiteAdminis well-structured and includes comprehensive tests for admin functionality. No issues were found in the provided hunks.x/perp/v2/keeper/amm_test.go (1)
- 284-302: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [24-421]
The test cases in
TestShiftPegMultiplierandTestShiftSwapInvariantare well-structured and follow theGiven,When,Thenpattern for clarity and readability. The new test functionsTestShiftPegMultiplier_FailandTestShiftSwapInvariant_Failare correctly testing error scenarios, which is in line with the summary. The use ofrequirestatements for assertions andtestapp.DefaultSudoRoot()for admin address is appropriate for the test setup. Overall, the changes in the test file are consistent with the summary and appear to be correct.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 changes in the test cases reflect the updates mentioned in the summary, with
ShiftSwapInvariantbeing used correctly with integer arguments, and the calculation ofstartingSwapInvariantusing integers. The test cases also correctly assert the expected state of the AMM after theShiftSwapInvariantoperation.x/perp/v2/keeper/fixture_test.go (1)
- 1-1: The creation of the
keeper_testpackage is confirmed and should be noted for any dependent packages that may need to adjust their imports or usage accordingly.x/perp/v2/keeper/msg_server.go (4)
167-168: The error handling for
sdk.AccAddressFromBech32has been removed with the assumption thatmsg.ValidateBasicis always called and correctly validates the sender address. It is crucial to ensure that this is the case and that there are no scenarios where an invalid address could bypass this validation.197-206: The new function
ShiftPegMultiplierhas been added. Ensure that the implementation follows best practices and that the sender's permissions are correctly validated, as this is an admin-only function.209-218: The new function
ShiftSwapInvarianthas been added. Ensure that the implementation follows best practices and that the sender's permissions are correctly validated, as this is an admin-only function.193-219: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [157-219]
The changes in the hunks do not appear to impact exported function signatures, global data structures, or global variables, which aligns with the summary provided.
x/perp/v2/module/genesis_test.go (3)
87-87: The variable
nibiruTeamhas been correctly updated tocommon.NIBIRU_TEAMas per the summary.89-90: The usage of
nibiruTeamto set theRootfield ofsudoersis correct and should not be affected by the case change of the variable value.84-90: No further issues found in the hunk.
x/perp/v2/types/amm.go (2)
389-390: The change from
ErrNegativeSwapInvarianttoErrNonPositiveSwapInvariantis consistent with the summary and reflects a more accurate error description whennewSwapInvariantis not positive.387-393: The rest of the code in the hunk appears to be correct and consistent with the expected behavior of the
CalcUpdateSwapInvariantCostfunction.x/perp/v2/types/codec.go (2)
16-20: The registration of
MsgShiftPegMultiplier{}andMsgShiftSwapInvariant{}inRegisterLegacyAminoCodecis consistent with the existing pattern and appears to be correct.31-35: The registration of
MsgShiftPegMultiplier{}andMsgShiftSwapInvariant{}inRegisterInterfacesis consistent with the existing pattern and appears to be correct.x/perp/v2/types/codec_test.go (1)
- 25-29: The addition of
MsgShiftPegMultiplierandMsgShiftSwapInvariantto theTestCodecfunction is correctly implemented. The test loop will now include these new message types in the codec tests.x/perp/v2/types/errors.go (1)
- 38-42: The renaming of
ErrNegativeSwapInvarianttoErrNonPositiveSwapInvariantand the update of the error message forErrNilSwapInvariantare reflected correctly in the code. Ensure that all references to these errors in the codebase are updated accordingly.x/perp/v2/types/msgs.go (4)
252-255: The summary states that
MsgChangeCollateralDenomwas renamed from "partial_close_msg" to "change_collateral_denom_msg", but the code shows thatMsgPartialClosestill has the type "partial_close_msg" andMsgChangeCollateralDenomhas the type "change_collateral_denom_msg". Please clarify this discrepancy.357-409: The addition of
GetSignersandGetSignBytesmethods forMsgShiftPegMultiplierandMsgShiftSwapInvariantis consistent with the summary and follows the pattern established by other message types in the file.357-409: The implementation of
ValidateBasicforMsgShiftPegMultiplierandMsgShiftSwapInvariantis consistent with the summary and follows the established pattern of validation in the file.284-285: The update to
MsgChangeCollateralDenom'sTypemethod to return "change_collateral_denom_msg" is consistent with the summary and aligns with the naming convention of other message types.x/perp/v2/types/msgs_test.go (4)
371-415: The summary indicates that several message types related to market orders and other operations have been removed, but the test cases for these message types are still present in the file. Please verify if these message types have indeed been removed and update the test cases accordingly.
376-415: The test cases for
MsgShiftPegMultiplierandMsgShiftSwapInvariantare well-structured and correctly check for specific error conditions, such as invalid pairs and nonpositive values.441-445: The addition of
MsgShiftPegMultiplierandMsgShiftSwapInvariantto the GetSigners test cases for both valid and invalid senders is correct and aligns with the new functionality.Also applies to: 456-457
- 595-600: The addition of
MsgShiftPegMultiplierandMsgShiftSwapInvariantto the GetSignBytes test cases is correct and ensures that the signing bytes are generated as expected for these new message types.x/spot/genesis_test.go (1)
- 15-19: The addition of
testapp.EnsureNibiruPrefix()in theTestGenesisfunction should be verified to ensure it does not alter the behavior of the test in an unintended way. Confirm that this call is correctly placed before the genesis state is created and that it aligns with the intended test setup.x/spot/keeper/balances_test.go (2)
55-55: The call to
testapp.EnsureNibiruPrefix()is correctly placed before the test application and context setup, ensuring that any required prefix configuration is applied beforehand.52-58: No further issues found in the provided hunk.
x/sudo/keeper/keeper.go (1)
- 141-146: The change to use
%wto wrap the error in theCheckPermissionsfunction is correct and follows best practices for error handling in Go, allowing errors to be unwrapped and checked against specific error types. Ensure that all dependent code that handles errors returned byCheckPermissionsis updated to handle the wrapped error correctly.
app/genesis.go
Outdated
| @@ -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 made without using its return value. Please verify if this call is necessary and if the side effects are intended.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1687 +/- ##
==========================================
- Coverage 74.19% 73.95% -0.25%
==========================================
Files 193 191 -2
Lines 15444 15122 -322
==========================================
- Hits 11459 11183 -276
+ Misses 3324 3286 -38
+ Partials 661 653 -8
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- CHANGELOG.md (2 hunks)
- x/perp/v2/keeper/amm.go (1 hunks)
- x/perp/v2/types/msgs_test.go (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- x/perp/v2/types/msgs_test.go
Additional comments: 2
x/perp/v2/keeper/amm.go (2)
19-27: The function
handleMarketUpdateCosthas been added or modified to handle the cost of market updates. Ensure that this change aligns with the PR objective of removing deprecated features and introducing new functionalities. The logic for handling zero, positive, and negative cost amounts appears to be correct, but the intentional decision to ignore errors when there is bad debt (negative cost case) should be carefully reviewed to ensure it aligns with the system's design and risk management policies.19-27: The summary does not mention the
handleMarketUpdateCostfunction, which is a key part of the hunk. This could indicate an inconsistency between the summary and the hunks. Please verify if the summary needs to be updated to reflect this change.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- .github/codecov.yml (1 hunks)
Additional comments: 1
.github/codecov.yml (1)
- 17-29: The changes to the
codecov.ymlfile are consistent with the summary provided. Therangehas been updated to40..92, and new parametersstatus.target,status.threshold, andstatus.removed_code_behaviorhave been added. Ensure that these changes align with the team's expectations for coverage reporting and that they are aware of the new coverage range and status behavior.
Overview
This PR only removes code.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
ShiftPegMultiplierandShiftSwapInvariantfor market adjustments.Bug Fixes
AMMmodule.AllocateEpochRebatesandWithdrawEpochRebatesfunctions.Documentation
Refactor
Tests
Chores
Style
Revert