Skip to content
Draft
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
2446db0
feat(sdk): add client-side validate_base_structure for document and t…
thepastaclaw Feb 20, 2026
472bcad
fix: replace unwrap() with safe if-let in validation error handling
thepastaclaw Feb 20, 2026
956c192
style: apply cargo fmt to rs-sdk and rs-dapi
thepastaclaw Feb 21, 2026
3235ac7
fix(wasm-sdk): add maxLength to indexed rarity field in test fixture
thepastaclaw Feb 21, 2026
5647f51
test(sdk): add validate_base_structure error path tests for document …
thepastaclaw Feb 21, 2026
fde8031
test(sdk): add validate_base_structure error path tests for all builders
thepastaclaw Feb 21, 2026
555940b
refactor(sdk): remove 5 redundant document nonce-masking tests
thepastaclaw Feb 21, 2026
89610b9
refactor(sdk): extract validate_batch_base_structure helper to reduce…
thepastaclaw Mar 16, 2026
c773a81
refactor(sdk): extract shared test helpers to reduce test duplication
thepastaclaw Mar 16, 2026
b81e065
test(sdk): add document builder sign() integration test
thepastaclaw Mar 16, 2026
52a04ff
fix(sdk): log dropped validation errors in validate_batch_base_structure
thepastaclaw Mar 17, 2026
8125b54
test(sdk): add happy-path sign() tests for all document builders
thepastaclaw Mar 17, 2026
10e173a
test(sdk): add happy-path sign() tests for all token builders
thepastaclaw Mar 17, 2026
868e1a6
test(sdk): assert non-empty signature in document builder happy-path …
thepastaclaw Mar 17, 2026
534d40a
Merge remote-tracking branch 'origin/v3.1-dev' into tracker-1116
thepastaclaw Apr 28, 2026
8742a0b
fix(sdk): validate batch structure before signing
thepastaclaw May 2, 2026
9ab76e3
feat(sdk): expose structured FFI consensus errors
thepastaclaw May 4, 2026
0ac3718
feat(sdk): add document transition pre-sign structure validators
thepastaclaw May 4, 2026
b7d7471
fix(sdk): refine FFI consensus error details
thepastaclaw May 4, 2026
02f8159
fix(sdk): preserve Swift consensus error details
thepastaclaw May 5, 2026
f54eb96
fix(sdk): preserve query consensus error details
thepastaclaw May 7, 2026
f11960b
fix(sdk): keep Swift consensus failures as SDKError
thepastaclaw May 7, 2026
ea12f77
fix(sdk): preserve structured consensus errors
thepastaclaw May 7, 2026
1f4c357
fix(swift-sdk): clarify consensus error details
thepastaclaw May 10, 2026
db0e002
Merge remote-tracking branch 'upstream/v3.1-dev' into tracker-1295
thepastaclaw May 17, 2026
be00490
fix(sdk): address batch validation review follow-up
thepastaclaw May 17, 2026
11fb8c4
fix(sdk): address structured consensus review follow-up
thepastaclaw May 17, 2026
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ book/book/
# gRPC coverage report
grpc-coverage-report.txt

# Git worktrees
/worktrees/
__pycache__/
.claude/settings.local.json
.claude/worktrees/
Expand Down
10 changes: 10 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,15 @@
* **sdk:** getSignableBytes is not compatible with sign and verify (#3048)
* **platform:** update PlatformAddress encoding and HRP constants (#3059)
* **platform:** 3.0 audit report fixes (#3053)
* **swift-sdk:** `SDKError.protocolError(String)` associated values are
clean human-readable messages again (no embedded payload). FFI errors
with structured consensus details now throw `SDKDetailedError`, which
wraps the mapped `SDKError` plus a `consensusErrors` array; callers
that want the details should catch `SDKDetailedError` in addition to
`SDKError`. Pointer-based FFI helpers
(`SDKError.consensusErrors(fromDashSDKError:)` and
`SDKError.fromDashSDKErrorWithConsensusErrors(_:)`) remain available
before `dash_sdk_error_free`.
* **sdk:** comprehensive Evo SDK refactoring (#2999)
* upgrade bincode to 2.0.1 (#2991)

Expand All @@ -29,6 +38,7 @@
* **dapi-grpc:** files generated outside sandbox
* **dashmate:** differentiate service ports between networks to avoid conflicts ([#3085](https://github.com/dashpay/platform/issues/3085))
* **platform:** 3.0 audit report fixes ([#3053](https://github.com/dashpay/platform/issues/3053))
* **swift-sdk:** preserve structured consensus details in Swift state-transition helper errors
* **sdk:** deserialization error due to outdated contract cache ([#3052](https://github.com/dashpay/platform/issues/3052))
* **sdk:** getSignableBytes is not compatible with sign and verify ([#3048](https://github.com/dashpay/platform/issues/3048))
* **sdk:** inconsistent document query operator ([#3039](https://github.com/dashpay/platform/issues/3039))
Expand Down
8 changes: 8 additions & 0 deletions packages/rs-dpp/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -181,10 +181,17 @@ abci = [
]
cbor = ["ciborium"]
validation = [
"batch-base-structure-validation",
"json-schema-validation",
"value-conversion",
"ed25519-dalek",
]
# Narrow feature that compiles only `BatchTransition::validate_base_structure`
# without pulling in json-schema-validation, value-conversion, or ed25519-dalek.
# Useful for clients that need pre-broadcast base-structure checks without
# the full validation toolchain. `state-transition-signing` enables this
# transitively so constructor pre-sign validation is always present there.
batch-base-structure-validation = []
# TODO: Tring to remove regexp
create-contested-document = []
json-conversion = ["value-conversion", "platform-value/json", "json-object"]
Expand Down Expand Up @@ -243,6 +250,7 @@ state-transition-signing = [
"state-transitions",
"message-signing",
"state-transition-validation",
"batch-base-structure-validation",
]
vote-serialization = []
vote-serde-conversion = ["serde-conversion"]
Expand Down
10 changes: 9 additions & 1 deletion packages/rs-dpp/src/errors/consensus/basic/basic_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,15 @@ use crate::data_contract::errors::DataContractError;

#[allow(clippy::large_enum_variant)]
#[derive(
Error, Debug, PlatformSerialize, PlatformDeserialize, Encode, Decode, PartialEq, Clone,
Error,
Debug,
PlatformSerialize,
PlatformDeserialize,
Encode,
Decode,
PartialEq,
Clone,
strum::IntoStaticStr,
)]
pub enum BasicError {
/*
Expand Down
10 changes: 9 additions & 1 deletion packages/rs-dpp/src/errors/consensus/fee/fee_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,15 @@ use crate::errors::ProtocolError;
use platform_serialization_derive::{PlatformDeserialize, PlatformSerialize};

#[derive(
Error, Debug, PartialEq, Encode, Decode, PlatformSerialize, PlatformDeserialize, Clone,
Error,
Debug,
PartialEq,
Encode,
Decode,
PlatformSerialize,
PlatformDeserialize,
Clone,
strum::IntoStaticStr,
)]
pub enum FeeError {
/*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,15 @@ use crate::errors::ProtocolError;
use platform_serialization_derive::{PlatformDeserialize, PlatformSerialize};

#[derive(
Error, Debug, PartialEq, Encode, Decode, PlatformSerialize, PlatformDeserialize, Clone,
Error,
Debug,
PartialEq,
Encode,
Decode,
PlatformSerialize,
PlatformDeserialize,
Clone,
strum::IntoStaticStr,
)]
pub enum SignatureError {
/*
Expand Down
10 changes: 9 additions & 1 deletion packages/rs-dpp/src/errors/consensus/state/state_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,15 @@ use crate::consensus::state::voting::vote_poll_not_found_error::VotePollNotFound
use super::document::document_timestamps_are_equal_error::DocumentTimestampsAreEqualError;

#[derive(
Error, Debug, PartialEq, Encode, Decode, PlatformSerialize, PlatformDeserialize, Clone,
Error,
Debug,
PartialEq,
Encode,
Decode,
PlatformSerialize,
PlatformDeserialize,
Clone,
strum::IntoStaticStr,
)]
pub enum StateError {
/*
Expand Down
14 changes: 14 additions & 0 deletions packages/rs-dpp/src/errors/protocol_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,9 @@ pub enum ProtocolError {
#[error(transparent)]
ConsensusError(Box<ConsensusError>),

#[error("Multiple consensus errors: {}", join_consensus_errors(.0))]
ConsensusErrors(Vec<ConsensusError>),
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

🟡 Suggestion: Adding ProtocolError::ConsensusErrors is a breaking public-enum change

ProtocolError is a public enum without #[non_exhaustive], so introducing the new ConsensusErrors(Vec<ConsensusError>) variant breaks any downstream crate that matches ProtocolError exhaustively. Internal callers were updated, but external consumers (rs-sdk users, third-party SDK wrappers) will fail to compile on first pickup. Either add #[non_exhaustive] going forward and call this out as a planned breaking release, or represent aggregated consensus failures differently (e.g., reuse the existing singular variant with a wrapper error type) so the public enum surface does not widen.

source: ['codex']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-dpp/src/errors/protocol_error.rs`:
- [SUGGESTION] lines 131-135: Adding `ProtocolError::ConsensusErrors` is a breaking public-enum change
  `ProtocolError` is a public enum without `#[non_exhaustive]`, so introducing the new `ConsensusErrors(Vec<ConsensusError>)` variant breaks any downstream crate that matches `ProtocolError` exhaustively. Internal callers were updated, but external consumers (rs-sdk users, third-party SDK wrappers) will fail to compile on first pickup. Either add `#[non_exhaustive]` going forward and call this out as a planned breaking release, or represent aggregated consensus failures differently (e.g., reuse the existing singular variant with a wrapper error type) so the public enum surface does not widen.


Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

💬 Nitpick: ConsensusErrors plural variant parallels singular ConsensusError without a canonicalization rule

errors_to_consensus_protocol_error hand-routes 0/1/>1 errors to None / ConsensusError / ConsensusErrors, and the wasm/FFI layers then re-split on the same shape. Downstream consumers (e.g. wasm_dpp::errors::from::from_dpp_err, wasm_dpp::errors::protocol_error::from_protocol_error) now must handle both ConsensusError(_) and ConsensusErrors(vec![one]) to be defensive. Either explicitly document that ConsensusErrors is never length 1 (canonicalization invariant maintained at every construction site) or collapse to a single ConsensusErrors variant and always use it. The current split creates two places where a future contributor must remember to handle both variants or risk masking the plural case as 'unexpected'.

source: ['claude']

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

💬 Nitpick: Existing ProtocolError::ConsensusError matches will not handle a single-element ConsensusErrors

Adding ProtocolError::ConsensusErrors(Vec<_>) is wire-safe (ProtocolError is not Platform-serialized). However, several call sites pattern-match ProtocolError::ConsensusError(err) to decide whether a verification result represents a recoverable failure (e.g. packages/wasm-dpp/src/data_contract/state_transition/data_contract_create_transition/mod.rs returns Ok(false) only for a singular ConsensusError -> SignatureError::InvalidStateTransitionSignatureError). If any future producer routes a signature failure through the plural variant, those branches will be skipped. Either collapse single-element ConsensusErrors to ConsensusError at construction, or audit and extend those match sites to also handle a one-element ConsensusErrors carrying a SignatureError.

source: ['claude']

#[error(transparent)]
Document(Box<DocumentError>),

Expand Down Expand Up @@ -355,3 +358,14 @@ impl From<InvalidVectorSizeError> for ProtocolError {
Self::InvalidVectorSizeError(err)
}
}

/// Join the `Display` representation of every inner [`ConsensusError`] with
/// `"; "`. Used by [`ProtocolError::ConsensusErrors`]'s `Display` impl so the
/// rendered message is human-readable instead of a `Debug`-formatted `Vec`.
fn join_consensus_errors(errors: &[ConsensusError]) -> String {
errors
.iter()
.map(ToString::to_string)
.collect::<Vec<_>>()
.join("; ")
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ mod convertible;
pub mod from_document;
pub mod v0;
mod v0_methods;
pub(crate) mod validate_structure;

use crate::block::block_info::BlockInfo;
use crate::data_contract::document_type::DocumentTypeRef;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
use crate::state_transition::batch_transition::document_create_transition::validate_structure::v0::DocumentCreateTransitionStructureValidationV0;
use crate::state_transition::batch_transition::DocumentCreateTransition;
use crate::validation::SimpleConsensusValidationResult;
use crate::ProtocolError;
use platform_value::Identifier;
use platform_version::version::PlatformVersion;

mod v0;

pub(crate) trait DocumentCreateTransitionStructureValidation {
fn validate_structure(
&self,
owner_id: Identifier,
platform_version: &PlatformVersion,
) -> Result<SimpleConsensusValidationResult, ProtocolError>;
}
Comment on lines +8 to +16
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

🟡 Suggestion: Constructor-only document validate_structure hooks are exposed as public DPP API

pub mod v0 and the pub trait DocumentCreateTransitionStructureValidation are public, and the same pattern repeats across the six document transition modules (create/delete/replace/transfer/purchase/update_price). The only consumer is validate_basic_structure_pre_sign inside the same crate (verified via grep — the trait import at validation/validate_basic_structure/mod.rs:2 is the sole user). These hooks are described in code comments as constructor-only / pre-sign validation helpers, but making them public turns an internal wiring detail into semver-facing API that downstream crates can begin depending on. Tightening mod v0, the trait, and the pub mod validate_structure re-exports to pub(crate) keeps the abstraction boundary tight without changing behavior. The token-side analogue follows the same pattern as precedent, but the new document-side modules introduced here have a genuinely crate-local consumer set today.

source: ['claude-rust-quality', 'codex-rust-quality']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/document_create_transition/validate_structure/mod.rs`:
- [SUGGESTION] lines 8-16: Constructor-only document `validate_structure` hooks are exposed as public DPP API
  `pub mod v0` and the `pub trait DocumentCreateTransitionStructureValidation` are public, and the same pattern repeats across the six document transition modules (create/delete/replace/transfer/purchase/update_price). The only consumer is `validate_basic_structure_pre_sign` inside the same crate (verified via grep — the trait import at `validation/validate_basic_structure/mod.rs:2` is the sole user). These hooks are described in code comments as constructor-only / pre-sign validation helpers, but making them public turns an internal wiring detail into semver-facing API that downstream crates can begin depending on. Tightening `mod v0`, the trait, and the `pub mod validate_structure` re-exports to `pub(crate)` keeps the abstraction boundary tight without changing behavior. The token-side analogue follows the same pattern as precedent, but the new document-side modules introduced here have a genuinely crate-local consumer set today.


impl DocumentCreateTransitionStructureValidation for DocumentCreateTransition {
fn validate_structure(
&self,
owner_id: Identifier,
platform_version: &PlatformVersion,
) -> Result<SimpleConsensusValidationResult, ProtocolError> {
// Dispatch via the DPP-owned version field. The drive-abci action
// validator has a separate field under
// `drive_abci.validation_and_processing.state_transitions.batch_state_transition`
// and is intentionally decoupled from this DPP/SDK pre-sign helper.
match platform_version
.dpp
.state_transitions
.documents
.documents_batch_transition
.validation
.document_create_transition_structure_validation
{
0 => self.validate_structure_v0(owner_id),
version => Err(ProtocolError::UnknownVersionMismatch {
method: "DocumentCreateTransition::validate_structure".to_string(),
known_versions: vec![0],
received: version,
}),
}
}
Comment on lines +28 to +43
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

🟡 Suggestion: SDK pre-sign DocumentCreateTransition::validate_structure shares dispatch field with the drive-abci action validator

The new DocumentCreateTransition::validate_structure dispatches on platform_version.drive_abci.validation_and_processing.state_transitions.batch_state_transition.document_create_transition_structure_validation. That same field is the existing dispatch source for DocumentCreateTransitionAction::validate_structure in packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/document/document_create_transition_action/mod.rs:48-61 — a different method on a different type at a different layer. Today every PlatformVersion defines this field as 0, so there is no live conflict. But when drive-abci needs a new validation algorithm for the action and bumps this field to 1, the SDK pre-sign helper will start returning ProtocolError::UnknownVersionMismatch { method: "DocumentCreateTransition::validate_structure", known_versions: vec![0], received: 1 } for every BatchTransition::new_document_creation_transition_from_document call, breaking SDK signing on protocol upgrade. The two validators serve different layers and their version-bump cadences shouldn't be coupled. Either introduce a separate field (e.g. dpp.state_transitions.documents.documents_batch_transition.create_pre_sign_validation) for the SDK-only call, or add a comment + helper that ensures both dispatch tables are bumped together when this field changes.

source: ['claude-general']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/document_create_transition/validate_structure/mod.rs`:
- [SUGGESTION] lines 24-38: SDK pre-sign `DocumentCreateTransition::validate_structure` shares dispatch field with the drive-abci action validator
  The new `DocumentCreateTransition::validate_structure` dispatches on `platform_version.drive_abci.validation_and_processing.state_transitions.batch_state_transition.document_create_transition_structure_validation`. That same field is the existing dispatch source for `DocumentCreateTransitionAction::validate_structure` in `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/document/document_create_transition_action/mod.rs:48-61` — a different method on a different type at a different layer. Today every `PlatformVersion` defines this field as `0`, so there is no live conflict. But when drive-abci needs a new validation algorithm for the action and bumps this field to `1`, the SDK pre-sign helper will start returning `ProtocolError::UnknownVersionMismatch { method: "DocumentCreateTransition::validate_structure", known_versions: vec![0], received: 1 }` for every `BatchTransition::new_document_creation_transition_from_document` call, breaking SDK signing on protocol upgrade. The two validators serve different layers and their version-bump cadences shouldn't be coupled. Either introduce a separate field (e.g. `dpp.state_transitions.documents.documents_batch_transition.create_pre_sign_validation`) for the SDK-only call, or add a comment + helper that ensures both dispatch tables are bumped together when this field changes.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
use crate::consensus::basic::document::InvalidDocumentTransitionIdError;
use crate::consensus::basic::BasicError;
use crate::consensus::ConsensusError;
use crate::document::Document;
use crate::state_transition::batch_transition::document_base_transition::document_base_transition_trait::DocumentBaseTransitionAccessors;
use crate::state_transition::batch_transition::document_base_transition::v0::v0_methods::DocumentBaseTransitionV0Methods;
use crate::state_transition::batch_transition::document_create_transition::v0::v0_methods::DocumentCreateTransitionV0Methods;
use crate::state_transition::batch_transition::DocumentCreateTransition;
use crate::validation::SimpleConsensusValidationResult;
use crate::ProtocolError;
use platform_value::Identifier;

pub(super) trait DocumentCreateTransitionStructureValidationV0 {
fn validate_structure_v0(
&self,
owner_id: Identifier,
) -> Result<SimpleConsensusValidationResult, ProtocolError>;
}

impl DocumentCreateTransitionStructureValidationV0 for DocumentCreateTransition {
fn validate_structure_v0(
&self,
owner_id: Identifier,
) -> Result<SimpleConsensusValidationResult, ProtocolError> {
let (expected_id, invalid_id) = match self {
DocumentCreateTransition::V0(transition) => (
Document::generate_document_id_v0(
&transition.base().data_contract_id(),
&owner_id,
transition.base().document_type_name(),
&transition.entropy(),
),
transition.base().id(),
),
};

if invalid_id != expected_id {
return Ok(SimpleConsensusValidationResult::new_with_error(
ConsensusError::BasicError(BasicError::InvalidDocumentTransitionIdError(
InvalidDocumentTransitionIdError::new(expected_id, invalid_id),
)),
));
}

Ok(SimpleConsensusValidationResult::default())
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use crate::prelude::IdentityNonce;
use crate::ProtocolError;
use platform_version::version::{FeatureVersion, PlatformVersion};

use crate::state_transition::batch_transition::batched_transition::document_delete_transition::validate_structure::DocumentDeleteTransitionStructureValidation;
use crate::state_transition::batch_transition::batched_transition::document_delete_transition::DocumentDeleteTransitionV0;
use crate::state_transition::batch_transition::batched_transition::DocumentDeleteTransition;
use crate::tokens::token_payment_info::TokenPaymentInfo;
Expand All @@ -27,15 +28,25 @@ impl DocumentDeleteTransition {
.bounds
.default_current_version,
) {
0 => Ok(DocumentDeleteTransitionV0::from_document(
document,
document_type,
token_payment_info,
identity_contract_nonce,
platform_version,
base_feature_version,
)?
.into()),
0 => {
let transition: DocumentDeleteTransition =
DocumentDeleteTransitionV0::from_document(
document,
document_type,
token_payment_info,
identity_contract_nonce,
platform_version,
base_feature_version,
)?
.into();
if let Some(error) = transition
.validate_structure(document_type, platform_version)?
.errors_to_consensus_protocol_error()
{
return Err(error);
}
Comment on lines +42 to +47
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

💬 Nitpick: 5 from_document constructors silently truncate validate_structure errors to the first one

Each of document_delete_transition/from_document.rs:42-49, document_replace_transition/from_document.rs:41-48, document_transfer_transition/from_document.rs:45-52, document_purchase_transition/from_document.rs:45-52, and document_update_price_transition/from_document.rs:44-51 extracts the first error via .into_iter().next() and discards the rest. The current v0 validators each emit at most one error (a single InvalidDocumentTransitionActionError), so this is observationally equivalent today. But the duplicated pattern silently truncates if any future v1 of these validators accumulates multiple errors — and the new SDK-level pre-sign helper validate_base_structure_pre_sign deliberately surfaces multi-error results as ProtocolError::ConsensusErrors, so the convention has already split. Either standardize on ProtocolError::ConsensusErrors here too, or document explicitly that v0 validators are required to produce ≤1 error.

source: ['claude-rust-quality']

Comment on lines +42 to +47
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

🟡 Suggestion: Five from_document constructors silently truncate validate_structure errors to the first one

Each of document_delete_transition/from_document.rs:42-49, document_replace_transition/from_document.rs:41-48, document_transfer_transition/from_document.rs:45-52, document_purchase_transition/from_document.rs:45-52, and document_update_price_transition/from_document.rs:44-51 extracts the first error via .errors.into_iter().next() and discards the rest. The current v0 validators each emit at most one InvalidDocumentTransitionActionError, so this is observationally equivalent today. But the same PR introduces ProtocolError::ConsensusErrors(Vec<_>) and routes the SDK-level pre-sign helper through it (validate_basic_structure/mod.rs:104-110), explicitly to preserve multiple deterministic failures. The convention is now split between these constructor sites and the batch pre-sign path. Either standardize on returning all accumulated errors via ProtocolError::ConsensusErrors, or add an explicit comment that v0 per-transition validators contractually produce ≤1 error so future maintainers don't extend a validator and silently lose detail at this seam.

source: ['claude-general', 'claude-rust-quality', 'codex-rust-quality']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/document_delete_transition/from_document.rs`:
- [SUGGESTION] lines 42-49: Five `from_document` constructors silently truncate `validate_structure` errors to the first one
  Each of `document_delete_transition/from_document.rs:42-49`, `document_replace_transition/from_document.rs:41-48`, `document_transfer_transition/from_document.rs:45-52`, `document_purchase_transition/from_document.rs:45-52`, and `document_update_price_transition/from_document.rs:44-51` extracts the first error via `.errors.into_iter().next()` and discards the rest. The current v0 validators each emit at most one `InvalidDocumentTransitionActionError`, so this is observationally equivalent today. But the same PR introduces `ProtocolError::ConsensusErrors(Vec<_>)` and routes the SDK-level pre-sign helper through it (validate_basic_structure/mod.rs:104-110), explicitly to preserve multiple deterministic failures. The convention is now split between these constructor sites and the batch pre-sign path. Either standardize on returning all accumulated errors via `ProtocolError::ConsensusErrors`, or add an explicit comment that v0 per-transition validators contractually produce ≤1 error so future maintainers don't extend a validator and silently lose detail at this seam.

Ok(transition)
}
version => Err(ProtocolError::UnknownVersionMismatch {
method: "DocumentDeleteTransition::from_document".to_string(),
known_versions: vec![0],
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
mod from_document;
pub mod v0;
pub mod v0_methods;
pub(crate) mod validate_structure;

use bincode::{Decode, Encode};
use derive_more::{Display, From};
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
use crate::data_contract::document_type::DocumentTypeRef;
use crate::state_transition::batch_transition::batched_transition::document_delete_transition::validate_structure::v0::DocumentDeleteTransitionStructureValidationV0;
use crate::state_transition::batch_transition::batched_transition::DocumentDeleteTransition;
use crate::validation::SimpleConsensusValidationResult;
use crate::ProtocolError;
use platform_version::version::PlatformVersion;

mod v0;

pub(crate) trait DocumentDeleteTransitionStructureValidation {
fn validate_structure(
&self,
document_type: DocumentTypeRef,
platform_version: &PlatformVersion,
) -> Result<SimpleConsensusValidationResult, ProtocolError>;
}

impl DocumentDeleteTransitionStructureValidation for DocumentDeleteTransition {
fn validate_structure(
&self,
document_type: DocumentTypeRef,
platform_version: &PlatformVersion,
) -> Result<SimpleConsensusValidationResult, ProtocolError> {
match platform_version
.dpp
.state_transitions
.documents
.documents_batch_transition
.validation
.document_delete_transition_structure_validation
{
0 => self.validate_structure_v0(document_type),
version => Err(ProtocolError::UnknownVersionMismatch {
method: "DocumentDeleteTransition::validate_structure".to_string(),
known_versions: vec![0],
received: version,
}),
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
use crate::consensus::basic::document::InvalidDocumentTransitionActionError;
use crate::data_contract::document_type::accessors::DocumentTypeV0Getters;
use crate::data_contract::document_type::DocumentTypeRef;
use crate::state_transition::batch_transition::batched_transition::DocumentDeleteTransition;
use crate::validation::SimpleConsensusValidationResult;
use crate::ProtocolError;

pub(super) trait DocumentDeleteTransitionStructureValidationV0 {
fn validate_structure_v0(
&self,
document_type: DocumentTypeRef,
) -> Result<SimpleConsensusValidationResult, ProtocolError>;
}

impl DocumentDeleteTransitionStructureValidationV0 for DocumentDeleteTransition {
fn validate_structure_v0(
&self,
document_type: DocumentTypeRef,
) -> Result<SimpleConsensusValidationResult, ProtocolError> {
// Mirrors the drive-abci action validator deletability check;
// contract-local and safe pre-sign.
if !document_type.documents_can_be_deleted() {
return Ok(SimpleConsensusValidationResult::new_with_error(
InvalidDocumentTransitionActionError::new(format!(
"documents of type {} can not be deleted",
document_type.name()
))
.into(),
));
}

Ok(SimpleConsensusValidationResult::default())
}
}
Comment on lines +15 to +34
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

🟡 Suggestion: 5 of 6 new per-document validate_structure_v0 impls are no-op stubs

The PR adds full module scaffolding (mod.rs + v0/mod.rs + a …StructureValidation trait + a …StructureValidationV0 trait + a version-dispatch site) for six document transition variants: create, delete, replace, transfer, purchase, update_price. Only DocumentCreateTransition::validate_structure_v0 performs any check (the entropy-derived id equality test). The other five impls — document_delete_transition/validate_structure/v0/mod.rs:9–13, document_purchase_transition/.../v0/mod.rs:9–13, document_replace_transition/.../v0/mod.rs:9–13, document_transfer_transition/.../v0/mod.rs:9–13, document_update_price_transition/.../v0/mod.rs:9–13 — all simply return Ok(SimpleConsensusValidationResult::default()). They contribute no validation but still widen the public DPP surface and add a version-dispatch site that future bumps must keep in sync. The cheap client-side checks the SDK builders could perform locally (delete-on-immutable-type, purchase-on-non-tradeable, set-price-on-disallowed-trade-mode, transfer-on-non-transferable, replace-on-immutable-type) are already enforced server-side in rs-drive-abci and the builders already resolve the document_type before constructing the transition, so the wiring is in place but the content is not. Either fill in the bodies in this PR or document explicitly in the PR description that these are dispatch surfaces for follow-up work.

source: ['claude-general', 'codex-general']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/document_delete_transition/validate_structure/v0/mod.rs`:
- [SUGGESTION] lines 9-13: 5 of 6 new per-document `validate_structure_v0` impls are no-op stubs
  The PR adds full module scaffolding (`mod.rs` + `v0/mod.rs` + a `…StructureValidation` trait + a `…StructureValidationV0` trait + a version-dispatch site) for six document transition variants: create, delete, replace, transfer, purchase, update_price. Only `DocumentCreateTransition::validate_structure_v0` performs any check (the entropy-derived id equality test). The other five impls — `document_delete_transition/validate_structure/v0/mod.rs:9–13`, `document_purchase_transition/.../v0/mod.rs:9–13`, `document_replace_transition/.../v0/mod.rs:9–13`, `document_transfer_transition/.../v0/mod.rs:9–13`, `document_update_price_transition/.../v0/mod.rs:9–13` — all simply return `Ok(SimpleConsensusValidationResult::default())`. They contribute no validation but still widen the public DPP surface and add a version-dispatch site that future bumps must keep in sync. The cheap client-side checks the SDK builders could perform locally (delete-on-immutable-type, purchase-on-non-tradeable, set-price-on-disallowed-trade-mode, transfer-on-non-transferable, replace-on-immutable-type) are already enforced server-side in `rs-drive-abci` and the builders already resolve the `document_type` before constructing the transition, so the wiring is in place but the content is not. Either fill in the bodies in this PR or document explicitly in the PR description that these are dispatch surfaces for follow-up work.

Loading
Loading