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

Add serde feature flag to primitives #13027

Merged
merged 27 commits into from
May 17, 2023
Merged

Add serde feature flag to primitives #13027

merged 27 commits into from
May 17, 2023

Conversation

haerdib
Copy link
Contributor

@haerdib haerdib commented Dec 29, 2022

Adds serde feature flag to the primitives. This way, serde (de)serialization can also be used in no_std environments.
The following crates now implement the serde flag:

  • sp-application-crypto
  • sp-arithmetic
  • sp-consensus-babe
  • sp-consensus-beefy
  • sp-consensus-grandpa
  • sp-consensus-slots
  • sp-core
  • sp-mmr-primitives
  • sp-npos-elections
  • sp-runtime
  • sp-storage
  • sp-test-primitives
  • sp-version
  • sp-weights

closes #12994

Kusama address: FsnxqJnqWVNMZZgxaQdhaCk9c5sL3WSggRCRqp1qEzk1L2i

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Dec 29, 2022

User @haerdib, please sign the CLA here.

@haerdib haerdib marked this pull request as draft December 29, 2022 12:40
@haerdib haerdib marked this pull request as ready for review December 29, 2022 14:18
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Something like that for the rest should do it.

primitives/application-crypto/Cargo.toml Outdated Show resolved Hide resolved
primitives/application-crypto/Cargo.toml Outdated Show resolved Hide resolved
primitives/application-crypto/Cargo.toml Outdated Show resolved Hide resolved
primitives/application-crypto/Cargo.toml Outdated Show resolved Hide resolved
primitives/application-crypto/Cargo.toml Outdated Show resolved Hide resolved
primitives/application-crypto/Cargo.toml Outdated Show resolved Hide resolved
primitives/application-crypto/Cargo.toml Show resolved Hide resolved
@haerdib haerdib changed the title Add serde_full feature flag to primitives Add serde feature flag to primitives Dec 30, 2022
@haerdib haerdib marked this pull request as draft December 30, 2022 09:54
primitives/arithmetic/src/biguint.rs Outdated Show resolved Hide resolved
primitives/consensus/slots/src/lib.rs Outdated Show resolved Hide resolved
primitives/application-crypto/Cargo.toml Show resolved Hide resolved
@haerdib haerdib changed the title Add serde feature flag to primitives Expand no_std availability of primitives and add serde feature flag Jan 3, 2023
@haerdib haerdib marked this pull request as ready for review January 3, 2023 12:13
@haerdib haerdib requested review from bkchr and removed request for kianenigma and acatangiu January 3, 2023 14:15
@haerdib
Copy link
Contributor Author

haerdib commented Jan 13, 2023

@bkchr Any news on this? I'll happily split the PR, keep it up to date.. if there's a chance, this can go through.

@stale
Copy link

stale bot commented Feb 12, 2023

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Feb 12, 2023
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Sorry for the long delay, but there are also still things that need to be improved!

primitives/runtime/src/lib.rs Outdated Show resolved Hide resolved
primitives/runtime/src/generic/digest.rs Show resolved Hide resolved
primitives/keystore/Cargo.toml Outdated Show resolved Hide resolved
primitives/finality-grandpa/src/lib.rs Outdated Show resolved Hide resolved
primitives/finality-grandpa/src/lib.rs Outdated Show resolved Hide resolved
primitives/core/src/crypto.rs Outdated Show resolved Hide resolved
primitives/core/src/crypto.rs Outdated Show resolved Hide resolved
primitives/core/src/crypto.rs Outdated Show resolved Hide resolved
primitives/core/src/crypto.rs Outdated Show resolved Hide resolved
primitives/application-crypto/src/lib.rs Outdated Show resolved Hide resolved
@haerdib haerdib requested a review from a team March 10, 2023 15:26
@haerdib haerdib changed the title Expand no_std availability of primitives and add serde feature flag Add serde feature flag to primitives Mar 13, 2023
primitives/test-primitives/src/lib.rs Show resolved Hide resolved
primitives/runtime/src/traits.rs Outdated Show resolved Hide resolved
primitives/core/src/lib.rs Outdated Show resolved Hide resolved
primitives/rpc/Cargo.toml Outdated Show resolved Hide resolved
@michalkucharczyk
Copy link
Contributor

michalkucharczyk commented May 2, 2023

LGTM.
I left some minor comments.

@haerdib Would you be willing to continue work on this (rebase)? I already did some work here: michalkucharczyk@17a7981, michalkucharczyk@218f1e8. But I am not sure how I can share it.

Copy link
Contributor Author

@haerdib haerdib left a comment

Choose a reason for hiding this comment

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

@haerdib Would you be willing to continue work on this (rebase)? I already did some work here: michalkucharczyk@17a7981, michalkucharczyk@218f1e8. But I am not sure how I can share it.

So does this PR still have a chance of being merged eventually? If so, I can do the rebase, that's not a problem. Thanks for the commits. I'll happily take a look at them 👍

primitives/test-primitives/src/lib.rs Show resolved Hide resolved
primitives/runtime/src/traits.rs Outdated Show resolved Hide resolved
primitives/core/src/lib.rs Outdated Show resolved Hide resolved
@substrate-tip-bot
Copy link

@bkchr Contributor did not properly post their account address.

Make sure the pull request description has: "{network} address: {address}".

@bkchr
Copy link
Member

bkchr commented May 16, 2023

@bkchr Contributor did not properly post their account address.

Make sure the pull request description has: "{network} address: {address}".

@haerdib pleae do what the bot says and ping me when you have done it :D

@haerdib
Copy link
Contributor Author

haerdib commented May 17, 2023

@bkchr thanks a lot for the tip! Honestly, didn't expect that 🥳

@bkchr
Copy link
Member

bkchr commented May 17, 2023

/tip medium

@substrate-tip-bot
Copy link

@bkchr A medium tip was successfully submitted for haerdib (FsnxqJnqWVNMZZgxaQdhaCk9c5sL3WSggRCRqp1qEzk1L2i on kusama).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fkusama-rpc.polkadot.io#/referenda tip

@michalkucharczyk
Copy link
Contributor

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 6507a79 into paritytech:master May 17, 2023
michalkucharczyk added a commit that referenced this pull request May 17, 2023
This is followup of #13027.

`Aura` need to enable `serde` feature in dependent crates, otherwise
test-substrate-runtime compilation fails with the following error if
`serde` is enabled:

```
  error: cannot find macro `format` in this scope
    -->
/home/miszka/parity/10-genesis-config/substrate-master/primitives/consensus/aura/src/lib.rs:50:3
     |
  50 |         app_crypto!(ed25519, AURA);
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = help: consider importing one of these items:
             scale_info::prelude::format
             sp_application_crypto::format
     = note: this error originates in the macro
`$crate::app_crypto_public_common_if_serde` which comes from the
expansion of the macro `app_crypto` (in Nightly builds, run with -Z
macro-backtrace for more info)
```
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/genesisconfig-in-wasm-runtime-a-step-towards-a-non-native-future/2887/1

paritytech-processbot bot pushed a commit that referenced this pull request May 18, 2023
This is followup of #13027.

`Aura` need to enable `serde` feature in dependent crates, otherwise
test-substrate-runtime compilation fails with the following error if
`serde` is enabled:

```
  error: cannot find macro `format` in this scope
    -->
/home/miszka/parity/10-genesis-config/substrate-master/primitives/consensus/aura/src/lib.rs:50:3
     |
  50 |         app_crypto!(ed25519, AURA);
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = help: consider importing one of these items:
             scale_info::prelude::format
             sp_application_crypto::format
     = note: this error originates in the macro
`$crate::app_crypto_public_common_if_serde` which comes from the
expansion of the macro `app_crypto` (in Nightly builds, run with -Z
macro-backtrace for more info)
```
@mordamax
Copy link
Contributor

Re-submitting this tip, as the original one (https://kusama.polkassembly.io/referenda/196) was wasted due to Lookup { len: 0 } in proposal

@mordamax
Copy link
Contributor

/tip medium

@substrate-tip-bot
Copy link

@mordamax A medium tip was successfully submitted for @haerdib (FsnxqJnqWVNMZZgxaQdhaCk9c5sL3WSggRCRqp1qEzk1L2i on kusama).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fkusama-rpc.polkadot.io#/referenda tip

gpestana pushed a commit that referenced this pull request May 27, 2023
* add serde_full feature flag

add serde_full to sp_runtime

add space to toml

add serde_full to application-crypto

add serde_full to arithmetic

fix arithmetic

add serde full to beefy

add serde full to consensus

add serde_full to core

add serdefull to finality grandpa

add serde_full to several primitives crates

make rpc no_std compatible

add scale info to runtime

make serializer no_std compatible

add serde full to storage

add full serde to version

add serde full to weights

add all serde_full features

add . to comment

add missing impl-serde

fix no-std build

fix build

add full_crypto to serde_full

serde_full also implements crypto

full_serde does not work with full_crytpo. needs std

no no_std serde impl possible

also for crypto std is necessary

no serde full for application crypto

fix arithmetic

fix tomls

fix some things

impl fmt for Signature

add serialize to Public

add impl_maybe_marker_serde_full

fix sp-application-crypto toml

add serde feature flag

fix clippy

fix toml grandpa

fix grandpa

rename if_std to if_serde

keystore is not no_std compatible

make keystore vrf no_std compatible

fix nopos-elections

fix rpc

fix serializer

fix test-primitives

fix version

add comment

add serde full only import for format string

remove all(serde_full and full_crypot) as serde_full enforces full_crypto

make comment better readable

even better comment

clean up rpc toml

clean up toml

clean up serializer toml

clean up storage toml

fix std build

update .lock

fix sp-version

move sp_std import

test extern crate alloc

replace sp_std with core

add missing core

sp_core: serde feature do not enforce full crypto

application-crypto: serde feature do not enforce full crypto

rename serde_full to serde

add dep:serde and alloc to default feature

add full_crypto and remove unnecessary debu/fmt impls for serde

update comment

remove obolsete change in display AccountId32

remove extra changes

minimize diff

revert keystore changes

remove std from keystore

remove full-crypto feature

fix serde import

fix comment

fix feature = serde

* rename serde_full to serde

* move #[doc(hidden)] back

* remove feature = full crypto require frm MultiSigner

* reorder serde and scale_info import

* fix bs58 missing alloc import in serde feature

* add `from_string` to serde feature and add unimplemented

* remove serde feature from fixed_point display

* Remove serde/alloc

Co-authored-by: Davide Galassi <[email protected]>

* Update primitives/consensus/babe/Cargo.toml

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

* Update primitives/arithmetic/src/fixed_point.rs

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

* revert `from_string`fixed impl back to std only

* remove duplicate runtime string impl

* use sp_std::alloc

* remove no_std compatible rpc

* remove no_std compatibility from serializer

* rename mpl_maybe_marker_serde to std_or_serde

* update .lock

* add sp-std to executor

* fix sp-std import

* fix sp_std::format import

* use crate import

* add serde feature

* Update primitives/core/src/lib.rs

---------

Co-authored-by: Davide Galassi <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
gpestana pushed a commit that referenced this pull request May 27, 2023
This is followup of #13027.

`Aura` need to enable `serde` feature in dependent crates, otherwise
test-substrate-runtime compilation fails with the following error if
`serde` is enabled:

```
  error: cannot find macro `format` in this scope
    -->
/home/miszka/parity/10-genesis-config/substrate-master/primitives/consensus/aura/src/lib.rs:50:3
     |
  50 |         app_crypto!(ed25519, AURA);
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = help: consider importing one of these items:
             scale_info::prelude::format
             sp_application_crypto::format
     = note: this error originates in the macro
`$crate::app_crypto_public_common_if_serde` which comes from the
expansion of the macro `app_crypto` (in Nightly builds, run with -Z
macro-backtrace for more info)
```
@rzadp
Copy link
Contributor

rzadp commented Jun 9, 2023

Re-submitting this tip, as the second one was wasted due to insufficient permissions to run remark and the approved referendum was not executed successfully.

Related: #14260

@rzadp
Copy link
Contributor

rzadp commented Jun 9, 2023

/tip medium

@substrate-tip-bot
Copy link

@rzadp Could not submit tip :( Notify someone here.

@rzadp
Copy link
Contributor

rzadp commented Jun 9, 2023

@rzadp Could not submit tip :( Notify someone here.

The tip has actually went through - it appears that we have incorrectly (?) treated an isRetracted status of an extrinsic as a failure.

nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* add serde_full feature flag

add serde_full to sp_runtime

add space to toml

add serde_full to application-crypto

add serde_full to arithmetic

fix arithmetic

add serde full to beefy

add serde full to consensus

add serde_full to core

add serdefull to finality grandpa

add serde_full to several primitives crates

make rpc no_std compatible

add scale info to runtime

make serializer no_std compatible

add serde full to storage

add full serde to version

add serde full to weights

add all serde_full features

add . to comment

add missing impl-serde

fix no-std build

fix build

add full_crypto to serde_full

serde_full also implements crypto

full_serde does not work with full_crytpo. needs std

no no_std serde impl possible

also for crypto std is necessary

no serde full for application crypto

fix arithmetic

fix tomls

fix some things

impl fmt for Signature

add serialize to Public

add impl_maybe_marker_serde_full

fix sp-application-crypto toml

add serde feature flag

fix clippy

fix toml grandpa

fix grandpa

rename if_std to if_serde

keystore is not no_std compatible

make keystore vrf no_std compatible

fix nopos-elections

fix rpc

fix serializer

fix test-primitives

fix version

add comment

add serde full only import for format string

remove all(serde_full and full_crypot) as serde_full enforces full_crypto

make comment better readable

even better comment

clean up rpc toml

clean up toml

clean up serializer toml

clean up storage toml

fix std build

update .lock

fix sp-version

move sp_std import

test extern crate alloc

replace sp_std with core

add missing core

sp_core: serde feature do not enforce full crypto

application-crypto: serde feature do not enforce full crypto

rename serde_full to serde

add dep:serde and alloc to default feature

add full_crypto and remove unnecessary debu/fmt impls for serde

update comment

remove obolsete change in display AccountId32

remove extra changes

minimize diff

revert keystore changes

remove std from keystore

remove full-crypto feature

fix serde import

fix comment

fix feature = serde

* rename serde_full to serde

* move #[doc(hidden)] back

* remove feature = full crypto require frm MultiSigner

* reorder serde and scale_info import

* fix bs58 missing alloc import in serde feature

* add `from_string` to serde feature and add unimplemented

* remove serde feature from fixed_point display

* Remove serde/alloc

Co-authored-by: Davide Galassi <[email protected]>

* Update primitives/consensus/babe/Cargo.toml

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

* Update primitives/arithmetic/src/fixed_point.rs

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

* revert `from_string`fixed impl back to std only

* remove duplicate runtime string impl

* use sp_std::alloc

* remove no_std compatible rpc

* remove no_std compatibility from serializer

* rename mpl_maybe_marker_serde to std_or_serde

* update .lock

* add sp-std to executor

* fix sp-std import

* fix sp_std::format import

* use crate import

* add serde feature

* Update primitives/core/src/lib.rs

---------

Co-authored-by: Davide Galassi <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
This is followup of paritytech#13027.

`Aura` need to enable `serde` feature in dependent crates, otherwise
test-substrate-runtime compilation fails with the following error if
`serde` is enabled:

```
  error: cannot find macro `format` in this scope
    -->
/home/miszka/parity/10-genesis-config/substrate-master/primitives/consensus/aura/src/lib.rs:50:3
     |
  50 |         app_crypto!(ed25519, AURA);
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = help: consider importing one of these items:
             scale_info::prelude::format
             sp_application_crypto::format
     = note: this error originates in the macro
`$crate::app_crypto_public_common_if_serde` which comes from the
expansion of the macro `app_crypto` (in Nightly builds, run with -Z
macro-backtrace for more info)
```
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. 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 T2-API This PR/Issue is related to APIs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement serde derivations also in no_std
9 participants