Model ChannelId as newtype u64 and improve validation
#2071
Conversation
|
Any idea why this test only fails in this branch (because of #2075) and was not caught before? |
@romac and I figured out that this was because the original test used the |
| @@ -256,7 +256,7 @@ impl From<Attributes> for Vec<Tag> { | |||
| if let Some(channel_id) = a.channel_id { | |||
| let channel_id = Tag { | |||
| key: CHANNEL_ID_ATTRIBUTE_KEY.parse().unwrap(), | |||
| value: channel_id.as_str().parse().unwrap(), | |||
There was a problem hiding this comment.
.to_string().parse() looks like an unnecessary string conversion roundtrip. But an ABCI tag value is a UTF-8 string super-encoded as base64 for an inexplicable reason. Go figure.
There was a problem hiding this comment.
Right, it seems unnecessary. But I don't think there's any other public constructor for Tag::Value. 🤔 So not sure how to fix this. IIUC, the tag value was originally designed to be opaque bytes and later it was decided that (event) indexing was required and we're now left with this base64 encoded string.
There was a problem hiding this comment.
Nothing to fix here, maybe just add a comment?
There was a problem hiding this comment.
Just noticed that we already have this comment above -> https://github.com/informalsystems/ibc-rs/pull/2071/files/91c4c04dd191753eeac90159d2912262dfee17e0#diff-5d54d6bafcc46a42f95443d7c324b7c79563ad1f089618a686a8eb0d54e8beb0L237-L242
/// # Note
/// The parsing of `Key`s and `Value`s never fails, because the
/// `FromStr` instance of `tendermint::abci::tag::{Key, Value}`
/// is infallible, even if it is not represented in the error type.
/// Once tendermint-rs improves the API of the `Key` and `Value` types,
/// we will be able to remove the `.parse().unwrap()` calls.There was a problem hiding this comment.
Yes, nothing to fix there, it's just an observation (maybe we should have an etiquette about this 😊)
| @@ -22,7 +22,7 @@ pub(crate) fn process( | |||
| // Unwrap the old channel end (if any) and validate it against the message. | |||
| let (mut new_channel_end, channel_id) = match &msg.previous_channel_id { | |||
There was a problem hiding this comment.
Now that ChannelId is Copy, would it be nicer to match it by value and avoid the dereferencing cruft in match arms?
There was a problem hiding this comment.
Good point. Also, we are passing ChannelId by reference (eg. &ChannelId in fn parameters) in a bunch of places where it would be more idiomatic to pass it by value.
There was a problem hiding this comment.
Do we always want to pass ChannelId by value? Halfway through this and the diff is huge, wondering if it's worth it ->
$ git diff --shortstat
49 files changed, 220 insertions(+), 220 deletions(-)
There was a problem hiding this comment.
It's more idiomatic but perhaps not worth it indeed. Let's file it in the backlog :)
There was a problem hiding this comment.
Added this as an entry to this meta issue https://github.com/informalsystems/ibc-rs/issues/1696
Closes: cosmos/ibc-rs#50
PR author checklist:
unclog.docs/).Reviewer checklist:
Files changedin the GitHub PR explorer.