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

Combine scale-info feature into codec, and wire it through ethereum-types #593

Merged
merged 10 commits into from
Sep 30, 2021
Merged

Combine scale-info feature into codec, and wire it through ethereum-types #593

merged 10 commits into from
Sep 30, 2021

Conversation

JoshOrndorff
Copy link
Contributor

ordian
ordian previously approved these changes Sep 29, 2021
ethereum-types/Cargo.toml Outdated Show resolved Hide resolved
ethereum-types/Cargo.toml Outdated Show resolved Hide resolved
@ascjones
Copy link
Member

This reminds me, now that we have scale-info 1.0 and it is integrated into substrate, we should probably:

  • Tighten up the the requirement to 1,0
  • Consider merging the scale-info feature into the codec feature: if you want one you get both.

ethereum-types/src/lib.rs Outdated Show resolved Hide resolved
@JoshOrndorff JoshOrndorff changed the title Add a scale-info feature to ethereum-types Combine scale-info feature into codec, and wire it through ethereum-types Sep 29, 2021
@JoshOrndorff
Copy link
Contributor Author

Okay, this is ready for review.

@ordian
Copy link
Member

ordian commented Sep 29, 2021

This reminds me, now that we have scale-info 1.0 and it is integrated into substrate, we should probably:

* Tighten up the the requirement to `1,0`

* Consider merging the `scale-info` feature into the `codec` feature: if you want one you get both.

That would technically make this change breaking, because scale-info now doesn't imply codec feature and we drop compatibility with 0.9.

@JoshOrndorff
Copy link
Contributor Author

Currently, I've applied @ascjones suggestions to use 1.0 and combine it all into the codec feature. Shall I:

  1. Keep it that way and bump crate versions here as necessary (I guess this would be minor bump where the functionality was only added major where the scale-info trait was removed?
  2. Go back to the less invasive approach of introducing a separate scale-info trait in each crate.

@ordian
Copy link
Member

ordian commented Sep 30, 2021

I would suggest reverting the changes for primitive-types and keeping everything else as is (modulo primitive-types/scale-info feature activation).

@JoshOrndorff
Copy link
Contributor Author

@ordian I believe I've done as you asked. Can you please review it again?

primitive-types/tests/scale_info.rs Outdated Show resolved Hide resolved
Co-authored-by: Andronik Ordian <write@reusable.software>
@ordian ordian merged commit df638ab into paritytech:master Sep 30, 2021
@JoshOrndorff
Copy link
Contributor Author

Thanks for the review and merge everyone! I'd like this change to trickle down to frontier in time for Sub0 conference on October 14th if possible. What would it take to get a release of ethereum-types published?

@ordian
Copy link
Member

ordian commented Sep 30, 2021

opened #594, will publish it soon

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

Successfully merging this pull request may close these issues.

None yet

5 participants