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.

Sign up for GitHub

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 sp-keystore no_std-compatible and fix the build-runtimes-polkavm CI job #3363

Merged
merged 2 commits into from
Feb 18, 2024
Merged

Make sp-keystore no_std-compatible and fix the build-runtimes-polkavm CI job #3363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1b76631
Make `sp-keystore` `no_std`-compatible
koute Feb 16, 2024
950f98d
Make the `build-runtimes-polkavm` mandatory
koute Feb 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .gitlab/pipeline/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,6 @@ build-runtimes-polkavm:
- SUBSTRATE_RUNTIME_TARGET=riscv cargo check -p westend-runtime
- SUBSTRATE_RUNTIME_TARGET=riscv cargo check -p rococo-runtime
- SUBSTRATE_RUNTIME_TARGET=riscv cargo check -p polkadot-test-runtime
allow_failure: true

.build-subkey:
stage: build
Expand Down
1 change: 0 additions & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 8 additions & 8 deletions substrate/primitives/externalities/src/extensions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,24 +78,24 @@ macro_rules! decl_extension {
$vis struct $ext_name (pub $inner);

impl $crate::Extension for $ext_name {
fn as_mut_any(&mut self) -> &mut dyn std::any::Any {
fn as_mut_any(&mut self) -> &mut dyn core::any::Any {
self
}

fn type_id(&self) -> std::any::TypeId {
std::any::Any::type_id(self)
fn type_id(&self) -> core::any::TypeId {
core::any::Any::type_id(self)
}
}

impl std::ops::Deref for $ext_name {
impl core::ops::Deref for $ext_name {
type Target = $inner;

fn deref(&self) -> &Self::Target {
&self.0
}
}

impl std::ops::DerefMut for $ext_name {
impl core::ops::DerefMut for $ext_name {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.0
}
Expand All @@ -115,12 +115,12 @@ macro_rules! decl_extension {
$vis struct $ext_name;

impl $crate::Extension for $ext_name {
fn as_mut_any(&mut self) -> &mut dyn std::any::Any {
fn as_mut_any(&mut self) -> &mut dyn core::any::Any {
self
}

fn type_id(&self) -> std::any::TypeId {
std::any::Any::type_id(self)
fn type_id(&self) -> core::any::TypeId {
core::any::Any::type_id(self)
}
}
}
Expand Down
5 changes: 2 additions & 3 deletions substrate/primitives/keystore/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@ targets = ["x86_64-unknown-linux-gnu"]

[dependencies]
codec = { package = "parity-scale-codec", version = "3.6.1", default-features = false, features = ["derive"] }
parking_lot = { version = "0.12.1", default-features = false }
thiserror = "1.0"
parking_lot = { version = "0.12.1", default-features = false, optional = true }
sp-core = { path = "../core", default-features = false }
sp-externalities = { path = "../externalities", default-features = false }

Expand All @@ -28,7 +27,7 @@ rand_chacha = "0.2.2"

[features]
default = ["std"]
std = ["codec/std", "sp-core/std", "sp-externalities/std"]
std = ["codec/std", "dep:parking_lot", "sp-core/std", "sp-externalities/std"]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
std = ["codec/std", "dep:parking_lot", "sp-core/std", "sp-externalities/std"]
std = ["codec/std", "parking_lot", "sp-core/std", "sp-externalities/std"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bkchr Hm... sure, I can do this, but just to confirm - do we want to never use the dep: syntax unless necessary?

I'm asking because this is not just a stylistic choice. The nice thing about the new (well, new-ish) dep: syntax it that it prevents an implicit feature from being defined, that is, if an optional dependency is not referred to with a dep: then cargo will implicitly define a new feature like this:

[features]
parking_lot = ["dep:parking_lot"]

And in consequence this now technically becomes part of the public API for the crate, so another crate which depends on the sp-keystore can then say:

[dependencies]
sp-keystore = { version = "...", features = ["parking_lot"] }

...which doesn't make much sense because the parking_lot dependency here is an internal dependency, so it makes zero sense for another crate to be able to enable it. And using it through dep: prevents this, because then cargo won't create this implicit feature.

Copy link
Member

Choose a reason for hiding this comment

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

Okay good points, then let's ignore what I said. I didn't realized that this would then not be exposed 👍


# This feature adds BLS crypto primitives.
# It should not be used in production since the implementation and interface may still
Expand Down
26 changes: 20 additions & 6 deletions substrate/primitives/keystore/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@

//! Keystore traits

#![cfg_attr(not(feature = "std"), no_std)]

extern crate alloc;

#[cfg(feature = "std")]
pub mod testing;

Expand All @@ -29,25 +33,35 @@ use sp_core::{
ecdsa, ed25519, sr25519,
};

use std::sync::Arc;
use alloc::{string::String, sync::Arc, vec::Vec};

/// Keystore error
#[derive(Debug, thiserror::Error)]
#[derive(Debug)]
pub enum Error {
/// Public key type is not supported
#[error("Key not supported: {0:?}")]
KeyNotSupported(KeyTypeId),
/// Validation error
#[error("Validation error: {0}")]
ValidationError(String),
/// Keystore unavailable
#[error("Keystore unavailable")]
Unavailable,
/// Programming errors
#[error("An unknown keystore error occurred: {0}")]
Other(String),
}

impl core::fmt::Display for Error {
fn fmt(&self, fmt: &mut core::fmt::Formatter) -> core::fmt::Result {
match self {
Error::KeyNotSupported(key_type) => write!(fmt, "Key not supported: {key_type:?}"),
Error::ValidationError(error) => write!(fmt, "Validation error: {error}"),
Error::Unavailable => fmt.write_str("Keystore unavailable"),
Error::Other(error) => write!(fmt, "An unknown keystore error occurred: {error}"),
}
}
}

#[cfg(feature = "std")]
impl std::error::Error for Error {}

/// Something that generates, stores and provides access to secret keys.
pub trait Keystore: Send + Sync {
/// Returns all the sr25519 public keys for the given key type.
Expand Down
Loading