-
Notifications
You must be signed in to change notification settings - Fork 82
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
Fix Pass ChannelId by value instead of reference since it impls Copy #542
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #542 +/- ##
==========================================
- Coverage 72.70% 72.53% -0.17%
==========================================
Files 126 126
Lines 15550 15523 -27
==========================================
- Hits 11306 11260 -46
- Misses 4244 4263 +19
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] | ||
pub struct ChannelId(u64); |
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.
Doesn't this break compatibility with other IBC chains? I don't think the spec requires that the channel ID be a u64
, only that it's an Identifier
, which is defined as
An
Identifier
is a bytestring used as a key for an object stored in state, such as a connection, channel, or light client.Identifiers MUST be non-empty (of positive integer length).
Identifiers MUST consist of characters in one of the following categories only:
- Alphanumeric
.
,_
,+
,-
,#
[
,]
,<
,>
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.
yes indeed, I did some digging into the history of this issue. It was created when ChannelId
was a u64
. However we changed it to String
in this PR, for exactly these reasons. I will close this PR and the issue as "won't do".
Thank you @DaviRain-Su for ultimately bringing this to light; sorry for needing to throw away all this code.
See #542 (comment) |
Closes: #537
Description
PR author checklist:
unclog
.docs/
).Reviewer checklist:
Files changed
in the GitHub PR explorer.