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.

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

No std upstream working #90

Closed
wants to merge 77 commits into from

Conversation

OverOrion
Copy link
Contributor

Add no_std support with feature flag alloc.

Description

This PR adds no_std support to the nostr crate.

Notes to the reviewers

I will open it as a draft PR and I ask you @yukibtc to have a look at it for a first round of feedback :).
The CI will need some changes as well, but I wanted to open the PR first, hope you don't mind.

Changelog notice

Checklists

All Submissions:

@OverOrion
Copy link
Contributor Author

Hey @yukibtc.

Just a polite ping, looking for feedback :)

Copy link
Member

@yukibtc yukibtc left a comment

Choose a reason for hiding this comment

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

Thank for working on this!
I marked some changes to do for now.
When it will be ready, can you squash commits and add CI for no_std?

Cargo.toml Outdated
@@ -10,8 +10,9 @@ members = [
"crates/nostr-sdk",
"crates/nostr-sdk-net",
"crates/nostr-sdk-sqlite",
"crates/ensure_no_std",
Copy link
Member

Choose a reason for hiding this comment

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

This crate is just for testing no_std, right?
If yes, can you move that to examples?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely, yes, this is just a simple test crate for no_std, will move it 👍

Comment on lines 8 to 44
pub use bech32::*;
pub use bech32;
#[cfg(feature = "nip06")]
pub use bip39::*;
#[cfg(feature = "nip06")]
pub use bitcoin::*;
pub use bitcoin_hashes::*;
pub use secp256k1::*;
pub use serde_json::*;
pub use bip39;
pub use secp256k1;
pub use url::*;

// Internal modules
pub use crate::event::builder::*;
pub use crate::event::id::*;
pub use crate::event::kind::*;
pub use crate::event::tag::*;
pub use crate::event::unsigned::*;
pub use crate::event::*;
pub use crate::key::*;
pub use crate::message::*;
pub use crate::types::*;
pub use crate::{Result, SECP256K1};
pub use crate::event;
pub use crate::event::builder;
pub use crate::event::id;
pub use crate::event::kind;
pub use crate::event::tag;
pub use crate::event::unsigned;
pub use crate::key;
pub use crate::message;
pub use crate::types;
pub use crate::Result;

#[cfg(feature = "std")]
pub use crate::SECP256K1;

// NIPs
#[cfg(feature = "nip04")]
pub use crate::nips::nip04::*;
pub use crate::nips::nip04;
#[cfg(feature = "nip05")]
pub use crate::nips::nip05::*;
pub use crate::nips::nip05;
#[cfg(feature = "nip06")]
pub use crate::nips::nip06::*;
pub use crate::nips::nip06;
#[cfg(feature = "nip11")]
pub use crate::nips::nip11::*;
pub use crate::nips::nip13::*;
pub use crate::nips::nip11;
pub use crate::nips::nip13;
#[cfg(feature = "nip19")]
pub use crate::nips::nip19::*;
pub use crate::nips::nip26::*;
pub use crate::nips::nip19;
pub use crate::nips::nip26;
#[cfg(feature = "nip46")]
pub use crate::nips::nip46::*;
pub use crate::nips::nip65::*;
pub use crate::nips::nip46;
pub use crate::nips::nip65;
Copy link
Member

Choose a reason for hiding this comment

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

Here is possible to leave the ::*?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am honestly not sure, because it gives me a warning:

 --> nostr/crates/nostr/src/prelude.rs:14:9
   |
14 | pub use bitcoin_hashes::*;
   |         ^^^^^^^^^^^^^^^^^ the name `error` in the type namespace is first re-exported here
15 | pub use secp256k1::*;
16 | pub use serde_json::*;
   |         ------------- but the name `error` in the type namespace is also re-exported here
   |
   = note: `#[warn(ambiguous_glob_reexports)]` on by default

I think it is most likely because an Error named enum is declared in each module.

Copy link
Member

@yukibtc yukibtc May 9, 2023

Choose a reason for hiding this comment

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

For now, add #![allow(ambiguous_glob_reexports)] in the prelude module.

Comment on lines 11 to 15
use alloc::{
string::{self, String, ToString},
vec,
vec::Vec,
};
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for nitpicking... can you rewrite this as:

use alloc::string::{self, String, ToString};
use alloc::vec::{self, Vec};

Also in other parts of code

Comment on lines 36 to 42
#[error(transparent)]
Json(#[from] serde_json::Error),
#[error("Serde json Error: {0}")]
Json(serde_json::Error),
Copy link
Member

Choose a reason for hiding this comment

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

I'm not very expert about no_std, thiserror not support the #[from] in no_std env?
If not support it, I think we can remove the thiserror deps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the problem is the following:

thiserror crate assumes that the enum/struct implements the std::error::Error trait, which is not available in no_std. However the thiserror-core crate switched to the core::error::Error trait, which is available on nightly.

If you are not going to use the thiserror heavily, then I would say removing it would be a good idea.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove thiserror dep.

Maybe is better to do that in another PR.
If you don't have enough time, I can do it in the next days.

Copy link
Contributor Author

@OverOrion OverOrion May 9, 2023

Choose a reason for hiding this comment

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

I would appreciate it if you did that in another PR, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Merged #102

Comment on lines 73 to 76
rand_core = { version = "0.6", features = [
"alloc",
"getrandom",
], optional = true, default-features = false }
Copy link
Member

Choose a reason for hiding this comment

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

Is possible to keep only the rand dep?

@OverOrion
Copy link
Contributor Author

Thanks for the review, will address this soon!

@yukibtc yukibtc added this to the Release 0.23 milestone May 6, 2023
@yukibtc yukibtc added the nostr label May 6, 2023
@yukibtc yukibtc modified the milestones: Release 0.23, Release 0.22 May 9, 2023
@OverOrion OverOrion marked this pull request as ready for review May 12, 2023 11:43
Copy link
Member

@yukibtc yukibtc left a comment

Choose a reason for hiding this comment

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

Thank you for the changes :)

Since two deps (bech32 and url_no_std) aren't released to crates.io yet, I prefer to wait to merge this to the master branch.
The code looks good to me, but I want to make some cleanups.
So, instead of approve and merge this PR, is okay for you if I move the code/commits to a new branch (no_std branch) in this repo and close this PR, so I can do the cleanup and merge by myself when the previous mentioned crates are published?

And, do you plan to open a PR to the url repo to add no_std support? (I saw that the url_no_std crate is your url fork)

@yukibtc
Copy link
Member

yukibtc commented May 12, 2023

Can you leave me your nostr pubkey? So when I'll release the new version with no_std support, I'll mention you.

@OverOrion
Copy link
Contributor Author

Since two deps (bech32 and url_no_std) aren't released to crates.io yet, I prefer to wait to merge this to the master branch.

Understandable, hopefully they get merged soon. The rust_url crate has an open PR, I just used forked that PR to have a stable reference :).

So, instead of approve and merge this PR, is okay for you if I move the code/commits to a new branch (no_std branch) in this repo and close this PR, so I can do the cleanup and merge by myself when the previous mentioned crates are published?

Absolutely, I wanted to do it myself, but can not find the time, much thanks for it in advance!

@OverOrion
Copy link
Contributor Author

Can you leave me your nostr pubkey? So when I'll release the new version with no_std support, I'll mention you.

Thanks, much appreciated here is my nostr public key:
npub1vjxrxzr5epxxf5qzxeachshvafhcj5utct5dlw60qx7ayvq95mhsypdl8u

@yukibtc yukibtc closed this May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants