test(perp): tests for denoms #1679
Conversation
WalkthroughThe recent updates introduce new testing and functionality for a trading system's position settlement. Two new action functions have been added to handle the settlement of trading positions, including a check for expected failures. Additionally, new test functions ensure the robustness of the Changes
Poem
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: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- x/perp/v2/integration/action/tx.go (2 hunks)
- x/perp/v2/keeper/msg_server_test.go (2 hunks)
Additional comments: 4
x/perp/v2/integration/action/tx.go (1)
- 3-8: The import of the "fmt" package is added, but there is no usage of it in the provided code. Verify if it's used elsewhere in the file or consider removing it if it's not needed.
x/perp/v2/keeper/msg_server_test.go (3)
17-20: The import of the
sudoerTypespackage is correctly reflected in the code. This import is necessary for the new test functions that have been added to the file.344-375: The tests for
ChangeCollateralDenomcorrectly handle various error scenarios, including insufficient permissions, invalid denom, and invalid sender address. These tests ensure that the message handler behaves as expected when encountering different types of invalid input.378-410: The new test cases for
MsgServerSettlePositionandMsgServerSettlePositionShouldFailare implemented as described in the summary. They test the settlement of positions under different market conditions, ensuring that the functionality works as intended and that the system behaves correctly when attempting to settle positions in an open market.
| type msgServerSettlePosition struct { | ||
| pair asset.Pair | ||
| traderAddress sdk.AccAddress | ||
| Version uint64 | ||
| } | ||
|
|
||
| func (m msgServerSettlePosition) Do(app *app.NibiruApp, ctx sdk.Context) (sdk.Context, error, bool) { | ||
| msgServer := keeper.NewMsgServerImpl(app.PerpKeeperV2) | ||
|
|
||
| // don't need to check response because it's already checked in clearing_house tests | ||
| _, err := msgServer.SettlePosition(sdk.WrapSDKContext(ctx), &types.MsgSettlePosition{ | ||
| Pair: m.pair, | ||
| Sender: m.traderAddress.String(), | ||
| Version: m.Version, | ||
| }) | ||
|
|
||
| return ctx, err, true | ||
| } | ||
|
|
||
| func MsgServerSettlePosition( | ||
| traderAddress sdk.AccAddress, | ||
| pair asset.Pair, | ||
| version uint64, | ||
| ) action.Action { | ||
| return msgServerSettlePosition{ | ||
| pair: pair, | ||
| traderAddress: traderAddress, | ||
| Version: version, | ||
| } | ||
| } | ||
|
|
||
| type msgServerSettlePositionShouldFail struct { | ||
| pair asset.Pair | ||
| traderAddress sdk.AccAddress | ||
| Version uint64 | ||
| } | ||
|
|
||
| func (m msgServerSettlePositionShouldFail) Do(app *app.NibiruApp, ctx sdk.Context) (sdk.Context, error, bool) { | ||
| msgServer := keeper.NewMsgServerImpl(app.PerpKeeperV2) | ||
|
|
||
| // don't need to check response because it's already checked in clearing_house tests | ||
| _, err := msgServer.SettlePosition(sdk.WrapSDKContext(ctx), &types.MsgSettlePosition{ | ||
| Pair: m.pair, | ||
| Sender: m.traderAddress.String(), | ||
| Version: m.Version, | ||
| }) | ||
| if err == nil { | ||
| return ctx, fmt.Errorf("should fail but no error returned"), true | ||
| } | ||
|
|
||
| return ctx, nil, true | ||
| } | ||
|
|
||
| func MsgServerSettlePositionShouldFail( | ||
| traderAddress sdk.AccAddress, | ||
| pair asset.Pair, | ||
| version uint64, | ||
| ) action.Action { | ||
| return msgServerSettlePositionShouldFail{ | ||
| pair: pair, | ||
| traderAddress: traderAddress, | ||
| Version: version, | ||
| } |
There was a problem hiding this comment.
The new structs msgServerSettlePosition and msgServerSettlePositionShouldFail have been added, and they implement the action.Action interface. The Do methods for both structs are correctly implemented, and the error handling in msgServerSettlePositionShouldFail is appropriate for a test that expects failure. However, the field Version in both structs should follow Go naming conventions and start with a lowercase letter unless it is exported.
- Version uint64
+ version uint64Commitable 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.
| type msgServerSettlePosition struct { | |
| pair asset.Pair | |
| traderAddress sdk.AccAddress | |
| Version uint64 | |
| } | |
| func (m msgServerSettlePosition) Do(app *app.NibiruApp, ctx sdk.Context) (sdk.Context, error, bool) { | |
| msgServer := keeper.NewMsgServerImpl(app.PerpKeeperV2) | |
| // don't need to check response because it's already checked in clearing_house tests | |
| _, err := msgServer.SettlePosition(sdk.WrapSDKContext(ctx), &types.MsgSettlePosition{ | |
| Pair: m.pair, | |
| Sender: m.traderAddress.String(), | |
| Version: m.Version, | |
| }) | |
| return ctx, err, true | |
| } | |
| func MsgServerSettlePosition( | |
| traderAddress sdk.AccAddress, | |
| pair asset.Pair, | |
| version uint64, | |
| ) action.Action { | |
| return msgServerSettlePosition{ | |
| pair: pair, | |
| traderAddress: traderAddress, | |
| Version: version, | |
| } | |
| } | |
| type msgServerSettlePositionShouldFail struct { | |
| pair asset.Pair | |
| traderAddress sdk.AccAddress | |
| Version uint64 | |
| } | |
| func (m msgServerSettlePositionShouldFail) Do(app *app.NibiruApp, ctx sdk.Context) (sdk.Context, error, bool) { | |
| msgServer := keeper.NewMsgServerImpl(app.PerpKeeperV2) | |
| // don't need to check response because it's already checked in clearing_house tests | |
| _, err := msgServer.SettlePosition(sdk.WrapSDKContext(ctx), &types.MsgSettlePosition{ | |
| Pair: m.pair, | |
| Sender: m.traderAddress.String(), | |
| Version: m.Version, | |
| }) | |
| if err == nil { | |
| return ctx, fmt.Errorf("should fail but no error returned"), true | |
| } | |
| return ctx, nil, true | |
| } | |
| func MsgServerSettlePositionShouldFail( | |
| traderAddress sdk.AccAddress, | |
| pair asset.Pair, | |
| version uint64, | |
| ) action.Action { | |
| return msgServerSettlePositionShouldFail{ | |
| pair: pair, | |
| traderAddress: traderAddress, | |
| Version: version, | |
| } | |
| type msgServerSettlePosition struct { | |
| pair asset.Pair | |
| traderAddress sdk.AccAddress | |
| version uint64 | |
| } | |
| func (m msgServerSettlePosition) Do(app *app.NibiruApp, ctx sdk.Context) (sdk.Context, error, bool) { | |
| msgServer := keeper.NewMsgServerImpl(app.PerpKeeperV2) | |
| // don't need to check response because it's already checked in clearing_house tests | |
| _, err := msgServer.SettlePosition(sdk.WrapSDKContext(ctx), &types.MsgSettlePosition{ | |
| Pair: m.pair, | |
| Sender: m.traderAddress.String(), | |
| Version: m.version, | |
| }) | |
| return ctx, err, true | |
| } | |
| func MsgServerSettlePosition( | |
| traderAddress sdk.AccAddress, | |
| pair asset.Pair, | |
| version uint64, | |
| ) action.Action { | |
| return msgServerSettlePosition{ | |
| pair: pair, | |
| traderAddress: traderAddress, | |
| version: version, | |
| } | |
| } | |
| type msgServerSettlePositionShouldFail struct { | |
| pair asset.Pair | |
| traderAddress sdk.AccAddress | |
| version uint64 | |
| } | |
| func (m msgServerSettlePositionShouldFail) Do(app *app.NibiruApp, ctx sdk.Context) (sdk.Context, error, bool) { | |
| msgServer := keeper.NewMsgServerImpl(app.PerpKeeperV2) | |
| // don't need to check response because it's already checked in clearing_house tests | |
| _, err := msgServer.SettlePosition(sdk.WrapSDKContext(ctx), &types.MsgSettlePosition{ | |
| Pair: m.pair, | |
| Sender: m.traderAddress.String(), | |
| Version: m.version, | |
| }) | |
| if err == nil { | |
| return ctx, fmt.Errorf("should fail but no error returned"), true | |
| } | |
| return ctx, nil, true | |
| } | |
| func MsgServerSettlePositionShouldFail( | |
| traderAddress sdk.AccAddress, | |
| pair asset.Pair, | |
| version uint64, | |
| ) action.Action { | |
| return msgServerSettlePositionShouldFail{ | |
| pair: pair, | |
| traderAddress: traderAddress, | |
| version: version, | |
| } |
x/perp/v2/keeper/msg_server_test.go
Outdated
| }) | ||
| require.ErrorContains(t, err, "spendable balance is smaller than 1luna") |
There was a problem hiding this comment.
The error message in the test case for DonateToEcosystemFund seems to be missing the actual balance in the error string. It would be more informative if the error message included the available balance.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1679 +/- ##
==========================================
+ Coverage 73.75% 73.90% +0.14%
==========================================
Files 192 192
Lines 15388 15388
==========================================
+ Hits 11350 11373 +23
+ Misses 3379 3355 -24
- Partials 659 660 +1 |
| * [#1639](https://github.com/NibiruChain/nibiru/pull/1639) - fix(perp): by default, disable new markets until they are toggled on. | ||
| * [#1652](https://github.com/NibiruChain/nibiru/pull/1652) - test: refactors cli.network suites with 'Integration' to use common function | ||
| * [#1659](https://github.com/NibiruChain/nibiru/pull/1659) - refactor(oracle): curate oracle default whitelist | ||
| * [#1679](https://github.com/NibiruChain/nibiru/pull/1679) - test(perp): add more tests for perp module msg server |
There was a problem hiding this comment.
The changelog entry for pull request #1679 is duplicated. It should only appear once to maintain clarity and avoid confusion.
Description
Purpose
Why is this PR important?
Summary by CodeRabbit
New Features
Tests
Documentation