-
Notifications
You must be signed in to change notification settings - Fork 586
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: allow module safe queries in ICA #5785
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #5785 +/- ##
==========================================
- Coverage 81.49% 81.45% -0.04%
==========================================
Files 199 200 +1
Lines 15203 15246 +43
==========================================
+ Hits 12389 12419 +30
- Misses 2348 2354 +6
- Partials 466 473 +7
|
WalkthroughThe changes introduce a comprehensive framework for querying interchain accounts, enhancing transaction execution between chains using IBC protocols. This includes testing for query functionality, codec registration for transaction handling, and a safe query mechanism via whitelisting. Additionally, it aligns with the objective of providing a query API for Interchain Accounts, addressing the need for native query support alongside ICA transactions. Changes
Assessment against linked issues
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 (
|
This PR is not ready to be merged because no e2e tests are written |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (3)
- e2e/tests/interchain_accounts/query_test.go (1 hunks)
- e2e/testsuite/codec.go (2 hunks)
- e2e/testsuite/tx.go (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- e2e/tests/interchain_accounts/query_test.go
- e2e/testsuite/codec.go
- e2e/testsuite/tx.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (3)
- modules/apps/27-interchain-accounts/host/keeper/export_test.go (1 hunks)
- modules/apps/27-interchain-accounts/host/keeper/keeper.go (5 hunks)
- modules/apps/27-interchain-accounts/host/keeper/keeper_test.go (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- modules/apps/27-interchain-accounts/host/keeper/keeper.go
- modules/apps/27-interchain-accounts/host/keeper/keeper_test.go
Additional comments: 1
modules/apps/27-interchain-accounts/host/keeper/export_test.go (1)
- 18-21: LGTM! This wrapper function
NewModuleQuerySafeWhitelist
is a good practice for enhancing testability of private functions without compromising encapsulation. It aligns well with the PR's objectives of improving the ICA module's functionality and ensuring comprehensive testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yml
Files selected for processing (4)
- modules/apps/27-interchain-accounts/host/keeper/export_test.go (1 hunks)
- modules/apps/27-interchain-accounts/host/keeper/keeper.go (5 hunks)
- modules/apps/27-interchain-accounts/host/keeper/keeper_test.go (4 hunks)
- modules/apps/27-interchain-accounts/host/keeper/msg_server.go (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- modules/apps/27-interchain-accounts/host/keeper/export_test.go
- modules/apps/27-interchain-accounts/host/keeper/keeper_test.go
- modules/apps/27-interchain-accounts/host/keeper/msg_server.go
Additional comments: 4
modules/apps/27-interchain-accounts/host/keeper/keeper.go (4)
- 8-13: The addition of protobuf-related imports is necessary for the new functionality related to module-safe queries. Ensure that these imports are used effectively within the file and that there are no unused imports to maintain code cleanliness.
- 45-49: The addition of
msgRouter
,queryRouter
, andmqsAllowList
to theKeeper
struct supports the new module-safe queries functionality. It's good practice to document struct fields, especially when introducing new concepts like module-safe queries, to enhance code readability and maintainability.Consider adding comments to these fields to explain their purpose and usage within the module.
- 61-61: The modification of the
NewKeeper
function signature to include aqueryRouter
parameter aligns with the introduction of module-safe queries. This change is crucial for routing queries appropriately within the ICA module.- 82-83: Initializing
queryRouter
andmqsAllowList
in theNewKeeper
function is essential for setting up the module-safe queries functionality. However, thenewModuleQuerySafeAllowList
function call here usespanic
for error handling, which might not align with the overall error handling strategy of the application.Consider revising the error handling approach in
newModuleQuerySafeAllowList
to return an error and handle it appropriately inNewKeeper
, enhancing the robustness of the module initialization process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 55-56: The addition of a new transaction message for querying the chain in the
apps/27-interchain-accounts
module is well-documented in the CHANGELOG. This update provides clear information on the enhancement made to the ICA module, which is crucial for developers and users to understand the new capabilities introduced.
Quality Gate passed for 'ibc-go'Issues Measures |
I will go ahead and merge the PR. Thank you @srdtrk for the superb work on this one. If someone would still like to do a review, please post your comments and we can deal with them in a separate PR. Once the backport for this PR is opened I will start working on reverting the API breaking change. |
* imp: initial modification of tx.proto * imp: ran 'make proto-all' * fix: compiler errors * imp: added query router interface * imp: added queryRouter to icahost keeper * imp: improved proto definitions * imp: ran 'make proto-all' * imp: added sdk.Msg helpers * feat: basic implementation * style: improved field names * imp: ran 'make proto-all' * fix: compiler errors * imp: ran gofumpt * feat: tests passing * feat: tests improved * test: removed unneeded code * imp: improved 'IsModuleSafe' function * imp: added IsModuleQuerySafe to msg_server * imp: added more test cases * fix: callbacks compiler * fix: non determinancy issues * fix: added query msg to codec * imp: whitelist logic added * e2e: new test added * fix: new test * fix: test * fix: e2e * fix: e2e * imp(e2e): added the QueryTxsByEvents function * fix: e2e * e2e: lint fix * fix: e2e * e2e: debug * fix: e2e * test: debug helpers * debug * test: added codec_test case * imp: additional test case * imp: added important unit test * r4r * e2e: debug * imp: added logs * fix: e2e * fix: e2e * fix: e2e * imp: added height to proto response * imp: ran 'make proto-all' * imp: added height * e2e: updated e2e to test height * imp: review suggestions * e2e: remove unneeded log * refactor: refactored 'ExtractValueFromEvents' * e2e: review item * imp: review item * nit: review item * docs: added godocs * test: unit test for mqsWhitelist added * imp: added logging * style: rename to allow list * add changelog --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit eecfa5c) # Conflicts: # CHANGELOG.md # e2e/testsuite/codec.go # e2e/testsuite/tx.go # modules/apps/27-interchain-accounts/host/types/msgs.go # modules/light-clients/08-wasm/testing/simapp/app.go
} | ||
|
||
// ValidateBasic implements sdk.HasValidateBasic | ||
func (msg MsgModuleQuerySafe) ValidateBasic() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed in the backport PR that we don't have tests for this function.
* imp: initial modification of tx.proto * imp: ran 'make proto-all' * fix: compiler errors * imp: added query router interface * imp: added queryRouter to icahost keeper * imp: improved proto definitions * imp: ran 'make proto-all' * imp: added sdk.Msg helpers * feat: basic implementation * style: improved field names * imp: ran 'make proto-all' * fix: compiler errors * imp: ran gofumpt * feat: tests passing * feat: tests improved * test: removed unneeded code * imp: improved 'IsModuleSafe' function * imp: added IsModuleQuerySafe to msg_server * imp: added more test cases * fix: callbacks compiler * fix: non determinancy issues * fix: added query msg to codec * imp: whitelist logic added * e2e: new test added * fix: new test * fix: test * fix: e2e * fix: e2e * imp(e2e): added the QueryTxsByEvents function * fix: e2e * e2e: lint fix * fix: e2e * e2e: debug * fix: e2e * test: debug helpers * debug * test: added codec_test case * imp: additional test case * imp: added important unit test * r4r * e2e: debug * imp: added logs * fix: e2e * fix: e2e * fix: e2e * imp: added height to proto response * imp: ran 'make proto-all' * imp: added height * e2e: updated e2e to test height * imp: review suggestions * e2e: remove unneeded log * refactor: refactored 'ExtractValueFromEvents' * e2e: review item * imp: review item * nit: review item * docs: added godocs * test: unit test for mqsWhitelist added * imp: added logging * style: rename to allow list * add changelog --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit eecfa5c) # Conflicts: # e2e/testsuite/codec.go # e2e/testsuite/tx.go # modules/apps/27-interchain-accounts/host/keeper/keeper.go # modules/apps/27-interchain-accounts/host/keeper/keeper_test.go # modules/apps/27-interchain-accounts/host/keeper/msg_server.go # modules/apps/27-interchain-accounts/host/keeper/msg_server_test.go # modules/apps/27-interchain-accounts/host/types/codec.go # modules/apps/27-interchain-accounts/host/types/codec_test.go # modules/apps/27-interchain-accounts/host/types/host.pb.go # modules/apps/27-interchain-accounts/host/types/msgs.go # modules/apps/27-interchain-accounts/host/types/tx.pb.go # modules/apps/callbacks/testing/simapp/app.go # modules/light-clients/08-wasm/testing/simapp/app.go # proto/ibc/applications/interchain_accounts/host/v1/tx.proto # testing/simapp/app.go
* feat: allow module safe queries in ICA (#5785) * imp: initial modification of tx.proto * imp: ran 'make proto-all' * fix: compiler errors * imp: added query router interface * imp: added queryRouter to icahost keeper * imp: improved proto definitions * imp: ran 'make proto-all' * imp: added sdk.Msg helpers * feat: basic implementation * style: improved field names * imp: ran 'make proto-all' * fix: compiler errors * imp: ran gofumpt * feat: tests passing * feat: tests improved * test: removed unneeded code * imp: improved 'IsModuleSafe' function * imp: added IsModuleQuerySafe to msg_server * imp: added more test cases * fix: callbacks compiler * fix: non determinancy issues * fix: added query msg to codec * imp: whitelist logic added * e2e: new test added * fix: new test * fix: test * fix: e2e * fix: e2e * imp(e2e): added the QueryTxsByEvents function * fix: e2e * e2e: lint fix * fix: e2e * e2e: debug * fix: e2e * test: debug helpers * debug * test: added codec_test case * imp: additional test case * imp: added important unit test * r4r * e2e: debug * imp: added logs * fix: e2e * fix: e2e * fix: e2e * imp: added height to proto response * imp: ran 'make proto-all' * imp: added height * e2e: updated e2e to test height * imp: review suggestions * e2e: remove unneeded log * refactor: refactored 'ExtractValueFromEvents' * e2e: review item * imp: review item * nit: review item * docs: added godocs * test: unit test for mqsWhitelist added * imp: added logging * style: rename to allow list * add changelog --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit eecfa5c) # Conflicts: # CHANGELOG.md # e2e/testsuite/codec.go # e2e/testsuite/tx.go # modules/apps/27-interchain-accounts/host/types/msgs.go # modules/light-clients/08-wasm/testing/simapp/app.go * fix conflicts + add GetSigners to message * delete e2e and 08-wasm folders * add tests for message functions * use boolean for expected test result * revert API breaking change in ica host NewKeeper * lint * panic if query router is nil --------- Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Damian Nolan <damiannolan@gmail.com>
* feat: allow module safe queries in ICA (#5785) * imp: initial modification of tx.proto * imp: ran 'make proto-all' * fix: compiler errors * imp: added query router interface * imp: added queryRouter to icahost keeper * imp: improved proto definitions * imp: ran 'make proto-all' * imp: added sdk.Msg helpers * feat: basic implementation * style: improved field names * imp: ran 'make proto-all' * fix: compiler errors * imp: ran gofumpt * feat: tests passing * feat: tests improved * test: removed unneeded code * imp: improved 'IsModuleSafe' function * imp: added IsModuleQuerySafe to msg_server * imp: added more test cases * fix: callbacks compiler * fix: non determinancy issues * fix: added query msg to codec * imp: whitelist logic added * e2e: new test added * fix: new test * fix: test * fix: e2e * fix: e2e * imp(e2e): added the QueryTxsByEvents function * fix: e2e * e2e: lint fix * fix: e2e * e2e: debug * fix: e2e * test: debug helpers * debug * test: added codec_test case * imp: additional test case * imp: added important unit test * r4r * e2e: debug * imp: added logs * fix: e2e * fix: e2e * fix: e2e * imp: added height to proto response * imp: ran 'make proto-all' * imp: added height * e2e: updated e2e to test height * imp: review suggestions * e2e: remove unneeded log * refactor: refactored 'ExtractValueFromEvents' * e2e: review item * imp: review item * nit: review item * docs: added godocs * test: unit test for mqsWhitelist added * imp: added logging * style: rename to allow list * add changelog --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit eecfa5c) # Conflicts: # e2e/testsuite/codec.go # e2e/testsuite/tx.go # modules/apps/27-interchain-accounts/host/keeper/keeper.go # modules/apps/27-interchain-accounts/host/keeper/keeper_test.go # modules/apps/27-interchain-accounts/host/keeper/msg_server.go # modules/apps/27-interchain-accounts/host/keeper/msg_server_test.go # modules/apps/27-interchain-accounts/host/types/codec.go # modules/apps/27-interchain-accounts/host/types/codec_test.go # modules/apps/27-interchain-accounts/host/types/host.pb.go # modules/apps/27-interchain-accounts/host/types/msgs.go # modules/apps/27-interchain-accounts/host/types/tx.pb.go # modules/apps/callbacks/testing/simapp/app.go # modules/light-clients/08-wasm/testing/simapp/app.go # proto/ibc/applications/interchain_accounts/host/v1/tx.proto # testing/simapp/app.go * fix conflicts * fix tests * gofumpt * do not set ica query router for callbacks * pls * fix comment * panic if query router is nil * break loop earlier * workaround for using module safe queries of x/staking * add comment --------- Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>
Description
closes: #5784
Commit Message / Changelog Entry
see the guidelines for commit messages. (view raw markdown for examples)
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/
) or specification (x/<module>/spec/
).godoc
comments.Files changed
in the Github PR explorer.Codecov Report
in the comment section below once CI passes.Summary by CodeRabbit
New Features
Tests
Documentation