refactor!: decouple client routing from encoding #5806
Conversation
… feat/02-client-router
…lls in 04-channel
… feat/02-client-router
… feat/02-client-router
… feat/02-client-router
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- docs/docs/03-light-clients/01-developer-guide/02-light-client-module.md (3 hunks)
Additional comments: 7
docs/docs/03-light-clients/01-developer-guide/02-light-client-module.md (7)
- 2-5: The document title and sidebar labels have been updated to reflect the new
LightClientModuleinterface. This change aligns with the PR's objective to decouple client routing from encoding, enhancing clarity and consistency in the documentation.- 9-9: The introduction to implementing the
LightClientModuleinterface is clear and concise. It effectively sets the stage for the detailed explanation of the interface's methods that follows.- 1-12: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [16-16]
The explanation of the
Statusmethod and its possible return values is comprehensive and informative. However, the term "misbehaviour" is used, which is British English. While not incorrect, it's important to ensure consistency in spelling conventions throughout the documentation. If the project predominantly uses American English, consider changing to "misbehavior".Consider standardizing the spelling of "misbehaviour" to "misbehavior" if the rest of the documentation primarily uses American English.
- 24-24: Renaming
GetTimestampAtHeighttoTimestampAtHeightsimplifies the method name while maintaining clarity about its functionality. This change is a good example of improving readability and conciseness in interface design.- 39-39: The link to the "Existence and non-existence proofs section" is a helpful addition, providing readers with a direct reference to further details on membership proofs. Ensuring that such links are up-to-date and accessible is crucial for maintaining the usefulness of the documentation.
- 44-44: Similar to the previous comment, the link to the "Existence and non-existence proofs section" for non-membership proofs is valuable for readers seeking more information. It's important to regularly verify that these links remain valid and lead to the intended content.
- 1-12: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [55-55]
The explanation of the
CheckForMisbehaviourmethod is clear, but it again uses the term "misbehaviour." As mentioned earlier, consider standardizing the spelling based on the predominant convention used in the project's documentation.Consider standardizing the spelling of "misbehaviour" to "misbehavior" if the rest of the documentation primarily uses American English.
| } | ||
|
|
||
| // AddRoute adds LightClientModule for a given module name. It returns the Router | ||
| // so AddRoute calls can be linked. It will panic if the Router is sealed. |
There was a problem hiding this comment.
we don't have a concept of sealing the router as far as I can see yea?
There was a problem hiding this comment.
bumped into it again on main. do we want to seal the router?
There was a problem hiding this comment.
note that it would not be needed if we do #6010 (and remove GetRouter on client Keeper, it would only have a SetRouter). Sealing would be indirect via keeping it as an inaccessible (after setting) private var.
| @@ -31,16 +31,22 @@ import ( | |||
| type Keeper struct { | |||
| storeKey storetypes.StoreKey | |||
| cdc codec.BinaryCodec | |||
| router *types.Router | |||
There was a problem hiding this comment.
more food for thought would be having this as a non-reference type and adding a SetRouter to the keeper similar to IBCKeeper's SetRouter. Happy to punt this for another time too.
There was a problem hiding this comment.
Feel free to open an issue and add needs discussion if you'd like!
Pleasure working with you on this too @colin-axner! ❤️ I agree that there has never been a clear structure for light client modules, and since IBC v1 all code has generally fallen into Will follow up on comments and issues and get this merged ASAP! 🚀 |
…e scoped correctly for later usage in misbehaviour test
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 (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- e2e/tests/core/02-client/client_test.go
| // GetRouter returns the light client module router. | ||
| func (k Keeper) GetRouter() *types.Router { | ||
| return k.router | ||
| } |
There was a problem hiding this comment.
As noted in thread https://github.com/cosmos/ibc-go/pull/5806/files#r1523681992
Refactor: GetRouter() can be changed to ClientModule(clientID string) or Route(clientID string) returning the light client module
There was a problem hiding this comment.
a few small comments, overall great work from @damiannolan and @colin-axner!
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: .coderabbit.yml
Files selected for processing (26)
- CHANGELOG.md (1 hunks)
- docs/docs/03-light-clients/01-developer-guide/01-overview.md (5 hunks)
- docs/docs/03-light-clients/01-developer-guide/03-client-state.md (1 hunks)
- docs/docs/03-light-clients/01-developer-guide/04-consensus-state.md (2 hunks)
- docs/docs/05-migrations/13-v8-to-v9.md (1 hunks)
- e2e/tests/transfer/localhost_test.go (1 hunks)
- modules/core/02-client/keeper/grpc_query_test.go (8 hunks)
- modules/core/02-client/keeper/keeper_test.go (3 hunks)
- modules/core/02-client/types/router.go (1 hunks)
- modules/core/keeper/msg_server.go (3 hunks)
- modules/light-clients/06-solomachine/doc.go (1 hunks)
- modules/light-clients/06-solomachine/store.go (1 hunks)
- modules/light-clients/07-tendermint/doc.go (1 hunks)
- modules/light-clients/07-tendermint/internal/keeper/keeper.go (1 hunks)
- modules/light-clients/07-tendermint/store.go (1 hunks)
- modules/light-clients/08-wasm/doc.go (1 hunks)
- modules/light-clients/08-wasm/keeper/msg_server_test.go (2 hunks)
- modules/light-clients/08-wasm/types/client_state_test.go (13 hunks)
- modules/light-clients/08-wasm/types/misbehaviour_handle_test.go (1 hunks)
- modules/light-clients/08-wasm/types/proposal_handle_test.go (1 hunks)
- modules/light-clients/08-wasm/types/querier_test.go (3 hunks)
- modules/light-clients/08-wasm/types/store.go (2 hunks)
- modules/light-clients/08-wasm/types/update_test.go (4 hunks)
- modules/light-clients/08-wasm/types/upgrade_test.go (4 hunks)
- modules/light-clients/09-localhost/doc.go (1 hunks)
- modules/light-clients/09-localhost/store.go (1 hunks)
Files skipped from review as they are similar to previous changes (16)
- e2e/tests/transfer/localhost_test.go
- modules/core/02-client/keeper/grpc_query_test.go
- modules/core/02-client/keeper/keeper_test.go
- modules/core/02-client/types/router.go
- modules/core/keeper/msg_server.go
- modules/light-clients/06-solomachine/store.go
- modules/light-clients/07-tendermint/internal/keeper/keeper.go
- modules/light-clients/07-tendermint/store.go
- modules/light-clients/08-wasm/types/client_state_test.go
- modules/light-clients/08-wasm/types/misbehaviour_handle_test.go
- modules/light-clients/08-wasm/types/proposal_handle_test.go
- modules/light-clients/08-wasm/types/querier_test.go
- modules/light-clients/08-wasm/types/store.go
- modules/light-clients/08-wasm/types/update_test.go
- modules/light-clients/08-wasm/types/upgrade_test.go
- modules/light-clients/09-localhost/store.go
Additional comments: 13
modules/light-clients/06-solomachine/doc.go (1)
- 2-2: The documentation accurately reflects the changes made to the
solomachinepackage, aligning with the Solo Machine light client implementation. It's clear and adheres to Go documentation conventions.modules/light-clients/07-tendermint/doc.go (1)
- 2-2: The documentation accurately reflects the changes made to the
tendermintpackage, including the addition ofLightClientModulealongsideClientStateandConsensusState. It's clear and adheres to Go documentation conventions.modules/light-clients/08-wasm/doc.go (1)
- 2-2: The documentation accurately reflects the changes made to the
wasmpackage, including the addition ofLightClientModulealongsideClientState,ConsensusState, andClientMessage. It's clear and adheres to Go documentation conventions.docs/docs/03-light-clients/01-developer-guide/03-client-state.md (1)
- 21-21: The documentation provides a clear explanation of the
ClientStateinterface. Consider using more formal and concise wording to enhance clarity, especially when describing the responsibilities of the light client implementer.docs/docs/03-light-clients/01-developer-guide/04-consensus-state.md (1)
- 1-7: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [27-27]
The documentation provides a clear explanation of the
ConsensusStateinterface. Consider using more formal and concise wording to enhance clarity, especially when describing the responsibilities of the light client implementer.docs/docs/05-migrations/13-v8-to-v9.md (1)
- 58-79: The migration guide from v8 to v9 is clear and detailed, providing valuable information for developers. To improve readability, consider adding spaces between sentences where needed and reviewing the document for any spelling mistakes.
docs/docs/03-light-clients/01-developer-guide/01-overview.md (4)
- 27-33: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The document's metadata (front matter) is correctly formatted, ensuring proper rendering on the website. However, ensure that the
slugaccurately reflects the document's location and purpose within the site's structure.
- 30-33: The terms
ClientState,ConsensusState, andClientMessageare correctly introduced and linked to their respective sections. However, ensure that the spelling of "misbehaviour" aligns with the project's preferred English variant (British vs. American English).- 41-41: Ensure that the link to the
LightClientModuleinterface is correct and accessible. This is crucial for developers who may want to reference the interface directly.- 95-95: The reference to the "Handling update and misbehaviour" section is correctly linked. Ensure that the document exists and is accessible to provide comprehensive guidance on updates and misbehaviour handling.
Verification successful
Given the context and the need to accurately verify the existence and accessibility of the document "05-updates-and-misbehaviour.md" within the local repository, a more suitable approach is required. The initial use of
curlwas not appropriate for this context. Let's proceed with a revised verification strategy.
The output indicates an error in executing the
statcommand, which affected the verification process. The conclusion that the file does not exist or is empty may not be accurate due to this error. To clarify the existence and content status of "05-updates-and-misbehaviour.md" without relying onstat, a revised approach is necessary.
The verification process has confirmed that the document "05-updates-and-misbehaviour.md" exists within the repository and is not empty. This supports the review comment regarding the correct linking and accessibility of the document for providing comprehensive guidance on updates and misbehaviour handling.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence and accessibility of the linked document. fd "05-updates-and-misbehaviour.md" --exec curl -o /dev/null --silent --head --fail {}Length of output: 88
Script:
#!/bin/bash # Verify the existence of the document and check if it's not empty. fd "05-updates-and-misbehaviour.md" --exec stat {} \; --exec test -s {} && echo "File exists and is not empty" || echo "File does not exist or is empty"Length of output: 185
Script:
#!/bin/bash # Verify the existence of the document and check if it's not empty by counting lines. fd "05-updates-and-misbehaviour.md" --exec wc -l {} \; | awk '{if ($1 > 0) print "File exists and is not empty"; else print "File does not exist or is empty";}'Length of output: 192
modules/light-clients/08-wasm/keeper/msg_server_test.go (2)
- 264-271: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [256-268]
The introduction of the
okvariable to handle the type assertion result forexpClientStateis a good practice, ensuring that the type assertion is successful before proceeding. This enhances the robustness of the test by avoiding potential panics due to failed type assertions.
- 264-271: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [256-268]
Ensure that comprehensive test coverage and proper error handling are maintained throughout the test suite. This includes verifying that all possible error paths are tested and that the tests remain consistent with the changes introduced in this PR.
CHANGELOG.md (1)
- 43-43: The change mentioned here is significant as it impacts the core/02-client and light-clients in the codebase. It's crucial to ensure that this decoupling does not introduce any regressions or unintended behavior changes in the IBC protocol's operation. It would be beneficial to have comprehensive testing around this change to validate its correctness and performance implications.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- e2e/tests/transfer/localhost_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- e2e/tests/transfer/localhost_test.go
| @@ -31,16 +31,22 @@ import ( | |||
| type Keeper struct { | |||
| storeKey storetypes.StoreKey | |||
| cdc codec.BinaryCodec | |||
| router *types.Router | |||
There was a problem hiding this comment.
Feel free to open an issue and add needs discussion if you'd like!
| // Deprecated: This function will be removed in a later release of ibc-go. | ||
| // NOTE: This function only supports querying latest consensus state of 07-tendermint client state implementations. |
There was a problem hiding this comment.
To be removed in #5991
| wasm.NewAppModule(app.WasmClientKeeper), | ||
| ibctm.AppModuleBasic{}, | ||
| solomachine.AppModuleBasic{}, | ||
| wasm.NewAppModule(app.WasmClientKeeper), // TODO(damian): see if we want to pass the lightclient module here, keeper is used in AppModule.RegisterServices etc |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (4)
- modules/core/02-client/keeper/events.go (3 hunks)
- modules/core/02-client/types/router.go (1 hunks)
- modules/core/02-client/types/store.go (1 hunks)
- modules/core/04-channel/types/expected_keepers.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- modules/core/02-client/keeper/events.go
- modules/core/02-client/types/router.go
- modules/core/02-client/types/store.go
- modules/core/04-channel/types/expected_keepers.go
|
|
Merged, feel free to leave post-merge reviews. We can follow up by opening issues and follow up PRs. |
Description
@colin-axner and I spent the week working through this proof-of-concept to attempt to decouple 02-client routing from encoding. This PR is the initial effort to that story, and feeds into #5084.
Below is a list of items that we would like to see completed before merging this to main of ibc-go, or even taking this PR out draft.
This PR handles the 02-client routing story within ibc-core, however does not address secondary level routing in 08-wasm yet - the final point of the list below is to handle that with "v2 routing". This likely means changes to the
Initialize()method of theLightClientModuleinterface in order to plumb secondary level routing information through.While discussing the initial design of the new interfaces added within this PR, @colin-axner and I felt it was a good opportunity to also look at the idea of a new
apigo module, separate to ibc-go. This has not been handled yet but conceptually this means moving interfaces for light client modules as well as ibc applications to a newapimodule.The goal with this would be reduce the need for clients and apps to stay in lock-step with ibc-go versioning, essentially closing #2471.
clientStateAPI should be updated to use client module (Move client state testing to light client module entry points #6001)closes: #5565
closes: #5942
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/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.Summary by CodeRabbit
ClientStateinterface, and documentation onClientStateandConsensusState.Routertype in the IBC module for mapping client types toLightClientModuleinstances.solomachine,tendermint,wasm, andlocalhostpackages.CHANGELOG.mdto reflect changes in light client routing and encoding structure.