Skip to content
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

Parameterize CompactForm String for optional SCALE impl #35

Merged
merged 5 commits into from
Jan 4, 2021

Conversation

ascjones
Copy link
Contributor

@ascjones ascjones commented Dec 7, 2020

By default, parity-scale-codec does not provide Encode/Decode impls for an owned String.
This is only provided under the "full" feature which is not used by the substrate runtime,
because it should not be used for consensus critical code. So in order for the CompactForm
to be integrated into the substrate runtime, or wherever the "full" feature cannot be used,
then we must parameterize the String type so that it can be both an &'static str on the
runtime side where it is encoded, and a String in client/consuming code where it is decoded.

Note that previously I had done something similar with OwnedForm discussed with @Robbepop here. But this or something similar is now required in case we want to generate the metadata directly inside the substrate runtime where Strings are not allowed.

By default, parity-scale-codec does not provide Encode/Decode impls for an owned String.
This is only provided under the "full" feature which is not used by the substrate runtime,
because it should not be used for consensus critical code. So in order for the CompactForm
to be integrated into the substrate runtime, or wherever the "full" feature cannot be used,
then we must parameterize the `String` type so that it can be both an `&'static str` on the
runtime side where it is encoded, and a `String` in client/consuming code where it is decoded.
@ascjones ascjones mentioned this pull request Dec 7, 2020
8 tasks
Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't say I fully understand the code surrounding your change here; the docs are a bit "academic" and I have a hard time following them at times.
The changes in this PR are straight forward and seems like a no-brainer per se.

src/form.rs Show resolved Hide resolved
@ascjones
Copy link
Contributor Author

I'd like @Robbepop to approve this too before merging, because he disagreed with my previous approach #19 (comment)

@dvdplm dvdplm mentioned this pull request Dec 17, 2020
@ascjones
Copy link
Contributor Author

@ascjones ascjones merged commit efaba4f into master Jan 4, 2021
@ascjones ascjones deleted the aj-compact-string branch January 4, 2021 12:53
@ascjones
Copy link
Contributor Author

ascjones commented Jan 4, 2021

Merged since other work already based on this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants