Change ChannelId representation to String #2361
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ICS 024 does not restrict channel IDs to the "channel-{N}" format.
mzabaluev
requested review from
adizere,
romac,
seanchen1991,
ancazamfir,
soareschen,
hu55a1n1 and
plafer
as code owners
- Create valid IDs with ChannelId::new (could be under valid length). - Format as the inner string with Display. - Derive Debug, no need for a manual definition, which printed it wrong.
Contrary to what is still documented in ICS 024, the minimum length accepted by ibc-go is 8 characters: https://github.com/cosmos/ibc-go/blob/e04964912c266bab923253c48d72cc8ec8b38f5e/modules/core/24-host/validate.go#L76-L81
The length limit is now 64 characters in accordance with ICS 024 and ibc-go, but longer than previous code admitted.
- File under the modules component. - Add an entry to bug-fixes mentioning the corrected enforcement of the length limit on channel IDs.
romac
reviewed
Jul 4, 2022
| /// A valid Identifier must be between 10-64 characters and only contain lowercase | ||
| /// alphabetic characters, | ||
| pub fn validate_channel_identifier(id: &str) -> Result<(), Error> { | ||
| validate_identifier(id, 8, 64) |
There was a problem hiding this comment.
There is discrepancy between the spec and the code here. The spec says channel ids must be 10 characters long or more but that would mean that channel-0 is not a valid channel id. We should clarify that.
There was a problem hiding this comment.
Clarified offline, the spec is wrong. Let's update the comment above then.
romac
requested changes
Jul 4, 2022
romac
reviewed
Jul 4, 2022
romac
approved these changes
Jul 4, 2022
hu55a1n1
pushed a commit
to hu55a1n1/hermes
that referenced
this pull request
Sep 13, 2022
ICS 024 does not restrict channel IDs to the "channel-{N}" format.
Change the internal representation of ChannelId to String, and modify the API accordingly to the similar ID types.
---
* Change ChannelId representation to String
ICS 024 does not restrict channel IDs to the "channel-{N}" format.
* More clone calls for ChannelId, clippy fixes
* fmt fix
* Changelog entry for informalsystems#2361
* ChannelId formatting fixes
- Create valid IDs with ChannelId::new (could be under valid length).
- Format as the inner string with Display.
- Derive Debug, no need for a manual definition, which printed it wrong.
* Relax the channel identifier valid length
Contrary to what is still documented in ICS 024,
the minimum length accepted by ibc-go is 8 characters:
https://github.com/cosmos/ibc-go/blob/e04964912c266bab923253c48d72cc8ec8b38f5e/modules/core/24-host/validate.go#L76-L81
* Add unit tests for validate_channel_identifier
* Tweak test data for excessively long channel IDs
The length limit is now 64 characters in accordance with ICS 024
and ibc-go, but longer than previous code admitted.
* Improve changelog entries for informalsystems#2361
- File under the modules component.
- Add an entry to bug-fixes mentioning the corrected enforcement
of the length limit on channel IDs.
* Fix outdated comment
* Clarify comment
Co-authored-by: Romain Ruetschi <romain@informal.systems>
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: cosmos/ibc-rs#39
Description
ICS 024 does not restrict channel IDs to the "channel-{N}" format.
Change the internal representation of
ChannelIdtoString, and modify the API accordingly to the similar ID types.PR author checklist:
unclog.docs/).Reviewer checklist:
Files changedin the GitHub PR explorer.