Skip to content

fix(sdk): validate batch base structure before signing#3133

Draft
thepastaclaw wants to merge 28 commits into
dashpay:v3.1-devfrom
thepastaclaw:feat/sdk-validate-all-transitions
Draft

fix(sdk): validate batch base structure before signing#3133
thepastaclaw wants to merge 28 commits into
dashpay:v3.1-devfrom
thepastaclaw:feat/sdk-validate-all-transitions

Conversation

@thepastaclaw
Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw commented Feb 20, 2026

Summary

Issue being fixed or feature implemented

Follow-up to #3096 per shumkov's review:
extend client-side base-structure validation coverage for SDK batch transition builders where DPP already exposes it.

PR #3096 added validation to identity and address transitions. This PR applies the same DPP constructor-backed validation path to document and token transition builders.

What was done?

Enabled the dpp batch-base-structure-validation feature for dash-sdk, so the DPP BatchTransition::new_* constructors validate base structure before signing.

  • Token builders now surface invalid token base-structure errors client-side before returning the transition.
  • Document builders now run the batch-level checks currently exposed by DPP's batch validator before signing. This is intentionally not document/token parity: SDK create builders normalize document IDs before calling the DPP constructor, so the constructor-only create-ID check is defense-in-depth rather than a user-reachable SDK builder error. Non-create transition-local checks live in the corresponding DPP from_document constructors, and broader per-document pre-sign validation parity remains follow-up work.

Document transitions touched:

  • create.rs, delete.rs, replace.rs, purchase.rs, set_price.rs,
    transfer.rs

Token builders touched:

  • burn.rs, claim.rs, config_update.rs, destroy.rs, purchase.rs, emergency_action.rs, freeze.rs, mint.rs, set_price.rs, transfer.rs,
    unfreeze.rs

Contract transitions: put_contract.rs already had structure validation via ensure_valid_state_transition_structure, so no changes were needed there.

Review follow-up removed the redundant post-sign SDK validate_batch_base_structure wrapper calls. The DPP constructors are now the single source of truth, and signing only adds signature fields that the base-structure validator does not inspect.

Latest review follow-ups preserve both singular ProtocolError::ConsensusError(...) and plural ProtocolError::ConsensusErrors(...) across wasm-sdk state-transition boundaries as structured WasmSdkError.consensusErrors entries. Singular wasm consensus errors now also promote their consensus code to the top-level WasmSdkError.code, while plural batches keep code = -1 and expose per-error codes in consensusErrors. They also remove a redundant inner batch-base-structure-validation cfg inside the state-transition-signing-only DPP sign helper, keep token pre-sign validation accumulating batch-level and per-transition consensus errors, clarify document-side pre-sign scope, and map rs-sdk-ffi protocol consensus errors to readable DashSDKError { code, message } values without changing the C ABI.

The Swift SDKError public case set remains frozen for source compatibility: public throwing wrappers continue to throw scalar SDKError values, and protocol failures map to .protocolError(String) with the original human-readable message and no hidden embedded payload. Structured Swift consensus details are available only while the original FFI DashSDKError* is alive via SDKError.fromDashSDKErrorWithConsensusErrors(_:) / SDKError.consensusErrors(fromDashSDKError:), or when a caller explicitly wraps known details in SDKDetailedError. They are intentionally not retained on scalar SDKError after the wrapper frees the FFI pointer; this avoids the previously attempted best-effort (code, message) sidecar because it could misattribute same-signature concurrent failures.

How Has This Been Tested?

  • cargo fmt --all -- --check
  • cargo check -p dpp --no-default-features --features state-transition-signing
    (passes with existing unused-import warnings in dpp under that reduced
    feature set)
  • cargo check -p wasm-sdk
  • cargo test -p wasm-sdk protocol_consensus_errors --lib
  • cargo check -p rs-sdk-ffi
  • cargo test -p dpp validate_base_structure --lib
  • cargo clippy -p dpp --no-default-features --features state-transition-signing --all-targets -- -D warnings
  • cargo clippy -p dpp --no-default-features --features state-transition-signing --all-targets -- -D warnings
  • cargo clippy -p wasm-sdk -p rs-sdk-ffi --all-targets -- -D warnings
  • cargo test --manifest-path packages/rs-sdk-ffi/Cargo.toml --lib sdk_protocol_consensus -- --nocapture
  • cargo clippy -p rs-sdk-ffi --all-targets -- -D warnings
  • cargo test -p wasm-sdk protocol_consensus_errors --lib
  • cargo clippy -p wasm-sdk --all-targets -- -D warnings
  • cargo check -p wasm-sdk
  • xcrun swift-format lint --strict packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/SDKErrorTests.swift
  • git diff --check
  • pre-commit hook during git commit --amend: cargo fmt --all -- --check +
    workspace cargo check
  • swift build in packages/swift-sdk attempted locally; blocked because
    DashSDKFFI.xcframework does not contain a binary artifact in this worktree
  • (cd packages/swift-sdk/SwiftTests && swift test --disable-sandbox --filter SDKErrorTests) attempted locally; blocked because DashSDKFFI.xcframework does not contain a binary artifact in this worktree

Breaking Changes

  • dpp::ProtocolError is now #[non_exhaustive] and includes plural ConsensusErrors, so downstream Rust crates that exhaustively match ProtocolError need a wildcard arm.
  • None for valid state transitions. Invalid token transitions now fail earlier client-side. Document builders now also run the batch-level validation currently exposed by DPP; deeper per-document client-side structure validation remains follow-up work.

Loading
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.

3 participants