chore: add gRPC validation for 29-fee #6069
Conversation
WalkthroughThese changes enhance the fee and channel management functionalities in the IBC protocol's fee module. They introduce setups and validations for fee-enabled transfers and queries, add a method to check channel existence, and improve test coverage for various scenarios including error handling. Additionally, fee configuration is integrated into channel setup utilities for easier fee enablement. Changes
Possibly related issues
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? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
bznein
requested review from
AdityaSripal,
charleenfei,
colin-axner,
damiannolan and
chatton
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (4)
- modules/apps/29-fee/keeper/grpc_query.go (3 hunks)
- modules/apps/29-fee/keeper/grpc_query_test.go (8 hunks)
- modules/apps/29-fee/keeper/keeper.go (1 hunks)
- modules/apps/29-fee/types/expected_keepers.go (1 hunks)
Path instructions used (4)
modules/apps/29-fee/types/expected_keepers.go (1)
**/*.go
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/29-fee/keeper/grpc_query.go (1)
**/*.go
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/29-fee/keeper/keeper.go (1)
**/*.go
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/29-fee/keeper/grpc_query_test.go (2)
**/*.go
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
**/*_test.go
Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request
Additional comments (13)
modules/apps/29-fee/types/expected_keepers.go (1)
- 23-23: The addition of the
HasChannelmethod to theChannelKeeperinterface is well-implemented. It enhances the module's ability to manage and validate channel-related operations effectively.modules/apps/29-fee/keeper/grpc_query.go (4)
- 16-17: The addition of imports for
channeltypesandhostis appropriate for the new validation logic introduced in this file.- 78-80: The integration of
validategRPCRequestin theIncentivizedPacketsForChannelfunction for validating port and channel IDs is correctly implemented, enhancing the robustness of gRPC requests.- 273-275: The integration of
validategRPCRequestin theFeeEnabledChannelfunction for validating port and channel IDs is correctly implemented, enhancing the robustness of gRPC requests.- 293-303: The implementation of the
validategRPCRequestfunction correctly validates port and channel IDs using thehostmodule, ensuring that gRPC requests adhere to expected standards and formats.modules/apps/29-fee/keeper/keeper.go (1)
- 81-84: The addition of the
HasChannelfunction to the Keeper is well-implemented. It effectively leverages the existingchannelKeeperto check the existence of a channel, enhancing the module's channel management capabilities.modules/apps/29-fee/keeper/grpc_query_test.go (7)
- 16-25: The introduction of the
enableFeeOnChannelutility function is a valuable addition to the testing suite, streamlining the setup process for tests involving fee-enabled channels.- 184-192: The introduction of the
setEmptyChannelutility function is a valuable addition to the testing suite, enabling the setup of test scenarios that require an empty channel.- 202-209: The addition of test cases for scenarios with empty channels enhances the comprehensiveness of the testing suite.
- 238-246: The addition of test cases for scenarios with no packets for a specified channel enhances the comprehensiveness of the testing suite.
- 253-259: The addition of test cases for scenarios where a channel is not found enhances the comprehensiveness of the testing suite.
- 263-269: The addition of test cases for scenarios with invalid IDs enhances the comprehensiveness of the testing suite.
- 296-299: The refactoring of setup in the test loop to use
ibctesting.NewPathandenableFeeOnChannelis a good practice, ensuring consistency and clarity in test setup.
There was a problem hiding this comment.
Thank you, @bznein. I left a couple of suggestions, but would be nice to wait for someone else from the team to see if they agree.
| ) | ||
|
|
||
| func enableFeeOnChannel(path *ibctesting.Path) { |
There was a problem hiding this comment.
(getting back to this, I think it might need a bit more work, as fee_test.go is a test package that can not be imported directly. It would need to go in a shared package between those. It can be made to work but I'll need to think a bit about it :)
There was a problem hiding this comment.
ah, yes, I see. Maybe we can add an internal package?
There was a problem hiding this comment.
What about adding them here?
It seems to already contain enough utility functions for testing and my understanding is that it would be fine there. I pushed this change but happy to revert and create a new package/put it somewhere else!
There was a problem hiding this comment.
the utils function suggestion makes sense to me!
There was a problem hiding this comment.
ah, ive just seen the discussion further below on where to put this funnction, strike this and lgtm!
| @@ -180,8 +199,14 @@ func (suite *KeeperTestSuite) TestQueryIncentivizedPacketsForChannel() { | |||
| { | |||
| "empty pagination", | |||
| func() { | |||
| setEmptyChannel() | |||
There was a problem hiding this comment.
You could instead create a new path with fee enable and use that channel:
| setEmptyChannel() | |
| path := ibctesting.NewTransferPath(suite.chainA, suite.chainB) | |
| enableFeeOnChannel(path) | |
| path.Setup() |
Let's see what others think.
There was a problem hiding this comment.
That seems clearer to me as well (and, just tested, it works) but I'll wait for more people to chime in!
| } | ||
|
|
||
| tc.malleate() | ||
| path.Setup() |
There was a problem hiding this comment.
Cannot you move this line to replace path.EndpointA.ChannelID = ibctesting.FirstChannelID on line 841? Then you could do the following:
{
"fee not enabled on channel",
func() {
expEnabled = false
path = ibctesting.NewPath(suite.chainA, suite.chainB)
path.Setup()
req = &types.QueryFeeEnabledChannelRequest{
PortId: path.EndpointA.ChannelConfig.PortID,
ChannelId: path.EndpointA.ChannelID,
}
},
true,
},so you don't need to make path a common variable.
Happy also to wait for other people's opinion.
There was a problem hiding this comment.
yes, that would work execpt for the test "fee not enabled on channel". In that test we disable the fee by reachin inside path and doing path.EndpointA.ChannelConfig.Version = ibcmock.Version, so if we call path.Setup() before tc.malleate() this would do nothing and the fee would stay enabled. There's probably a way to achieve both but I am not familiar enough with the code here to find it :)
There was a problem hiding this comment.
I tested locally with the changes for fee not enabled on channel and it seemed to work, because we are doing the request for a new channel that doesn't have fee enabled. Let me know if I'm missing something though!
There was a problem hiding this comment.
whoops I totally misread your code snippet and did something different! Thanks for testing!
Co-authored-by: Carlos Rodriguez <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (1)
- modules/apps/29-fee/keeper/keeper.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/apps/29-fee/keeper/keeper.go
Additional Context Used
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (1)
- modules/apps/29-fee/keeper/grpc_query_test.go (8 hunks)
Additional Context Used
Path-based Instructions (1)
modules/apps/29-fee/keeper/grpc_query_test.go (2)
**/*.go:
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
**/*_test.go:
Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request
Additional comments not posted (9)
modules/apps/29-fee/keeper/grpc_query_test.go (9)
16-16: The addition of theibcmockimport is appropriate for the context of these tests, as it allows for more realistic mock setups and interactions with the IBC modules.
19-25: TheenableFeeOnChannelfunction is a useful addition for setting up test environments with fee-enabled channels. It centralizes the configuration needed across multiple tests, promoting code reuse and maintainability.
184-192: ThesetEmptyChannelfunction is a good practice for ensuring a clean state in tests that require an empty channel setup. This function enhances test clarity and isolation.
202-209: Adding test cases with empty pagination and specific channel IDs, including the use ofsetEmptyChannel, improves the robustness of the tests by covering edge cases and ensuring the correct handling of empty or uninitialized states.
238-246: Including test cases for scenarios where no packets are available for a specified channel is important for completeness. It ensures the system behaves as expected under various operational conditions.
252-270: The addition of test cases for "channel not found" and "invalid ID" scenarios is crucial for validating error handling and input validation logic. These tests ensure that the system gracefully handles erroneous or malicious inputs.
296-299: Refactoring the setup in the test loop to useibctesting.NewPathandenableFeeOnChannelis a significant improvement. It not only makes the test setup more concise and readable but also ensures consistency in how channels are configured across tests.
805-832: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [784-829]
The modifications and additions to the
TestQueryFeeEnabledChanneltest cases, including handling scenarios like "fee not enabled on channel," "channel not found," and "invalid ID," are essential for thorough testing. These changes ensure that the system's behavior is verified across a wide range of conditions.
839-849: The use ofpath.Setup()within the test cases, especially after configuring the path withenableFeeOnChannel, is a good practice. It ensures that the test environment is correctly initialized before each test case is executed, contributing to the reliability of the test outcomes.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (1)
- modules/apps/29-fee/keeper/grpc_query_test.go (8 hunks)
Additional Context Used
Path-based Instructions (1)
modules/apps/29-fee/keeper/grpc_query_test.go (2)
**/*.go:
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
**/*_test.go:
Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request
Additional comments not posted (8)
modules/apps/29-fee/keeper/grpc_query_test.go (8)
16-16: The import ofibcmockis a new addition to support the testing of gRPC validation. This change is aligned with the PR objectives to enhance testing coverage.
19-25: TheenableFeeOnChannelfunction is a new utility function introduced to configure a mock fee version and channel configuration for testing purposes. This function improves test setup readability and reusability across different test cases.Consider adding a brief comment above the function to explain its purpose and usage within the test suite for clarity and maintainability.
184-192: ThesetEmptyChannelfunction is another utility function added to facilitate testing scenarios where an empty channel needs to be set up. This function enhances the modularity and readability of test setups involving channel configurations.Similar to the previous utility function, adding a brief comment explaining the function's purpose would enhance code readability and maintainability.
202-211: The modification to include pagination and query parameters in the test case setup forTestQueryIncentivizedPacketsForChannelis a positive change that aligns with the PR objectives to enhance gRPC validation and testing coverage. This change ensures that the pagination functionality is adequately tested.
240-248: Adding a test case for "no packets for specified channel" by utilizing thesetEmptyChannelfunction demonstrates good testing practice by covering edge cases and ensuring the robustness of the gRPC validation logic.
255-272: The addition of test cases for "channel not found" and "invalid ID" is crucial for ensuring comprehensive testing coverage. These cases validate the error handling logic for invalid input scenarios, contributing to the overall reliability of the gRPC requests within the IBC fee module.
298-301: The setup code block usingenableFeeOnChannelandpath.Setup()is repeated across multiple test cases. While this repetition is somewhat necessary for isolated test case setups, consider exploring ways to reduce boilerplate code if possible, perhaps through a shared setup function for common configurations. However, given the context of testing, this is more of a nitpick than a requirement.
807-840: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [786-837]
The test function
TestQueryFeeEnabledChannelincludes comprehensive test cases covering scenarios such as "success", "empty request", "fee not enabled on channel", "channel not found", and "invalid ID". These test cases are well-structured and contribute significantly to validating the gRPC validation logic's correctness and error handling capabilities.One minor suggestion is to ensure consistency in the use of test setup functions across all test cases to maintain readability and reduce potential confusion. For example, the use of
enableFeeOnChannelandpath.Setup()is a good practice that should be consistently applied where applicable.
There was a problem hiding this comment.
Actionable comments posted: 4
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (6)
- modules/apps/29-fee/fee_test.go (1 hunks)
- modules/apps/29-fee/ibc_middleware_test.go (1 hunks)
- modules/apps/29-fee/keeper/grpc_query_test.go (7 hunks)
- modules/apps/29-fee/keeper/keeper_test.go (2 hunks)
- modules/apps/callbacks/callbacks_test.go (2 hunks)
- testing/utils.go (2 hunks)
Additional Context Used
GitHub Check Runs (1)
lint failure (4)
testing/utils.go: [failure] 13-13:
File is notgci-ed with --skip-generated -s standard -s default -s blank -s dot -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) -s prefix(github.com/cometbft/cometbft) -s prefix(github.com/cosmos/ibc-go) --custom-order (gci)
testing/utils.go: [failure] 16-16:
File is notgci-ed with --skip-generated -s standard -s default -s blank -s dot -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) -s prefix(github.com/cometbft/cometbft) -s prefix(github.com/cosmos/ibc-go) --custom-order (gci)
Path-based Instructions (6)
modules/apps/29-fee/fee_test.go (2)
**/*.go:
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
**/*_test.go:
Assess the unit test code assessing sufficient code coverage for the changes associated in the pull requesttesting/utils.go (1)
**/*.go:
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/callbacks/callbacks_test.go (2)
**/*.go:
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
**/*_test.go:
Assess the unit test code assessing sufficient code coverage for the changes associated in the pull requestmodules/apps/29-fee/keeper/keeper_test.go (2)
**/*.go:
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
**/*_test.go:
Assess the unit test code assessing sufficient code coverage for the changes associated in the pull requestmodules/apps/29-fee/keeper/grpc_query_test.go (2)
**/*.go:
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
**/*_test.go:
Assess the unit test code assessing sufficient code coverage for the changes associated in the pull requestmodules/apps/29-fee/ibc_middleware_test.go (2)
**/*.go:
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
**/*_test.go:
Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request
Additional comments not posted (11)
modules/apps/29-fee/fee_test.go (2)
35-35: The use ofibctesting.EnableFeeOnChannelsimplifies the process of enabling fees on channels and ensures consistency across tests. This is a good practice as it reduces code duplication and potential for errors.
39-39: The application ofibctesting.EnableFeeOnChannelfor the path between chain A and chain C follows the same positive pattern as the previous comment. It's a good improvement for maintainability and readability of the test setup.testing/utils.go (1)
87-94: The addition ofEnableFeeOnChannelis a valuable utility for test setups, ensuring that fee functionality can be easily enabled on channels. This function enhances the modularity and reusability of the test code.modules/apps/callbacks/callbacks_test.go (1)
103-103: Replacing manual fee configuration withibctesting.EnableFeeOnChannelinSetupMockFeeTestis a positive change, promoting code reuse and simplifying the test setup. This aligns well with best practices for maintainable and readable test code.modules/apps/29-fee/keeper/keeper_test.go (2)
48-48: Utilizingibctesting.EnableFeeOnChannelfor enabling fee functionality on the channel in the test setup is a significant improvement. It enhances code clarity and reduces the potential for manual errors in configuration.
52-52: The application ofibctesting.EnableFeeOnChannelfor another path in the test setup is consistent with the previous positive feedback. It's a good practice that improves the maintainability of the test code.modules/apps/29-fee/keeper/grpc_query_test.go (5)
175-183: The functionsetEmptyChannelis a good addition for setting up test conditions. However, consider adding a brief comment above the function to explain its purpose and usage within the test suite. This aligns with the Go style guide's recommendation for documenting exported and unexported functions, enhancing code readability and maintainability.
193-202: The use ofibctesting.NewTransferPathandibctesting.EnableFeeOnChannelwithin the test case setup is a clean approach for configuring the test environment. This pattern ensures that the necessary channel configurations and fee settings are applied consistently across tests. Good job on maintaining readability and reusability in test setups.
231-239: In the test case "no packets for specified channel," callingsetEmptyChanneleffectively simulates a scenario where a channel exists but has no packets. This is a crucial test case for validating the behavior of the gRPC query in edge cases. It's good practice to cover such scenarios to ensure the robustness of the implementation.
246-263: The test cases for "channel not found" and "invalid ID" are essential for verifying the error handling of the gRPC queries. These tests ensure that the system behaves as expected when encountering invalid input parameters. It's important to cover these negative test cases to validate the error paths and improve the overall reliability of the system.
798-831: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [777-828]
The test function
TestQueryFeeEnabledChanneleffectively covers various scenarios, including success, empty request, fee not enabled, channel not found, and invalid ID. This comprehensive coverage is commendable as it ensures the gRPC query's behavior is thoroughly validated under different conditions. However, ensure consistency in handling the setup for each test case to maximize code reuse and maintainability.
| path.EndpointB.ChannelConfig.PortID = ibctesting.MockFeePort | ||
| path.EndpointA.ChannelConfig.Version = mockFeeVersion | ||
| path.EndpointB.ChannelConfig.Version = mockFeeVersion | ||
| ibctesting.EnableFeeOnChannel(path) |
There was a problem hiding this comment.
The function EnableFeeOnChannel is used to enable fees on the channel setup, which is a refactoring improvement. However, it's important to ensure that this function is implemented correctly and adheres to the expected behavior of enabling fees. If not already covered, adding a specific unit test for EnableFeeOnChannel would be beneficial to verify its functionality in isolation.
Would you like me to help with creating a unit test for EnableFeeOnChannel?
| path := ibctesting.NewTransferPath(suite.chainA, suite.chainB) | ||
| ibctesting.EnableFeeOnChannel(path) | ||
| path.Setup() | ||
|
|
There was a problem hiding this comment.
Repeating the setup of a new transfer path and enabling fees on the channel in multiple test cases suggests that this setup is a common requirement for these tests. Consider refactoring this setup into a helper function or leveraging the suite's SetupTest method to reduce duplication and improve test maintainability. This approach adheres to the DRY (Don't Repeat Yourself) principle, making the tests easier to read and maintain.
testing/utils.go
Outdated
| @@ -10,9 +10,11 @@ | |||
| "github.com/cosmos/cosmos-sdk/codec" | |||
| sdk "github.com/cosmos/cosmos-sdk/types" | |||
| govtypesv1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1" | |||
| "github.com/cosmos/ibc-go/v8/modules/apps/29-fee/types" | |||
There was a problem hiding this comment.
The import of the types package from ibc-go should follow the import grouping conventions as per the Uber Golang style guide. Group it with other github.com/cosmos/ibc-go imports for better readability.
- "github.com/cosmos/cosmos-sdk/codec"
+
"github.com/cosmos/ibc-go/v8/modules/apps/29-fee/types"
+ "github.com/cosmos/cosmos-sdk/codec"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.
| "github.com/cosmos/ibc-go/v8/modules/apps/29-fee/types" | |
| "github.com/cosmos/ibc-go/v8/modules/apps/29-fee/types" | |
| "github.com/cosmos/cosmos-sdk/codec" |
There was a problem hiding this comment.
make lint-fix should address all these for ya
There was a problem hiding this comment.
@DimitrisJim, thank you for pointing out the use of make lint-fix for addressing linting issues, including import grouping conventions. It's great to see projects with tailored linting setups that streamline code style adherence. I'll keep this in mind for future reviews within this project.
New Learnings Added
User: DimitrisJim
PR: cosmos/ibc-go#6069
File: testing/utils.go:13-13
Timestamp: 2024-04-01T12:04:14.151Z
Learning: The `ibc-go` project prefers using `make lint-fix` to address linting issues, including import grouping conventions.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure path-based instructions.
testing/utils.go
Outdated
|
|
||
| abci "github.com/cometbft/cometbft/abci/types" | ||
| cmttypes "github.com/cometbft/cometbft/types" | ||
| ibcmock "github.com/cosmos/ibc-go/v8/testing/mock" |
There was a problem hiding this comment.
The import of the ibcmock package should be grouped with other github.com/cosmos/ibc-go imports to adhere to the Uber Golang style guide.
- "github.com/cosmos/cosmos-sdk/x/gov/types/v1"
+
ibcmock "github.com/cosmos/ibc-go/v8/testing/mock"
+ "github.com/cosmos/cosmos-sdk/x/gov/types/v1"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.
| ibcmock "github.com/cosmos/ibc-go/v8/testing/mock" | |
| ibcmock "github.com/cosmos/ibc-go/v8/testing/mock" | |
| "github.com/cosmos/cosmos-sdk/x/gov/types/v1" |
testing/utils.go
Outdated
| @@ -81,3 +83,12 @@ func UnmarshalMsgResponses(cdc codec.Codec, data []byte, msgs ...codec.ProtoMars | |||
|
|
|||
| return nil | |||
| } | |||
|
|
|||
| // EnableFeeOnChannel enables fee on a channel given a path. | |||
| func EnableFeeOnChannel(path *Path) { | |||
There was a problem hiding this comment.
I'd put this into path.go of testing/, seems like that is a better home for it. Should probably be made a method on Path but lets leave that for another time (theres some refactoring going on with the testing/ pkg and best to just open an issue to track it).
testing/utils.go
Outdated
| @@ -10,9 +10,11 @@ | |||
| "github.com/cosmos/cosmos-sdk/codec" | |||
| sdk "github.com/cosmos/cosmos-sdk/types" | |||
| govtypesv1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1" | |||
| "github.com/cosmos/ibc-go/v8/modules/apps/29-fee/types" | |||
There was a problem hiding this comment.
make lint-fix should address all these for ya
testing/utils.go
Outdated
| @@ -81,3 +83,12 @@ func UnmarshalMsgResponses(cdc codec.Codec, data []byte, msgs ...codec.ProtoMars | |||
|
|
|||
| return nil | |||
| } | |||
|
|
|||
| // EnableFeeOnChannel enables fee on a channel given a path. | |||
There was a problem hiding this comment.
I'd also probably name it EnableFeeOnPath?
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (5)
- modules/apps/29-fee/fee_test.go (1 hunks)
- modules/apps/29-fee/ibc_middleware_test.go (1 hunks)
- modules/apps/29-fee/keeper/grpc_query_test.go (7 hunks)
- modules/apps/29-fee/keeper/keeper_test.go (2 hunks)
- testing/path.go (4 hunks)
Files skipped from review as they are similar to previous changes (4)
- modules/apps/29-fee/fee_test.go
- modules/apps/29-fee/ibc_middleware_test.go
- modules/apps/29-fee/keeper/keeper_test.go
- testing/path.go
Additional Context Used
Path-based Instructions (1)
modules/apps/29-fee/keeper/grpc_query_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (4)
modules/apps/29-fee/keeper/grpc_query_test.go (4)
193-201: The setup usingibctesting.NewTransferPathWithFeeEnabledandpath.Setup()is clean and ensures that the test environment is correctly initialized. This approach enhances the readability and maintainability of the test cases.
230-238: Adding a test case for "no packets for specified channel" using thesetEmptyChannelfunction is a good practice. It ensures that the module behaves correctly in scenarios where a channel exists but has no packets associated with it.
244-263: The test cases for "channel not found" and "invalid ID" are important for ensuring robust error handling in the module. These cases validate that the module responds correctly to invalid input, which is crucial for maintaining the integrity of the system.
836-837: The use ofibctesting.NewPathWithFeeEnabledfor setting up a test path with fee enabled is a clean and efficient way to prepare the test environment. This approach reduces boilerplate code and enhances the clarity of the test setup.
| const emptyChannelID = "channel-10" | ||
| setEmptyChannel := func() { | ||
| suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetChannel( | ||
| suite.chainA.GetContext(), | ||
| ibctesting.MockFeePort, | ||
| emptyChannelID, | ||
| channeltypes.Channel{}, | ||
| ) | ||
| } |
There was a problem hiding this comment.
The setEmptyChannel function is a useful addition for setting up an empty channel in tests. It correctly uses the ChannelKeeper.SetChannel method to achieve this. However, consider the minor naming convention adjustment for consistency with Go's naming conventions:
- const emptyChannelId = "channel-10"
+ const emptyChannelID = "channel-10"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.
| const emptyChannelID = "channel-10" | |
| setEmptyChannel := func() { | |
| suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetChannel( | |
| suite.chainA.GetContext(), | |
| ibctesting.MockFeePort, | |
| emptyChannelID, | |
| channeltypes.Channel{}, | |
| ) | |
| } | |
| const emptyChannelID = "channel-10" | |
| setEmptyChannel := func() { | |
| suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetChannel( | |
| suite.chainA.GetContext(), | |
| ibctesting.MockFeePort, | |
| emptyChannelID, | |
| channeltypes.Channel{}, | |
| ) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (2)
- testing/mock/mock.go (2 hunks)
- testing/path.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- testing/path.go
Additional Context Used
GitHub Check Runs (1)
lint failure (5)
testing/mock/mock.go: [failure] 20-20:
File is notgci-ed with --skip-generated -s standard -s default -s blank -s dot -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) -s prefix(github.com/cometbft/cometbft) -s prefix(github.com/cosmos/ibc-go) --custom-order (gci)
testing/mock/mock.go: [failure] 25-25:
File is notgci-ed with --skip-generated -s standard -s default -s blank -s dot -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) -s prefix(github.com/cometbft/cometbft) -s prefix(github.com/cosmos/ibc-go) --custom-order (gci)
testing/mock/mock.go: [failure] 51-51:
File is notgci-ed with --skip-generated -s standard -s default -s blank -s dot -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) -s prefix(github.com/cometbft/cometbft) -s prefix(github.com/cosmos/ibc-go) --custom-order (gci)
Path-based Instructions (1)
testing/mock/mock.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (2)
testing/mock/mock.go (2)
25-25: The import offeetypesis necessary for the new feature. However, the file's imports should be organized according to the Golang style guide. Consider using a tool likegcito automatically format the imports.
51-51: The initialization ofMockFeeVersionis appropriate for setting up mock data. Ensure that the use ofMustMarshalJSONaligns with your error handling strategy, as it will panic on error. This is typically acceptable in testing or mock setups.
There was a problem hiding this comment.
one more small nit + the linter complains, but overall looks great to me, thank you @bznein!
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (2)
- modules/apps/29-fee/keeper/grpc_query_test.go (6 hunks)
- testing/mock/mock.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- modules/apps/29-fee/keeper/grpc_query_test.go
- testing/mock/mock.go
Co-authored-by: Carlos Rodriguez <[email protected]>
Co-authored-by: Carlos Rodriguez <[email protected]>
There was a problem hiding this comment.
LGTM! I think these E2Es may be failing for an unrelated reason, we can hold off on merging until they've been re-run with a full pass.
This PR ended up being trickier that it originally seemed (needing to dive into the fee wiring for the test setup)
Nice job! 🚀
| @@ -247,6 +276,9 @@ func (suite *KeeperTestSuite) TestQueryIncentivizedPacketsForChannel() { | |||
| suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), identifiedPacketFees.PacketId, types.NewPacketFees(identifiedPacketFees.PacketFees)) | |||
| } | |||
|
|
|||
| path := ibctesting.NewTransferPathWithFeeEnabled(suite.chainA, suite.chainB) | |||
| path.Setup() | |||
There was a problem hiding this comment.
I think if you move this call to path.Setup after the tc.malleate() we won't need to call path.Setup() in each of the malleate fns when we are changing the path.
There was a problem hiding this comment.
I tried but it is not working on those tests, since the req that we build uses values (channelID,portID) that, based on my understanding, are set when calling setup()). So we need them to be set before (or simultaneously) we call the tc.malleate() function
There was a problem hiding this comment.
Just for completeness, this is how it fails:
--- FAIL: TestKeeperTestSuite (3.14s)
--- FAIL: TestKeeperTestSuite/TestQueryIncentivizedPacketsForChannel (3.13s)
--- FAIL: TestKeeperTestSuite/TestQueryIncentivizedPacketsForChannel/Case_empty_pagination (0.47s)
/workspaces/ibc-go/modules/apps/29-fee/keeper/grpc_query_test.go:286:
Error Trace: /workspaces/ibc-go/modules/apps/29-fee/keeper/grpc_query_test.go:286
/go/pkg/mod/github.com/stretchr/[email protected]/suite/suite.go:115
Error: Received unexpected error:
rpc error: code = InvalidArgument desc = identifier cannot be blank: invalid identifier
Test: TestKeeperTestSuite/TestQueryIncentivizedPacketsForChannel/Case_empty_pagination
--- FAIL: TestKeeperTestSuite/TestQueryIncentivizedPacketsForChannel/Case_no_packets_for_specified_channel (0.47s)
/workspaces/ibc-go/modules/apps/29-fee/keeper/grpc_query_test.go:286:
Error Trace: /workspaces/ibc-go/modules/apps/29-fee/keeper/grpc_query_test.go:286
/go/pkg/mod/github.com/stretchr/[email protected]/suite/suite.go:115
Error: Received unexpected error:
rpc error: code = InvalidArgument desc = identifier cannot be blank: invalid identifier
Test: TestKeeperTestSuite/TestQueryIncentivizedPacketsForChannel/Case_no_packets_for_specified_channel
There was a problem hiding this comment.
ah I see, in that case I think it's fine to leave as is 👍
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (1)
- modules/apps/29-fee/keeper/grpc_query_test.go (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/apps/29-fee/keeper/grpc_query_test.go
There was a problem hiding this comment.
dropping my approval here, great job @bznein, definitely a bigger change than expected!
Description
Added missing gRPC validation in apps/29-fee
closes: #4140
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.Summary by CodeRabbit
New Features
Refactor
Tests