feat: adding ConsensusHost interface for custom self client/consensus state validation
#6055
Conversation
…s state validation
WalkthroughWalkthroughThe changes introduce a 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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -60,13 +60,13 @@ func NewKeeper( | |||
| } | |||
|
|
|||
| clientKeeper := clientkeeper.NewKeeper(cdc, key, paramSpace, stakingKeeper, upgradeKeeper) | |||
There was a problem hiding this comment.
Ideally, I think we should be replacing the stakingKeeper arg to core ibc's NewKeeper with a types.ConsensusHost and have users pipe in whatever consensus host they are using explicitly in app.go.
E.g. ibctm.NewConsensusHost(app.StakingKeeper) in app.go as an arg to ibc.NewKeeper(...)
thoughts?
| ClientKeeper clientkeeper.Keeper | ||
| ClientKeeper *clientkeeper.Keeper | ||
| ConnectionKeeper connectionkeeper.Keeper | ||
| ChannelKeeper channelkeeper.Keeper | ||
| PortKeeper *portkeeper.Keeper |
There was a problem hiding this comment.
@chatton and I have been discussing the mismatch of pointer and value types used here for a while, I think others are also aware of it - this was an issue with the PortKeeper below and caused problems with the router in the past.
@chatton volunteered to open an issue about this for discussion. I would be in favour of having consistency here, which would minimise the amount of debugging when things aren't quite lining up with what we expect to happen.
I think its probably best to not be dereferencing keepers when passing args all over the place - i.e. *k.ClientKeeper and such
|
|
||
| // ConsensusHost implements the 02-client clienttypes.ConsensusHost interface | ||
| type ConsensusHost struct { | ||
| stakingKeeper clienttypes.StakingKeeper |
There was a problem hiding this comment.
todo: this expected keeper interface for StakingKeeper should be moved to 07-tendermint as well
There was a problem hiding this comment.
redefined in 07-tendermint, should be removed from clienttypes if and when the NewKeeper arg is changed.
see: #6055 (comment)
damiannolan
requested review from
colin-axner,
AdityaSripal,
charleenfei,
chatton,
DimitrisJim,
srdtrk and
crodriguezvega
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: .coderabbit.yml
Files selected for processing (20)
- modules/core/02-client/abci.go (1 hunks)
- modules/core/02-client/keeper/keeper.go (8 hunks)
- modules/core/02-client/keeper/keeper_test.go (5 hunks)
- modules/core/02-client/keeper/migrations.go (1 hunks)
- modules/core/02-client/migrations/v7/genesis_test.go (2 hunks)
- modules/core/02-client/proposal_handler.go (1 hunks)
- modules/core/02-client/types/client.go (2 hunks)
- modules/core/02-client/types/errors.go (1 hunks)
- modules/core/02-client/types/genesis_test.go (1 hunks)
- modules/core/03-connection/keeper/handshake_test.go (2 hunks)
- modules/core/genesis.go (1 hunks)
- modules/core/keeper/keeper.go (2 hunks)
- modules/core/migrations/v7/genesis_test.go (2 hunks)
- modules/light-clients/07-tendermint/consensus_host.go (1 hunks)
- modules/light-clients/07-tendermint/consensus_host_test.go (1 hunks)
- modules/light-clients/08-wasm/testing/simapp/app.go (1 hunks)
- modules/light-clients/08-wasm/types/consensus_host.go (1 hunks)
- testing/mock/consensus_host.go (1 hunks)
- testing/simapp/upgrades.go (1 hunks)
- testing/simapp/upgrades/upgrades.go (1 hunks)
Additional comments: 26
testing/mock/consensus_host.go (1)
- 12-15: Consider adding documentation comments to the
ConsensusHoststruct and its methods. This will improve code readability and maintainability, especially for developers unfamiliar with the mock's intended use.modules/core/02-client/proposal_handler.go (1)
- 18-18: The change to accept a pointer to
keeper.KeeperinNewClientProposalHandleris approved. Ensure all calls toNewClientProposalHandlerthroughout the codebase have been updated to reflect this change.Verification successful
The calls to
NewClientProposalHandleracross the codebase have been updated to passkeeper.Keeperby reference, as required by the change in its signature. This confirms the initial review comment's correctness.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to NewClientProposalHandler and ensure they pass Keeper by reference. ast-grep --lang go --pattern $'NewClientProposalHandler($_)'Length of output: 61
Script:
#!/bin/bash # Use ripgrep to search for calls to NewClientProposalHandler in a broader context. rg 'NewClientProposalHandler' --glob '*.go'Length of output: 716
modules/core/02-client/keeper/migrations.go (1)
- 12-16: The change to use a pointer for
Keeperin theMigratorstruct andNewMigratorfunction is approved. Ensure all uses of theMigratorstruct throughout the codebase have been updated to reflect this change.modules/core/02-client/abci.go (1)
- 12-12: The change to accept a pointer to
keeper.KeeperinBeginBlockeris approved. Ensure all calls toBeginBlockerthroughout the codebase have been updated to reflect this change.modules/light-clients/08-wasm/types/consensus_host.go (1)
- 14-17: Consider adding documentation comments to the
WasmConsensusHoststruct and its methods. This will improve code readability and maintainability, especially for developers unfamiliar with the struct's intended use.testing/simapp/upgrades.go (1)
- 53-53: The change to dereference
ClientKeeperwhen callingCreateV7LocalhostUpgradeHandleris approved. Ensure all related upgrade handler setups throughout the codebase have been updated to reflect this change.Verification successful
Given the lack of output from the initial script, it's necessary to conduct a broader search to ensure all instances of
CreateV7LocalhostUpgradeHandlerare identified, regardless of the number of arguments. This will help verify the consistency of the change across the codebase.
The attempts to find other instances of
CreateV7LocalhostUpgradeHandlercalls in the codebase did not yield any results. This suggests that the change to dereferenceClientKeeperwhen callingCreateV7LocalhostUpgradeHandler, as observed in the provided context, is consistent and likely unique or not widely used within the codebase. Ensure to manually verify this aspect if the broader context of the codebase changes in the future.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to CreateV7LocalhostUpgradeHandler and ensure they pass ClientKeeper by reference. ast-grep --lang go --pattern $'CreateV7LocalhostUpgradeHandler($_, $_, $_)'Length of output: 76
Script:
#!/bin/bash # Broaden the search for any calls to CreateV7LocalhostUpgradeHandler to capture all instances. ast-grep --lang go --pattern $'CreateV7LocalhostUpgradeHandler($$$)'Length of output: 69
modules/core/keeper/keeper.go (2)
- 31-31: The changes to use pointers for
ClientKeeperandPortKeeperin theKeeperstruct are approved. Ensure all uses of theKeeperstruct throughout the codebase have been updated to reflect these changes.Verification successful
Given the lack of output from the script, it's necessary to take a broader approach to verify the changes made to the
Keeperstruct, specifically the use of pointers forClientKeeperandPortKeeper. To ensure thorough verification, a new script will be generated to search for any instantiation of theKeeperstruct and examine the initialization ofClientKeeperandPortKeeperwithin those instances.
The output from the script provides a comprehensive list of
NewKeeperfunction calls across various modules and tests within the codebase. However, our primary focus is on themodules/core/keeper/keeper.gofile, where theNewKeeperfunction relevant to our review comment is defined. The output confirms the instantiation ofclientKeeper,connectionKeeper,portKeeper, andchannelKeeperwithin this file, withclientKeeperandportKeeperbeing explicitly instantiated as pointers, as per the changes reviewed.This evidence supports the conclusion that the changes to use pointers for
ClientKeeperandPortKeeperin theKeeperstruct have been correctly implemented in themodules/core/keeper/keeper.gofile. Given the specific nature of the review comment and the evidence provided by the script output, it's reasonable to conclude that the changes have been verified within the scope of the provided context.* 63-65: The changes in the `NewKeeper` function to initialize `ClientKeeper` and `PortKeeper` as pointers are approved. Ensure all calls to `NewKeeper` throughout the codebase have been updated to reflect these changes.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for instantiations of Keeper and ensure they use pointers for ClientKeeper and PortKeeper. ast-grep --lang go --pattern $'NewKeeper($_, $_, $_, $_, $_, $_, $_)'Length of output: 70
Script:
#!/bin/bash # Broaden the search to find any instantiation of the Keeper struct and check the initialization of ClientKeeper and PortKeeper. rg --type go 'NewKeeper'Length of output: 17291
Verification successful
Given the lack of output from the initial script, it's necessary to broaden the search to ensure all calls to
NewKeeperare captured, regardless of their specific arguments. Let's use a more general pattern withast-grepand also employrgto search for the function name across all Go files.
The output from the scripts provides a comprehensive list of
NewKeeperfunction calls across various modules and packages within the codebase. However, it does not specifically highlight calls to theNewKeeperfunction within the context of the initial review comment, which pertains to themodules/core/keeper/keeper.gofile and the initialization ofClientKeeper,ConnectionKeeper,PortKeeper, andChannelKeeper.Given the broad search results, it's clear that the
NewKeeperfunction is widely used across different contexts, but the specific changes related to pointer initialization in themodules/core/keeper/keeper.gofile are not directly addressed by this output. The relevant lines from themodules/core/keeper/keeper.gofile are indeed present in the output, confirming the changes made to the initialization of the keepers as pointers. This aligns with the initial review comment's approval of these changes.Since the output confirms the presence of the specific changes in the
modules/core/keeper/keeper.gofile and does not indicate any issues with the wayNewKeeperis called in relation to the changes made, it supports the conclusion that the changes are correctly implemented in the context of the review comment.Based on this analysis, the changes to the
NewKeeperfunction initialization in themodules/core/keeper/keeper.gofile are verified to be correctly implemented as per the review comment's approval.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to NewKeeper and ensure they initialize ClientKeeper and PortKeeper as pointers. ast-grep --lang go --pattern $'NewKeeper($_, $_, $_, $_, $_, $_, $_)'Length of output: 70
Script:
#!/bin/bash # Use a more general pattern with ast-grep to find calls to NewKeeper ast-grep --lang go --pattern $'NewKeeper($$$)' # Use rg to search for the function name across all Go files to ensure no calls are missed rg 'NewKeeper' --type goLength of output: 17339
modules/core/02-client/types/errors.go (1)
- 40-40: The addition of
ErrClientTypeNotSupportedwith a unique error code and descriptive message is appropriate for handling unsupported client types. This aligns well with the PR's objectives to enhance flexibility in client and consensus state validation.modules/core/02-client/types/client.go (1)
- 25-29: The introduction of the
ConsensusHostinterface with methods for consensus state validation is a significant enhancement. It provides a clear and flexible contract for implementing custom validation logic, supporting the PR's goal of extending IBC's compatibility with various consensus mechanisms.testing/simapp/upgrades/upgrades.go (1)
- 82-82: Passing
clientKeeperas a pointer in thePruneExpiredConsensusStatesfunction call is correctly aligned with the updated function signature. This ensures the function operates as intended with the providedclientKeeper.modules/core/02-client/migrations/v7/genesis_test.go (2)
- 36-36: Dereferencing
ClientKeeperin theExportGenesisfunction call is correctly done to match the updated function signature. This ensures the function is called with the appropriate argument type.- 111-111: Dereferencing
ClientKeeperin theExportGenesisfunction call is correctly done to match the updated function signature. This ensures the function is called with the appropriate argument type.modules/light-clients/07-tendermint/consensus_host.go (1)
- 24-39: The introduction of the
ConsensusHoststruct andStakingKeeperinterface for the Tendermint light client, along with the implementation of theConsensusHostinterface methods, is well-executed. These additions provide the necessary functionality for custom validation logic in the Tendermint light client, aligning with the PR's objectives.modules/core/migrations/v7/genesis_test.go (2)
- 64-64: Dereferencing
ClientKeeperin theExportGenesisfunction call is correctly done to match the updated function signature. This ensures the function is called with the appropriate argument type.- 138-138: Dereferencing
ClientKeeperin theExportGenesisfunction call is correctly done to match the updated function signature. This ensures the function is called with the appropriate argument type.modules/light-clients/07-tendermint/consensus_host_test.go (2)
- 16-105: The tests for the
GetSelfConsensusStatemethod of theConsensusHoststruct are comprehensive, covering various scenarios including zero height, height greater than the latest height, and custom consensus host logic. This thorough testing ensures the method's functionality is correctly validated.- 107-241: The tests for the
ValidateSelfClientmethod of theConsensusHoststruct are comprehensive, covering success cases, error cases such as frozen client and incorrect chainID, and custom self validator logic. This thorough testing ensures the method's functionality is correctly validated.modules/core/02-client/types/genesis_test.go (1)
- 35-35: The change to dereference
suite.chainA.App.GetIBCKeeper().ClientKeeperwhen passing it toExportGenesisseems appropriate, assuming theExportGenesisfunction's signature now expects aClientKeeperobject directly instead of a pointer. Please ensure this change aligns with the updated function signature.modules/core/02-client/keeper/keeper.go (3)
- 30-30: The addition of the
consensusHostfield to theKeeperstruct is a significant change. It's crucial to ensure that this new field is properly documented to explain its purpose and how it's used within theKeepermethods.Consider adding a comment above the
consensusHostfield in theKeeperstruct to explain its role, especially how it enables custom validation logic for different consensus mechanisms.
- 315-315: The use of
k.consensusHost.GetSelfConsensusStatewithinGetSelfConsensusStatemethod is a good implementation of theConsensusHostinterface. However, ensure that there's a fallback or error handling in caseconsensusHostisnil, which could happen if it's not set explicitly after the keeper is initialized.Verification successful
The analysis of the provided script output and the surrounding context in
handshake.gosuggests that while there are no explicit nil checks forconsensusHostimmediately before the calls toGetSelfConsensusState, the architectural design and initialization patterns of the application likely ensure thatclientKeeperand its components, includingconsensusHost, are non-nil when these methods are called. This is inferred from the operations performed onclientKeeperbefore the calls toGetSelfConsensusState, which would not be possible ifclientKeeperwere nil. Therefore, the original review comment's concern about ensuring a fallback or error handling for a nilconsensusHostmay be mitigated by the application's design and initialization process.* 323-323: The `ValidateSelfClient` method correctly delegates the validation logic to the `consensusHost`, aligning with the PR's objective to support diverse consensus mechanisms. This approach enhances modularity and extensibility.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all code paths that call GetSelfConsensusState ensure consensusHost is not nil. grep -rn "GetSelfConsensusState" . --include=*.go | grep -v "keeper.go"Length of output: 2752
Script:
#!/bin/bash # Examine the relevant sections in handshake.go for nil checking or architectural guarantees regarding consensusHost grep -A 10 -B 10 "GetSelfConsensusState" ./modules/core/03-connection/keeper/handshake.goLength of output: 2104
modules/core/03-connection/keeper/handshake_test.go (1)
- 222-236: The addition of the test case "override self consensus host" is a valuable contribution to ensuring the new
ConsensusHostfunctionality behaves as expected when a custom validation logic is provided. This test case effectively simulates a scenario where the self consensus host's validation logic fails, which is crucial for testing the flexibility and extensibility of the IBC connection handshake process.modules/core/02-client/keeper/keeper_test.go (2)
- 47-47: Consolidating variable declarations for
testClientHeightimproves readability and maintainability by grouping related declarations together. This change aligns with best practices for code organization.- 84-84: Updating the assignment of
suite.keeperin theSetupTestfunction to use theapp.IBCKeeper.ClientKeeperdirectly is a good practice. It ensures that the test suite uses the same keeper instance as the application, which can help avoid inconsistencies and makes the test setup more straightforward.modules/light-clients/08-wasm/testing/simapp/app.go (1)
- 421-421: The addition of a blank line in the
NewSimAppfunction is a minor formatting change and does not impact the functionality of the code. However, it's essential to maintain consistent formatting throughout the codebase for readability and maintainability.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- modules/light-clients/08-wasm/types/consensus_host.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/light-clients/08-wasm/types/consensus_host.go
There was a problem hiding this comment.
Thanks for pulling out these changes from the feat branch, @damiannolan and @chatton. I will review more thoroughly later today, but the changes are looking good to me.
There are theoretically several API breaking changes, but I wonder if we need to document all of them, since I am not sure the changes are in functions that users will use? Like for example the change in BeginBlocker or in the client keeper NewMigrator function... What do you guys think? And do you think it will be manageable to revert these API breaking changes when back porting to v8.2?
I can also start working on adding docs for this and open a separate PR for it.
We should also add changelog for this PR. :)
Pasting what I wrote in dms for documenting purposes: I’ll try to refresh my head a bit around the wasm client state wrapper a bit more, but looking briefly - its because the wrapped type (envelope) still contains the crucial part of info we need for instantiating the contract - the checksum. And currently, even with the changes we’ve made for client routing, we don’t have a way to plumb through this “secondary level routing info” through the client APIs. At the moment, contracts are actually prevented from storing whatever they want due to this function (so this would need to be modified): https://github.com/cosmos/ibc-go/blob/main/modules/light-clients/08-wasm/types/vm.go#L235-L263 |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (1)
- modules/light-clients/07-tendermint/consensus_host_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/light-clients/07-tendermint/consensus_host_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (1)
- modules/light-clients/08-wasm/types/consensus_host_test.go (1 hunks)
Additional Context Used
GitHub Check Runs (1)
lint failure (5)
modules/light-clients/08-wasm/types/consensus_host_test.go: [failure] 5-5:
File is notgci-ed with --skip-generated -s standard -s default -s blank -s dot -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) -s prefix(github.com/cometbft/cometbft) -s prefix(github.com/cosmos/ibc-go) --custom-order (gci)
modules/light-clients/08-wasm/types/consensus_host_test.go: [failure] 15-15:
File is notgofumpt-ed (gofumpt)
modules/light-clients/08-wasm/types/consensus_host_test.go: [warning] 14-14:
unused-receiver: method receiver 'suite' is not referenced in method's body, consider removing or renaming it as _ (revive)
Path-based Instructions (1)
modules/light-clients/08-wasm/types/consensus_host_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (2)
modules/light-clients/08-wasm/types/consensus_host_test.go (2)
15-15: Format the file withgofumptto adhere to the Golang style guide.
18-97: Ensure comprehensive test coverage for theConsensusHostinterface, particularly focusing on the various failure and success scenarios outlined. It's crucial to validate the behavior of the interface under different conditions to ensure robustness and reliability of the IBC protocol enhancements.Additionally, consider adding more detailed test cases that cover edge cases and more complex scenarios, especially those involving interactions with the WASM clients and the Tendermint consensus mechanism. This will help in ensuring that the
ConsensusHostinterface works as expected across a wide range of situations and client types.
|
|
||
| import ( | ||
| "github.com/cosmos/cosmos-sdk/codec" | ||
| sdk "github.com/cosmos/cosmos-sdk/types" |
There was a problem hiding this comment.
Apply the recommended import order to conform with the Golang style guide.
import (
+ "github.com/cosmos/cosmos-sdk/codec"
+ sdk "github.com/cosmos/cosmos-sdk/types"
+ wasmtesting "github.com/cosmos/ibc-go/modules/light-clients/08-wasm/testing"
+ "github.com/cosmos/ibc-go/modules/light-clients/08-wasm/types"
+ clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
+ "github.com/cosmos/ibc-go/v8/modules/core/exported"
+ ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint"
+ "github.com/cosmos/ibc-go/v8/testing/mock"
- "github.com/cosmos/cosmos-sdk/codec"
- sdk "github.com/cosmos/cosmos-sdk/types"
- wasmtesting "github.com/cosmos/ibc-go/modules/light-clients/08-wasm/testing"
- "github.com/cosmos/ibc-go/modules/light-clients/08-wasm/types"
- clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
- "github.com/cosmos/ibc-go/v8/modules/core/exported"
- ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint"
- "github.com/cosmos/ibc-go/v8/testing/mock"
)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.
| sdk "github.com/cosmos/cosmos-sdk/types" | |
| import ( | |
| "github.com/cosmos/cosmos-sdk/codec" | |
| sdk "github.com/cosmos/cosmos-sdk/types" | |
| wasmtesting "github.com/cosmos/ibc-go/modules/light-clients/08-wasm/testing" | |
| "github.com/cosmos/ibc-go/modules/light-clients/08-wasm/types" | |
| clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" | |
| "github.com/cosmos/ibc-go/v8/modules/core/exported" | |
| ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint" | |
| "github.com/cosmos/ibc-go/v8/testing/mock" | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 4
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (2)
- modules/light-clients/08-wasm/types/consensus_host.go (1 hunks)
- modules/light-clients/08-wasm/types/consensus_host_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/light-clients/08-wasm/types/consensus_host_test.go
Additional Context Used
Path-based Instructions (1)
modules/light-clients/08-wasm/types/consensus_host.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
| // WasmConsensusHost implements the 02-client types.ConsensusHost interface. | ||
| type WasmConsensusHost struct { | ||
| cdc codec.BinaryCodec | ||
| delegate clienttypes.ConsensusHost | ||
| } | ||
|
|
||
| var _ clienttypes.ConsensusHost = (*WasmConsensusHost)(nil) |
There was a problem hiding this comment.
Consider documenting the WasmConsensusHost struct and its methods to improve code readability and maintainability.
+ // WasmConsensusHost wraps a delegate ConsensusHost to provide wasm-specific logic for consensus state and client validation.
type WasmConsensusHost struct {
+ // cdc is the codec used for marshaling and unmarshaling data.
cdc codec.BinaryCodec
+ // delegate is the underlying ConsensusHost that performs the actual consensus state and client validation logic.
delegate clienttypes.ConsensusHost
}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.
| // WasmConsensusHost implements the 02-client types.ConsensusHost interface. | |
| type WasmConsensusHost struct { | |
| cdc codec.BinaryCodec | |
| delegate clienttypes.ConsensusHost | |
| } | |
| var _ clienttypes.ConsensusHost = (*WasmConsensusHost)(nil) | |
| // WasmConsensusHost wraps a delegate ConsensusHost to provide wasm-specific logic for consensus state and client validation. | |
| type WasmConsensusHost struct { | |
| // cdc is the codec used for marshaling and unmarshaling data. | |
| cdc codec.BinaryCodec | |
| // delegate is the underlying ConsensusHost that performs the actual consensus state and client validation logic. | |
| delegate clienttypes.ConsensusHost | |
| } | |
| var _ clienttypes.ConsensusHost = (*WasmConsensusHost)(nil) |
| func NewWasmConsensusHost(cdc codec.BinaryCodec, delegate clienttypes.ConsensusHost) (*WasmConsensusHost, error) { | ||
| if cdc == nil { | ||
| return nil, fmt.Errorf("wasm consensus host codec is nil") | ||
| } | ||
|
|
||
| if delegate == nil { | ||
| return nil, fmt.Errorf("wasm delegate consensus host is nil") | ||
| } | ||
|
|
||
| return &WasmConsensusHost{ | ||
| cdc: cdc, | ||
| delegate: delegate, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Ensure error messages are consistent and informative. Consider including more context in the error messages for NewWasmConsensusHost.
- return nil, fmt.Errorf("wasm consensus host codec is nil")
+ return nil, fmt.Errorf("cannot create WasmConsensusHost: codec is nil")
- return nil, fmt.Errorf("wasm delegate consensus host is nil")
+ return nil, fmt.Errorf("cannot create WasmConsensusHost: delegate consensus host is nil")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.
| func NewWasmConsensusHost(cdc codec.BinaryCodec, delegate clienttypes.ConsensusHost) (*WasmConsensusHost, error) { | |
| if cdc == nil { | |
| return nil, fmt.Errorf("wasm consensus host codec is nil") | |
| } | |
| if delegate == nil { | |
| return nil, fmt.Errorf("wasm delegate consensus host is nil") | |
| } | |
| return &WasmConsensusHost{ | |
| cdc: cdc, | |
| delegate: delegate, | |
| }, nil | |
| } | |
| func NewWasmConsensusHost(cdc codec.BinaryCodec, delegate clienttypes.ConsensusHost) (*WasmConsensusHost, error) { | |
| if cdc == nil { | |
| return nil, fmt.Errorf("cannot create WasmConsensusHost: codec is nil") | |
| } | |
| if delegate == nil { | |
| return nil, fmt.Errorf("cannot create WasmConsensusHost: delegate consensus host is nil") | |
| } | |
| return &WasmConsensusHost{ | |
| cdc: cdc, | |
| delegate: delegate, | |
| }, nil | |
| } |
| // ValidateSelfClient implements the 02-client types.ConsensusHost interface. | ||
| func (w *WasmConsensusHost) ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error { | ||
| if w.cdc == nil { | ||
| return errorsmod.Wrapf(clienttypes.ErrInvalidClient, "wasm consensus host codec is nil") | ||
| } | ||
|
|
||
| if w.delegate == nil { | ||
| return errorsmod.Wrapf(clienttypes.ErrInvalidClient, "wasm delegate consensus host is nil") | ||
| } | ||
|
|
||
| wasmClientState, ok := clientState.(*ClientState) | ||
| if !ok { | ||
| return errorsmod.Wrapf(clienttypes.ErrInvalidClient, "client must be a wasm client, expected: %T, got: %T", ClientState{}, wasmClientState) | ||
| } | ||
|
|
||
| if wasmClientState.Data == nil { | ||
| return errorsmod.Wrapf(clienttypes.ErrInvalidClient, "wasm client state data is nil") | ||
| } | ||
|
|
||
| // unmarshal the wasmClientState bytes into the ClientState interface and call self validation | ||
| var unwrappedClientState exported.ClientState | ||
| if err := w.cdc.UnmarshalInterface(wasmClientState.Data, &unwrappedClientState); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return w.delegate.ValidateSelfClient(ctx, unwrappedClientState) | ||
| } |
There was a problem hiding this comment.
Move the nil check for w to the beginning of the ValidateSelfClient method to prevent potential nil pointer dereference errors.
+ if w == nil {
+ return errorsmod.Wrapf(clienttypes.ErrInvalidClient, "wasm consensus host is nil")
+ }Add this check at the start of the method.
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.
| // ValidateSelfClient implements the 02-client types.ConsensusHost interface. | |
| func (w *WasmConsensusHost) ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error { | |
| if w.cdc == nil { | |
| return errorsmod.Wrapf(clienttypes.ErrInvalidClient, "wasm consensus host codec is nil") | |
| } | |
| if w.delegate == nil { | |
| return errorsmod.Wrapf(clienttypes.ErrInvalidClient, "wasm delegate consensus host is nil") | |
| } | |
| wasmClientState, ok := clientState.(*ClientState) | |
| if !ok { | |
| return errorsmod.Wrapf(clienttypes.ErrInvalidClient, "client must be a wasm client, expected: %T, got: %T", ClientState{}, wasmClientState) | |
| } | |
| if wasmClientState.Data == nil { | |
| return errorsmod.Wrapf(clienttypes.ErrInvalidClient, "wasm client state data is nil") | |
| } | |
| // unmarshal the wasmClientState bytes into the ClientState interface and call self validation | |
| var unwrappedClientState exported.ClientState | |
| if err := w.cdc.UnmarshalInterface(wasmClientState.Data, &unwrappedClientState); err != nil { | |
| return err | |
| } | |
| return w.delegate.ValidateSelfClient(ctx, unwrappedClientState) | |
| } | |
| // ValidateSelfClient implements the 02-client types.ConsensusHost interface. | |
| func (w *WasmConsensusHost) ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error { | |
| if w == nil { | |
| return errorsmod.Wrapf(clienttypes.ErrInvalidClient, "wasm consensus host is nil") | |
| } | |
| if w.cdc == nil { | |
| return errorsmod.Wrapf(clienttypes.ErrInvalidClient, "wasm consensus host codec is nil") | |
| } | |
| if w.delegate == nil { | |
| return errorsmod.Wrapf(clienttypes.ErrInvalidClient, "wasm delegate consensus host is nil") | |
| } | |
| wasmClientState, ok := clientState.(*ClientState) | |
| if !ok { | |
| return errorsmod.Wrapf(clienttypes.ErrInvalidClient, "client must be a wasm client, expected: %T, got: %T", ClientState{}, wasmClientState) | |
| } | |
| if wasmClientState.Data == nil { | |
| return errorsmod.Wrapf(clienttypes.ErrInvalidClient, "wasm client state data is nil") | |
| } | |
| // unmarshal the wasmClientState bytes into the ClientState interface and call self validation | |
| var unwrappedClientState exported.ClientState | |
| if err := w.cdc.UnmarshalInterface(wasmClientState.Data, &unwrappedClientState); err != nil { | |
| return err | |
| } | |
| return w.delegate.ValidateSelfClient(ctx, unwrappedClientState) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (1)
- modules/light-clients/08-wasm/testing/simapp/app.go (1 hunks)
Additional Context Used
Path-based Instructions (1)
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 (2)
modules/light-clients/08-wasm/testing/simapp/app.go (2)
421-421: The addition of a blank line after initializingIBCKeeperis a minor stylistic change and is acceptable.
421-421: Ensure that the updated deprecation comment regarding the registration of proposal types is clear and informative, guiding future developers on handling this deprecated functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (3)
- modules/light-clients/07-tendermint/consensus_host_test.go (1 hunks)
- modules/light-clients/08-wasm/types/consensus_host.go (1 hunks)
- modules/light-clients/08-wasm/types/consensus_host_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- modules/light-clients/07-tendermint/consensus_host_test.go
- modules/light-clients/08-wasm/types/consensus_host.go
- modules/light-clients/08-wasm/types/consensus_host_test.go
|
|
Let's add something in migration doc in a follow up. cc @chatton |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional Context Used
Path-based Instructions (1)
CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Additional comments not posted (1)
CHANGELOG.md (1)
56-56: The documentation of the addition of theConsensusHostinterface is clear and concise.
…us state validation (#6055) Co-authored-by: chatton <github.qpeyb@simplelogin.fr> (cherry picked from commit 50d2a08) # Conflicts: # CHANGELOG.md # modules/core/02-client/keeper/keeper.go # modules/core/02-client/keeper/keeper_test.go # modules/core/02-client/types/errors.go # modules/light-clients/08-wasm/testing/simapp/app.go
…us state validation (backport #6055) (#6111) * feat: adding `ConsensusHost` interface for custom self client/consensus state validation (#6055) Co-authored-by: chatton <github.qpeyb@simplelogin.fr> (cherry picked from commit 50d2a08) # Conflicts: # CHANGELOG.md # modules/core/02-client/keeper/keeper.go # modules/core/02-client/keeper/keeper_test.go # modules/core/02-client/types/errors.go # modules/light-clients/08-wasm/testing/simapp/app.go * chore: resolve conflicts and adapt api breaking changes * refactor: rm api breaking changes for consensus host feature * chore: readd staking keeper to reduce diffs * chore: adapt last api breaking changes --------- Co-authored-by: Damian Nolan <damiannolan@gmail.com>
…us state validation (backport #6055) (#6547) * feat: adding `ConsensusHost` interface for custom self client/consensus state validation (#6055) Co-authored-by: chatton <github.qpeyb@simplelogin.fr> (cherry picked from commit 50d2a08) # Conflicts: # CHANGELOG.md # modules/core/02-client/keeper/keeper.go # modules/core/02-client/keeper/keeper_test.go # modules/core/02-client/types/errors.go * restore changes from release/v8.3.x * chore: update changelog --------- Co-authored-by: Damian Nolan <damiannolan@gmail.com>
Description
Co-authored-by: chatton github.qpeyb@simplelogin.fr
Adds the
ConsensusHostinterface type to 02-client as well as implementation for07-tendermintand08-wasm.closes: #5315
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
WasmConsensusHoststruct for wasm-wrapped consensus in the IBC package.