Additional unit tests for Client State #1873
Conversation
Wizdave97
requested review from
adizere,
ancazamfir and
romac
as code owners
|
One more thing actually: it would be good to add a changelog entry. Documentation on how to do that is
It may be a good idea to read our contributing doc in its entirety. Thank you @Wizdave97! |
|
|
||
| #[derive(Clone, Debug, PartialEq)] | ||
| struct ClientStateParams { | ||
| id: ChainId, |
There was a problem hiding this comment.
Unrelated to this issue, but I see that ibc-go restricts 07-tendermint ChainIds to tendermint::MaxChainID -> cosmos/ibc-go#177
@adizere Should we add this check to our ClientState::new ctor as well?
There was a problem hiding this comment.
Good idea, I think we should! Seems minor so we can do it in almost any module-related PR. I will capture the missing piece in a separate issue.
| trusting_period: Duration, | ||
| unbonding_period: Duration, | ||
| max_clock_drift: Duration, | ||
| latest_height: Height, |
There was a problem hiding this comment.
We're also not checking if the latest_height's revision_number matches the ChainId's version.
There was a problem hiding this comment.
I took the liberty to document all the missing checks in a separate issue: #1893
| trust_level: TrustThreshold, | ||
| trusting_period: Duration, | ||
| unbonding_period: Duration, | ||
| max_clock_drift: Duration, |
There was a problem hiding this comment.
ibc-go doesn't allow a zero max_clock_drift. Not sure why this would be required. 🤔
| max_clock_drift: Duration, | ||
| latest_height: Height, | ||
| proof_specs: ProofSpecs, | ||
| upgrade_path: Vec<String>, |
There was a problem hiding this comment.
Apparently, each key in upgrade_path must be non-empty and this must also be checked.
|
Nice findings @hu55a1n1. Documented them in cosmos/ibc-rs#22. Again -- great work David! |
* add more unit tests for client state * minor fixes * update error string formatting * add changelog
Partial fix for: https://github.com/informalsystems/ibc-rs/issues/1706
Description
PR author checklist:
unclog.docs/).Reviewer checklist:
Files changedin the GitHub PR explorer.