Skip to content

feat: introduce no_std support for ff, curve, and crypto #306

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

Merged
merged 1 commit into from
Feb 23, 2023

Conversation

tdelabro
Copy link
Contributor

No description provided.

@tdelabro tdelabro force-pushed the no_std-support branch 3 times, most recently from 33eaca7 to 302939c Compare February 22, 2023 20:02
@tdelabro tdelabro marked this pull request as ready for review February 22, 2023 21:28
@tdelabro tdelabro changed the title feat: introduce no_std support for ff and crypto feat: introduce no_std support for ff, curve, and crypto Feb 22, 2023
@xJonathanLEI xJonathanLEI self-assigned this Feb 23, 2023
@xJonathanLEI
Copy link
Owner

xJonathanLEI commented Feb 23, 2023

Found this clippy lint that might be useful:

rust-lang/rust-clippy#7724

Clippy rule link:

https://rust-lang.github.io/rust-clippy/master/#std_instead_of_core

@xJonathanLEI
Copy link
Owner

xJonathanLEI commented Feb 23, 2023

Note that we should also add CI steps for building without the std feature to make sure we're not accidentally breaking no_std support.

Edit: found this: https://github.com/KodrAus/rust-no-std

@xJonathanLEI
Copy link
Owner

xJonathanLEI commented Feb 23, 2023

Hmmm I just tried running this:

console~ ~$ cargo build --package starknet-crypto --no-default-features~ ~

to make sure it really works for no_std, but I'm getting errors. Any ideas?

Edit: never mind it looks like I have to also enable alloc feature when using no_std.

@xJonathanLEI
Copy link
Owner

xJonathanLEI commented Feb 23, 2023

Is it possible that we make the crate work with everything turned off (not even alloc), but with reduced functionality? Anything that requires alloc gets gated by any(feature = "std", feature = "alloc"). Then at least the crate would work with a bare default-features = false.

Or maybe we should just unconditionally enable alloc when std is off - the crate doesn't build without it anyways.

@tdelabro
Copy link
Contributor Author

Is it possible that we make the crate work with everything turned off (not even alloc), but with reduced functionality?

Yeah totally. alloc is only used by starknet-ff here:

impl Serialize for FieldElement {
    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
    where
        S: serde::Serializer,
    {
        serializer.serialize_str(&ToString::to_string(&self))
    }
}

impl<'de> Deserialize<'de> for FieldElement {
    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
    where
        D: serde::Deserializer<'de>,
    {
        let value = String::deserialize(deserializer)?;
        Self::from_str(&value).map_err(serde::de::Error::custom)
    }
}

So what we can do is:

[dependencies]
serde = { version = "1.0.152", default-features = false, optional = true }

[features]
serde = ["alloc", "dep:serde"]
alloc = [""serde?/alloc"]

@tdelabro
Copy link
Contributor Author

tdelabro commented Feb 23, 2023

@xJonathanLEI I pushed a new version. Where the tree crates are full #![no_std] and don't relly at all on std.
They occasionally need alloc to perform some specific actions that are behind feature flags, such as serde or display-field-element.

I also removed thiserror this error entirely and implemented it manually instead.

I think you will like this version better

@tdelabro tdelabro force-pushed the no_std-support branch 3 times, most recently from 6614cf5 to 2d7d6dc Compare February 23, 2023 13:19
Copy link
Owner

@xJonathanLEI xJonathanLEI left a comment

Choose a reason for hiding this comment

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

Thanks for updating this! Looks good to me apart from a few minor issues

Copy link
Owner

@xJonathanLEI xJonathanLEI left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks a lot for working on this!

@xJonathanLEI xJonathanLEI merged commit a3a0050 into xJonathanLEI:master Feb 23, 2023
@tdelabro tdelabro deleted the no_std-support branch February 23, 2023 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants