Skip to content

test(dpp): require shielded-client in shield-from-asset-lock signing tests#3827

Merged
QuantumExplorer merged 2 commits into
v3.1-devfrom
claude/flamboyant-lamarr-8ed97f
Jun 9, 2026
Merged

test(dpp): require shielded-client in shield-from-asset-lock signing tests#3827
QuantumExplorer merged 2 commits into
v3.1-devfrom
claude/flamboyant-lamarr-8ed97f

Conversation

@QuantumExplorer

@QuantumExplorer QuantumExplorer commented Jun 9, 2026

Copy link
Copy Markdown
Member

Issue being fixed or feature implemented

Primary fix (dpp test-cfg): cargo check --workspace --all-targets fails to compile the dpp lib-test target on a clean v3.1-dev-based tree:

error[E0433]: failed to resolve: could not find `builder` in `shielded`
error[E0432]: unresolved import `crate::shielded::builder`

The signing_tests module for ShieldFromAssetLockTransition imports crate::shielded::builder::*, but pub mod builder; in shielded/mod.rs is gated on the shielded-client feature (which pulls in grovedb-commitment-tree / Orchard). The test module was gated only on state-transition-signing + core_key_wallet. Under --all-targets/--workspace feature unification, the lib-test target gets those two features enabled without shielded-client, so crate::shielded::builder does not exist and compilation fails.

Additional fix (platform-wallet, required to get CI green): CI's cargo clippy --workspace --all-features was already broken on v3.1-dev — independently of this PR — by code merged in #3819:

error[E0277]: the trait bound `PersistenceError: From<String>` is not satisfied
error: could not compile `platform-wallet-ffi` (lib) due to 2 previous errors

SledFfiPersister::load returns Result<_, PersistenceError>, but the OVK outgoing-notes load path returned its errors via .into() on a String/&str, and PersistenceError has no From<String>. This fails the workspace clippy step that the "Tests (macOS)" check runs, so every open PR is red on it. Since this PR's CI runs the same workspace clippy, it cannot go green without this fix included.

What was done?

  1. test(dpp) — added feature = "shielded-client" to the signing_tests gate (both the mod declaration in shield_from_asset_lock_transition/mod.rs and the inner #![cfg(all(...))] of signing_tests.rs, with an explanatory comment). The test genuinely depends on shielded-client: it calls build_shield_from_asset_lock_transition_with_signer, which lives inside the builder module, whose body uses grovedb_commitment_tree (a dep pulled in only by shielded-client). Requiring it is the correct alignment, not a workaround — exposing pub mod builder; without shielded-client is not viable since the module would fail to compile for lack of that dep. The other in-module builder test references (shield.rs, unshield.rs, etc.) live inside the builder module, so they only compile when shielded-client is on and need no change.

  2. fix(platform-wallet) — built both outgoing-notes load error sites in persistence.rs with PersistenceError::backend(..) (the same constructor the sibling incoming-notes / sync-state paths in the same fn load already use; E: Into<Box<dyn StdError + Send + Sync>>, which String/&str satisfy) instead of .into(). No behavior change beyond the errors being typed correctly. This is a drive-by fix for a pre-existing base-branch breakage; happy to split it into a standalone PR if maintainers prefer, but note it's needed for this PR's CI to pass.

How Has This Been Tested?

  • Reproduced the original dpp errors with cargo check -p dpp --tests --no-default-features --features state-transition-signing,core_key_wallet → after the fix, clean.
  • cargo check -p dpp --all-features --tests — passes.
  • cargo check --workspace --all-targets — passes (exit 0); the originally-failing command.
  • Reproduced the platform-wallet-ffi clippy errors with cargo clippy -p platform-wallet-ffi --all-features --locked -- --no-deps -D warnings → after the fix, clean.
  • cargo clippy --workspace --all-features --locked -- --no-deps -D warnings (the exact CI command) — passes with no errors or warnings.
  • cargo fmt — clean on both crates.

Breaking Changes

None. A test-only cfg-gate change plus an error-construction fix; no production behavior or consensus is affected.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Tests
    • Tightened test compilation conditions so shielded state transition tests only run under the correct feature combinations, improving test reliability across build configurations.
  • Bug Fixes
    • Improved error handling for shielded wallet restore paths so failures produce clearer, explicit error results instead of implicit string-based errors.

…tests

The `signing_tests` module for `ShieldFromAssetLockTransition` imports
`crate::shielded::builder::*`, but `pub mod builder;` is gated on the
`shielded-client` feature (it pulls in `grovedb-commitment-tree` / Orchard).
The test module was only gated on `state-transition-signing` + `core_key_wallet`,
so under `cargo check --workspace --all-targets` feature unification the lib-test
target enabled those two features WITHOUT `shielded-client` and failed to compile:

    error[E0433]: failed to resolve: could not find `builder` in `shielded`
    error[E0432]: unresolved import `crate::shielded::builder`

Align the gate with the test's actual dependency by adding
`feature = "shielded-client"` to both the `mod signing_tests;` declaration and
the file's inner `#![cfg(...)]`. The test genuinely needs the builder (it calls
`build_shield_from_asset_lock_transition_with_signer`), which cannot exist without
`shielded-client`, so requiring it is correct rather than a workaround.

Verified with `cargo check --workspace --all-targets` and
`cargo check -p dpp --all-features --tests` (both pass).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: cf0f8167-1106-44ee-9061-aa3be1c40dc0

📥 Commits

Reviewing files that changed from the base of the PR and between 0b54703 and a395cba.

📒 Files selected for processing (1)
  • packages/rs-platform-wallet-ffi/src/persistence.rs

📝 Walkthrough

Walkthrough

Adds shielded-client to the signing-tests feature gates for shield_from_asset_lock_transition and changes shielded outgoing-notes restore failures to return explicit PersistenceError::backend(...) errors.

Changes

Shielded signing + persistence fixes

Layer / File(s) Summary
Signing tests feature gating
packages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/mod.rs, packages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/signing_tests.rs
The cfg(all(...)) attribute on the signing_tests module declaration and the #![cfg(...)] module-level attribute were extended to require feature = "shielded-client" alongside test, state-transition-signing, and core_key_wallet.
Shielded restore persistence errors
packages/rs-platform-wallet-ffi/src/persistence.rs
In the FFIPersister shielded outgoing-notes restore path, return PersistenceError::backend(format!(...)) when the callback pair is half-wired or when the callback returns a non-zero rc, instead of converting formatted strings into errors implicitly.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • dashpay/platform#3603: Overlaps with shielded persistence/load wiring in packages/rs-platform-wallet-ffi/src/persistence.rs.

Suggested labels

ready for final review

Suggested reviewers

  • shumkov
  • thepastaclaw

Poem

A rabbit hopped through code at night,
Tweaked gates so tests compile just right,
When callbacks fail or wires fray,
Errors now shout in clear display,
Hops, munches, fixes — tidy delight! 🐰

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding the shielded-client feature requirement to the shield-from-asset-lock signing tests.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/flamboyant-lamarr-8ed97f

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov

codecov Bot commented Jun 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.91%. Comparing base (da9d3fe) to head (a395cba).

Additional details and impacted files
@@              Coverage Diff              @@
##           v3.1-dev    #3827       +/-   ##
=============================================
- Coverage     87.01%   69.91%   -17.11%     
=============================================
  Files          2677       19     -2658     
  Lines        329958     2712   -327246     
=============================================
- Hits         287118     1896   -285222     
+ Misses        42840      816    -42024     
Components Coverage Δ
dpp ∅ <ø> (∅)
drive ∅ <ø> (∅)
drive-abci ∅ <ø> (∅)
sdk ∅ <ø> (∅)
dapi-client ∅ <ø> (∅)
platform-version ∅ <ø> (∅)
platform-value ∅ <ø> (∅)
platform-wallet ∅ <ø> (∅)
drive-proof-verifier ∅ <ø> (∅)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…eError::backend

`SledFfiPersister::load` returns `Result<_, PersistenceError>`, and
`PersistenceError` does not implement `From<String>`. The OVK outgoing-notes
load path (added in #3819) returned its errors with `.into()` on a `String`/
`&str`, which fails to compile under `cargo clippy --workspace --all-features`:

    error[E0277]: the trait bound `PersistenceError: From<String>` is not satisfied
    error: could not compile `platform-wallet-ffi` (lib) due to 2 previous errors

This broke the workspace clippy check on `v3.1-dev` (and therefore every open
PR's "Tests (macOS)" job). Build both error sites with `PersistenceError::backend(..)`
instead — the same constructor the sibling incoming-notes / sync-state paths in
this function already use (`E: Into<Box<dyn StdError + Send + Sync>>`, which `String`
and `&str` satisfy). No behavior change beyond the errors now being typed correctly.

Verified with `cargo clippy --workspace --all-features --locked -- --no-deps -D warnings`.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@QuantumExplorer QuantumExplorer left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed

@QuantumExplorer QuantumExplorer merged commit b4aec4f into v3.1-dev Jun 9, 2026
34 of 36 checks passed
@QuantumExplorer QuantumExplorer deleted the claude/flamboyant-lamarr-8ed97f branch June 9, 2026 19:25
@QuantumExplorer QuantumExplorer added this to the v4.0.0 milestone Jun 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant