Channel query validation & tests #163
Conversation
modules/src/ics04_channel/channel.rs
Outdated
| .collect::<Result<Vec<_>, _>>() | ||
| .map_err(|e| Kind::IdentifierError.context(e))?; | ||
|
|
||
| // No explicit validation is necessary for the version attribute (opaque field). |
There was a problem hiding this comment.
Then what's the point of it? :)
Is it checked in some other context?
There was a problem hiding this comment.
The full details on the version field are here:
https://github.com/cosmos/ics/tree/master/spec/ics-004-channel-and-packet-semantics#versioning
The version field may be checked in other contexts (e.g., during a handshake, but as far as I can tell, it is not). But the point is that this field may contain fairly arbitrary data, according to the ICS4 spec. Looking through the cosmos-sdk codebase, however, I see that there is some basic validation on this field:
I'm adding a TODO to clarify this.
There was a problem hiding this comment.
It seems that there might be a mistake in cosmos-sdk code. We should permit an empty version string for ChannelEnd, so basically no validation is required here.
There was a problem hiding this comment.
Looks good.
I feel like we should do something about the version check but I don't have enough context yet to understand what that version entails.
Thanks for fixing my comments. :)
The only thing that could get better here is the same advice Ismail gave me: decompose different issues into separate branches/PRs. I think the ics24 fix could have gone into its own PR. But I'm still learning this feat too.
|
Ah, one more thing! If you merge master into your branch, you can get rid of the nightly coverage error. If others are picky about it, it's an easy solution. |
| // Parses the State out from a i32. | ||
| pub fn from_i32(s: i32) -> Result<Self, error::Error> { | ||
| match s { | ||
| 0 => Ok(Self::Uninitialized), |
There was a problem hiding this comment.
Just out of curiosity, is the Uninitialized state an implementation requirement ? Just asking because in the ics4 spec the Channel state only has
enum ChannelState { INIT, OPENTRY, OPEN, CLOSED, }
I know the Golang implementation has the uninitialized state too, so maybe the spec should be updated ?
There was a problem hiding this comment.
I think this is an oversight in the cosmos/ICS repo and should be fixed in ICS4. I pinged Chris about it, will update here with details!
There was a problem hiding this comment.
Opened an issue in cosmos/ibc#452.
…48 #164) * Refactored the location of JSON files for serialization testing (#118). * Validation for channel end + tests template; needs more tests (#148). * Refining tests for channel end. * Aligned identifier validation with ICS024 & updated tests (#164). * Added TODO re: version validation; fix VALID_SPECIAL_CHARS to remove space. * Fixing the TODO for version field validation.
…lsystems#163 #148 #164) * Refactored the location of JSON files for serialization testing (#118). * Validation for channel end + tests template; needs more tests (#148). * Refining tests for channel end. * Aligned identifier validation with ICS024 & updated tests (#164). * Added TODO re: version validation; fix VALID_SPECIAL_CHARS to remove space. * Fixing the TODO for version field validation.
Closes: cosmos/ibc-rs#130
Closes: cosmos/ibc-rs#133
Closes: cosmos/ibc-rs#126
Description
For contributor use:
docs/) and code commentsFiles changedin the Github PR explorer