`alloc` feature added, to gate unicode normalization #57

michalkucharczyk · 2023-11-02T08:14:47Z

This PR introduces the alloc feature.

The alloc is intended to use in no-std environments which are allowed to use alloc. New feature enables:

the unicode-normalization, and all related methods (parse_in,normalize_utf8_cow,parse,to_seed)
fn to_entropy() method as Vec is available in alloc,

Some formatting sneaked in, too.

Fix for #55.

michalkucharczyk · 2023-11-02T09:39:43Z

Maybe I should introduce alloc feature and gate unicode-normalization and to_entropy with it?

Cargo.toml

tcharding · 2023-11-02T22:11:30Z

Otherwise, looks good! Thanks.

Cargo.toml

tcharding · 2023-11-08T19:47:08Z

Thanks for fixing it up! Can you squash the fixes into the patches that they fix please. We don't squash when merging so each patch should be correct on its own. Sorry for taking so long to get back to this.

michalkucharczyk · 2023-11-09T06:45:34Z

I'd squash all the commits to the single one, and force push. @tcharding would that work for you?

tcharding · 2023-11-10T01:41:14Z

Yep, that is fine for me.

michalkucharczyk · 2023-11-13T08:39:15Z

@tcharding I've squashed all the patches into single one.

benma · 2023-11-14T03:36:02Z

Thanks for the effort @michalkucharczyk. I started on the same before seeing this 🙈 here is the issue that should be referenced: #55

Please see #59 too which re-enables some unit tests that accidentally never ran - could be relevant.

michalkucharczyk · 2023-11-14T08:19:59Z

Thanks for the effort @michalkucharczyk. I started on the same before seeing this 🙈 here is the issue that should be referenced: #55

Description updated.

Please see #59 too which re-enables some unit tests that accidentally never ran - could be relevant.

I've check the tests (with feature) locally: all of them are green. Let's keep features vs feature fix in #59.

Cargo.toml

src/lib.rs

tcharding

ACK c62f91b

tcharding · 2023-11-15T22:21:13Z

Thanks man!

src/lib.rs

michalkucharczyk · 2024-02-01T11:21:19Z

@stevenroose thanks for review - all addressed. would appreciate your feedback again.

michalkucharczyk · 2024-02-02T23:13:50Z

what is wrong with enabling it? if we are in std it shall be propagated there, right?

…

On Sat, 3 Feb 2024 at 00:05, Tobin C. Harding ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Cargo.toml <#57 (comment)> : > @@ -13,8 +13,9 @@ edition = "2018" [features] default = [ "std" ] -std = [ "unicode-normalization", "serde/std" ] +std = [ "alloc", "serde/std", "unicode-normalization/std" ] Yes but we do not need the unicode-normalization "std" feature so why enable it? Also "unicode-normalization" is enabled already in "alloc". — Reply to this email directly, view it on GitHub <#57 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANF4TUBGDBRCP4AP3GGFGLYRVWNBAVCNFSM6AAAAAA62PCI2KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNRQGYYDCMBUGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Parity Technologies is a limited company registered in England and Wales with registered number 09760015 and registered office at c/o Ignition Law, 1 Sans Walk, London, England, EC1R 0LT. This message is intended solely for the addressee(s) and may contain confidential information. If you have received this message in error, please notify us, and immediately and permanently delete it. Do not use, copy or disclose the information contained in this message or in any attachment. For information about how we process data and monitor communications please see our Privacy policy (https://www.parity.io/privacy/ <https://www.parity.io/privacy/>)and for terms of use please see our Terms of Use policy (https://www.parity.io/terms/ <https://www.parity.io/terms/>).

tcharding · 2024-02-02T23:09:25Z

Cargo.toml

@@ -49,7 +50,7 @@ zeroize = { version = "1.5", features = ["zeroize_derive"], optional = true }

 # Unexported dependnecies
 bitcoin_hashes = { version = ">=0.12, <=0.13", default-features = false }
-unicode-normalization = { version = "=0.1.22", optional = true }
+unicode-normalization = { version = "=0.1.22", default-features = false, optional = true }


Oh I did not notice before but I don't think this should be a fixed version? Should be version = "0.1.22".

No then it will upgrade and this crate has repeatedly broken MSRV for some of my projects. It's an internal dep so the version used shouldn't matter. Luckily 0.1.22 now is over a year old and seems like he saturated the project too. Which is ok :)

It makes it a bit hard to package on distributions though if dependencies get out of sync, one would have to patch the Cargo.toml file to allow other versions.

Yeah I rekon we should handle MSRV breakage with pinning not in the manifest like this.

Cargo.toml

tcharding · 2024-02-02T23:20:28Z

what is wrong with enabling it? if we are in std it shall be propagated there, right?

I think we are talking about "unicode-normalization" from "std", right? If its an optional dependency then it should be optional, if its enabled in "std" its not really optional.

michalkucharczyk · 2024-02-02T23:31:45Z

I wanted it to be optional in no-std. Originally it was always enabled in std, so I kept it enabled (with default features on, which is std) to be backward compatible.

…

On Sat, 3 Feb 2024 at 00:20, Tobin C. Harding ***@***.***> wrote: what is wrong with enabling it? if we are in std it shall be propagated there, right? I think we are talking about "unicode-normalization" from "std", right? If its an optional dependency then it should be optional, if its enabled in "std" its not really optional. — Reply to this email directly, view it on GitHub <#57 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANF4TVZI5QA45C3J4TXSRTYRVYETAVCNFSM6AAAAAA62PCI2KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRUHA4TKMZVHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Parity Technologies is a limited company registered in England and Wales with registered number 09760015 and registered office at c/o Ignition Law, 1 Sans Walk, London, England, EC1R 0LT. This message is intended solely for the addressee(s) and may contain confidential information. If you have received this message in error, please notify us, and immediately and permanently delete it. Do not use, copy or disclose the information contained in this message or in any attachment. For information about how we process data and monitor communications please see our Privacy policy (https://www.parity.io/privacy/ <https://www.parity.io/privacy/>)and for terms of use please see our Terms of Use policy (https://www.parity.io/terms/ <https://www.parity.io/terms/>).

tcharding · 2024-02-02T23:50:24Z

Oh I see. I'll ack this as is then and we can look at the features more closely later. Thanks for explaining.

tcharding

ACK eb35a97

michalkucharczyk · 2024-02-09T12:58:09Z

Is this failure: Continuous integration / Test - 1.41.1 toolchain something I should worry about? (btw: I see it is also failing for other commits).

stevenroose · 2024-02-09T18:33:29Z

Can you rebase on master so that the MSRV build works? Thanks.

This commit introduces the `alloc` feature. The alloc feature is intended to use in no-std environments which are allowed to use alloc. New feature enables: - the unicode-normalization, and all related methods (parse_in,normalize_utf8_cow,parse,to_seed) - to_entropy() method as Vec is available in alloc,

michalkucharczyk · 2024-02-10T11:09:06Z

Can you rebase on master so that the MSRV build works? Thanks.

Done (force pushed).

tcharding · 2024-02-10T19:29:06Z

@stevenroose can you kick CI please. On #64 as well.

michalkucharczyk · 2024-03-05T15:33:40Z

any updates here?

This functionality is required for #1984. This PR enables [`sp-keyring`](https://github.com/paritytech/polkadot-sdk/blob/21d36b7b4229c4d5225944f197918cde23fda4ea/substrate/primitives/keyring/src/sr25519.rs#L31-L40) in `no-std` environments, allowing to generate the public key (e.g. `AccountKeyring::Alice.public().to_ss58check()`), which can be later used in the any of built-in [_runtime-genesis-config_ variant](https://github.com/paritytech/polkadot-sdk/blob/21d36b7b4229c4d5225944f197918cde23fda4ea/polkadot/node/service/src/chain_spec.rs#L1066-L1073). The proposal is as follows: - expose [`core::Pair` trait](https://github.com/paritytech/polkadot-sdk/blob/d6f15306282e3de848a09c9aa9cba6f95a7811f0/substrate/primitives/core/src/crypto.rs#L832) in `no-std`, - `full_crypto` feature enables `sign` method, - `std` feature enables `generate_with_phrase` and `generate` methods (randomness is required), - All other functionality, currently gated by `full_crypto` will be available unconditionally (`no-std`): -- `from_string` -- `from_string_with_seed` -- `from seed` -- `from_seed_slice` -- `from_phrase` -- `derive` -- `verify` --- Depends on rust-bitcoin/rust-bip39#57 --------- Co-authored-by: command-bot <> Co-authored-by: Davide Galassi <[email protected]>

This functionality is required for paritytech#1984. This PR enables [`sp-keyring`](https://github.com/paritytech/polkadot-sdk/blob/21d36b7b4229c4d5225944f197918cde23fda4ea/substrate/primitives/keyring/src/sr25519.rs#L31-L40) in `no-std` environments, allowing to generate the public key (e.g. `AccountKeyring::Alice.public().to_ss58check()`), which can be later used in the any of built-in [_runtime-genesis-config_ variant](https://github.com/paritytech/polkadot-sdk/blob/21d36b7b4229c4d5225944f197918cde23fda4ea/polkadot/node/service/src/chain_spec.rs#L1066-L1073). The proposal is as follows: - expose [`core::Pair` trait](https://github.com/paritytech/polkadot-sdk/blob/d6f15306282e3de848a09c9aa9cba6f95a7811f0/substrate/primitives/core/src/crypto.rs#L832) in `no-std`, - `full_crypto` feature enables `sign` method, - `std` feature enables `generate_with_phrase` and `generate` methods (randomness is required), - All other functionality, currently gated by `full_crypto` will be available unconditionally (`no-std`): -- `from_string` -- `from_string_with_seed` -- `from seed` -- `from_seed_slice` -- `from_phrase` -- `derive` -- `verify` --- Depends on rust-bitcoin/rust-bip39#57 --------- Co-authored-by: command-bot <> Co-authored-by: Davide Galassi <[email protected]>

michalkucharczyk mentioned this pull request Nov 2, 2023

Support for keyring in runtimes paritytech/polkadot-sdk#2044

Merged

michalkucharczyk changed the title ~~unicode_normalization enabled in no-std~~ alloc feature added, to gate unicode normalization Nov 2, 2023

tcharding reviewed Nov 2, 2023

View reviewed changes

Cargo.toml Show resolved Hide resolved

Cargo.toml Outdated Show resolved Hide resolved

michalkucharczyk commented Nov 2, 2023

View reviewed changes

Cargo.toml Show resolved Hide resolved

michalkucharczyk commented Nov 2, 2023

View reviewed changes

Cargo.toml Show resolved Hide resolved

michalkucharczyk requested a review from tcharding November 3, 2023 17:14

michalkucharczyk force-pushed the mku-unicode-normalization-in-no-std branch from e63f0b2 to 1372108 Compare November 13, 2023 08:37

tcharding reviewed Nov 14, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

src/lib.rs Show resolved Hide resolved

src/lib.rs Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

michalkucharczyk force-pushed the mku-unicode-normalization-in-no-std branch from 1372108 to c62f91b Compare November 15, 2023 07:08

michalkucharczyk requested a review from tcharding November 15, 2023 07:09

tcharding approved these changes Nov 15, 2023

View reviewed changes

stevenroose reviewed Jan 29, 2024

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

michalkucharczyk force-pushed the mku-unicode-normalization-in-no-std branch from c62f91b to 9180d56 Compare February 1, 2024 11:15

michalkucharczyk force-pushed the mku-unicode-normalization-in-no-std branch from 9180d56 to eb35a97 Compare February 2, 2024 08:37

tcharding reviewed Feb 2, 2024

View reviewed changes

tcharding approved these changes Feb 2, 2024

View reviewed changes

stevenroose mentioned this pull request Feb 9, 2024

Release 2.1.0 #54

Merged

michalkucharczyk added 2 commits February 10, 2024 12:07

formatting

09d89c5

michalkucharczyk force-pushed the mku-unicode-normalization-in-no-std branch from eb35a97 to 86353a5 Compare February 10, 2024 11:08

michalkucharczyk mentioned this pull request Mar 12, 2024

Remove deps to parity-bip39 paritytech/polkadot-sdk#3655

Open

stevenroose merged commit 6f6ec34 into rust-bitcoin:master Mar 28, 2024

michalkucharczyk mentioned this pull request Mar 28, 2024

compilation fix #67

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`alloc` feature added, to gate unicode normalization #57

`alloc` feature added, to gate unicode normalization #57

michalkucharczyk commented Nov 2, 2023 •

edited

Loading

michalkucharczyk commented Nov 2, 2023

tcharding commented Nov 2, 2023

tcharding commented Nov 8, 2023

michalkucharczyk commented Nov 9, 2023

tcharding commented Nov 10, 2023

michalkucharczyk commented Nov 13, 2023

benma commented Nov 14, 2023 •

edited

Loading

michalkucharczyk commented Nov 14, 2023

tcharding left a comment

tcharding commented Nov 15, 2023

michalkucharczyk commented Feb 1, 2024

michalkucharczyk commented Feb 2, 2024 via email

tcharding Feb 2, 2024

stevenroose Feb 9, 2024

jeandudey Feb 12, 2024

tcharding Feb 12, 2024

tcharding commented Feb 2, 2024

michalkucharczyk commented Feb 2, 2024 via email

tcharding commented Feb 2, 2024

tcharding left a comment

michalkucharczyk commented Feb 9, 2024 •

edited

Loading

stevenroose commented Feb 9, 2024 •

edited

Loading

michalkucharczyk commented Feb 10, 2024

tcharding commented Feb 10, 2024

michalkucharczyk commented Mar 5, 2024

alloc feature added, to gate unicode normalization #57

alloc feature added, to gate unicode normalization #57

Conversation

michalkucharczyk commented Nov 2, 2023 • edited Loading

michalkucharczyk commented Nov 2, 2023

tcharding commented Nov 2, 2023

tcharding commented Nov 8, 2023

michalkucharczyk commented Nov 9, 2023

tcharding commented Nov 10, 2023

michalkucharczyk commented Nov 13, 2023

benma commented Nov 14, 2023 • edited Loading

michalkucharczyk commented Nov 14, 2023

tcharding left a comment

Choose a reason for hiding this comment

tcharding commented Nov 15, 2023

michalkucharczyk commented Feb 1, 2024

michalkucharczyk commented Feb 2, 2024 via email

tcharding Feb 2, 2024

Choose a reason for hiding this comment

stevenroose Feb 9, 2024

Choose a reason for hiding this comment

jeandudey Feb 12, 2024

Choose a reason for hiding this comment

tcharding Feb 12, 2024

Choose a reason for hiding this comment

tcharding commented Feb 2, 2024

michalkucharczyk commented Feb 2, 2024 via email

tcharding commented Feb 2, 2024

tcharding left a comment

Choose a reason for hiding this comment

michalkucharczyk commented Feb 9, 2024 • edited Loading

stevenroose commented Feb 9, 2024 • edited Loading

michalkucharczyk commented Feb 10, 2024

tcharding commented Feb 10, 2024

michalkucharczyk commented Mar 5, 2024

`alloc` feature added, to gate unicode normalization #57

`alloc` feature added, to gate unicode normalization #57

michalkucharczyk commented Nov 2, 2023 •

edited

Loading

benma commented Nov 14, 2023 •

edited

Loading

michalkucharczyk commented Feb 9, 2024 •

edited

Loading

stevenroose commented Feb 9, 2024 •

edited

Loading