imp: pass client store provider as arg to client modules #6028
Conversation
WalkthroughWalkthroughThe primary focus of the changes is on decentralizing the initialization of the Changes
Assessment against linked issues
Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
DimitrisJim
requested review from
colin-axner,
AdityaSripal,
damiannolan,
srdtrk,
chatton,
charleenfei and
crodriguezvega
as code owners
|
dunno why I kept this in draft, opening for review. Can ignore linting issue will fix tomorrow. edit: whoops, seems the rebase messed with me, I'll fix em up. |
![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 (10)
- modules/apps/callbacks/testing/simapp/app.go (1 hunks)
- modules/core/02-client/keeper/keeper.go (1 hunks)
- modules/core/02-client/types/router.go (2 hunks)
- modules/core/exported/client.go (1 hunks)
- modules/light-clients/06-solomachine/light_client_module.go (2 hunks)
- modules/light-clients/07-tendermint/light_client_module.go (2 hunks)
- modules/light-clients/08-wasm/light_client_module.go (2 hunks)
- modules/light-clients/08-wasm/testing/simapp/app.go (1 hunks)
- modules/light-clients/09-localhost/light_client_module.go (1 hunks)
- testing/simapp/app.go (1 hunks)
Additional comments: 19
modules/core/02-client/types/router.go (3)
- 11-11: The removal of the
storeProviderfield from theRouterstruct simplifies the router's structure, aligning with the PR's objectives to streamline the initialization process for light client modules. This change should reduce complexity and potential sources of errors related to uninitialized variables.- 15-17: The
NewRouterfunction has been correctly updated to reflect the removal of thestoreProvider. It now initializes theRouterstruct without requiring aStoreKeyparameter, which simplifies the router initialization process.- 22-22: The
AddRoutemethod's documentation and implementation have been appropriately updated to remove references to thestoreProvider. This change is consistent with the PR's goal of simplifying the router's structure by directly passing keys to light client modules during their initialization.modules/light-clients/09-localhost/light_client_module.go (1)
- 27-29: The
NewLightClientModulefunction has been correctly updated to initialize thestoreProviderfield directly using thestoretypes.StoreKeyparameter. This change aligns with the PR's objectives to streamline the initialization process for light client modules and eliminate uninitialized variables post-initialization.modules/core/exported/client.go (1)
- 54-59: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The removal of the
RegisterStoreProvidermethod from theLightClientModuleinterface simplifies the interface and aligns with the PR's objectives to streamline the initialization process for light client modules. This change should make the interface more straightforward and reduce the burden on light client developers.modules/light-clients/06-solomachine/light_client_module.go (2)
- 5-5: The addition of the
storetypesimport is necessary for the changes made to theNewLightClientModulefunction, allowing the use ofstoretypes.StoreKeyin the function signature. This change is consistent with the PR's objectives to directly pass keys to light client modules during their initialization.- 23-26: The
NewLightClientModulefunction has been correctly updated to include akey storetypes.StoreKeyparameter and initialize thestoreProviderfield directly. This change aligns with the PR's objectives to streamline the initialization process for light client modules and eliminate uninitialized variables post-initialization.modules/light-clients/07-tendermint/light_client_module.go (2)
- 5-5: The addition of the
storetypesimport is necessary for the changes made to theNewLightClientModulefunction, allowing the use ofstoretypes.StoreKeyin the function signature. This change is consistent with the PR's objectives to directly pass keys to light client modules during their initialization.- 25-28: The
NewLightClientModulefunction has been correctly updated to include akey storetypes.StoreKeyparameter and initialize thestoreProviderfield directly. This change aligns with the PR's objectives to streamline the initialization process for light client modules and eliminate uninitialized variables post-initialization.modules/light-clients/08-wasm/light_client_module.go (2)
- 6-6: The addition of the
storetypesimport is necessary for the changes made to theNewLightClientModulefunction, allowing the use ofstoretypes.StoreKeyin the function signature. This change is consistent with the PR's objectives to directly pass keys to light client modules during their initialization.- 25-28: The
NewLightClientModulefunction has been correctly updated to include akey storetypes.StoreKeyparameter and initialize thestoreProviderfield directly. This change aligns with the PR's objectives to streamline the initialization process for light client modules and eliminate uninitialized variables post-initialization.modules/core/02-client/keeper/keeper.go (1)
- 42-42: The
NewKeeperfunction has been modified to initialize therouterwithout passing thekeyparameter. This change aligns with the PR's objective to simplify the router's complexity by directly passing keys to light client modules during their initialization. However, it's crucial to ensure that all references and usages of therouterthroughout the codebase are updated to reflect this new initialization pattern. Additionally, consider adding comments to explain the rationale behind this change for future maintainability.Ensure that all usages of the
routerand light client modules are consistent with this new initialization approach.testing/simapp/app.go (2)
- 566-566: The initialization of the
tmLightClientModulenow includes passingkeys[ibcexported.StoreKey]directly. This change aligns with the PR objectives and simplifies the initialization process by ensuring that the light client module has direct access to its required store key. Ensure that thekeysmap is correctly populated withibcexported.StoreKeybefore this point in the code.- 569-569: Similarly, the
smLightClientModuleinitialization now directly receiveskeys[ibcexported.StoreKey]. This modification is consistent with the PR's goal of streamlining the initialization process for light client modules. As with the previous comment, verify that thekeysmap contains theibcexported.StoreKeyat this stage.modules/apps/callbacks/testing/simapp/app.go (2)
- 574-574: The addition of
keys[ibcexported.StoreKey]as a parameter to theibctm.NewLightClientModulefunction aligns with the PR's objective to pass keys directly to light client modules during their initialization. This change simplifies the initialization process and eliminates the need for uninitialized variables post-initialization.- 577-577: Similarly, the addition of
keys[ibcexported.StoreKey]as a parameter to thesolomachine.NewLightClientModulefunction is consistent with the PR's objectives and enhances the initialization process for the solomachine light client module.modules/light-clients/08-wasm/testing/simapp/app.go (3)
- 608-608: The initialization of the Tendermint light client module (
tmLightClientModule) now includes passingkeys[ibcexported.StoreKey]directly. This change aligns with the PR objectives and simplifies the initialization process by directly providing necessary keys.- 611-611: The initialization of the Solo Machine light client module (
smLightClientModule) has been updated to includekeys[ibcexported.StoreKey]as a parameter. This modification is consistent with the PR's goal of streamlining the initialization process for light client modules.- 614-614: The initialization of the Wasm light client module (
wasmLightClientModule) now passeskeys[ibcexported.StoreKey]directly to theNewLightClientModulefunction. This change is in line with the PR's objectives to simplify the router's complexity and improve the initialization process.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (7)
- modules/apps/callbacks/testing/simapp/app.go (1 hunks)
- modules/core/02-client/keeper/keeper.go (1 hunks)
- modules/core/02-client/types/router_test.go (2 hunks)
- modules/light-clients/06-solomachine/light_client_module.go (2 hunks)
- modules/light-clients/08-wasm/light_client_module.go (2 hunks)
- modules/light-clients/08-wasm/testing/simapp/app.go (1 hunks)
- testing/simapp/app.go (1 hunks)
Additional Context Used
Path-based Instructions (7)
modules/core/02-client/types/router_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/light-clients/06-solomachine/light_client_module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/light-clients/08-wasm/light_client_module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/core/02-client/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.testing/simapp/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/callbacks/testing/simapp/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/light-clients/08-wasm/testing/simapp/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (14)
modules/core/02-client/types/router_test.go (2)
62-63: The modifications to theNewLightClientModuleandNewRouterfunction calls in the test align with the PR objectives and correctly reflect the changes made to these functions.
128-129: The adjustments to theNewLightClientModuleandNewRouterfunction calls in this test are consistent with the PR's objectives, accurately reflecting the changes made to these functions.modules/light-clients/06-solomachine/light_client_module.go (2)
25-28: The addition of thekeyparameter to theNewLightClientModulefunction and its use to initialize thestoreProviderfield align with the PR objectives, simplifying the initialization process for the light client module.
25-28: The removal of theRegisterStoreProvidermethod is justified by the direct initialization of thestoreProviderwithin theNewLightClientModulefunction, streamlining the initialization process.modules/light-clients/08-wasm/light_client_module.go (2)
25-28: The addition of thekeyparameter to theNewLightClientModulefunction and its use to initialize thestoreProviderfield align with the PR objectives, simplifying the initialization process for the light client module.
25-28: The removal of theRegisterStoreProvidermethod is justified by the direct initialization of thestoreProviderwithin theNewLightClientModulefunction, streamlining the initialization process.modules/core/02-client/keeper/keeper.go (1)
37-37: The modification to theNewKeeperfunction, creating arouterwithout passing thekeyparameter totypes.NewRouter(), aligns with the PR objectives and correctly reflects the changes made to simplify the router's structure.testing/simapp/app.go (2)
566-566: This change correctly initializes the Tendermint light client module with the necessary store key, aligning with the PR's objectives.
569-569: This change correctly initializes the Solo Machine light client module with the necessary store key, aligning with the PR's objectives.modules/apps/callbacks/testing/simapp/app.go (2)
574-574: Ensure thekeys[ibcexported.StoreKey]parameter is correctly passed and utilized within theibctm.NewLightClientModulefunction.Verification successful
The
NewLightClientModulefunction within theibctmmodule correctly utilizes the passedStoreKeyparameter. This is evident from the function definition and its usage:func NewLightClientModule(key storetypes.StoreKey, cdc codec.BinaryCodec, authority string) LightClientModule { return LightClientModule{ keeper: keeper.NewKeeper(cdc, authority), storeProvider: clienttypes.NewStoreProvider(key), } }The
StoreKeyparameter is used to initialize astoreProviderviaclienttypes.NewStoreProvider(key), indicating that the parameter is correctly passed and utilized within the function.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the `NewLightClientModule` function within the `ibctm` module correctly utilizes the passed `StoreKey`. ast-grep --lang go --pattern $'func NewLightClientModule($_, $_, $_) $_ { $$$ }' --include '*/ibctm/*.go'Length of output: 309
Script:
#!/bin/bash # Verify that the `NewLightClientModule` function within the `ibctm` module correctly utilizes the passed `StoreKey`. ast-grep --lang go --pattern $'func NewLightClientModule($_, $_, $_) $_ { $$$ }'Length of output: 712
577-577: Ensure thekeys[ibcexported.StoreKey]parameter is correctly passed and utilized within thesolomachine.NewLightClientModulefunction.Verification successful
The verification process confirms that the
keys[ibcexported.StoreKey]parameter is correctly passed and utilized within thesolomachine.NewLightClientModulefunction. The parameter is used to initialize thestoreProviderfield of theLightClientModuleinstance, indicating proper utilization within the function.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the `NewLightClientModule` function within the `solomachine` module correctly utilizes the passed `StoreKey`. ast-grep --lang go --pattern $'func NewLightClientModule($_, $_) $_ { $$$ }' --include '*/solomachine/*.go'Length of output: 311
Script:
#!/bin/bash # Verify that the `NewLightClientModule` function within the `solomachine` module correctly utilizes the passed `StoreKey`. ast-grep --lang go --pattern $'func NewLightClientModule($_, $_) $_ { $$$ }'Length of output: 1886
modules/light-clients/08-wasm/testing/simapp/app.go (3)
609-609: Ensure thekeys[ibcexported.StoreKey]parameter is correctly passed and utilized within theNewLightClientModulefunction of the Tendermint light client module.
612-612: Ensure thekeys[ibcexported.StoreKey]parameter is correctly passed and utilized within theNewLightClientModulefunction of the Solo Machine light client module.
615-615: Ensure thekeys[ibcexported.StoreKey]parameter is correctly passed and utilized within theNewLightClientModulefunction of the Wasm light client module.
|
|
| @@ -40,8 +33,6 @@ func (rtr *Router) AddRoute(clientType string, module exported.LightClientModule | |||
| } | |||
|
|
|||
| rtr.routes[clientType] = module | |||
|
|
|||
| module.RegisterStoreProvider(rtr.storeProvider) | |||
There was a problem hiding this comment.
For context: this was the main thing we wanted to remove with this PR!
| // RegisterStoreProvider is called by core IBC when a LightClientModule is added to the router. | ||
| // It allows the LightClientModule to set a ClientStoreProvider which supplies isolated prefix client stores | ||
| // to IBC light client instances. | ||
| RegisterStoreProvider(storeProvider ClientStoreProvider) |
There was a problem hiding this comment.
can't leave review so LGTMing verbally! Thanks for follow up @damiannolan!
|
damiannolan
changed the title
Description
Opening as draft, parent issue requires discussion.
Upsides:
Downsides:
closes: #6009
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
storetypes.StoreKeyacross various light client modules for simplified initialization.LightClientModuleinterface by removing theRegisterStoreProvidermethod.NewSimAppfunction with additional parameters.