Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

node/network/bridge: Define protocol names as str #1655

Merged
2 commits merged into from
Aug 28, 2020
Merged

Conversation

mxinden
Copy link
Contributor

@mxinden mxinden commented Aug 28, 2020

Companion for paritytech/substrate#6967.

Notification protocol names are in practice always valid utf8 strings.
Instead of treating them as such in the type system, thus far they were
casted to a [u8] at creation time.

With this commit protocol names are instead treated as valid utf8
strings throughout the codebase and passed as Cow<'static, str>
instead of Cow<'static, [u8]>. Among other things this eliminates the
need for string casting when logging.

As far as I can tell none of these protocol names are used anywhere.
I still think it is worth keeping them in sync.

@mxinden mxinden added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes A4-companion A PR that needs a companion PR to merge in parallel for one of its downstream dependencies. C1-low PR touches the given topic and has a low impact on builders. labels Aug 28, 2020
@mxinden mxinden requested a review from ordian August 28, 2020 14:21
@ghost
Copy link

ghost commented Aug 28, 2020

Waiting for commit status.

@ghost ghost merged commit af8dcd0 into master Aug 28, 2020
@ghost ghost deleted the mxinden-protocol-name-str branch August 28, 2020 15:51
ordian added a commit that referenced this pull request Aug 29, 2020
* master:
  node/network/bridge: Define protocol names as str (#1655)
  Companion PR: add weightinfo for collective (#1524)
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. A4-companion A PR that needs a companion PR to merge in parallel for one of its downstream dependencies. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants