-
Notifications
You must be signed in to change notification settings - Fork 124
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
Make Api no_std
compatible
#377
Conversation
ee62727
to
175e991
Compare
a5210cd
to
e5b0b78
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool we were not so far away after all! :D I only have a comment about documentation.
node-api/src/metadata.rs
Outdated
use codec::{Decode, Encode, Error as CodecError}; | ||
use frame_metadata::{ | ||
PalletConstantMetadata, RuntimeMetadata, RuntimeMetadataLastVersion, RuntimeMetadataPrefixed, | ||
StorageEntryMetadata, META_RESERVED, | ||
}; | ||
use scale_info::{form::PortableForm, PortableRegistry, Type}; | ||
use sp_core::storage::StorageKey; | ||
|
||
#[cfg(feature = "std")] | ||
use serde::Serialize; | ||
|
||
// We use `BTreeMap` because we can't use `HashMap` in `no_std`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you move this comment too?
+ Serialize | ||
+ DeserializeOwned |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we enforce something that is more than what the trait bounds in substrate guarantee Hence, we should maybe add a comment that those types need to re-implement serialization similar to how you re-implemented it for substrate types. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and a nice improvement. The type duplication from substrate is a bit unfortunate, let's hope they agree to provide serialization also in no_std environments.
remove rpc crate dep always use alloc make api no-std compatible fix NumberOrHex update deps remove unused error make rpc open as well add postcard as Deserializer fix rpc params some further fixes amek rpc no-std compatible enfore Serialize and Deserialize instead of Maybe compile :) fix CI fix build fmt fix rebase errors add insert_with_allocation_test works fix no_std build more copy paste add no_std compatibility fix taplo remove sp_std cargo fmt remove extra space remove std feature obsolete remove obsolsete std feature remove unused comment Fix ci.yml (#389) * fix ci.yml * fix ci fmt impl serialize for NumberOrHex remove unnecessary redefined types extract serde impls from types and import whatever possible add from impls and tests add comment some more comments udpate comment add storage change set fix from serde impl fix build remove std only feature to subscription let it compile again :) fix tests remove some more std only feature make tx payment no_std compatible add extrinsic to no_std compatibility add balances extrinisc to no-std
84a40e2
to
d03d7cd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful, thanks for the extra documentation!
🌈 Featuring added
no_std
compatibility for the fullApi
included traits and the implementations (except for the rpc clients)sp-std
dependency. Whenever possible, depend oncore
oralloc
directly. Thestd
version itself points to them as well. No need for an extra library to do that,primitives/serde_impls
to implement serde also inno_std
. Wherever sensible, the conversion from substrate type and redefined type has been implemented withFrom
. So the newly self-implemented types should not pose a big problem for the user. Another good thing: If substrate updates the types, this will be noticed quite fast via the unit tests.MaybeSerializeDeserialize
because this only enforces (De)Serialization only instd
mode.NumberOrHex
becausesp-rpc
is notno_std
compatible.src/api/error.rs
closes #279