chore(dashcore): drop no-std supp in dashcore#521
Conversation
📝 WalkthroughWalkthroughRemoved conditional no-std/std compilation and reworked feature wiring: Cargo manifests updated to stop forcing "std" in some consumers; many std-gated Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
99b001c to
0900af5
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #521 +/- ##
=============================================
+ Coverage 66.06% 66.37% +0.30%
=============================================
Files 311 311
Lines 64976 64976
=============================================
+ Hits 42928 43127 +199
+ Misses 22048 21849 -199
|
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
0900af5 to
5581f99
Compare
5581f99 to
80bbdbb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dash/src/lib.rs (2)
31-44:⚠️ Potential issue | 🟡 MinorDocumentation still references removed
no-stdfeature.Lines 42-43 document the
no-stdfeature flag, but this PR is removing no-std support. Additionally, line 33 describesstdas "default" when it's now effectively required. Please update the documentation to reflect these changes.📝 Suggested documentation update
//! ## Available feature flags //! -//! * `std` - the usual dependency on `std` (default). +//! * `std` - dependency on `std` (required). //! * `secp-recovery` - enables calculating public key from a signature and message. //! * `signer` - enables singing and validation ECDSA helpers. //! * `base64` - (dependency), enables encoding of PSBTs and message signatures. //! * `unstable` - enables unstable features for testing. //! * `rand` - (dependency), makes it more convenient to generate random values. //! * `bincode` - (dependency), implements bincode serialization and deserialization. //! * `serde` - (dependency), implements `serde`-based serialization and deserialization. //! * `secp-lowmemory` - optimizations for low-memory devices. -//! * `no-std` - enables additional features required for this crate to be usable -//! without std. Does **not** disable `std`. Depends on `core2`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash/src/lib.rs` around lines 31 - 44, Update the crate-level documentation in dash/src/lib.rs to remove the stale `no-std` feature entry and adjust the `std` feature description: delete the bullet mentioning `no-std` (lines referencing the `no-std` feature) and change the `std` bullet (the line that currently says "`std` - the usual dependency on `std` (default).") to reflect that `std` is required (e.g., "`std` - required; this crate depends on the standard library"). Leave the rest of the feature list unchanged.
64-65:⚠️ Potential issue | 🟡 MinorMigrate from
alloctostdequivalents after removing no-std support.The
extern crate allocdeclaration and#[macro_use]are actively used in four files:
dash/src/hash_types.rs:78–use crate::alloc::string::ToStringdash/src/ephemerealdata/instant_lock.rs:6–use alloc::vec::Vecdash/src/consensus/serde.rs:19–use crate::alloc::string::ToStringdash/src/blockdata/script/mod.rs:51–use alloc::rc::RcIf no-std support is being removed and std is now required, replace these with their
std::equivalents (or use the standard prelude) and remove theextern crate allocdeclaration fromlib.rs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash/src/lib.rs` around lines 64 - 65, Remove the top-level #[macro_use] extern crate alloc declaration in lib.rs and replace all uses of alloc types with their std equivalents: change alloc::vec::Vec to std::vec::Vec (or plain Vec), crate::alloc::string::ToString to std::string::ToString (or use the prelude's ToString), and alloc::rc::Rc to std::rc::Rc; update the four affected modules (hash_types.rs, ephemerealdata/instant_lock.rs, consensus/serde.rs, blockdata/script/mod.rs) to import the std types or rely on the standard prelude and delete the extern crate alloc line from lib.rs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dash/Cargo.toml`:
- Line 22: The examples 'handshake', 'ecdsa-psbt', and 'taproot-psbt' reference
a non-existent "std" feature in Cargo.toml; remove the stale requirement by
deleting the line `required-features = ["std"]` from the handshake example and
by removing `"std"` from the required-features arrays for ecdsa-psbt and
taproot-psbt so their required-features only list valid features defined in the
[features] section.
In `@internals/src/error.rs`:
- Around line 18-19: The docs for the write_err! macro are out of date: they
claim the source text is appended in no-std mode but the implementation (see the
let _ = &$source; line and the write!($writer, $string $(, $args)*) call) now
always formats only $string and $args. Update the macro documentation/comment to
state that write_err! no longer appends source text in any mode, clarify that
$source is only referenced to silence clippy and not emitted, and remove or
rephrase any wording implying conditional no-std behavior for source appending.
---
Outside diff comments:
In `@dash/src/lib.rs`:
- Around line 31-44: Update the crate-level documentation in dash/src/lib.rs to
remove the stale `no-std` feature entry and adjust the `std` feature
description: delete the bullet mentioning `no-std` (lines referencing the
`no-std` feature) and change the `std` bullet (the line that currently says
"`std` - the usual dependency on `std` (default).") to reflect that `std` is
required (e.g., "`std` - required; this crate depends on the standard library").
Leave the rest of the feature list unchanged.
- Around line 64-65: Remove the top-level #[macro_use] extern crate alloc
declaration in lib.rs and replace all uses of alloc types with their std
equivalents: change alloc::vec::Vec to std::vec::Vec (or plain Vec),
crate::alloc::string::ToString to std::string::ToString (or use the prelude's
ToString), and alloc::rc::Rc to std::rc::Rc; update the four affected modules
(hash_types.rs, ephemerealdata/instant_lock.rs, consensus/serde.rs,
blockdata/script/mod.rs) to import the std types or rely on the standard prelude
and delete the extern crate alloc line from lib.rs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9a81fad0-3470-48c9-a4db-934741eb1137
📒 Files selected for processing (31)
dash-spv/Cargo.tomldash/Cargo.tomldash/src/address.rsdash/src/amount.rsdash/src/base58.rsdash/src/bip152.rsdash/src/bip158.rsdash/src/blockdata/block.rsdash/src/blockdata/locktime/absolute.rsdash/src/blockdata/locktime/relative.rsdash/src/blockdata/script/mod.rsdash/src/blockdata/transaction/outpoint.rsdash/src/consensus/encode.rsdash/src/crypto/ecdsa.rsdash/src/crypto/key.rsdash/src/crypto/sighash.rsdash/src/crypto/taproot.rsdash/src/ephemerealdata/chain_lock.rsdash/src/ephemerealdata/instant_lock.rsdash/src/error.rsdash/src/lib.rsdash/src/merkle_tree/block.rsdash/src/network/mod.rsdash/src/pow.rsdash/src/sign_message.rsdash/src/string.rsdash/src/taproot.rsinternals/src/error.rskey-wallet-ffi/Cargo.tomlkey-wallet-manager/Cargo.tomlrpc-json/Cargo.toml
💤 Files with no reviewable changes (22)
- dash/src/crypto/taproot.rs
- dash/src/ephemerealdata/chain_lock.rs
- dash/src/blockdata/block.rs
- dash/src/network/mod.rs
- dash/src/consensus/encode.rs
- dash/src/bip158.rs
- dash/src/sign_message.rs
- dash/src/blockdata/locktime/absolute.rs
- dash/src/crypto/sighash.rs
- dash/src/blockdata/locktime/relative.rs
- dash/src/blockdata/script/mod.rs
- dash/src/string.rs
- dash/src/amount.rs
- dash/src/address.rs
- dash/src/base58.rs
- dash/src/bip152.rs
- dash/src/taproot.rs
- dash/src/pow.rs
- dash/src/crypto/ecdsa.rs
- dash/src/merkle_tree/block.rs
- dash/src/error.rs
- dash/src/blockdata/transaction/outpoint.rs
80bbdbb to
57e23a3
Compare
57e23a3 to
b191e23
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dash/src/ephemerealdata/instant_lock.rs (1)
5-12: Consider simplifying Vec imports if std is now unconditional.Since the PR drops no-std support,
stdis always available. Bothalloc::vec::Vecandstd::vec::Vecare the same type, so the conditional import logic adds unnecessary complexity. You could simplify to:use std::vec::Vec;Or rely on the prelude and remove these imports entirely, since
Vecis already in scope by default.If this is an intentional intermediate state (per the PR description about hashes std dependency), consider adding a brief comment explaining why
alloc::vec::Vecis still used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash/src/ephemerealdata/instant_lock.rs` around lines 5 - 12, The conditional imports of Vec in instant_lock.rs (the #[cfg(not(test)) use alloc::vec::Vec;] and #[cfg(test)] pub use std::vec::Vec;) are unnecessary now that std is always available; remove both conditional imports and replace with a single use std::vec::Vec; (or remove them entirely and rely on the prelude) to simplify the module imports—update any comment near the top of instant_lock.rs if you intend to keep alloc for historical reasons and reference the Vec import removal in that comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet-manager/Cargo.toml`:
- Line 21: The dashcore dependency in Cargo.toml currently pulls in its default
features (secp-recovery, bincode) and should be made explicit to match
key-wallet's pattern; update the dashcore entry to include default-features =
false so dashcore does not enable its defaults transitively (ensure the
dependency line referencing dashcore in Cargo.toml is changed to include
default-features = false while keeping the path = "../dash" unchanged).
---
Nitpick comments:
In `@dash/src/ephemerealdata/instant_lock.rs`:
- Around line 5-12: The conditional imports of Vec in instant_lock.rs (the
#[cfg(not(test)) use alloc::vec::Vec;] and #[cfg(test)] pub use std::vec::Vec;)
are unnecessary now that std is always available; remove both conditional
imports and replace with a single use std::vec::Vec; (or remove them entirely
and rely on the prelude) to simplify the module imports—update any comment near
the top of instant_lock.rs if you intend to keep alloc for historical reasons
and reference the Vec import removal in that comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2cb4bf0f-b3e3-4e6d-9768-ab06ab84c749
📒 Files selected for processing (31)
dash-spv/Cargo.tomldash/Cargo.tomldash/src/address.rsdash/src/amount.rsdash/src/base58.rsdash/src/bip152.rsdash/src/bip158.rsdash/src/blockdata/block.rsdash/src/blockdata/locktime/absolute.rsdash/src/blockdata/locktime/relative.rsdash/src/blockdata/script/mod.rsdash/src/blockdata/transaction/outpoint.rsdash/src/consensus/encode.rsdash/src/crypto/ecdsa.rsdash/src/crypto/key.rsdash/src/crypto/sighash.rsdash/src/crypto/taproot.rsdash/src/ephemerealdata/chain_lock.rsdash/src/ephemerealdata/instant_lock.rsdash/src/error.rsdash/src/lib.rsdash/src/merkle_tree/block.rsdash/src/network/mod.rsdash/src/pow.rsdash/src/sign_message.rsdash/src/string.rsdash/src/taproot.rsinternals/src/error.rskey-wallet-ffi/Cargo.tomlkey-wallet-manager/Cargo.tomlrpc-json/Cargo.toml
💤 Files with no reviewable changes (22)
- dash/src/address.rs
- dash/src/base58.rs
- dash/src/crypto/sighash.rs
- dash/src/blockdata/locktime/absolute.rs
- dash/src/ephemerealdata/chain_lock.rs
- dash/src/blockdata/block.rs
- dash/src/blockdata/script/mod.rs
- dash/src/pow.rs
- dash/src/bip152.rs
- dash/src/string.rs
- dash/src/crypto/taproot.rs
- dash/src/consensus/encode.rs
- dash/src/sign_message.rs
- dash/src/blockdata/transaction/outpoint.rs
- dash/src/bip158.rs
- dash/src/error.rs
- dash/src/taproot.rs
- dash/src/merkle_tree/block.rs
- dash/src/blockdata/locktime/relative.rs
- dash/src/crypto/ecdsa.rs
- dash/src/network/mod.rs
- dash/src/amount.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- key-wallet-ffi/Cargo.toml
- rpc-json/Cargo.toml
- dash/src/crypto/key.rs
- dash/Cargo.toml
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
rpc-json/Cargo.toml (1)
29-29: Avoid implicit default-feature coupling ondashcore.This currently enables
dashcoredefaults implicitly (includingbincode). Keepdefault-features = falseand list only required features explicitly to prevent future feature drift.🔧 Suggested manifest adjustment
-dashcore = { path = "../dash", features=["secp-recovery", "rand-std", "signer", "serde"] } +dashcore = { path = "../dash", default-features = false, features=["secp-recovery", "rand-std", "signer", "serde"] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rpc-json/Cargo.toml` at line 29, The dashcore dependency currently pulls in default features implicitly; update the Cargo.toml dashcore entry to set default-features = false and explicitly enumerate only the required features (e.g., "secp-recovery", "rand-std", "signer", "serde") so the dependency line for dashcore disables defaults and lists the exact features to avoid unintended feature drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dash/Cargo.toml`:
- Line 22: The crate-level docs in dash/src/lib.rs still mention legacy
`std`/`no-std` feature flags which no longer match the manifest (see Cargo.toml
features like the default set with "secp-recovery" and "bincode"); update the
documentation block in dash/src/lib.rs to remove or replace any references to
`std` and `no-std` and instead describe the current feature model (e.g., listing
actual feature names such as "secp-recovery" and "bincode" or referring readers
to Cargo.toml), ensuring the crate doc comment (the top-level module doc ///! or
/*! */ section) accurately reflects the current feature flags and integration
guidance.
- Line 22: The change that alters default features in Cargo.toml (the line
setting default = ["secp-recovery", "bincode"]) drops no-std support and should
be treated as a breaking change; update the PR title and commit message to
include a BREAKING: marker (or add an explicit breaking note in the PR/release
notes) and ensure the changelog/release entry documents the removed no-std
support so downstream users are aware; also verify pr-title.yml workflow rules
align with this new classification.
In `@dash/src/lib.rs`:
- Line 124: The change unconditionally exposing std via `pub use std::io` breaks
no-std support but the crate docs and advertised feature flags (the doc comments
in lib.rs that describe std/no-std) and the PR title were not updated; update
the crate-level documentation in lib.rs to remove or clearly mark that no-std is
no longer supported, adjust the feature flag docs to reflect that `std` is now
required (or document the new support matrix), and change the PR title to
indicate a breaking change (e.g., prefix with "BREAKING:") so workflows and
downstream consumers are aware; ensure references to `pub use std::io` remain
and that any cargo feature declarations or README statements match this new
requirement.
---
Nitpick comments:
In `@rpc-json/Cargo.toml`:
- Line 29: The dashcore dependency currently pulls in default features
implicitly; update the Cargo.toml dashcore entry to set default-features = false
and explicitly enumerate only the required features (e.g., "secp-recovery",
"rand-std", "signer", "serde") so the dependency line for dashcore disables
defaults and lists the exact features to avoid unintended feature drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c8fcdef0-930f-47c2-80b0-b7d23427a117
📒 Files selected for processing (32)
dash-spv/Cargo.tomldash/Cargo.tomldash/src/address.rsdash/src/amount.rsdash/src/base58.rsdash/src/bip152.rsdash/src/bip158.rsdash/src/blockdata/block.rsdash/src/blockdata/locktime/absolute.rsdash/src/blockdata/locktime/relative.rsdash/src/blockdata/script/mod.rsdash/src/blockdata/transaction/outpoint.rsdash/src/consensus/encode.rsdash/src/consensus/serde.rsdash/src/crypto/ecdsa.rsdash/src/crypto/key.rsdash/src/crypto/sighash.rsdash/src/crypto/taproot.rsdash/src/ephemerealdata/chain_lock.rsdash/src/ephemerealdata/instant_lock.rsdash/src/error.rsdash/src/lib.rsdash/src/merkle_tree/block.rsdash/src/network/mod.rsdash/src/pow.rsdash/src/sign_message.rsdash/src/string.rsdash/src/taproot.rsinternals/src/error.rskey-wallet-ffi/Cargo.tomlkey-wallet-manager/Cargo.tomlrpc-json/Cargo.toml
💤 Files with no reviewable changes (22)
- dash/src/bip152.rs
- dash/src/address.rs
- dash/src/merkle_tree/block.rs
- dash/src/sign_message.rs
- dash/src/crypto/taproot.rs
- dash/src/consensus/serde.rs
- dash/src/crypto/sighash.rs
- dash/src/blockdata/locktime/relative.rs
- dash/src/amount.rs
- dash/src/error.rs
- dash/src/taproot.rs
- dash/src/blockdata/block.rs
- dash/src/ephemerealdata/chain_lock.rs
- dash/src/consensus/encode.rs
- dash/src/base58.rs
- dash/src/blockdata/locktime/absolute.rs
- dash/src/string.rs
- dash/src/crypto/ecdsa.rs
- dash/src/pow.rs
- dash/src/bip158.rs
- dash/src/network/mod.rs
- dash/src/blockdata/transaction/outpoint.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- dash/src/ephemerealdata/instant_lock.rs
- dash-spv/Cargo.toml
- internals/src/error.rs
- key-wallet-manager/Cargo.toml
b191e23 to
b408a3e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internals/src/error.rs (1)
14-15:⚠️ Potential issue | 🟠 MajorSource error details are now silently discarded from error messages.
The
write_err!macro now binds$sourceonly to silence clippy but never includes it in the output. Multiple callers rely on this macro to format errors with their sources:
dash/src/parse.rs:62–ParseIntErrorloses underlyingcore::num::ParseIntErrordetailsdash/src/string.rs:57–FromHexError::ParseHexloses the parsing errordash/src/error.rs:26–Error::Encodeloses the encoding error detailsUsers will see messages like
"failed to parse '...' as a... integer"without the underlying cause. Consider whether this loss of diagnostic context is acceptable, or if the source should be appended viastd::error::Error::source()now that std is always available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internals/src/error.rs` around lines 14 - 15, The write_err! macro is currently binding $source only to silence clippy and not including it in the output, which drops underlying error context (affecting callers like ParseIntError in dash::parse, FromHexError::ParseHex, and Error::Encode); update the write_err! macro so it appends the source error to the formatted message instead of discarding it—e.g., format the primary message via write!($writer, $string $(, $args)*) and then, if $source is present, write the source using Display (or use std::error::Error::source() if appropriate) so the resulting output includes the original error details (ensure $source is formatted safely and no clippy warning remains).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internals/src/error.rs`:
- Around line 14-15: The write_err! macro is currently binding $source only to
silence clippy and not including it in the output, which drops underlying error
context (affecting callers like ParseIntError in dash::parse,
FromHexError::ParseHex, and Error::Encode); update the write_err! macro so it
appends the source error to the formatted message instead of discarding it—e.g.,
format the primary message via write!($writer, $string $(, $args)*) and then, if
$source is present, write the source using Display (or use
std::error::Error::source() if appropriate) so the resulting output includes the
original error details (ensure $source is formatted safely and no clippy warning
remains).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4e4152d8-1de1-4165-b996-d1bdfb268618
📒 Files selected for processing (32)
dash-spv/Cargo.tomldash/Cargo.tomldash/src/address.rsdash/src/amount.rsdash/src/base58.rsdash/src/bip152.rsdash/src/bip158.rsdash/src/blockdata/block.rsdash/src/blockdata/locktime/absolute.rsdash/src/blockdata/locktime/relative.rsdash/src/blockdata/script/mod.rsdash/src/blockdata/transaction/outpoint.rsdash/src/consensus/encode.rsdash/src/consensus/serde.rsdash/src/crypto/ecdsa.rsdash/src/crypto/key.rsdash/src/crypto/sighash.rsdash/src/crypto/taproot.rsdash/src/ephemerealdata/chain_lock.rsdash/src/ephemerealdata/instant_lock.rsdash/src/error.rsdash/src/lib.rsdash/src/merkle_tree/block.rsdash/src/network/mod.rsdash/src/pow.rsdash/src/sign_message.rsdash/src/string.rsdash/src/taproot.rsinternals/src/error.rskey-wallet-ffi/Cargo.tomlkey-wallet-manager/Cargo.tomlrpc-json/Cargo.toml
💤 Files with no reviewable changes (22)
- dash/src/ephemerealdata/chain_lock.rs
- dash/src/bip152.rs
- dash/src/address.rs
- dash/src/base58.rs
- dash/src/blockdata/locktime/relative.rs
- dash/src/crypto/sighash.rs
- dash/src/error.rs
- dash/src/bip158.rs
- dash/src/string.rs
- dash/src/consensus/serde.rs
- dash/src/blockdata/transaction/outpoint.rs
- dash/src/amount.rs
- dash/src/taproot.rs
- dash/src/blockdata/block.rs
- dash/src/sign_message.rs
- dash/src/crypto/ecdsa.rs
- dash/src/merkle_tree/block.rs
- dash/src/consensus/encode.rs
- dash/src/blockdata/locktime/absolute.rs
- dash/src/network/mod.rs
- dash/src/crypto/taproot.rs
- dash/src/pow.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- dash/src/blockdata/script/mod.rs
- dash-spv/Cargo.toml
- key-wallet-ffi/Cargo.toml
- dash/src/lib.rs
- dash/src/ephemerealdata/instant_lock.rs
need hashes std to be dropped first
Summary by CodeRabbit
Refactor
Chores