imp(e2e): added generic grpc querier #5979
Conversation
srdtrk
requested review from
colin-axner,
AdityaSripal,
crodriguezvega,
charleenfei,
damiannolan,
chatton and
DimitrisJim
as code owners
WalkthroughThe changes consolidate the handling of gRPC queries across the end-to-end test suite into a unified approach, utilizing a generic Changes
Assessment against linked 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 (
|
![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: 9
Configuration used: .coderabbit.yml
Files selected for processing (22)
- e2e/tests/core/02-client/client_test.go (14 hunks)
- e2e/tests/core/03-connection/connection_test.go (4 hunks)
- e2e/tests/interchain_accounts/base_test.go (11 hunks)
- e2e/tests/interchain_accounts/gov_test.go (4 hunks)
- e2e/tests/interchain_accounts/groups_test.go (4 hunks)
- e2e/tests/interchain_accounts/incentivized_test.go (9 hunks)
- e2e/tests/interchain_accounts/localhost_test.go (9 hunks)
- e2e/tests/interchain_accounts/params_test.go (4 hunks)
- e2e/tests/interchain_accounts/upgrades_test.go (11 hunks)
- e2e/tests/transfer/authz_test.go (5 hunks)
- e2e/tests/transfer/base_test.go (11 hunks)
- e2e/tests/transfer/incentivized_test.go (23 hunks)
- e2e/tests/transfer/localhost_test.go (4 hunks)
- e2e/tests/transfer/upgrades_test.go (15 hunks)
- e2e/tests/upgrades/genesis_test.go (4 hunks)
- e2e/tests/upgrades/upgrade_test.go (31 hunks)
- e2e/tests/wasm/grandpa_test.go (10 hunks)
- e2e/tests/wasm/upgrade_test.go (4 hunks)
- e2e/testsuite/query/grpc_query.go (1 hunks)
- e2e/testsuite/query/queries.go (1 hunks)
- e2e/testsuite/testsuite.go (7 hunks)
- e2e/testsuite/tx.go (4 hunks)
Check Runs (6)
e2e / e2e-tests (TestMsgTransfer_WithMemo, TestTransferTestSuite) completed (1)
e2e / e2e-tests (TestMsgTransfer_Succeeds_Nonincentivized, TestTransferTestSuite) completed (1)
e2e / e2e-tests (TestMsgTransfer_Localhost, TestTransferLocalhostTestSuite) completed (1)
e2e / e2e-tests (TestMsgTransfer_Fails_InvalidAddress, TestTransferTestSuite) completed (1)
e2e / e2e-tests (TestMsgSendTx_SuccessfulTransfer_AfterUpgradingOrdertoUnordered, TestInterchainAccount... completed (1)
e2e / e2e-tests (TestMaxExpectedTimePerBlockParam, TestConnectionTestSuite) completed (1)
Additional comments: 152
e2e/testsuite/query/grpc_query.go (2)
- 14-39: Creating a new gRPC connection for each query might lead to performance issues. Consider reusing connections or implementing a connection pool for efficiency.
- 41-59: The
getProtoPathfunction relies on specific naming conventions in type URLs, which could lead to maintenance challenges. Consider a more robust method for deriving gRPC paths.e2e/testsuite/query/queries.go (9)
- 72-81: The
ClientStatusfunction is concise and correctly queries the client status. No issues found.- 84-98: The
GetValidatorSetByHeightfunction correctly queries and sorts the validator set by address. This is a good use of thesort.SliceStablefunction for deterministic ordering.- 114-124: The
Channelfunction implementation is straightforward and correctly queries channel information. No issues found.- 127-137: The
CounterPartyPayeefunction is implemented correctly. It's a good practice to ensure error messages are informative for easier debugging.- 140-154: The
IncentivizedPacketsForChannelfunction is correctly implemented to query incentivized packets for a channel. Good use of the genericGRPCQueryfunction.- 157-166: The
FeeEnabledChannelfunction correctly queries the fee-enabled status of a channel. This is a straightforward and clean implementation.- 169-178: In the
TotalEscrowForDenomfunction, the implementation is correct. It's important to ensure that error handling is consistent across all query functions.- 181-192: The
PacketAcknowledgementsfunction is implemented correctly. It queries packet acknowledgements efficiently using the genericGRPCQueryfunction.- 195-204: The
UpgradeErrorfunction correctly queries for upgrade errors. This function is a good example of how to leverage the genericGRPCQueryfor specific query types.e2e/tests/interchain_accounts/groups_test.go (4)
- 23-23: The import of the
querypackage is correctly added to utilize the new generic gRPC querier functionality. This change aligns with the PR's objective to streamline querying processes.- 71-71: The change from
queryClient.GroupPoliciesByGrouptoquery.GRPCQuery[grouptypes.QueryGroupPoliciesByGroupResponse]is a good example of utilizing the new generic gRPC querier. This simplifies the querying process and reduces boilerplate code.- 144-144: The method call change from
s.QueryInterchainAccounttoQueryInterchainAccountseems to be missing context. Ensure thatQueryInterchainAccountis correctly defined and accessible within this context.- 202-202: The changes from
s.QueryBalancetoquery.Balancecorrectly utilize the newquerypackage for balance queries. This is in line with the PR's goal to centralize and simplify querying processes.Also applies to: 209-209
e2e/tests/upgrades/genesis_test.go (3)
- 24-24: The addition of the
querypackage import aligns with the PR's objective to centralize and simplify the querying process in the IBC e2e testing suite.- 104-110: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [107-113]
Replacing
s.QueryBalancewithquery.Balanceis a good practice as it utilizes the newly introduced generic gRPC querier, reducing boilerplate code and enhancing maintainability.
- 116-123: Utilizing
query.GRPCQueryfor querying interchain account information is a significant improvement. It simplifies the querying process by using a single function capable of handling any query request.e2e/tests/transfer/authz_test.go (4)
- 9-9: Adding the import for
github.com/strangelove-ventures/interchaintest/v8/ibcis necessary for the new functionality introduced in this file.- 21-21: The addition of the
querypackage import is crucial for utilizing the new generic gRPC querier functionality within the e2e testing suite.- 35-45: Introducing the
QueryGranterGrantsfunction to retrieveGrantAuthorizationsusing thequery.GRPCQuerymethod is a significant enhancement. It simplifies the process of querying grant authorizations by leveraging the generic gRPC querier.- 154-157: Replacing
suite.QueryBalancecalls withquery.Balancefor balance verification is a good practice. It leverages the newly introduced generic gRPC querier, enhancing code maintainability and reducing complexity.e2e/testsuite/tx.go (4)
- 26-26: The addition of the
querypackage import supports the use of the new generic gRPC querier functionality within the e2e testing suite, aligning with the PR's objectives.- 183-194: Changing the type of
govProposalto a pointer and updating the query call to usequery.GRPCQuerywith thegovtypesv1.QueryProposalResponsetype is a significant improvement. It simplifies the querying process for governance proposals by leveraging the generic gRPC querier.- 224-241: Adjusting the assignment of
proposalRespandproposalvariables in theExecuteAndPassGovV1Beta1Proposalfunction to usequery.GRPCQuerywith thegovtypesv1beta1.QueryProposalResponsetype is a good practice. It enhances the querying process for governance proposals by utilizing the generic gRPC querier.- 250-260: Updating the
waitForGovV1Beta1ProposalToPassfunction to usequery.GRPCQuerywith thegovtypesv1beta1.QueryProposalResponsetype for querying governance proposals is a significant improvement. It simplifies and standardizes the querying process across different governance proposal types.e2e/tests/interchain_accounts/upgrades_test.go (17)
- 23-23: The introduction of the
querypackage is a significant improvement in terms of code maintainability and extensibility. By centralizing query logic, it simplifies the process of querying blockchain states across different modules.- 82-82: The replacement of direct query calls with
query.GRPCQueryor specific query functions from thequerypackage is a notable change. However, it's important to ensure that theQueryInterchainAccountfunction correctly handles errors and edge cases, as it directly impacts the reliability of the test suite.- 86-86: Utilizing the
query.Channelfunction here streamlines the process of fetching channel information. It's crucial to verify that the function is used consistently across all test cases to maintain code uniformity.- 133-133: The use of
query.Balanceto verify token transfers is a good practice. It abstracts away the complexity of querying balances, making the test code cleaner and more readable.- 140-140: The call to
query.Channelfor channel information before upgrading the channel showcases the utility of thequerypackage in simplifying test setup procedures. Ensure that the channel information is accurately retrieved and used in the context of the test.- 147-147: Querying the module account address using
query.ModuleAccountAddressis a clean way to fetch necessary data for governance proposals. This approach enhances the readability and maintainability of the test code.- 158-158: Re-querying the channel after an upgrade operation using
query.Channelis essential for verifying the upgrade's success. It's important to ensure that the query accurately reflects the channel's new state.- 170-170: Similar to the previous comment, verifying the channel upgrade on the counterparty chain is crucial. The use of
query.Channelhere should be carefully checked to ensure it accurately captures the channel's state post-upgrade.- 212-212: Re-using
query.Balanceto verify the token transfer after the channel upgrade is a good practice. It demonstrates the utility of thequerypackage in reducing code duplication and simplifying test assertions.- 268-268: The use of a direct query call to fetch the interchain account before the channel upgrade test is a critical step. It's important to ensure that this query accurately retrieves the interchain account information.
- 272-272: Utilizing
query.Channelto fetch channel information before initiating a channel upgrade is a streamlined approach. Consistency in using thequerypackage across different test scenarios is key to maintaining code quality.- 283-283: Querying the channel post-upgrade to verify the inclusion of the fee version is an important validation step. The use of
query.Channelhere should be carefully reviewed to ensure it accurately reflects the channel's upgraded state.- 292-292: The call to
query.FeeEnabledChannelto verify if a channel is fee-enabled post-upgrade is a clear example of thequerypackage's utility in simplifying test assertions. Ensure that this function accurately determines the fee status of the channel.- 298-298: Repeating the verification of the fee-enabled status on the counterparty chain is crucial for ensuring the upgrade's consistency across both chains. The accuracy of the
query.FeeEnabledChannelfunction in this context should be verified.- 307-307: Similar to the previous comment, verifying the fee-enabled status on the counterparty chain is essential. The use of
query.FeeEnabledChannelhere should be carefully reviewed to ensure it accurately reflects the channel's state.- 369-369: Querying the channel state post-timeout using
query.Channelis a critical step in verifying the channel closure. It's important to ensure that this query accurately captures the channel's closed state.- 376-376: Similar to the previous comment, verifying the channel closure on the counterparty chain is crucial. The accuracy of the
query.Channelfunction in this context should be verified to ensure it accurately reflects the channel's state.e2e/tests/interchain_accounts/incentivized_test.go (13)
- 22-22: The introduction of the
querypackage is a significant improvement in terms of code maintainability and extensibility. By centralizing query logic, it simplifies the process of querying blockchain states across different modules.- 89-89: The replacement of direct query calls with
query.GRPCQueryor specific query functions from thequerypackage is a notable change. However, it's important to ensure that theQueryInterchainAccountfunction correctly handles errors and edge cases, as it directly impacts the reliability of the test suite.- 120-120: Utilizing the
query.CounterPartyPayeefunction here streamlines the process of fetching counterparty payee information. It's crucial to verify that the function is used consistently across all test cases to maintain code uniformity.- 126-126: The use of
query.IncentivizedPacketsForChannelto verify the absence of incentivized packets before a transaction is a good practice. It abstracts away the complexity of querying packet information, making the test code cleaner and more readable.- 168-168: Re-using
query.IncentivizedPacketsForChannelto verify the presence of incentivized packets after a transaction showcases the utility of thequerypackage in reducing code duplication and simplifying test assertions.- 183-183: The call to
query.IncentivizedPacketsForChannelfor verifying that packets have been relayed is essential for ensuring the success of incentivized transactions. It's important to ensure that the query accurately reflects the absence of pending packets.- 189-189: Querying the balance of the recipient account using
query.Balanceto verify token transfers is a good practice. It simplifies the validation process by abstracting away direct query calls.- 267-267: The use of a direct query call to fetch the interchain account before the incentivized transaction test is a critical step. It's important to ensure that this query accurately retrieves the interchain account information.
- 288-288: Utilizing
query.CounterPartyPayeeto fetch counterparty payee information before executing an incentivized transaction is a streamlined approach. Consistency in using thequerypackage across different test scenarios is key to maintaining code quality.- 294-294: The call to
query.IncentivizedPacketsForChannelto verify the absence of incentivized packets before a failed transaction is a clear example of thequerypackage's utility in simplifying test assertions. Ensure that this function accurately determines the packet status.- 337-337: Repeating the verification of incentivized packets after a failed transaction is crucial for ensuring the test's integrity. The accuracy of the
query.IncentivizedPacketsForChannelfunction in this context should be verified.- 352-352: Similar to the previous comment, verifying that packets have been relayed after a failed transaction is essential. The use of
query.IncentivizedPacketsForChannelhere should be carefully reviewed to ensure it accurately reflects the packet status.- 358-358: Querying the balance of the recipient account using
query.Balanceto verify the absence of token transfers in a failed transaction is a good practice. It demonstrates the utility of thequerypackage in reducing code duplication and simplifying test assertions.e2e/tests/transfer/upgrades_test.go (14)
- 17-17: The import of the
querypackage aligns with the PR's objective to centralize and simplify querying processes across the e2e testing suite. This change is consistent with the described enhancements.- 84-84: Utilizing the
query.Channelfunction from thequerypackage for channel information retrieval is a significant improvement in terms of code maintainability and simplicity. This change effectively replaces direct gRPC client calls with a more abstracted and reusable approach.- 99-99: The replacement of direct balance queries with the
query.Balancefunction from thequerypackage is a clear example of the PR's objective to streamline querying processes. This enhances code readability and maintainability.- 106-106: Repeating the use of
query.Balancefor balance queries further demonstrates the PR's commitment to reducing redundancy and simplifying the querying logic across different blockchain modules.- 113-113: The use of
query.Channelfor retrieving channel information post-upgrade is consistent and ensures that the querying process remains streamlined and maintainable. This is a good practice in keeping the codebase clean and efficient.- 122-122: Querying for fee-enabled channels using the
query.FeeEnabledChannelfunction is a direct application of the newly introduced querying mechanisms. This specific query function adds to the extensibility of the testing suite by allowing easy addition of similar queries in the future.- 128-128: Repeating the pattern of using
querypackage functions for channel information retrieval on the counterparty chain demonstrates a consistent approach to simplifying the querying process across the board. This consistency is key to maintainable and readable code.- 137-137: The application of
query.FeeEnabledChannelfor the counterparty chain further emphasizes the PR's objective to centralize and simplify querying processes. This consistent use of thequerypackage across different test scenarios is commendable.- 144-144: Utilizing
query.PacketAcknowledgementsfor retrieving packet acknowledgements is a good example of how the new querying mechanism can be extended to various types of queries, enhancing the test suite's flexibility and maintainability.- 153-153: The repeated use of
query.PacketAcknowledgementsto verify the state post-pruning demonstrates the utility of thequerypackage in making the code more concise and focused on the testing logic rather than the specifics of query execution.- 181-181: Replacing direct calls with
query.CounterPartyPayeefor querying counterparty payee information aligns with the PR's goal of simplifying and centralizing the querying process. This change contributes to a more maintainable and scalable codebase.- 192-192: The use of
query.IncentivizedPacketsForChannelbefore sending incentivized transfer packets is a strategic application of the new querying mechanism, showcasing its versatility and the ease with which it can be integrated into existing tests.- 205-205: Verifying the absence of incentivized packets after relaying using
query.IncentivizedPacketsForChannelis a clear demonstration of the benefits brought by the centralized querying mechanism, particularly in terms of code readability and maintainability.- 211-211: The consistent use of
query.Balancefor balance queries across different test scenarios, as seen here for verifying token receipt by walletB, underscores the PR's objective to streamline and centralize the querying process.e2e/tests/interchain_accounts/base_test.go (15)
- 22-22: The import of the
querypackage is in line with the PR's objective to centralize and simplify the querying process within the e2e testing suite. This change is consistent with the described enhancements and improves code maintainability.- 46-56: The addition of the
QueryInterchainAccountfunction is a significant enhancement, providing a centralized and simplified way to query interchain account information. This function encapsulates the querying logic, making the tests more readable and maintainable.- 103-103: Replacing direct querying calls with the
QueryInterchainAccountfunction for verifying interchain accounts is a clear demonstration of the PR's objective to streamline the querying process. This enhances code readability and maintainability.- 159-159: The use of
query.Balancefor balance queries is consistent with the PR's goal of simplifying and centralizing the querying process. This change contributes to a more maintainable and scalable codebase.- 162-162: Repeating the use of
query.Balancefor balance queries further demonstrates the PR's commitment to reducing redundancy and simplifying the querying logic across different blockchain modules.- 202-202: Utilizing the
QueryInterchainAccountfunction for interchain account verification in a different test scenario shows the function's versatility and the ease with which it can be integrated into existing tests.- 213-213: The use of
query.Balanceto verify the host wallet's balance before attempting a transfer is a good practice, ensuring that the test's preconditions are met. This is another example of the centralized querying mechanism's utility.- 252-252: Repeating the pattern of using
query.Balancefor balance verification after a failed transfer attempt is consistent with the PR's objective to centralize and simplify querying processes. This consistency is key to maintainable and readable code.- 300-300: The repeated use of
QueryInterchainAccountfor interchain account verification in yet another test scenario underscores the utility of the newly introduced querying mechanism in making the code more concise and focused on the testing logic.- 304-304: Utilizing
query.Channelto verify channel information post-interchain account registration is consistent with the PR's goal of simplifying and centralizing the querying process. This enhances code readability and maintainability.- 364-364: The use of
query.Channelto verify the channel's state after a timeout event is a strategic application of the new querying mechanism, showcasing its versatility and the ease with which it can be integrated into existing tests.- 371-371: The consistent use of
query.Balancefor balance queries across different test scenarios, as seen here for verifying token transfer failure, underscores the PR's objective to streamline and centralize the querying process.- 374-374: Repeating the use of
query.Balanceto verify the host account's balance after a failed transfer attempt further demonstrates the PR's commitment to reducing redundancy and simplifying the querying logic across different blockchain modules.- 393-393: Utilizing
query.Channelto verify the state of a newly opened channel post-interchain account re-registration is consistent with the PR's goal of simplifying and centralizing the querying process. This enhances code readability and maintainability.- 434-434: The use of
query.Balancefor balance queries in the context of verifying token transfers after reopening an interchain account is a clear demonstration of the benefits brought by the centralized querying mechanism, particularly in terms of code readability and maintainability.e2e/tests/interchain_accounts/localhost_test.go (6)
- 22-22: The import of the
querypackage aligns with the PR objectives to centralize and simplify querying operations across the test suite. This change should enhance maintainability and reduce boilerplate code.- 112-116: The replacement of direct channel query calls with
query.Channelfrom thequerypackage is a significant improvement. It centralizes the querying logic, which is in line with the PR's objectives. However, ensure that thequery.Channelfunction is thoroughly tested to handle various query scenarios and error conditions.- 187-187: The use of
query.Balancefor querying balances demonstrates the PR's objective of simplifying and centralizing query operations. This should make the test suite more maintainable and reduce redundancy. Ensure thatquery.Balanceis well-tested, especially for edge cases and error handling.- 267-271: Reusing the
query.Channelfunction in another test case further demonstrates the utility of thequerypackage in simplifying query operations across different test scenarios. This is a positive change towards achieving the PR's goal of centralizing query logic.- 339-344: The checks for channel states being
CLOSEDusing thequery.Channelfunction in the test case for verifying channel closure is a good use of the centralized querying logic provided by thequerypackage. This showcases the package's utility in handling various query needs within the test suite.- 472-472: The usage of
query.Balancein this test case further demonstrates the utility of thequerypackage in centralizing and simplifying query operations across the test suite. This aligns with the PR's objectives and should be beneficial for maintainability.e2e/tests/transfer/base_test.go (10)
- 22-22: The addition of the
querypackage import is appropriate and necessary for the changes made in this file. This aligns with the PR's objective to utilize the new generic gRPC querier for simplifying the querying process.- 38-38: The refactoring of
QueryTransferParamsto usequery.GRPCQueryis a significant improvement. It simplifies the querying process by eliminating the need for direct gRPC client calls and specific query functions. This change enhances code maintainability and readability.- 85-85: The use of
query.TotalEscrowForDenomfor querying the total escrow amount is a clear example of how the newquerypackage can simplify and centralize querying logic. This eliminates redundant code and contributes to the overall goal of improving code maintainability.- 102-102: Replacing direct balance queries with
query.Balanceis another step towards centralizing and simplifying the querying process. This change is consistent with the PR's objectives and improves the code's extensibility for future enhancements.- 121-121: Continuing the pattern, the use of
query.Balancefor querying balances in different scenarios demonstrates the flexibility and utility of the newquerypackage. This approach reduces complexity and enhances the code's readability.- 127-127: The conditional use of
query.TotalEscrowForDenombased on feature support is a good practice. It ensures that the code remains compatible with different versions of the underlying systems, thereby enhancing the code's robustness.- 147-147: The use of
query.TotalEscrowForDenomfor querying un-escrowed tokens aligns with the PR's goal of simplifying the querying process. This change makes the code more maintainable and easier to understand.- 266-266: Querying the module account address using
query.ModuleAccountAddressis a clear demonstration of the benefits of the newquerypackage. It centralizes and simplifies the process of obtaining module account addresses, which is crucial for governance-related operations.- 329-329: Similarly, the addition of
query.ModuleAccountAddressin theTestReceiveEnabledParamfunction showcases the utility of thequerypackage in streamlining the querying process across different test scenarios.- 465-465: The consistent use of
query.ModuleAccountAddressfor querying module account addresses in various test functions further emphasizes the advantages of the newquerypackage. This approach enhances code maintainability and simplifies the querying process.e2e/testsuite/testsuite.go (7)
- 20-25: The addition of the
querypackage import is necessary for the changes made in this file, enabling the use of the new generic gRPC querier. This change is consistent with the PR's objectives and enhances the code's maintainability.- 313-321: Refactoring the
GetChainANativeBalancefunction to usequery.GRPCQueryfor balance queries is a significant improvement. It centralizes the querying logic, making the code more maintainable and readable. This change aligns with the PR's goal of simplifying the querying process.- 327-336: Similarly, the
GetChainBNativeBalancefunction's refactoring to usequery.GRPCQuerydemonstrates the utility of the newquerypackage in centralizing balance queries. This approach reduces complexity and enhances code readability.- 342-349: The use of
query.GRPCQueryin theAssertPacketRelayedfunction for querying packet commitments is another example of how the newquerypackage simplifies the querying process. This change contributes to the code's maintainability and aligns with the PR's objectives.- 357-363: Utilizing
query.GRPCQueryfor querying denom metadata in theAssertHumanReadableDenomfunction showcases the benefits of centralizing querying logic. This modification simplifies the code and aligns with the PR's goal of enhancing maintainability.- 454-454: Querying the module account address using
query.ModuleAccountAddressin theSetUpgradeTimeoutParamfunction is a clear demonstration of the advantages of the newquerypackage. It centralizes and simplifies the querying process, enhancing code maintainability.- 465-465: The consistent use of
query.ModuleAccountAddressfor querying module account addresses in theInitiateChannelUpgradefunction further emphasizes the utility of thequerypackage. This approach streamlines the querying process and improves code maintainability.e2e/tests/core/02-client/client_test.go (5)
- 34-34: The import of the
querypackage aligns with the PR's objective to centralize and simplify the querying process in the e2e testing suite. This change is expected to enhance maintainability and reduce boilerplate code.- 57-57: Replacing direct
queryClient.ClientParamscalls withquery.GRPCQuery[clienttypes.QueryClientParamsResponse]is a significant improvement. It abstracts the underlying gRPC call mechanism, making the test code cleaner and more maintainable.- 77-77: The consistent replacement of various specific query and status check calls with their counterparts in the
querypackage across the test suite is a commendable effort towards achieving the PR's objectives. This change not only reduces the complexity and amount of code but also improves the maintainability and scalability of the test suite.Also applies to: 81-81, 108-108, 110-110, 120-120, 130-130, 134-134, 208-208, 214-214, 227-227, 233-233, 291-291, 297-297, 304-304, 313-313, 319-319, 350-350, 366-366, 382-382, 391-391, 426-426, 461-461, 485-485
- 57-57: The use of generics with
query.GRPCQuery[clienttypes.QueryClientParamsResponse]demonstrates a modern approach to handling gRPC queries, leveraging Go's type parameters feature. This approach enhances type safety and reduces the risk of runtime errors related to type assertions.- 57-57: While the changes made are in line with the PR's objectives, it's important to verify that the new
query.GRPCQueryfunction and the specific query functions within thequerypackage have been thoroughly tested. This verification ensures that the refactoring does not introduce any regressions or unexpected behaviors in the test suite.e2e/tests/transfer/incentivized_test.go (24)
- 19-19: The import of the
querypackage aligns with the PR's objective to centralize and simplify querying processes in e2e tests. This change should enhance maintainability and reduce boilerplate code.- 76-76: Utilizing
query.CounterPartyPayeeto verify counterparty payee addresses is a good practice. It abstracts the querying logic, making the test code cleaner and more maintainable.- 103-103: Replacing direct query calls with
query.IncentivizedPacketsForChannelis a significant improvement. It encapsulates the querying logic, contributing to code modularity and readability.- 117-117: The use of
query.IncentivizedPacketsForChannelhere demonstrates the flexibility and reusability of thequerypackage for different test scenarios. This is a positive change towards reducing code duplication.- 142-142: Again, the use of
query.IncentivizedPacketsForChannelfor querying post-relay packet state showcases the utility of thequerypackage in simplifying the test setup. This consistency in querying approach is commendable.- 193-193: Reusing
query.CounterPartyPayeein different test cases without direct gRPC calls simplifies the test logic and adheres to DRY principles. This is a good practice.- 217-217: The consistent use of
query.IncentivizedPacketsForChannelacross different test scenarios enhances the test suite's maintainability and readability.- 231-231: The validation of incentivized packets through the
querypackage in this scenario further demonstrates the package's utility in simplifying and centralizing query operations.- 256-256: The application of
query.IncentivizedPacketsForChannelfor verifying the state after packet relay in this test case is consistent with the PR's objectives. This enhances the test suite's clarity and maintainability.- 314-314: The repeated use of
query.CounterPartyPayeeacross various test cases without direct gRPC calls is a testament to thequerypackage's effectiveness in centralizing query logic.- 320-320: Employing
query.IncentivizedPacketsForChannelfor initial state verification before operations is a good practice, ensuring test reliability and readability.- 331-331: The validation of incentivized packets through the
querypackage in this multi-message scenario underscores the package's versatility and utility in the test suite.- 355-355: Using
query.IncentivizedPacketsForChannelto check the state after relaying packets in a multi-message scenario is consistent with the PR's goal of simplifying querying processes. This is a positive change.- 412-412: The consistent application of
query.CounterPartyPayeeacross different test scenarios without direct gRPC calls simplifies the test logic and adheres to DRY principles.- 443-443: Utilizing
query.IncentivizedPacketsForChannelfor initial state verification in this timeout scenario demonstrates thequerypackage's utility in simplifying and centralizing query operations.- 454-454: The validation of incentivized packets through the
querypackage in the timeout scenario further demonstrates the package's utility in simplifying and centralizing query operations.- 479-479: Employing
query.IncentivizedPacketsForChannelto check the state after relaying packets in a timeout scenario aligns with the PR's objectives. This enhances the test suite's clarity and maintainability.- 538-538: The use of
query.IncentivizedPacketsForChannelfor initial state verification before operations in a scenario without a counterparty address is a good practice, ensuring test reliability and readability.- 552-552: Validating incentivized packets through the
querypackage in a scenario without a counterparty address underscores the package's versatility and utility in the test suite.- 578-578: Using
query.IncentivizedPacketsForChannelto check the state after relaying packets in a scenario without a counterparty address is consistent with the PR's goal of simplifying querying processes. This is a positive change.- 629-629: The repeated use of
query.CounterPartyPayeeacross various test cases without direct gRPC calls is a testament to thequerypackage's effectiveness in centralizing query logic.- 656-656: Employing
query.IncentivizedPacketsForChannelfor initial state verification in a multiple senders scenario is a good practice, ensuring test reliability and readability.- 675-675: The validation of incentivized packets through the
querypackage in a multiple senders scenario further demonstrates the package's utility in simplifying and centralizing query operations.- 723-723: Utilizing
query.IncentivizedPacketsForChannelto check the state after relaying packets in a multiple senders scenario aligns with the PR's objectives. This enhances the test suite's clarity and maintainability.e2e/tests/wasm/grandpa_test.go (8)
- 28-28: The introduction of the
querypackage and its usage here is a positive change, aligning with the PR's objective to streamline and simplify the querying process in the e2e testing suite. This approach enhances code maintainability by centralizing query logic.- 406-406: The use of
query.ClientStatehere correctly replaces the previous direct gRPC client call, streamlining the querying process for the client state. This change is consistent with the PR's goal of simplifying and centralizing query operations.- 577-577: The replacement of the removed
clientStatusfunction withquery.ClientStatusis a good example of how the new querying approach simplifies the code. By using a centralized query function, the test suite becomes more maintainable and easier to extend.- 582-582: Similarly, the use of
query.ClientStatusfor checking the substitute client's status demonstrates the benefits of the new querying mechanism. This change contributes to reducing boilerplate code and improving code readability.- 589-589: Utilizing
query.ModuleAccountAddressto fetch the module account address for the governance module is another example of the streamlined querying process. This centralized approach to querying enhances the test suite's consistency and maintainability.- 601-601: The repeated use of
query.ClientStatusto verify the status of the subject client after a governance proposal execution further illustrates the benefits of the centralized querying mechanism introduced in this PR.- 606-606: The use of
query.ClientStatusto check the status of both the subject and substitute clients post-governance proposal execution is consistent with the PR's objectives. This approach simplifies the querying process and enhances code maintainability.- 635-638: The implementation of
query.GRPCQueryinPushNewWasmClientProposalto fetch the code response based on the checksum is a significant improvement. It demonstrates the flexibility and utility of the new generic gRPC querier by allowing for a wide range of query types to be handled in a unified manner.e2e/tests/upgrades/upgrade_test.go (7)
- 28-28: The import of the
querypackage aligns with the PR's objective to centralize and simplify gRPC queries across the e2e testing suite. This change should enhance maintainability and reduce boilerplate code.- 166-166: Replacing direct balance queries with
query.Balancecalls is a significant improvement in terms of code maintainability and consistency. Ensure that thequery.Balancefunction is thoroughly tested to handle various edge cases and error scenarios.- 310-314: The replacement of direct client status queries with
query.ClientStatuscalls is a positive change, promoting code reuse and simplifying the test logic. However, it's crucial to verify that thequery.ClientStatusfunction accurately handles different client states and returns the expected status.- 587-596: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [581-593]
The use of
query.ModuleAccountAddressandquery.GRPCQueryfor updating and querying client parameters post-upgrade demonstrates a good application of the newquerypackage. It's important to ensure these functions are resilient to changes in the underlying modules and maintain compatibility with future IBC versions.
- 649-655: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [652-667]
The transition from direct queries to using
query.IncentivizedPacketsForChannelfor handling incentivized packets is a notable improvement. This change should make the code more readable and easier to maintain. However, ensure that thequerypackage's functions are adequately tested, especially for edge cases like empty packet lists or query failures.
- 828-840: The use of
query.GRPCQueryfor querying channel parameters post-upgrade is a good practice, ensuring consistency and reducing direct gRPC calls. It's essential to validate that the channel parameters are correctly fetched and that the upgrade timeout values meet the expected criteria.- 999-1000: The
ClientStatefunction is a utility method for querying the current client state by client ID. Ensure that error handling is robust and that the function gracefully handles scenarios where the client ID does not exist or the query fails for other reasons.
e2e/testsuite/query/queries.go
Outdated
| // Balance returns the balance of a specific denomination for a given account by address. | ||
| func Balance(ctx context.Context, chain ibc.Chain, address string, denom string) (math.Int, error) { | ||
| res, err := GRPCQuery[banktypes.QueryBalanceResponse](ctx, chain, &banktypes.QueryBalanceRequest{ | ||
| Address: address, | ||
| Denom: denom, | ||
| }) | ||
| if err != nil { | ||
| return math.Int{}, err | ||
| } | ||
|
|
||
| return res.Balance.Amount, nil |
There was a problem hiding this comment.
In the Balance function, consider handling the case where the res.Balance is nil to avoid potential nil pointer dereferences.
+ if res.Balance == nil {
+ return math.Int{}, fmt.Errorf("balance response is nil for address %s and denom %s", address, denom)
+ }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.
| // Balance returns the balance of a specific denomination for a given account by address. | |
| func Balance(ctx context.Context, chain ibc.Chain, address string, denom string) (math.Int, error) { | |
| res, err := GRPCQuery[banktypes.QueryBalanceResponse](ctx, chain, &banktypes.QueryBalanceRequest{ | |
| Address: address, | |
| Denom: denom, | |
| }) | |
| if err != nil { | |
| return math.Int{}, err | |
| } | |
| return res.Balance.Amount, nil | |
| // Balance returns the balance of a specific denomination for a given account by address. | |
| func Balance(ctx context.Context, chain ibc.Chain, address string, denom string) (math.Int, error) { | |
| res, err := GRPCQuery[banktypes.QueryBalanceResponse](ctx, chain, &banktypes.QueryBalanceRequest{ | |
| Address: address, | |
| Denom: denom, | |
| }) | |
| if err != nil { | |
| return math.Int{}, err | |
| } | |
| if res.Balance == nil { | |
| return math.Int{}, fmt.Errorf("balance response is nil for address %s and denom %s", address, denom) | |
| } | |
| return res.Balance.Amount, nil |
| s.Require().NoError(err) | ||
| s.Require().NotNil(channelEndB) | ||
|
|
||
| s.Require().Equal(channelEndA.ConnectionHops, channelEndB.ConnectionHops) | ||
| }) | ||
|
|
||
| t.Run("verify interchain account registration and deposit funds", func(t *testing.T) { | ||
| interchainAccAddress, err := s.QueryInterchainAccount(ctx, chainA, userAWallet.FormattedAddress(), exported.LocalhostConnectionID) | ||
| interchainAccAddress, err := QueryInterchainAccount(ctx, chainA, userAWallet.FormattedAddress(), exported.LocalhostConnectionID) |
There was a problem hiding this comment.
The direct call to QueryInterchainAccount here, as previously noted, indicates an inconsistency in the abstraction of query operations. Consistency in using the query package for all querying operations is essential for maintainability and aligning with the PR's objectives.
Ensure that QueryInterchainAccount is either refactored into the query package or consistently use the query package for all querying operations.
| interchainAccAddress, err := QueryInterchainAccount(ctx, chainA, userAWallet.FormattedAddress(), exported.LocalhostConnectionID) | ||
| s.Require().NoError(err) | ||
| s.Require().NotZero(len(interchainAccAddress)) | ||
|
|
||
| balance, err := s.QueryBalance(ctx, chainA, interchainAccAddress, chainADenom) | ||
| balance, err := query.Balance(ctx, chainA, interchainAccAddress, chainADenom) |
There was a problem hiding this comment.
The inconsistency in using the query package for querying interchain account addresses and balances, as seen with the direct call to QueryInterchainAccount and the use of query.Balance, needs to be addressed. Consistent use of the query package is crucial for the maintainability and clarity of the test suite.
Refactor QueryInterchainAccount into the query package or ensure consistent usage of the query package for all querying operations.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (8)
- e2e/tests/interchain_accounts/base_test.go (10 hunks)
- e2e/tests/interchain_accounts/gov_test.go (4 hunks)
- e2e/tests/interchain_accounts/groups_test.go (4 hunks)
- e2e/tests/interchain_accounts/incentivized_test.go (9 hunks)
- e2e/tests/interchain_accounts/localhost_test.go (9 hunks)
- e2e/tests/interchain_accounts/upgrades_test.go (11 hunks)
- e2e/testsuite/query/grpc_query.go (1 hunks)
- e2e/testsuite/query/queries.go (1 hunks)
Files skipped from review as they are similar to previous changes (8)
- e2e/tests/interchain_accounts/base_test.go
- e2e/tests/interchain_accounts/gov_test.go
- e2e/tests/interchain_accounts/groups_test.go
- e2e/tests/interchain_accounts/incentivized_test.go
- e2e/tests/interchain_accounts/localhost_test.go
- e2e/tests/interchain_accounts/upgrades_test.go
- e2e/testsuite/query/grpc_query.go
- e2e/testsuite/query/queries.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- e2e/testsuite/query/grpc_query.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- e2e/testsuite/query/grpc_query.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: .coderabbit.yml
Files selected for processing (1)
- e2e/testsuite/testsuite.go (7 hunks)
Additional comments: 1
e2e/testsuite/testsuite.go (1)
- 20-25: The addition of the
querypackage import and removal of direct gRPC client initialization and query calls align with the PR's objectives to centralize querying mechanisms. This change should enhance maintainability and scalability by reducing redundancy and simplifying the querying process.
e2e/testsuite/testsuite.go
Outdated
| commitmentResp, err := query.GRPCQuery[channeltypes.QueryPacketCommitmentResponse](ctx, chain, &channeltypes.QueryPacketCommitmentRequest{ | ||
| PortId: portID, | ||
| ChannelId: channelID, | ||
| Sequence: sequence, | ||
| }) | ||
| s.Require().ErrorContains(err, "packet commitment hash not found") | ||
|
|
There was a problem hiding this comment.
Using query.GRPCQuery for asserting packet relayed is a good use of the new centralized querying mechanism. It simplifies the code and makes it more consistent. However, the error message check s.Require().ErrorContains(err, "packet commitment hash not found") assumes a specific error message format.
Ensure that the error message format is standardized and documented, or consider using error types or codes for more reliable error handling.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: .coderabbit.yml
Files selected for processing (1)
- e2e/testsuite/testsuite.go (7 hunks)
Additional comments: 1
e2e/testsuite/testsuite.go (1)
- 25-25: The addition of the
querypackage import is a crucial part of this refactoring effort, enabling the use of the new genericquery.GRPCQueryfunction across the e2e testing suite. This change supports the PR's objective of streamlining gRPC queries by reducing boilerplate and improving maintainability.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- e2e/tests/core/02-client/client_test.go (14 hunks)
Files skipped from review as they are similar to previous changes (1)
- e2e/tests/core/02-client/client_test.go
srdtrk
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: .coderabbit.yml
Files selected for processing (4)
- e2e/tests/interchain_accounts/base_test.go (10 hunks)
- e2e/tests/upgrades/upgrade_test.go (31 hunks)
- e2e/tests/wasm/upgrade_test.go (4 hunks)
- e2e/testsuite/testsuite.go (7 hunks)
Files skipped from review as they are similar to previous changes (2)
- e2e/tests/interchain_accounts/base_test.go
- e2e/tests/wasm/upgrade_test.go
Additional comments: 39
e2e/testsuite/testsuite.go (1)
- 20-25: The addition of the
querypackage import and removal of thegrpcClientsmap align with the PR's objective to streamline the querying process by introducing a centralized querying mechanism. This change should enhance maintainability and reduce redundancy in the codebase.e2e/tests/upgrades/upgrade_test.go (38)
- 28-28: The import of the
querypackage aligns with the PR's objective to consolidate gRPC query clients into a single, generic function. This change should streamline the testing codebase and reduce redundancy.- 166-166: Replacing direct calls to
s.QueryBalancewithquery.Balanceis a significant improvement. It demonstrates the practical application of the new genericGRPCQueryfunction, enhancing code modularity and maintainability.- 194-194: The repeated use of
query.Balancefor querying balances post-upgrade validates the effectiveness of the new querying mechanism across different test scenarios. This consistency is crucial for maintainability.- 213-213: Utilizing
query.Balancefor balance checks in various test scenarios, including after chain upgrades and token transfers, showcases the versatility of the new generic querying function.- 244-244: The use of
query.Balanceto verify token balances before and after chain upgrades further emphasizes the utility of the new generic querying mechanism in simplifying the test suite.- 268-268: Continued use of
query.Balancefor balance verification post-upgrade demonstrates the adaptability of the new querying mechanism to different testing scenarios, enhancing code reuse and simplification.- 310-310: Replacing
s.QueryClientStatuswithquery.ClientStatusfor client status checks aligns with the PR's goal of streamlining gRPC queries. This change contributes to reducing code duplication and improving test suite maintainability.- 314-314: The consistent application of
query.ClientStatusacross different test cases for client status verification further demonstrates the effectiveness of the new generic querying mechanism in simplifying the test suite.- 349-349: Utilizing
query.ClientStatusfor checking the status of clients, including solomachine clients, showcases the versatility of the new querying mechanism in handling various query types efficiently.- 380-380: The application of
query.Balancefor balance verification in tests involving solomachine clients further illustrates the adaptability and utility of the new generic querying function across diverse testing scenarios.- 404-404: The use of
query.ClientStatusfor verifying client statuses post-upgrade demonstrates the new querying mechanism's capability to streamline and simplify the test suite, even in complex upgrade scenarios.- 408-408: Continued application of
query.ClientStatusfor client status checks post-upgrade reinforces the utility of the new generic querying mechanism in enhancing code modularity and maintainability across various test cases.- 421-421: Employing
query.Balancefor balance checks post-upgrade in tests involving solomachine clients validates the effectiveness of the new querying mechanism in simplifying balance verification across different scenarios.- 475-475: The repeated use of
query.Balancefor balance verification in tests post-upgrade highlights the new querying mechanism's role in streamlining the test suite and reducing code duplication.- 490-490: Utilizing
query.ClientStatusandquery.GRPCQueryfor querying client statuses and connection responses post-upgrade demonstrates the adaptability of the new querying mechanism to various query types, enhancing test suite maintainability.- 503-503: The application of
query.TotalEscrowForDenomfor querying total escrow amounts post-upgrade showcases the utility of the new generic querying function in handling diverse query requests efficiently.- 566-566: Continued use of
query.Balancefor balance verification in tests post-upgrade emphasizes the new querying mechanism's effectiveness in simplifying the test suite across different testing scenarios.- 581-581: Replacing
s.QueryModuleAccountAddresswithquery.ModuleAccountAddressfor querying module account addresses aligns with the PR's objective of consolidating gRPC queries, enhancing code modularity and maintainability.- 590-590: The use of
query.GRPCQueryfor querying client parameters post-upgrade demonstrates the new querying mechanism's capability to streamline and simplify the test suite, even in complex scenarios involving parameter updates.- 652-652: Replacing
s.QueryIncentivizedPacketsForChannelwithquery.IncentivizedPacketsForChannelfor querying incentivized packets aligns with the PR's goal of streamlining gRPC queries, reducing code duplication and improving test suite maintainability.- 667-667: The consistent application of
query.IncentivizedPacketsForChannelacross different test cases for querying incentivized packets further demonstrates the effectiveness of the new generic querying mechanism in simplifying the test suite.- 705-705: Utilizing
query.Balancefor querying module account balances showcases the versatility of the new querying mechanism in handling various query types efficiently, enhancing code reuse and simplification.- 721-721: The repeated use of
query.Balancefor balance verification in tests involving fee payments validates the effectiveness of the new querying mechanism in simplifying balance checks across different scenarios.- 828-828: Employing
query.GRPCQueryfor querying channel parameters post-upgrade demonstrates the new querying mechanism's capability to streamline and simplify the test suite, even in complex scenarios involving channel upgrades.- 837-837: The consistent application of
query.GRPCQueryfor querying channel parameters on both chains post-upgrade reinforces the utility of the new generic querying mechanism in enhancing code modularity and maintainability across various test cases.- 861-861: The use of
query.Balancefor balance verification in tests post-channel upgrade highlights the new querying mechanism's role in streamlining the test suite and reducing code duplication, even in complex scenarios involving packet relays.- 867-867: Continued application of
query.Balancefor balance checks post-channel upgrade in tests involving packet relays further demonstrates the effectiveness of the new generic querying mechanism in simplifying the test suite across diverse testing scenarios.- 874-874: Verifying channel upgrades and fee enablement using
query.Channelandfeetypes.MetadataFromVersionshowcases the adaptability of the new querying mechanism to various query types, enhancing test suite maintainability and modularity.- 883-883: The use of
query.FeeEnabledChannelfor verifying fee enablement on channels post-upgrade aligns with the PR's objective of streamlining gRPC queries, reducing code duplication and improving test suite maintainability.- 889-889: Employing
query.Channelandfeetypes.MetadataFromVersionfor verifying channel upgrades and fee enablement on both chains post-upgrade demonstrates the new querying mechanism's capability to handle diverse query requests efficiently.- 898-898: The consistent application of
query.FeeEnabledChannelfor fee enablement checks on channels across both chains post-upgrade reinforces the utility of the new generic querying mechanism in enhancing code modularity and maintainability.- 905-905: Utilizing
query.PacketAcknowledgementsfor querying packet acknowledgements pre and post-pruning showcases the versatility of the new querying mechanism in handling various query types efficiently, enhancing code reuse and simplification.- 914-914: The repeated use of
query.PacketAcknowledgementsfor querying packet acknowledgements post-pruning validates the effectiveness of the new querying mechanism in simplifying the test suite across different scenarios involving packet acknowledgements.- 942-942: Replacing
s.QueryCounterPartyPayeewithquery.CounterPartyPayeefor querying counterparty payee addresses aligns with the PR's goal of streamlining gRPC queries, reducing code duplication and improving test suite maintainability.- 953-953: The use of
query.IncentivizedPacketsForChannelfor querying incentivized packets pre and post fee payment demonstrates the new querying mechanism's capability to streamline and simplify the test suite, even in complex scenarios involving fee payments.- 975-975: Continued application of
query.IncentivizedPacketsForChannelfor querying incentivized packets post fee payment reinforces the utility of the new generic querying mechanism in enhancing code modularity and maintainability across various test cases.- 981-981: Employing
query.Balancefor balance verification in tests involving incentivized transfer packets further illustrates the adaptability and utility of the new generic querying function across diverse testing scenarios.- 999-1000: The
ClientStatefunction demonstrates a direct application of thequery.GRPCQuerygeneric function for querying client states. This usage exemplifies the PR's objective to streamline gRPC queries across the test suite, enhancing code modularity and maintainability.
| balanceResp, err := query.GRPCQuery[banktypes.QueryBalanceResponse](ctx, chainA, &banktypes.QueryBalanceRequest{ | ||
| Address: user.FormattedAddress(), | ||
| Denom: chainA.Config().Denom, | ||
| }) |
There was a problem hiding this comment.
Refactoring the balance querying function to use query.GRPCQuery simplifies the querying process and makes the code more maintainable. However, ensure that error handling is robust and that the context passed to GRPCQuery includes any necessary metadata or deadlines.
Consider adding more detailed error handling or logging to provide clearer insights into failures during the querying process.
|
|
||
| balanceResp, err := query.GRPCQuery[banktypes.QueryBalanceResponse](ctx, chainB, &banktypes.QueryBalanceRequest{ | ||
| Address: user.FormattedAddress(), | ||
| Denom: chainB.Config().Denom, | ||
| }) |
There was a problem hiding this comment.
Similar to the previous comment, using query.GRPCQuery for querying balance on chain B is a significant improvement. Ensure that error handling and context management are adequately addressed to prevent potential issues during the querying process.
| denomMetadataResp, err := query.GRPCQuery[banktypes.QueryDenomMetadataResponse](ctx, chain, &banktypes.QueryDenomMetadataRequest{ | ||
| Denom: chainIBCDenom.IBCDenom(), | ||
| }) | ||
| s.Require().NoError(err) | ||
|
|
||
| denomMetadata := denomMetadataResp.Metadata | ||
|
|
There was a problem hiding this comment.
The use of query.GRPCQuery for asserting human-readable denom metadata is another excellent application of the centralized querying mechanism. It makes the code cleaner and more maintainable. However, the assertions on the metadata fields (base, name, display, symbol) rely on specific formatting and naming conventions.
Ensure that these conventions are well-documented and consistently applied across the codebase to avoid potential mismatches or confusion.
| @@ -442,7 +448,7 @@ | |||
| // SetUpgradeTimeoutParam creates and submits a governance proposal to execute the message to update 04-channel params with a timeout of 1s | |||
| func (s *E2ETestSuite) SetUpgradeTimeoutParam(ctx context.Context, chain ibc.Chain, wallet ibc.Wallet) { | |||
| const timeoutDelta = 1000000000 // use 1 second as relative timeout to force upgrade timeout on the counterparty | |||
| govModuleAddress, err := s.QueryModuleAccountAddress(ctx, govtypes.ModuleName, chain) | |||
| govModuleAddress, err := query.ModuleAccountAddress(ctx, govtypes.ModuleName, chain) | |||
There was a problem hiding this comment.
The call to query.ModuleAccountAddress for setting upgrade timeout parameters is a good example of leveraging the new querying package to simplify the retrieval of module account addresses. However, it's crucial to handle potential errors gracefully and provide informative error messages to aid in debugging.
Consider adding more detailed error handling around the query.ModuleAccountAddress call to ensure that any issues encountered are clearly communicated to the developer.
| @@ -453,7 +459,7 @@ | |||
|
|
|||
| // InitiateChannelUpgrade creates and submits a governance proposal to execute the message to initiate a channel upgrade | |||
| func (s *E2ETestSuite) InitiateChannelUpgrade(ctx context.Context, chain ibc.Chain, wallet ibc.Wallet, portID, channelID string, upgradeFields channeltypes.UpgradeFields) { | |||
| govModuleAddress, err := s.QueryModuleAccountAddress(ctx, govtypes.ModuleName, chain) | |||
| govModuleAddress, err := query.ModuleAccountAddress(ctx, govtypes.ModuleName, chain) | |||
There was a problem hiding this comment.
Similarly, using query.ModuleAccountAddress for initiating channel upgrades demonstrates the benefits of the centralized querying mechanism. As with the previous comment, ensure that error handling is robust and that any issues encountered during the querying process are clearly communicated.
Enhance error handling around the query.ModuleAccountAddress call to provide clearer insights into failures and facilitate troubleshooting.
| s.Require().Equal(clienttypes.ZeroHeight(), upgradeTimeout.Height) | ||
| s.Require().Equal(uint64(time.Minute*10), upgradeTimeout.Timestamp) | ||
| }) | ||
| }) | ||
|
|
||
| t.Run("execute gov proposal to initiate channel upgrade", func(t *testing.T) { | ||
| chA, err := s.QueryChannel(ctx, chainA, channelA.PortID, channelA.ChannelID) | ||
| chA, err := query.Channel(ctx, chainA, channelA.PortID, channelA.ChannelID) |
There was a problem hiding this comment.
Initiating a channel upgrade through a governance proposal is a critical operation. It's important to ensure that the upgrade fields are correctly set and that the channel upgrade does not adversely affect ongoing IBC operations. Consider adding more detailed tests around the channel upgrade process to cover various scenarios and edge cases.
Would you like assistance in generating additional test cases to cover various scenarios and edge cases for channel upgrades?
| } | ||
|
|
||
| // Create a connection to the gRPC server. | ||
| grpcConn, err := grpc.Dial( |
There was a problem hiding this comment.
I guess instead of doing a Dial for each query we could init the grpc conn, store a ref to it in the e2esuite, and have a generic Query function that takes the e2esuite as an arg?
I doubt this re-dialing costs much considering we're using local endpoints though so I'd be perfectly fine with this too.
There was a problem hiding this comment.
Yeah, if there is a convincing argument showing that dialing costs matter, then I'd be happy to create this global
There was a problem hiding this comment.
nice consideration! But TBH, creating it every time means we never need to worry about re-initializing connections (e.g. after chain restarts) and just generally simplifies things. I think we should wait until this is a problem.
| ) | ||
|
|
||
| // Queries the chain with a query request and deserializes the response to T | ||
| func GRPCQuery[T any](ctx context.Context, chain ibc.Chain, req proto.Message, opts ...grpc.CallOption) (*T, error) { |
There was a problem hiding this comment.
we should be able to not return an error here, since I doubt we'd ever want to ever test grpc query err cases in e2es. Looking at invocations does reinforce that thought (hoping I didn't miss anything!), we always require no error after a call. Thoughts?
There was a problem hiding this comment.
There are cases we want to test the error. In fact, we do this to check whether a packet commitment is missing here: https://github.com/cosmos/ibc-go/pull/5979/files#diff-a36fcd03b468866255ec8560b99276d8897eccd88d3876b1534e6c9e9b48c800R347
There was a problem hiding this comment.
I think it's reasonable enough to leave it up to caller to handle the error, especially since this fn isn't associated with a testing.T in any way.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (3)
- e2e/tests/core/02-client/client_test.go (14 hunks)
- e2e/tests/interchain_accounts/base_test.go (13 hunks)
- e2e/tests/transfer/localhost_test.go (4 hunks)
Files skipped from review as they are similar to previous changes (3)
- e2e/tests/core/02-client/client_test.go
- e2e/tests/interchain_accounts/base_test.go
- e2e/tests/transfer/localhost_test.go
| ) | ||
|
|
||
| // Queries the chain with a query request and deserializes the response to T | ||
| func GRPCQuery[T any](ctx context.Context, chain ibc.Chain, req proto.Message, opts ...grpc.CallOption) (*T, error) { |
There was a problem hiding this comment.
I think it's reasonable enough to leave it up to caller to handle the error, especially since this fn isn't associated with a testing.T in any way.
| ) | ||
|
|
||
| // Queries the chain with a query request and deserializes the response to T | ||
| func GRPCQuery[T any](ctx context.Context, chain ibc.Chain, req proto.Message, opts ...grpc.CallOption) (*T, error) { |
There was a problem hiding this comment.
is there a further constract we can put on T here? Maybe enforce it its a proto message?
Not an expert with generics by any means but something like
type QueryResponse interface {
proto.Message
}
func GRPCQuery[T QueryResponse] (){}not foolproof but a little better than any WDYT?
There was a problem hiding this comment.
linter complaining when I do this + I think keeping it simple is good imo
| } | ||
|
|
||
| // Create a connection to the gRPC server. | ||
| grpcConn, err := grpc.Dial( |
There was a problem hiding this comment.
nice consideration! But TBH, creating it every time means we never need to worry about re-initializing connections (e.g. after chain restarts) and just generally simplifies things. I think we should wait until this is a problem.
There was a problem hiding this comment.
Actionable comments posted: 2
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (8)
- e2e/tests/interchain_accounts/base_test.go (13 hunks)
- e2e/tests/interchain_accounts/localhost_test.go (9 hunks)
- e2e/tests/interchain_accounts/params_test.go (6 hunks)
- e2e/tests/interchain_accounts/query_test.go (2 hunks)
- e2e/tests/interchain_accounts/upgrades_test.go (11 hunks)
- e2e/tests/upgrades/genesis_test.go (4 hunks)
- e2e/testsuite/query/grpc_query.go (1 hunks)
- e2e/testsuite/tx.go (4 hunks)
Additional Context Used
Path-based Instructions (8)
e2e/testsuite/query/grpc_query.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
e2e/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"e2e/tests/interchain_accounts/query_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
e2e/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"e2e/tests/upgrades/genesis_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
e2e/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"e2e/tests/interchain_accounts/params_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
e2e/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"e2e/tests/interchain_accounts/upgrades_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
e2e/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"e2e/testsuite/tx.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
e2e/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"e2e/tests/interchain_accounts/localhost_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
e2e/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"e2e/tests/interchain_accounts/base_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
e2e/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (36)
e2e/testsuite/query/grpc_query.go (1)
14-14: The function documentation is clear and concise, providing a good overview of its purpose and usage.e2e/tests/interchain_accounts/query_test.go (2)
20-20: The import of thequerypackage is correctly added to utilize the new genericqueryfunction within the tests. This change aligns with the PR's objective to streamline gRPC querying in the e2e testing suite.
68-68: The replacement ofs.QueryInterchainAccountwithquery.InterchainAccountis a good application of the new genericqueryfunction, enhancing the test's maintainability by reducing direct dependencies on specific query clients.e2e/tests/upgrades/genesis_test.go (3)
24-24: The import of thequerypackage is correctly added to utilize the new genericqueryfunction within the tests. This change aligns with the PR's objective to streamline gRPC querying in the e2e testing suite.
107-107: The replacement ofs.QueryBalancewithquery.Balanceis a good application of the new genericqueryfunction, enhancing the test's maintainability by reducing direct dependencies on specific query clients.
116-119: The use ofquery.GRPCQuerywithcontrollertypes.QueryInterchainAccountResponseas the generic type parameter is a clear example of how the new genericqueryfunction can be utilized to streamline querying across different modules. This change improves code readability and maintainability.e2e/tests/interchain_accounts/params_test.go (3)
24-24: The import of thequerypackage is correctly added to utilize the new genericqueryfunction within the tests. This change aligns with the PR's objective to streamline gRPC querying in the e2e testing suite.
44-44: The replacement of direct query calls withquery.GRPCQueryfor controller params is a good application of the new genericqueryfunction, enhancing the test's maintainability by reducing direct dependencies on specific query clients.
52-52: Similarly, the replacement of direct query calls withquery.GRPCQueryfor host params demonstrates the versatility and utility of the new genericqueryfunction in streamlining the querying process across different modules.e2e/tests/interchain_accounts/upgrades_test.go (3)
23-23: The import of thequerypackage is correctly added to utilize the new genericqueryfunction within the tests. This change aligns with the PR's objective to streamline gRPC querying in the e2e testing suite.
82-82: The replacement ofs.QueryInterchainAccountwithquery.InterchainAccountis a good application of the new genericqueryfunction, enhancing the test's maintainability by reducing direct dependencies on specific query clients.
86-86: The use ofquery.Channelto replace direct query calls for channel information is another example of how the new genericqueryfunction can streamline querying processes across different modules, improving code readability and maintainability.e2e/testsuite/tx.go (4)
29-29: The import of thequerypackage is correctly added to facilitate the use of the newGRPCQueryfunction. This aligns with the PR's objective to streamline gRPC querying by consolidating multiple query clients into a single, generic function.
186-197: The modification to usequery.GRPCQuerywithgovtypesv1.QueryProposalResponseinwaitForGovV1ProposalToPassis a good example of the intended consolidation of gRPC queries. This change enhances maintainability by reducing redundancy and streamlining the querying process.
227-244: The use ofquery.GRPCQueryinExecuteAndPassGovV1Beta1Proposalfor querying proposal responses is consistent with the PR's goal. It's important to ensure that thequery.GRPCQueryfunction is thoroughly tested, especially since it now handles diverse query types.
253-263: The refactoring to usequery.GRPCQueryfor querying proposal responses inwaitForGovV1Beta1ProposalToPassis consistent with the changes made in other parts of the file. This demonstrates a uniform approach to querying, which is beneficial for code maintainability.e2e/tests/interchain_accounts/localhost_test.go (5)
22-22: The import of thequerypackage is correctly added, enabling the use of the newGRPCQueryfunction for querying operations related to interchain accounts. This change supports the PR's goal of reducing redundancy and enhancing maintainability.
112-116: The replacement of direct method calls with calls toquery.Channelfor querying channel ends is a good application of the newquerypackage. This change simplifies the querying process and aligns with the PR's objectives.
124-124: Usingquery.InterchainAccountto query interchain account addresses demonstrates the intended use of thequerypackage to streamline querying operations. Ensure that all query operations related to interchain accounts are consistently using this package.
138-138: Repeating the use ofquery.InterchainAccountfor querying interchain account addresses before sending packets is consistent with the PR's goal. This consistency in using thequerypackage enhances code readability and maintainability.
187-187: The use ofquery.Balanceto query balances is another example of how thequerypackage is being utilized to streamline querying operations. This change contributes to reducing code duplication and improving maintainability.e2e/tests/interchain_accounts/base_test.go (15)
24-24: The addition of thequerypackage import is consistent with the PR's objective to centralize gRPC queries. Ensure that thequerypackage is thoroughly tested, especially since it now plays a critical role in the e2e testing suite.
92-92: The replacement of direct query calls with thequery.InterchainAccountfunction is a good example of code centralization. However, ensure that thequerypackage'sInterchainAccountfunction handles all edge cases previously covered by the direct query method.
148-151: Using thequery.Balancefunction to replace direct balance queries is in line with the PR's goals. It's important to verify that this function accurately handles different denominations and error scenarios, similar to the original implementation.
191-191: Repeating the use ofquery.InterchainAccountin another test case demonstrates consistency in applying the new querying approach. Ensure that this does not introduce any performance regressions due to potential overhead from the generic querying mechanism.
202-202: The use ofquery.Balanceto check the host account's balance before attempting a transfer is a prudent check. Confirm that thequerypackage's implementation provides real-time balance information to avoid stale data issues.
241-241: Again, thequery.Balancefunction is used to verify the outcome of a transaction. It's crucial to ensure that this method accurately reflects changes in account balances post-transaction, considering eventual consistency models in distributed systems.
289-289: The consistent use ofquery.InterchainAccountacross different test scenarios is noted. It's essential to validate that the underlying gRPC calls are optimized for performance, especially in test environments where speed is critical.
293-293: The introduction of thequery.Channelfunction to check channel states is a significant change. Ensure that this function can accurately determine the state of channels, including nuanced states that might be relevant for specific tests.
353-353: The use ofquery.Channelto verify channel closure due to timeout is a critical test assertion. This highlights the importance of thequerypackage's ability to accurately reflect channel states, which must be thoroughly tested.
360-363: Utilizingquery.Balanceto confirm that tokens were not transferred due to a channel timeout is a good practice. This reinforces the need for thequerypackage to provide accurate and timely balance information.
382-382: Thequery.Channelfunction is used to verify the state of a newly opened channel. This is a crucial check, and it's important to ensure that thequerypackage can handle scenarios where channels are reopened or have their state changed.
423-423: The use ofquery.Balanceto verify successful token transfer after reopening an interchain account channel is consistent with previous test cases. It's essential to ensure that thequerypackage's balance query accurately reflects such transactions.
469-469: The repeated use ofquery.InterchainAccountacross various test scenarios demonstrates the PR's objective to centralize query functionality. It's important to ensure that this does not introduce any unintended side effects, especially in complex test scenarios.
493-493: Introducing thequery.ModuleAccountAddressfunction to obtain a module account address is a significant change. Ensure that this function is capable of accurately retrieving addresses for various modules, considering the dynamic nature of blockchain state.
533-536: The use of a genericquery.GRPCQueryfunction to retrieve a governance proposal demonstrates the flexibility of the newquerypackage. It's crucial to verify that this generic querying mechanism does not compromise the specificity and accuracy needed for such queries.
| func getProtoPath(req proto.Message) (string, error) { | ||
| typeURL := "/" + proto.MessageName(req) | ||
|
|
||
| switch { | ||
| case strings.Contains(typeURL, "Query"): | ||
| return getQueryProtoPath(typeURL) | ||
| case strings.Contains(typeURL, "cosmos.base.tendermint"): | ||
| return getCmtProtoPath(typeURL) | ||
| default: | ||
| return "", fmt.Errorf("unsupported typeURL: %s", typeURL) | ||
| } | ||
| } | ||
|
|
||
| func getQueryProtoPath(queryTypeURL string) (string, error) { | ||
| queryIndex := strings.Index(queryTypeURL, "Query") | ||
| if queryIndex == -1 { | ||
| return "", fmt.Errorf("invalid typeURL: %s", queryTypeURL) | ||
| } | ||
|
|
||
| // Add to the index to account for the length of "Query" | ||
| queryIndex += len("Query") | ||
|
|
||
| // Add a slash before the query | ||
| urlWithSlash := queryTypeURL[:queryIndex] + "/" + queryTypeURL[queryIndex:] | ||
| if !strings.HasSuffix(urlWithSlash, "Request") { | ||
| return "", fmt.Errorf("invalid typeURL: %s", queryTypeURL) | ||
| } | ||
|
|
||
| return strings.TrimSuffix(urlWithSlash, "Request"), nil | ||
| } | ||
|
|
||
| func getCmtProtoPath(cmtTypeURL string) (string, error) { | ||
| cmtIndex := strings.Index(cmtTypeURL, "Get") | ||
| if cmtIndex == -1 { | ||
| return "", fmt.Errorf("invalid typeURL: %s", cmtTypeURL) | ||
| } | ||
|
|
||
| // Add a slash before the commitment | ||
| urlWithSlash := cmtTypeURL[:cmtIndex] + "Service/" + cmtTypeURL[cmtIndex:] | ||
| if !strings.HasSuffix(urlWithSlash, "Request") { | ||
| return "", fmt.Errorf("invalid typeURL: %s", cmtTypeURL) | ||
| } | ||
|
|
||
| return strings.TrimSuffix(urlWithSlash, "Request"), nil | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The verification process revealed that there are no tests specifically targeting the functions getProtoPath, getQueryProtoPath, and getCmtProtoPath within the e2e/testsuite/query directory. This finding contradicts the assumption made in the review comment about the importance of thorough testing for all supported typeURLs. Therefore, it's crucial to address this gap in test coverage to ensure the reliability and correctness of these functions.
Analysis chain
The getProtoPath function and its helpers getQueryProtoPath and getCmtProtoPath are well-structured and provide a clear mechanism for deriving the gRPC service path from the request type. This dynamic approach enhances the flexibility of the GRPCQuery function. Ensure that all supported typeURLs are thoroughly tested to prevent runtime errors due to unsupported or incorrectly formatted typeURLs.
Given the initial script did not produce any output, it's essential to take a more granular approach to verify the presence of the functions and their tests separately. Let's start by confirming the presence of the functions getProtoPath, getQueryProtoPath, and getCmtProtoPath in the Go files.
To further investigate the presence of tests for the functions getProtoPath, getQueryProtoPath, and getCmtProtoPath, let's search for any test functions in the e2e/testsuite/query directory or its subdirectories that might reference these methods.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that all supported typeURLs are covered by unit tests.
rg --type go "getProtoPath|getQueryProtoPath|getCmtProtoPath" --files-with-matches | xargs rg "Test"
Length of output: 100
Script:
#!/bin/bash
# Verify the presence of the functions in the Go files.
rg --type go "func (getProtoPath|getQueryProtoPath|getCmtProtoPath)\("
Length of output: 349
Script:
#!/bin/bash
# Search for test functions in the e2e/testsuite/query directory.
rg --type go "Test" e2e/testsuite/query/
Length of output: 40
|
Description
closes: #5714
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
querypackage, improving code maintainability and consistency.