types%feat(secret): extract array-bound codec paths to dedicated module, add new macros dlgt_scodec! and derive_sbytes!, expand derive_{,s}bytes! scope, extract CompactSize - #24
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughChangesThe PR adds Codec modernization
Fixed issue severity: Not emitted because the provided context does not state the impact of a pre-existing defect. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
|
Warning This pull request may have conflicts, please coordinate with the authors of these pull requests. Potential conflicts |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
pkgs/types/src/secret.rs (1)
51-61: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winState that
ArrayBufdoes not wipe itself on drop.
ArrayBufimplementsZeroizebut notDrop. A bareArrayBufthat goes out of scope leaves its contents in memory. The wipe only happens when the buffer is moved intoArrEncoderorArrDecoder, or when the caller wraps it inZeroizing.pkgs/pkc/src/ecdsa/secret_ops.rsalready relies on theZeroizingwrapper and documents that requirement at its own call site.The type is exported at the crate root and is the crate's staging buffer for key material, so the obligation belongs in this doc comment. The omission of
Dropis correct, becausepkgs/types/src/adapters.rsusesArrayBuffor public Base58 data; only the documentation needs the addition.📝 Proposed doc addition
/// Fixed-size encode buffer backed by `[u8; N]`. /// +/// Does not wipe on drop. For secret material either move the buffer into +/// [`ArrEncoder`], or wrap it in `Zeroizing`; a bare drop leaves the bytes +/// in memory. +/// /// # Panics /// /// Writing more than `N` bytes (via the [`EncodeBuf`] impl) panics with an /// index-out-of-bounds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkgs/types/src/secret.rs` around lines 51 - 61, Update the documentation comment for the exported ArrayBuf type to explicitly state that it does not wipe its contents when dropped, and that callers must use Zeroizing or move it into ArrEncoder or ArrDecoder when cleanup is required. Do not add a Drop implementation or change the existing behavior.pkgs/types/src/entity.rs (2)
383-385: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExported macros resolve names against the invoking crate. Both sites emit code whose name resolution depends on what the caller imported, rather than routing through
$crateor an absolute path. The macros compile today only because the current call sites happen to have the right items in scope. Route every generated path through$crate::or a leading::.
pkgs/types/src/entity.rs#L383-L385: replace the.encode(buf)method call with$crate::codec::BaseCodec::encode(&..., buf), matching the fully qualifieddecodeat line 379 andHashable::hashat line 392.pkgs/types/src/macros.rs#L262-L285: change the fourcore::convert::Fromandcore::convert::TryFrompaths to::core::convert::Fromand::core::convert::TryFrom.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkgs/types/src/entity.rs` around lines 383 - 385, Update pkgs/types/src/entity.rs lines 383-385 in the generated encode method to call BaseCodec::encode through $crate::codec::BaseCodec, matching the existing fully qualified decode and Hashable calls. In pkgs/types/src/macros.rs lines 262-285, prefix all four core::convert::From and core::convert::TryFrom paths with :: so exported macros resolve independently of caller imports.
296-313: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse dash-types-owned serde gating in the exported byte macros.
All current in-repository callers declare a local
serdefeature and directserdedependency. When a downstream crate invokes the macro with onlydash-types/serde, the generated implementations are omitted. Use a$crate::__privateserde re-export and a dash-types-side cfg marker for these implementations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkgs/types/src/entity.rs` around lines 296 - 313, Update both generated Serialize and Deserialize implementations in the exported byte macros to use the dash-types-owned serde cfg marker instead of cfg(feature = "serde"), so downstream invocations honor dash-types/serde. Replace direct ::serde references with the serde re-export under $crate::__private, including Serializer, Deserializer, and de::Error paths, while preserving the existing hex encoding and decoding behavior.pkgs/types/src/macros.rs (1)
262-285: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
::core::convertfor the exported macro paths.The generated impls reference
core::convert::Fromandcore::convert::TryFromwithout the leading::. In an exported macro these paths resolve against the invoking crate's scope, so a local item namedcoreshadows them. Every other macro added in this PR uses::core::(seederive_bytes!andderive_sbytes!). Aligntype_cvrt!with that convention.♻️ Proposed fix
- impl<$($impl_generics)*> core::convert::From<&$src> for $dst { + impl<$($impl_generics)*> ::core::convert::From<&$src> for $dst { fn from($v: &$src) -> Self { $body } } - impl<$($impl_generics)*> core::convert::From<$src> for $dst { + impl<$($impl_generics)*> ::core::convert::From<$src> for $dst { fn from(v: $src) -> Self { Self::from(&v) } } }; (`@parse` [$($impl_generics:tt)*] TryFrom<$src:ty> for $dst:ty, $err:ty, |$v:ident| $body:expr) => { - impl<$($impl_generics)*> core::convert::TryFrom<&$src> for $dst { + impl<$($impl_generics)*> ::core::convert::TryFrom<&$src> for $dst { type Error = $err; fn try_from($v: &$src) -> Result<Self, Self::Error> { $body } } - impl<$($impl_generics)*> core::convert::TryFrom<$src> for $dst { + impl<$($impl_generics)*> ::core::convert::TryFrom<$src> for $dst { type Error = $err; fn try_from(v: $src) -> Result<Self, Self::Error> { Self::try_from(&v) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkgs/types/src/macros.rs` around lines 262 - 285, Update the generated impl paths in the type_cvrt! parsing arms to use absolute ::core::convert::From and ::core::convert::TryFrom, including their associated Result references, so invocation-crate items named core cannot shadow them. Keep the conversion behavior unchanged and align both borrowed and owned implementations.contrib/semgrep/types.yml (1)
8-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMatch parenthesis-form invocations in
types-macro-no-codec.The pattern requires
{after the macro name.make_bytes!(Foo, 20);andimpl_bytes!(Foo, 20);use parentheses and evade the rule. This PR adds parenthesis-form invocations of these macros across the workspace, so the brace-only form is now the less common spelling. Widen the delimiter class to keep the guard rail effective insidepkgs/types/src.Note that
pkgs/types/src/entity.rsdefinesmake_bytes!and calls$crate::impl_bytes!and$crate::derive_bytes!inside it. A widened pattern would fire there, so that file needs anosemgrepcomment or a path exclusion.♻️ Proposed change
- pattern-regex: '\b(?:make|impl)_(?:bytes|num|type)!\s*\{' + pattern-regex: '\b(?:make|impl)_(?:bytes|num|type)!\s*[({]'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contrib/semgrep/types.yml` around lines 8 - 9, Update the pattern-regex in types-macro-no-codec to match both brace- and parenthesis-delimited invocations after make_bytes, impl_bytes, and related macros, while preserving the existing macro-name scope. Add a nosemgrep annotation to the intentional definitions and internal calls in entity.rs, or exclude that file, so the widened rule remains limited to unintended usage under pkgs/types/src.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkgs/p2p_core/src/msg/headers.rs`:
- Around line 70-71: Constrain encoding in Headers::encode
(pkgs/p2p_core/src/msg/headers.rs, lines 70-71) and Headers2::encode
(pkgs/p2p_core/src/msg/headers2.rs, lines 59-60) to MAX_HEADERS: compute the
bounded header count, write that count, and encode exactly the corresponding
number of headers rather than the full collection.
In `@pkgs/primitives/src/types/addrv1.rs`:
- Around line 64-67: Rename the consuming conversion method from to_bytes to
into_bytes in the address type implementation, preserving its const behavior,
return type, and direct self.0 conversion.
In `@pkgs/types/src/serialize.rs`:
- Around line 9-24: Update the `hex` module documentation to describe only
`Vec<u8>` support, and add `# Errors` sections to both `hex::serialize` and
`hex::deserialize` documenting their possible serialization or hex-parsing
failures, matching the style used by the `utf8` module.
---
Nitpick comments:
In `@contrib/semgrep/types.yml`:
- Around line 8-9: Update the pattern-regex in types-macro-no-codec to match
both brace- and parenthesis-delimited invocations after make_bytes, impl_bytes,
and related macros, while preserving the existing macro-name scope. Add a
nosemgrep annotation to the intentional definitions and internal calls in
entity.rs, or exclude that file, so the widened rule remains limited to
unintended usage under pkgs/types/src.
In `@pkgs/types/src/entity.rs`:
- Around line 383-385: Update pkgs/types/src/entity.rs lines 383-385 in the
generated encode method to call BaseCodec::encode through
$crate::codec::BaseCodec, matching the existing fully qualified decode and
Hashable calls. In pkgs/types/src/macros.rs lines 262-285, prefix all four
core::convert::From and core::convert::TryFrom paths with :: so exported macros
resolve independently of caller imports.
- Around line 296-313: Update both generated Serialize and Deserialize
implementations in the exported byte macros to use the dash-types-owned serde
cfg marker instead of cfg(feature = "serde"), so downstream invocations honor
dash-types/serde. Replace direct ::serde references with the serde re-export
under $crate::__private, including Serializer, Deserializer, and de::Error
paths, while preserving the existing hex encoding and decoding behavior.
In `@pkgs/types/src/macros.rs`:
- Around line 262-285: Update the generated impl paths in the type_cvrt! parsing
arms to use absolute ::core::convert::From and ::core::convert::TryFrom,
including their associated Result references, so invocation-crate items named
core cannot shadow them. Keep the conversion behavior unchanged and align both
borrowed and owned implementations.
In `@pkgs/types/src/secret.rs`:
- Around line 51-61: Update the documentation comment for the exported ArrayBuf
type to explicitly state that it does not wipe its contents when dropped, and
that callers must use Zeroizing or move it into ArrEncoder or ArrDecoder when
cleanup is required. Do not add a Drop implementation or change the existing
behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 62c8e037-f6b7-40f0-a946-b09954a67c9d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (37)
contrib/codeql/zeroize.qlcontrib/semgrep/types.ymlpkgs/p2p_core/src/msg/addr.rspkgs/p2p_core/src/msg/headers.rspkgs/p2p_core/src/msg/headers2.rspkgs/p2p_core/src/primitives/command.rspkgs/p2p_core/src/primitives/user_agent.rspkgs/pkc/src/bls/public_bytes.rspkgs/pkc/src/bls/schemes.rspkgs/pkc/src/bls/secret_bytes.rspkgs/pkc/src/bls/sig_bytes.rspkgs/pkc/src/bls_chia/sk.rspkgs/pkc/src/bls_ietf/sk.rspkgs/pkc/src/ecdsa/public_bytes.rspkgs/pkc/src/ecdsa/public_hash.rspkgs/pkc/src/ecdsa/secret_bytes.rspkgs/pkc/src/ecdsa/secret_ops.rspkgs/pkc/src/ecdsa/sig_bytes.rspkgs/pkc/src/ecdsa/sig_rec_bytes.rspkgs/primitives/src/block.rspkgs/primitives/src/gov.rspkgs/primitives/src/payload/cbtx.rspkgs/primitives/src/support.rspkgs/primitives/src/transaction.rspkgs/primitives/src/types/addrv1.rspkgs/primitives/src/types/addrv2.rspkgs/primitives/src/types/netinfo.rspkgs/types/Cargo.tomlpkgs/types/src/adapters.rspkgs/types/src/codec.rspkgs/types/src/compact.rspkgs/types/src/entity.rspkgs/types/src/hex.rspkgs/types/src/lib.rspkgs/types/src/macros.rspkgs/types/src/secret.rspkgs/types/src/serialize.rs
💤 Files with no reviewable changes (2)
- pkgs/types/src/hex.rs
- pkgs/pkc/src/bls/schemes.rs
1 participant
Additional Information
The macro helpers have been mirrored to match both public and secret types, see table below
entity.rs)secret.rs)impl_type!impl_stype!Encodable+Decodable, staged throughVecEncoder/VecDecoderor the wipingArrEncoder/ArrDecoderimpl_bytes!impl_sbytes!BaseCodec+From<[u8; N]>for a fixed-size byte newtype +impl_type!derive_bytes!derive_sbytes!Clone,Copy,Default,Eq,PartialEq,Ord,PartialOrd,Hash,AsRef<[u8]>,AsRef<[u8; N]>,From<Self> for [u8; N],is_null,Debug,Display,Serialize,Deserialize(Secret)Drop,ZeroizeOnDrop,AsRef<[u8]>,AsRef<[u8; N]>,is_null,Debug,DisplayCopy,Default,Ord,PartialOrd,Hash,From<Self> for [u8; N],Serialize,Deserializedlgt_codec!dlgt_scodec!BaseCodecandHashablemake_bytes!make_sbytes!Macro generics are now forwarded bare (
@parse [$($g)*]) with the impl template supplying the brackets (impl<$($g)*>), rather than mixing two conventions. The previous mixture meantdlgt_codec!could not hand its generics off toimpl_type!at all, so it was usable only on concrete types. Two semgrep rules (types-macro-generics-bracketed,types-macro-impl-unbracketed) hold the convention in place.type_cvrt!anddlgt_codec!consequently gain thefor[...]prefix in line with other macros.Breaking Changes
dash_types::ArrayBufno longer implementsPartialEq,EqorHash, and itsDebugprintsArrayBuf { len: n }instead of the backing array. Comparing two buffers, keying a map on one, or deriving those traits on a struct holding one will no longer compile.DebugandDisplayfor byte newtypes are now emitted byderive_{,s}bytes!rather than written per-type, and resolve the type name throughcore::any::type_name.impl_bytes!andimpl_sbytes!take the type first and the length second, matching every other macro in the family. Invocations must be updated fromimpl_bytes!(16, Example)toimpl_bytes!(Example, 16).Both now accept exactly one type per invocation and construct through
Self::from_bytesrather than the tuple field. A newtype passed to either must therefore exposefrom_bytesandas_bytes;The
Decodableread limit forimpl_bytes!types is nowNrather thanMAX_SER_SIZE(32 MiB).impl_bytes!previously forwarded toimpl_type!($name)without a maximum, so a 12-byteCommandStringwas willing to buffer 32 MiB before it decoded. Over-long streaming input now stops at the limit instead of reachingDecodeError::TrailingBytes.derive_bytes!andderive_sbytes!now emitis_null,DebugandDisplay.dash_types::BufferDecoder<T, E>dash_types::VecDecoder<T, E>VecEncoderdash_types::codec::ArrayBuf<N>dash_types::ArrayBuf<N>dash_types::impl_sbyte!dash_types::impl_sbytes!impl_bytes!dash_types::codec::read_compact_u64dash_types::CompactSize::decode(_)?.get()dash_types::codec::read_compact_sizedash_types::CompactSize::decode(_)?.into_len(limit)?dash_types::codec::write_compact_u64,dash_types::codec::write_compact_sizedash_types::CompactSize::from(_).encode(buf)How Has This Been Tested?
Checklist