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

frame: support for serde added #14261

Merged
merged 8 commits into from
Jun 1, 2023
Merged

frame: support for serde added #14261

merged 8 commits into from
Jun 1, 2023

Conversation

michalkucharczyk
Copy link
Contributor

@michalkucharczyk michalkucharczyk commented May 29, 2023 •
edited
Loading

  • enabled serde features in dependent crates, no gate feature introduced, linker should do the job and strip unused code.
  • frame::staking: added impl of serde::Serialize, serde::Deserialize for enum Forcing
  • primitives::runtime: impl_opaque_keys macro provides Serialize/Deserialize impl if serde is enabled
  • primitives::staking: added impl of serde::Serialize, serde::Deserialize for enum StakerStatus
  • serde::Serialize/Deserialize implemented for pallets GenesisConfig in no-std

Step towards: paritytech/polkadot-sdk#25

polkadot companion: paritytech/polkadot#7312
cumulus companion: paritytech/cumulus#2660

@michalkucharczyk michalkucharczyk added C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit B1-note_worthy Changes should be noted in the release notes T1-runtime This PR/Issue is related to the topic “runtime”. labels May 29, 2023
@michalkucharczyk
frame: support for serde added
d00d664 
- enabled `serde` features in dependent crates, no gate feature introduced, linker should do the job and strip unused code.

- frame::staking: added impl of `serde::Serialize, serde::Deserialize` for `enum Forcing`

- primitives::runtime: impl_opaque_keys macro provides `Serialize/Deserialize` impl if `serde` is enabled

- primitives::staking: added impl of `serde::Serialize`, `serde::Deserialize` for `enum StakerStatus`
@michalkucharczyk
frame::support: serde for pallets' GenesisConfig enabled in no-std
1a4c96c
bkchr
bkchr approved these changes May 30, 2023
View reviewed changes
primitives/staking/Cargo.toml Outdated Show resolved Hide resolved
This was referenced May 31, 2023
Merged
Merged
@michalkucharczyk michalkucharczyk marked this pull request as ready for review May 31, 2023 15:26
@michalkucharczyk michalkucharczyk requested review from a team May 31, 2023 15:26
@michalkucharczyk michalkucharczyk requested review from a team May 31, 2023 15:26
melekes
melekes approved these changes Jun 1, 2023
View reviewed changes
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

@michalkucharczyk
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 2577379 into master Jun 1, 2023
@paritytech-processbot paritytech-processbot bot deleted the mku-serde-pulled-into-frame branch June 1, 2023 15:35
@nazar-pc nazar-pc mentioned this pull request Jun 16, 2023
Merged
1 task
@nazar-pc
Copy link
Contributor

It is somewhat confusing that serde is an optional feature in some case, but essentially is not optional as far as runtime is concerned. We have custom Block struct implementation and for it serde has to be unconditionally enabled now because it is unconditionally enabled in frame-support, propagating MaybeSerialize requirement everywhere as the result, which was unfortunate as it slows down compilation.

@bkchr
Copy link
Member

bkchr commented Jun 18, 2023

which was unfortunate as it slows down compilation.

I mean this is expect-able, but how much does it slow down the compilation?

@nazar-pc
Copy link
Contributor

Doing a/b testing would be tricky, hard to say the exact impact.

@svyatonik svyatonik mentioned this pull request Jun 21, 2023
Merged
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
@michalkucharczyk @bkchr @nathanwhit
frame: support for serde added (paritytech#14261)
c5fbe1d 
* frame: support for serde added

- enabled `serde` features in dependent crates, no gate feature introduced, linker should do the job and strip unused code.

- frame::staking: added impl of `serde::Serialize, serde::Deserialize` for `enum Forcing`

- primitives::runtime: impl_opaque_keys macro provides `Serialize/Deserialize` impl if `serde` is enabled

- primitives::staking: added impl of `serde::Serialize`, `serde::Deserialize` for `enum StakerStatus`

* frame::support: serde for pallets' GenesisConfig enabled in no-std

* Cargo.lock updated

* Update primitives/staking/Cargo.toml

Co-authored-by: Bastian Köcher <[email protected]>

* fix

* Cargo.lock update + missed serde/std in beefy

---------

Co-authored-by: Bastian Köcher <[email protected]>
@xlc
Copy link
Contributor

xlc commented Jul 20, 2023

This is confusing to me. I don't really want all the extra code in my runtime to serialize/deserialize types. Those codes are never going to be used and bloat my wasm.

@koute
Copy link
Contributor

koute commented Jul 20, 2023

Those codes are never going to be used and bloat my wasm.

LTO should automatically remove most of the unused stuff. But assuming it somehow doesn't, can you show and measure how much exactly it bloats up your WASM blob?

@xlc
Copy link
Contributor

xlc commented Jul 20, 2023 •
edited
Loading

I need to make my code compile before I can have numbers. And with paritytech/polkadot-sdk#25 in mind, one of the goal is to be able to build genesis in wasm, so that those code won't be optimized out by LTO eventually.

But that brings up a good point, I guess we could have some CI task to measure the kitchen sink runtime wasm code size so we can see if a feature increased it significantly. Something like this paritytech/smoldot#2994 (comment)

@michalkucharczyk
Copy link
Contributor Author

This is confusing to me. I don't really want all the extra code in my runtime to serialize/deserialize types. Those codes are never going to be used and bloat my wasm.

-> paritytech/polkadot-sdk#25

@brenzi brenzi mentioned this pull request Jul 22, 2023
Merged
@crystalin crystalin mentioned this pull request Oct 20, 2023
Closed
@JesseAbram JesseAbram mentioned this pull request Oct 26, 2023
Merged
@brenzi brenzi mentioned this pull request Jan 21, 2024
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@melekes melekes melekes approved these changes

@sam0x17 sam0x17 sam0x17 approved these changes

@bkchr bkchr bkchr approved these changes

@davxy davxy davxy approved these changes

@acatangiu acatangiu acatangiu approved these changes

@andresilva andresilva Awaiting requested review from andresilva

Assignees
No one assigned
Labels
B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@michalkucharczyk @nazar-pc @bkchr @xlc @koute @melekes @sam0x17 @davxy @acatangiu