-
Notifications
You must be signed in to change notification settings - Fork 326
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Channel query validation & tests #163
Changes from 4 commits
e50e552
8f21ae6
b35841a
76061f4
1d2997c
a879caf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,6 +40,18 @@ impl State { | |
Self::Closed => "CLOSED", | ||
} | ||
} | ||
|
||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just out of curiosity, is the Uninitialized state an implementation requirement ? Just asking because in the ics4 spec the Channel state only has
I know the Golang implementation has the uninitialized state too, so maybe the spec should be updated ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Opened an issue in cosmos/ibc#452. |
||
1 => Ok(Self::Init), | ||
2 => Ok(Self::TryOpen), | ||
3 => Ok(Self::Open), | ||
4 => Ok(Self::Closed), | ||
_ => fail!(error::Kind::UnknownState, s), | ||
} | ||
} | ||
} | ||
|
||
impl std::str::FromStr for State { | ||
|
@@ -73,6 +85,16 @@ impl Order { | |
Self::Ordered => "ORDERED", | ||
} | ||
} | ||
|
||
// Parses the Order out from a i32. | ||
pub fn from_i32(nr: i32) -> Result<Self, error::Error> { | ||
match nr { | ||
0 => Ok(Self::None), | ||
1 => Ok(Self::Unordered), | ||
2 => Ok(Self::Ordered), | ||
_ => fail!(error::Kind::UnknownOrderType, nr), | ||
} | ||
} | ||
} | ||
|
||
impl std::str::FromStr for Order { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then what's the point of it? :)
Is it checked in some other context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
https://github.com/cosmos/cosmos-sdk/blob/e6a56226a88ec4e8761b1388a3a666af627d9e2e/x/ibc/04-channel/types/channel.go#L72
I'm adding a TODO to clarify this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
cosmos/cosmos-sdk#6830