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

Add a TypeDef to handle Compact types #53

Merged
merged 36 commits into from
Feb 2, 2021

Conversation

dvdplm
Copy link
Contributor

@dvdplm dvdplm commented Jan 18, 2021

Update the derive macro to handle the #[codec(compact) attribute from parity-scale-codec using a new TypeDefCompact variant similarly to #48.

Closes #8

Supersedes #42

ascjones and others added 26 commits December 7, 2020 11:49
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.
Add a `compact` member to `Field` to indicate it is `scale_codec::Compact`
Add test that illustrates the registry in the presence of compact types.
Add a macro to cleanup boilerplate
test_suite/tests/derive.rs Outdated Show resolved Hide resolved
@dvdplm dvdplm self-assigned this Jan 27, 2021
derive/src/lib.rs Show resolved Hide resolved
test_suite/tests/derive.rs Outdated Show resolved Hide resolved
src/prelude.rs Outdated Show resolved Hide resolved
src/ty/mod.rs Outdated Show resolved Hide resolved
test_suite/tests/derive.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

Awesome! Would like a ✔️ from @Robbepop before merging.

Copy link
Contributor

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

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

LGTM

One nit pick that should be handled as a follow-up PR.

.push(parse_quote!(#ty : ::scale::HasCompact));
where_clause
.predicates
.push(parse_quote!(<#ty as ::scale::HasCompact>::Type : ::scale_info::TypeInfo + 'static));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think our use of ::scale is becoming a problem. ink! imports parity-scale-codec crate as scale but from what I remember Substrate imports it as codec. We need a solution that works for both. Probably Basti's https://crates.io/crates/proc-macro-crate could help here. For now I am fine and we can fix this later.

Copy link
Member

Choose a reason for hiding this comment

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

this also seem to break primitive-types: paritytech/parity-common#519

@ascjones ascjones merged commit 177c568 into master Feb 2, 2021
@ascjones ascjones deleted the dp-add-TypeDef-to-handle-Compact-types branch February 2, 2021 11:46
This was referenced Feb 3, 2021
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.

Support Compact types
5 participants