No std upstream working #90

OverOrion · 2023-04-21T14:29:49Z

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:

I've signed all my commits
I followed the contribution guidelines
I ran make precommit before committing

OverOrion · 2023-05-02T06:13:29Z

Hey @yukibtc.

Just a polite ping, looking for feedback :)

yukibtc

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?

yukibtc · 2023-05-02T07:38:43Z

Cargo.toml

@@ -10,8 +10,9 @@ members = [
    "crates/nostr-sdk",
    "crates/nostr-sdk-net",
    "crates/nostr-sdk-sqlite",
+    "crates/ensure_no_std",


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

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

yukibtc · 2023-05-02T07:42:21Z

crates/nostr/src/prelude.rs

-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;


Here is possible to leave the ::*?

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.

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

yukibtc · 2023-05-02T07:45:06Z

crates/nostr/src/nips/nip19.rs

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


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

yukibtc · 2023-05-02T07:47:45Z

crates/nostr/src/event/builder.rs

-    #[error(transparent)]
-    Json(#[from] serde_json::Error),
+    #[error("Serde json Error: {0}")]
+    Json(serde_json::Error),


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.

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.

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.

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

Merged #102

yukibtc · 2023-05-02T08:03:38Z

crates/nostr/Cargo.toml

+rand_core = { version = "0.6", features = [
+    "alloc",
+    "getrandom",
+], optional = true, default-features = false }


Is possible to keep only the rand dep?

OverOrion · 2023-05-05T07:29:52Z

Thanks for the review, will address this soon!

This reverts commit 77ef1c8.

yukibtc

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 · 2023-05-12T13:44:44Z

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

OverOrion · 2023-05-12T14:39:33Z

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 · 2023-05-12T14:43:28Z

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 reviewed May 2, 2023

View reviewed changes

yukibtc added this to the Release 0.23 milestone May 6, 2023

yukibtc assigned OverOrion May 6, 2023

yukibtc added the nostr label May 6, 2023

yukibtc modified the milestones: Release 0.23, Release 0.22 May 9, 2023

yukibtc mentioned this pull request May 9, 2023

nostr: remove thiserror dep #102

Merged

OverOrion force-pushed the no_std_upstream_working branch from 2dc247e to ee3c989 Compare May 12, 2023 11:23

OverOrion and others added 20 commits May 12, 2023 13:24

nostr: add std feature

ee655f3

nostr: use thiserror-core with alloc feature

598f99b

nostr: make time module no_std compatible

23ce7a0

nostr: make nostr crate no_std compatible

c018213

This reverts commit 77ef1c8.

nostr: no_std support

20cffbf

nostr: port nip26

1380fe9

nostr: port nip13

2d4d690

nostr: port nip65

d7c6469

nostr: outsourcing time for no_std

98374cd

nostr: implement TimeSupplier trait for std environments

67cc842

nostr: compiling with --no-default-features --features=alloc

6edffb3

nostr: EventBuilder cleanup

33a6d0f

nostr: add missing documentation comments

77b08b2

nostr: make sure dependencies are no_std compatible

32c2975

nostr: adjust secp256k1 related things in no_std

501b2b5

no_std: switch back to upstream rust-url

7c16daa

nostr: compiling

0c80a80

nostr: accept that other crates don't implement error

0196a15

add no std test crate

1ddbbb2

make it work

084354f

OverOrion added 23 commits May 12, 2023 13:29

fix event/id.rs file

ccd238f

fix nips/nip58.rs file

ccca677

nostr: use core::error::Error where possible

9d4e542

remove duplicate impl froms

94647c2

fix missing import in nip58.rs

f041765

nostr/nips: port NIP58 to no_std

8e05474

minor fixes

0135ca7

gha: add job for no_std nostr

f1f518a

examples format, should be dropped

1ac775e

fix ensure_no_std ci

44987eb

gha: fix std build

41de424

fix std::error::Error in vanity.rs

a37e0ca

nostr-*: add nostr with std feature

26759e7

nostr: fix vanity feature

15c5de1

fix clippy warnings

da450d0

nostr: vanity.rs fix

2af0f3f

nostr: fix clippy warning

a2d86bd

nostr/examples/nip65: fix example

7b82286

fix bech32 imports

fd736df

nostr: fix WASM build

d264da8

partially revert examples modification

7b1a4f3

fix preludes

2add303

nostr/message/subscription: remove duplicate fmt import

b389fdb

OverOrion force-pushed the no_std_upstream_working branch from ee3c989 to b389fdb Compare May 12, 2023 11:43

OverOrion marked this pull request as ready for review May 12, 2023 11:43

yukibtc reviewed May 12, 2023

View reviewed changes

yukibtc closed this May 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No std upstream working #90

No std upstream working #90

OverOrion commented Apr 21, 2023

OverOrion commented May 2, 2023

yukibtc left a comment

yukibtc May 2, 2023

OverOrion May 9, 2023

yukibtc May 2, 2023

OverOrion May 9, 2023

yukibtc May 9, 2023 •

edited

yukibtc May 2, 2023

yukibtc May 2, 2023

OverOrion May 9, 2023

yukibtc May 9, 2023

OverOrion May 9, 2023 •

edited

yukibtc May 11, 2023

yukibtc May 2, 2023

OverOrion commented May 5, 2023

yukibtc left a comment

yukibtc commented May 12, 2023 •

edited

OverOrion commented May 12, 2023

OverOrion commented May 12, 2023

No std upstream working #90

No std upstream working #90

Conversation

OverOrion commented Apr 21, 2023

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

OverOrion commented May 2, 2023

yukibtc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yukibtc May 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OverOrion May 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OverOrion commented May 5, 2023

yukibtc left a comment

Choose a reason for hiding this comment

yukibtc commented May 12, 2023 • edited

OverOrion commented May 12, 2023

OverOrion commented May 12, 2023

yukibtc May 9, 2023 •

edited

OverOrion May 9, 2023 •

edited

yukibtc commented May 12, 2023 •

edited