feat(perp): move close market from Wasm Binding to MsgCloseMarket as part of #1642 #1749
Conversation
WalkthroughThe recent update introduces a new gRPC transaction message 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 (
|
Unique-Divine
changed the title
jgimeno
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1749 +/- ##
==========================================
- Coverage 73.89% 73.32% -0.57%
==========================================
Files 192 194 +2
Lines 15142 15436 +294
==========================================
+ Hits 11189 11319 +130
- Misses 3297 3457 +160
- Partials 656 660 +4
|
….com:NibiruChain/nibiru into feat/migrate-set-market-enabled-to-msdserver
![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 ignored due to filter (1)
- x/perp/v2/types/tx.pb.go
Files selected for processing (16)
- 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 (2 hunks)
- wasmbinding/exec_test.go (2 hunks)
- wasmbinding/message_plugin.go (1 hunks)
- x/perp/v2/integration/action/settlement.go (1 hunks)
- x/perp/v2/keeper/admin.go (5 hunks)
- x/perp/v2/keeper/admin_test.go (7 hunks)
- x/perp/v2/keeper/clearing_house_test.go (2 hunks)
- x/perp/v2/keeper/hooks_test.go (4 hunks)
- x/perp/v2/keeper/msg_server.go (1 hunks)
- x/perp/v2/keeper/msg_server_test.go (1 hunks)
- x/perp/v2/keeper/settlement_test.go (6 hunks)
- x/perp/v2/types/msgs.go (1 hunks)
Additional comments: 33
wasmbinding/exec_perp.go (1)
- 39-44: The changes within the
InsuranceFundWithdrawmethod and the visible part of theCreateMarketmethod appear to be correct and show no issues.x/perp/v2/integration/action/settlement.go (1)
- 16-49: The addition of the
senderfield to thecloseMarketandcloseMarketShouldFailstructs and the corresponding updates to theDomethods and functions are correctly implemented.wasmbinding/message_plugin.go (1)
- 78-83: The removal of the code block related to setting market enablement in the
DispatchMsgfunction aligns with the PR's objective to refactor the functionality intoMsgCloseMarket.wasmbinding/bindings/msg.go (1)
- 16-21: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [16-52]
The removal of the
SetMarketEnabledtype, the addition of theDonateToInsuranceFundtype, and the update to theCreateMarkettype with the newPegMultfield are correctly implemented.x/perp/v2/keeper/hooks_test.go (3)
7-10: The addition of imports for
github.com/stretchr/testify/requireandgithub.com/NibiruChain/nibiru/x/common/testutilis appropriate for the test setup.28-30: The initialization of
adminUserusingsdk.AccAddressFromBech32(testutil.ADDR_SUDO_ROOT)is correctly implemented for use in test cases.132-132: The modification of the
CloseMarketfunction call to include theadminUserparameter aligns with the changes in the main codebase.wasmbinding/exec_perp_test.go (2)
202-207: The removal of the
DoSetMarketEnabledTestfunction is consistent with the changes in the main codebase where theSetMarketEnabledfunctionality has been deprecated.214-219: The update to the
TestSadPaths_InvalidPairtest, removing calls to the now non-existentDoSetMarketEnabledTest, is correctly implemented.x/perp/v2/keeper/admin.go (4)
156-157: The comment for the
ChangeCollateralDenommethod has been updated to remove the colon. This is a minor documentation change and does not affect the functionality.169-170: The comment for the
UnsafeChangeCollateralDenommethod has been updated to remove the colon. This is a minor documentation change and does not affect the functionality.182-183: The comment for the
ShiftPegMultipliermethod has been updated to remove the colon. This is a minor documentation change and does not affect the functionality.228-229: The comment for the
ShiftSwapInvariantmethod has been updated to remove the colon. This is a minor documentation change and does not affect the functionality.x/perp/v2/keeper/settlement_test.go (3)
7-7: The import statement for the
requirepackage fromgithub.com/stretchr/testifyhas been added. This is typically used for assertions in tests, which is a standard practice.28-30: The
adminUservariable has been introduced and is being used as a parameter in theCloseMarketfunction calls. Ensure that theadminUseris properly initialized and used in all relevant test cases.Verification successful
The
adminUservariable is properly initialized and used in all relevant test cases across the five identified test files. The initialization pattern is consistent with the expectations set in the review comment.
* 51-51: The `CloseMarket` function call in the test case has been updated to include the `adminUser` parameter. Verify that the `adminUser` is being passed correctly and that the test case logic reflects the new permission checks.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search to find test files that should use adminUser. Expecting list of test files. rg --type go 'adminUser' --files-with-matches # Search within test files to ensure adminUser is initialized correctly. Expecting list of initializations. ast-grep --lang go --pattern $'adminUser, err := sdk.AccAddressFromBech32($_)'Length of output: 845
Verification successful
The shell script output confirms that the
CloseMarketfunction is being called withadminUseras a parameter in various test files, including thesettlement_test.gofile mentioned in the review comment. This aligns with the changes described in the AI-generated summary and the review comment.Given this information, the
adminUserparameter is being passed correctly to theCloseMarketfunction in the test cases, as per the review comment's request for verification.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search within test files to ensure CloseMarket is called with adminUser. Expecting list of calls. ast-grep --lang go --pattern $'CloseMarket($_, adminUser)'Length of output: 704
x/perp/v2/keeper/msg_server.go (1)
- 222-227: A new
CloseMarketfunction has been added to themsgServertype. This function is responsible for closing a market and includes permission checks. Ensure that the function is correctly integrated into the message server and that the permission checks are consistent with the rest of the system.x/perp/v2/types/msgs.go (1)
- 411-431: The implementation of
MsgCloseMarketwith its methodsValidateBasicandGetSignersseems to follow the established patterns of the other message types in the file. TheValidateBasicmethod correctly checks for a valid sender address and validates thePairobject. TheGetSignersmethod retrieves the sender's address and panics if there's an error, which is consistent with the other message types. However, it's important to ensure that the panic is an acceptable behavior in this context, as it will cause the application to crash if the sender's address is invalid at runtime.wasmbinding/exec_test.go (1)
- 16-21: The removal of the
TestSetMarketEnabledfunction is consistent with the PR's objective of moving the close market functionality from Wasm binding to a message-based system. This change likely means that the test is no longer relevant. However, ensure that equivalent tests are implemented for the newMsgCloseMarketfunctionality to maintain coverage.proto/nibiru/perp/v2/tx.proto (2)
53-55: The addition of the
CloseMarketRPC method is consistent with the PR's objective to move the close market functionality to a message-based system. Ensure that the corresponding backend implementation enforces the admin-only access as indicated by the comment.442-448: The
MsgCloseMarketmessage structure is well-defined with appropriate fields and gogoproto annotations. The use of custom types and non-nullable annotations for thepairfield is consistent with other messages in the file.x/perp/v2/keeper/admin_test.go (5)
104-105: The
adminUseris correctly derived from a predefined constant address and the error is properly checked. This is a good practice for setting up test cases with known accounts.166-167: The use of
adminUserin theCloseMarketcall is appropriate and aligns with the new functionality being tested. It verifies that the market can be closed by an admin user, which is a key part of the new feature.195-196: The
adminAccountis correctly derived from a predefined constant address and the error is properly checked. This ensures that the tests are using the correct admin account for operations that require admin privileges.209-209: The
CloseMarketfunction is correctly called with theadminAccountparameter in the test case, ensuring that the market closure functionality is tested with the appropriate admin credentials.293-306: The addition of a test case to verify that non-admin users cannot close the market is a good practice. It ensures that the admin-only restriction is enforced and functioning as expected.
x/perp/v2/keeper/msg_server_test.go (2)
- 411-412: The introduction of
adminUserand the associated error handling is a good practice to ensure that the user has the necessary permissions to perform the action. However, it's important to verify that theADDR_SUDO_ROOTis correctly set up in the test environment and that the error is handled appropriately in all test cases.Verification successful
The
ADDR_SUDO_ROOTconstant is defined inx/common/testutil/const.goand is used across various test files, includingmsg_server_test.gowhere theadminUseris being set. This confirms that theADDR_SUDO_ROOTis indeed set up in the test environment and is being used consistently.
- The
ADDR_SUDO_ROOTis properly defined in the test utilities.- The
adminUseris derived fromADDR_SUDO_ROOTin multiple test files, indicating a pattern of use that is likely intentional and correct.* 421-421: The modification to the `CloseMarket` function to include `adminUser` as a parameter aligns with the PR's objective to move close market functionality to a message-based system. Ensure that the `CloseMarket` function's implementation is updated accordingly and that all tests reflect this change.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that ADDR_SUDO_ROOT is set in the test environment grep -R "ADDR_SUDO_ROOT" .Length of output: 856
Verification successful
The output from the
rgcommand provides us with several references toCloseMarketwithin Go files. Notably, we can see thatCloseMarketis indeed a method with the following signature:func (k admin) CloseMarket(ctx sdk.Context, pair asset.Pair, sender sdk.AccAddress) (err error) {This is found in the
admin.gofile. It confirms that theCloseMarketmethod has been updated to include anadminUserparameter, which is referred to assenderin the context of the method signature.Additionally, there are multiple test files, such as
settlement_test.go,msg_server_test.go,hooks_test.go,clearing_house_test.go, andadmin_test.go, where theCloseMarketmethod is called with theadminUserparameter, indicating that the tests have been updated to reflect the change in the method signature.Based on the provided output, we can conclude:
- The
CloseMarketmethod has been updated to accept anadminUserparameter.- The tests have been updated to pass the
adminUserparameter to theCloseMarketmethod.Therefore, the review comment is consistent with the changes made in the codebase.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the CloseMarket function is updated to accept the adminUser parameter ast-grep --lang go --pattern $'CloseMarket($_, $_, $_)'Length of output: 56
Script:
#!/bin/bash # Search for the CloseMarket function definition to confirm its existence and parameter list ast-grep --lang go --pattern $'func ($_ CloseMarket)($$$)'Length of output: 59
Script:
#!/bin/bash # Search for any occurrence of "CloseMarket" within Go files to locate its usage rg 'CloseMarket' --type goLength of output: 8331
CHANGELOG.md (2)
70-70: Ensure the changelog entry for PR feat(perp): move close market from Wasm Binding to MsgCloseMarket as part of #1642 #1749 accurately describes the change and is clear to the readers.
70-70: The format and placement of the new changelog entry for PR feat(perp): move close market from Wasm Binding to MsgCloseMarket as part of #1642 #1749 are correct and follow the project's changelog conventions.
x/perp/v2/keeper/clearing_house_test.go (3)
32-33: The addition of
adminUserwith error handling is correctly implemented.891-891: The
CloseMarketfunction is now being called withadminUseras an argument, which aligns with the PR's objective to move close market functionality to a message-based system.888-894: The test case for closing the market after it has been disabled is correctly implemented, ensuring that no new positions can be opened after the market is closed.
x/perp/v2/keeper/admin.go
Outdated
| func (k admin) CloseMarket(ctx sdk.Context, pair asset.Pair, sender sdk.AccAddress) (err error) { | ||
| if err := k.SudoKeeper.CheckPermissions(sender, ctx); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
The verification process has identified the following issues:
- Calls to the old
CloseMarketmethod signature with two parameters are still present in the codebase. - There are no detected calls to the new
CloseMarketmethod signature with three parameters, which includes thesender.
These findings suggest that the calls to CloseMarket have not been updated to match the new method signature. This needs to be addressed to ensure consistency and correct functionality after the changes made in the PR.
Analysis chain
The CloseMarket method now includes a sender parameter and checks permissions using SudoKeeper. Ensure that all calls to this method have been updated to include the new sender parameter.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search to find files that call CloseMarket. Expecting list of dependent files.
rg --type go 'CloseMarket\(' --files-with-matches
# Search old signature to find omissions. Expecting no results.
ast-grep --lang go --pattern $'CloseMarket($_, $_)'
# Search new signature to validate usage. Expecting list of locations where the new signature is used.
ast-grep --lang go --pattern $'CloseMarket($_, $_, $_)'
Length of output: 1427
There was a problem hiding this comment.
I could not find any case that has two methods. I think you are confused with the action.
There was a problem hiding this comment.
@jgimeno, the search results confirm that there are indeed calls to both the old and new CloseMarket method signatures within the codebase. The old signature is used in several files, including tests and integration actions, while the new signature is also present in a few locations.
Here's a summary of the findings:
-
The old
CloseMarketmethod signature with two parameters is found in:x/perp/v2/types/tx.pb.gox/perp/v2/keeper/msg_server.gox/perp/v2/keeper/msg_server_test.gox/perp/v2/keeper/settlement_test.gox/perp/v2/keeper/clearing_house_test.gox/perp/v2/keeper/admin_test.gox/perp/v2/keeper/hooks_test.gox/perp/v2/integration/action/settlement.go
-
The new
CloseMarketmethod signature with three parameters is used in:x/perp/v2/types/tx.pb.gox/perp/v2/keeper/admin.gox/perp/v2/keeper/admin_test.gox/perp/v2/keeper/msg_server.gox/perp/v2/integration/action/settlement.go
It appears that the transition to the new method signature is partially complete. Would you like to address these inconsistencies in this PR, or would you prefer to handle them in a subsequent one? If the latter, I can open a new GitHub issue to track this task.
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to the CloseMarket method with the old signature (two parameters).
rg --type go 'CloseMarket\([^,]+,[^,]+\)' --files-with-matches
# Search for calls to the CloseMarket method with the new signature (three parameters).
rg --type go 'CloseMarket\([^,]+,[^,]+,[^,]+\)' --files-with-matches
Length of output: 564
…et-enabled-to-msdserver
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- x/perp/v2/types/tx.pb.go
Files selected for processing (14)
- CHANGELOG.md (1 hunks)
- proto/nibiru/perp/v2/tx.proto (2 hunks)
- wasmbinding/bindings/msg.go (2 hunks)
- wasmbinding/exec_perp.go (2 hunks)
- wasmbinding/exec_perp_test.go (2 hunks)
- wasmbinding/exec_test.go (2 hunks)
- wasmbinding/message_plugin.go (1 hunks)
- x/perp/v2/integration/action/settlement.go (1 hunks)
- x/perp/v2/keeper/clearing_house_test.go (2 hunks)
- x/perp/v2/keeper/msg_server.go (1 hunks)
- x/perp/v2/keeper/msg_server_test.go (1 hunks)
- x/perp/v2/keeper/sudo.go (5 hunks)
- x/perp/v2/keeper/sudo_test.go (7 hunks)
- x/perp/v2/types/msgs.go (1 hunks)
Files skipped from review due to trivial changes (3)
- wasmbinding/exec_perp.go
- wasmbinding/exec_perp_test.go
- wasmbinding/exec_test.go
Additional comments: 25
wasmbinding/message_plugin.go (1)
- 64-67: The removal of the case handling the Perp module's controller aligns with the PR's objective to refactor the close market functionality.
wasmbinding/bindings/msg.go (2)
14-19: The removal of the
SetMarketEnabledtype and its associated fields from theNibiruMsgstruct is consistent with the PR's objective to refactor the close market functionality.36-38: The addition of the
enabledfield with anomitemptytag to theMarketParamstype is consistent with the PR's objective to refactor the close market functionality.x/perp/v2/integration/action/settlement.go (2)
14-30: The addition of the
sender sdk.AccAddressparameter to thecloseMarketstruct and its usage in theCloseMarketfunction align with the PR's objective to include the sender's account address in the market closure process.33-49: The addition of the
sender sdk.AccAddressparameter to thecloseMarketShouldFailstruct and its usage in theCloseMarketShouldFailfunction align with the PR's objective to include the sender's account address in the market closure process.x/perp/v2/keeper/sudo.go (2)
129-138: The addition of the
sender sdk.AccAddressparameter to theCloseMarketfunction and the check for permissions usingSudoKeeper.CheckPermissionsalign with the PR's objective to include the sender's account address in the market closure process.163-164: The comment updates for functions like
ChangeCollateralDenomreflect the removal of the[SUDO]tag and the updated function names, which is a cleanup activity likely for clarity and consistency.x/perp/v2/keeper/msg_server.go (1)
- 236-241: The addition of the
CloseMarketmethod to themsgServertype, which closes a market by calling theCloseMarketmethod of thek.Sudo()object, introduces new functionality related to market closure consistent with the PR's objectives.x/perp/v2/types/msgs.go (1)
- 412-432: The implementation of
MsgCloseMarketwith itsValidateBasicandGetSignersmethods follows the established pattern of other message types in the file. TheValidateBasicmethod correctly checks for a valid sender address and validates thePairobject. TheGetSignersmethod retrieves the sender's address and panics if there's an error, which is consistent with the other message types.However, ensure that the panic in
GetSignersis the desired behavior, as it will cause the application to crash if an invalid address is encountered. This could be a security risk if not handled properly upstream.Verification successful
The grep output shows that the use of
panicis quite common throughout the codebase, including in the same file (msgs.go) where theGetSignersmethod is defined. This indicates that the use ofpanicinGetSignersis consistent with the application's error handling strategy. Additionally, the changelog mentions efforts to remove unnecessary panics, which implies that the remaining uses ofpanicare considered necessary by the developers.Given this context, the use of
panicin theGetSignersmethod seems to be a deliberate choice that aligns with the broader error handling approach of the application.
- The use of
panicinGetSignersis consistent with the application's error handling strategy.- The changelog indicates an effort to remove unnecessary panics, suggesting that the remaining panics are intentional.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the panic behavior in GetSigners is consistent with the application's error handling strategy. grep -r "panic" .Length of output: 10779
proto/nibiru/perp/v2/tx.proto (2)
- 58-60: The addition of the
CloseMarketRPC method in the protobuf definition is consistent with the description provided in the PR objectives. It is marked as admin-only, which suggests that it should be restricted to certain privileged users. Ensure that the corresponding access control checks are implemented in the application logic to enforce this restriction.Verification successful
The review of the
CloseMarketfunction in bothmsg_server.goandsudo.gofiles confirms that access control is enforced through theCheckPermissionsmethod, which checks the permissions of the sender before allowing the market to be closed. This aligns with the PR objectives and the review comment.* 462-473: The protobuf message `MsgCloseMarket` is well-defined with a `sender` and `pair` field, following the established pattern for transaction messages. The `pair` field uses a custom type and is marked as non-nullable, which is appropriate for a required field.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that access control for CloseMarket is properly implemented. grep -r "CloseMarket" .Length of output: 9146
Script:
#!/bin/bash # Examine the CloseMarket function in msg_server.go for access control logic. cat ./x/perp/v2/keeper/msg_server.go # Examine the CloseMarket function in sudo.go for access control logic. cat ./x/perp/v2/keeper/sudo.goLength of output: 16201
Ensure that the
pairfield is validated on the application level to prevent any invalid or malicious input from being processed.Verification successful
To verify that the
pairfield inMsgCloseMarketis validated in the application logic, we should look for theValidateBasicmethod of theMsgCloseMarketmessage or any other validation logic that might be associated with it. Let's search for theValidateBasicmethod within theMsgCloseMarkettype definition.
To find the validation logic for the
MsgCloseMarketmessage, we will usegrepto search for references toMsgCloseMarketand any associatedValidateBasicmethod or similar validation functions in the codebase.
The search results indicate that there is a
ValidateBasicmethod forMsgCloseMarketdefined inx/perp/v2/types/msgs.go. To verify the validation logic for thepairfield, we should examine the implementation of this method.
The
ValidateBasicmethod forMsgCloseMarketincludes a check for thesenderfield to ensure it is a valid address and also callsm.Pair.Validate(). This indicates that there is indeed validation logic for thepairfield as required.
- The
senderfield is checked to be a valid address.- The
pairfield is validated by calling itsValidatemethod.Based on the provided script output, the review comment is consistent with the codebase, as the
pairfield is validated on the application level.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the pair field in MsgCloseMarket is validated in the application logic. grep -r "Validate" .Length of output: 32204
Script:
#!/bin/bash # Search for the ValidateBasic method within the MsgCloseMarket type definition ast-grep --lang proto --pattern $'message MsgCloseMarket { $$$ ValidateBasic($_) { $$$ } $$$ }'Length of output: 221
Script:
#!/bin/bash # Search for references to MsgCloseMarket and any associated validation logic grep -r "MsgCloseMarket" .Length of output: 5757
Script:
#!/bin/bash # Examine the implementation of the ValidateBasic method for MsgCloseMarket cat ./x/perp/v2/types/msgs.go | grep -A 10 "func (m MsgCloseMarket) ValidateBasic()"Length of output: 362
x/perp/v2/keeper/sudo_test.go (8)
96-101: The introduction of
adminUserand its use inadmin.CreateMarketappears correct. Ensure thatadminUseris consistently used wherever an admin address is required in the test cases.158-158: The use of
adminUserin theadmin.CloseMarketcall is consistent with the changes made in the previous hunk. This is a good use of the newly introduced variable.187-191: The introduction of
adminAccountand its use in theCloseMarketfunction call is correct. Ensure thatadminAccountis consistently used wherever an admin address is required in the test cases.201-201: The use of
adminAccountin theCloseMarketcall is consistent with the changes made in the previous hunk. This is a good use of the newly introduced variable.223-223: The use of
adminAccountin theCloseMarketcall is consistent with the changes made in the previous hunks. This is a good use of the newly introduced variable.255-257: The use of
adminAccountin theCloseMarketcall and the subsequent failure checks is consistent with the changes made in the previous hunks. This is a good use of the newly introduced variable.280-280: The use of
adminAccountin theCloseMarketcall is consistent with the changes made in the previous hunks. This is a good use of the newly introduced variable.285-298: The addition of a test case to verify that a non-admin cannot close a market is a good practice to ensure proper access control. Ensure that the test case correctly simulates the non-admin scenario and that the expected failure occurs.
x/perp/v2/keeper/msg_server_test.go (1)
- 421-421: The addition of
adminUseras a parameter to theCloseMarketfunction call aligns with the PR's objective to refactor the close market functionality. Ensure that theCloseMarketfunction is updated accordingly to handle this new parameter.CHANGELOG.md (3)
68-72: The entries added to the "State Machine Breaking" section correctly reference pull requests with a brief description of the changes. Ensure that these changes are reflected in the codebase and that any necessary migrations or updates are planned for the next mainnet version.
74-74: The "Non-breaking/Compatible Improvements" section does not have any new entries in this hunk. This is consistent with the summary provided.
71-71: The addition of the entry for pull request feat(perp): move close market from Wasm Binding to MsgCloseMarket as part of #1642 #1749 under "Dapp modules: perp, spot, etc" is consistent with the PR objectives and AI-generated overview. It documents the transition of the close market functionality from Wasm Binding to
MsgCloseMarket.x/perp/v2/keeper/clearing_house_test.go (2)
32-34: The introduction of
adminUservariable and its usage in theCloseMarketfunction call is a significant change. Ensure that theadminUserhas the appropriate permissions to close the market and that this change is reflected in all relevant test cases.891-891: The call to
CloseMarketwithin the test case is a key change. Verify that theCloseMarketfunction behaves as expected when called with the newadminUserparameter.Verification successful
The search results confirm that the
CloseMarketfunction is being called with theadminUserparameter in various test files within thex/perp/v2/keeper/directory. This indicates that theadminUserparameter is indeed being used in tests for theCloseMarketfunction, which aligns with the change mentioned in the review comment.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that CloseMarket is properly tested with the new adminUser parameter. rg --type go "CloseMarket\(.*adminUser.*\)" x/perp/v2/keeper/Length of output: 739
| adminUser, err := sdk.AccAddressFromBech32(testutil.ADDR_SUDO_ROOT) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
It's good practice to handle potential errors when initializing adminUser with sdk.AccAddressFromBech32. Ensure that the error is checked and handled appropriately to avoid panics during test execution.
+ require.NoError(t, err)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.
| adminUser, err := sdk.AccAddressFromBech32(testutil.ADDR_SUDO_ROOT) | |
| require.NoError(t, err) | |
| adminUser, err := sdk.AccAddressFromBech32(testutil.ADDR_SUDO_ROOT) | |
| require.NoError(t, err) |
There was a problem hiding this comment.
Hey coderabbit, we already have that error check
| adminUser, err := sdk.AccAddressFromBech32(testutil.ADDR_SUDO_ROOT) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Hey coderabbit, we already have that error check
Purpose / Abstract
Summary by CodeRabbit