refactor(core): redundancy cleanup + 0.11.0 - #46
Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 2 high |
🟢 Metrics 37 complexity · -101 duplication
Metric Results Complexity 37 Duplication -101
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
The PR successfully reduces code redundancy, as evidenced by the removal of 9 code clones and Codacy's positive assessment. Key improvements include the transition to structured validation errors and significant performance gains in access policy evaluation through allocation-free string comparisons.
A critical logic fix in the keystore addresses a potential data loss scenario by ensuring that rapid key rotations do not result in identifier collisions. However, a functional gap remains in the passport.rs validation logic where sector-data checks are bypassed in wasm32 targets. Additionally, while the technical changes are sound, the PR lacks a description, which complicates the audit trail for significant logic shifts included in this 'refactor'.
About this PR
- The PR description is empty and contains only template placeholders. Given that this PR includes significant API changes (structured validation errors) and logic fixes (keystore collisions) alongside general refactoring, providing context for these changes is necessary for long-term maintenance.
Test suggestions
- Found JWS verifier robustness: verify that malformed or adversarial compact JWS inputs do not cause panics.
- Found Keystore rotation collision: verify that successive rotations within the same millisecond do not overwrite archived records.
- Found Access policy normalization: verify that 'disassembly_instructions' matches 'disassemblyInstructions' without string allocation.
- Found Repairability drift guard: ensure the ruleset status map agrees with the actual resolution logic for active/pending categories.
- Found Numeric validation: verify 'sums_to' correctly handles floating point tolerance and finite-value checks.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| /// `entries.sort_by_key` still recovers chronological order, since a v7 UUID's | ||
| /// leading bits are a millisecond timestamp. | ||
| fn archived_key_name(key_id: &str) -> String { | ||
| format!("{key_id}#archived-{}", uuid::Uuid::now_v7()) |
There was a problem hiding this comment.
🔴 HIGH RISK
This migration to time-ordered identifiers is a critical fix for potential collisions in the key archive. By ensuring every rotation gets a unique identifier that remains time-sortable, we prevent accidental overwrites of historical signing keys that are still needed to verify legacy signatures.
| @@ -0,0 +1,72 @@ | |||
| //! Property tests for the JWS verifier's hostile-input surface. | |||
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: Adding proptests to the JWS verification surface significantly hardens the library against adversarial inputs. This ensures that malformed or malicious credentials scanned from untrusted sources cannot crash the verification process.
| /// runs this once per policy-tiered field, per document key, at every | ||
| /// recursion depth of [`super::filter::filter_by_access_tier`], so avoiding an | ||
| /// allocation per comparison matters there. | ||
| fn keys_match_normalized(a: &str, b: &str) -> bool { |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: This optimization removes a per-field string allocation during document filtering. Since access control checks run recursively through potentially large JSON payloads, this change provides a significant performance boost for high-throughput verification nodes.
| .ok() | ||
| .and_then(|v| v.as_str().map(String::from)) | ||
| .unwrap_or_default(); | ||
| let chemistry_str = enum_wire_str(&b.battery_chemistry); |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: BatteryChemistry now provides a specialized wire_str() method that returns a static string. The generic enum_wire_str helper used here is significantly more expensive as it performs JSON serialization (allocating a Value tree) followed by string conversion.
| let chemistry_str = enum_wire_str(&b.battery_chemistry); | |
| let chemistry_str = b.battery_chemistry.wire_str(); |
|
|
||
| // Sector-data validation: JSON Schema + cross-field rules (fibre sum, SVHC, etc.). | ||
| // Excluded from wasm32 builds because jsonschema depends on reqwest's blocking API. | ||
| #[cfg(not(target_arch = "wasm32"))] |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: Structured validation is now implemented, but note that the #[cfg(not(target_arch = "wasm32"))] block for sector-data validation means that wasm32 callers will only receive structural errors, potentially leading to 'valid' passports that fail more rigorous server-side checks.
| if s.len() != len || !s.bytes().all(|b| b.is_ascii_digit()) { | ||
| return Err(Gs1KeyCheck::InvalidFormat); | ||
| } | ||
| let digits: Vec<u8> = s.bytes().map(|b| b - b'0').collect(); |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: The input string is already validated to be exactly 'len' (13 or 14) ASCII digits. Allocating a Vec for such a small, fixed-length dataset is unnecessary overhead. Consider refactoring check_gs1_key to use a stack-allocated array or an iterator to avoid the allocation.
| /// [`crate::domain::validation::validate_sector_data`] (non-wasm32 only) | ||
| pub fn validate(&self) -> Result<(), crate::domain::error::DppError> { | ||
| let mut issues = Vec::new(); | ||
| use crate::domain::field_error::{FieldError, ValidationErrors}; |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: The move to structured field errors using JSON pointers (RFC 6901) improves the developer experience for consumers of the dpp-domain crate. It enables precise error highlighting in user interfaces by mapping validation failures directly to document fields.
Summary
24 commits of redundancy cleanup across every crate in the workspace, plus the version bump and CHANGELOG entry that shipping them requires — this branch carries 5 breaking API changes. Bumps the workspace from 0.10.0 to 0.11.0 (pre-1.0, minor bump permits breaking changes per
docs/governance/VERSIONING.md).Related issue
None — internal redundancy-audit cleanup, no tracked issue.
Changes
Full detail is in
CHANGELOG.mdunder[0.11.0]. Summary by category:Breaking
DppSectorPlugin::generate_passporttakesPluginInputby value;meta()/capabilities()gained default impls built from two new required methods (plugin_identity(),schema_version_range()) instead of every plugin hand-assembling the full structs.RulesetId/RulesetVersionnow wrap&'static strinstead ofString(no longerDeserialize— every use in this crate was always a compile-time literal).Stringto the validatedGtinnewtype.countryOfOrigin/country_of_originacross 7 sectors that previously used sector-specific names (countryOfManufacture,countryOfProduction,countryOfManufacturing). New minor schema versions for each affected sector.MaterialEntry::origin_countryrenamed tocountry_of_originfor consistency with the above.Added
dpp_calc::co2e::calculate_asof— compute against the ruleset version effective on a given date, for reproducing historical calculations under the ruleset actually in force at the time.AbiResult/PluginCapabilities); CI-only.Changed
dpp-registry::RegistryStatusCodenow re-exported from the crate root.plugins/sector-*) consolidated into their own shared Cargo workspace; duplicated codec-dispatch, country-code table, andthreshold_statuslogic moved intodpp-plugin-sdk.Passport::validate()returns structured, JSON-Pointer-addressed field errors instead of a flat list.dpp-tests, benches,dpp-crypto,dpp-plugin-traits), shared numeric-tolerance helpers indpp-rules, shared receipt/threshold helpers indpp-calc,dpp-cryptokeystore cleanup avoiding an unnecessary decrypt-for-pubkey round trip.Fixed
dpp-digital-link's anddpp-plugin-traits' docs.Performance
dpp-digital-link's JSON-LD passport context is now cached instead of rebuilt per call.BatteryData's AAS submodel builder callsBatteryChemistry::wire_str()directly instead of the generic JSON-round-trip helper (Codacy review finding).Gtin/Glncheck-digit validation uses a stack buffer instead of heap-allocating aVecper call (Codacy review finding).Codacy review
7 inline comments, reviewed individually:
battery.rswire_str(),gtin.rsstack buffer) — see73af0ae.passport.rs:230) flagged that sector-data JSON-Schema validation is#[cfg(not(target_arch = "wasm32"))]-gated, so a wasm32 caller ofPassport::validate()only gets structural checks. Reviewed and declined: this is a pre-existing, already-documented constraint (both the method's doc comment and an inline comment call it out explicitly) driven byjsonschemadepending onreqwest's blocking API, which isn't wasm32-compatible. Not a regression introduced by this branch; fixing it properly means swapping the schema-validation library or restructuring the plugin ABI, both out of scope here.Checklist
just lintpasses locally (cargo clippy --workspace -- -D warnings)just fmtapplied (cargo fmt --all)just testpasses (cargo nextest run --workspace).envfiles in the diff[0.11.0]entry with migration notes for every breaking change; Cargo.toml version bumped in lockstep (root + all 7 internal path-dependency pins)