-
Notifications
You must be signed in to change notification settings - Fork 54
fix(sdk): validate batch base structure before signing #3133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: v3.1-dev
Are you sure you want to change the base?
Changes from 24 commits
2446db0
472bcad
956c192
3235ac7
5647f51
fde8031
555940b
89610b9
c773a81
b81e065
52a04ff
8125b54
10e173a
868e1a6
534d40a
8742a0b
9ab76e3
0ac3718
b7d7471
02f8159
f54eb96
f11960b
ea12f77
1f4c357
db0e002
be00490
11fb8c4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,6 +131,9 @@ pub enum ProtocolError { | |
| #[error(transparent)] | ||
| ConsensusError(Box<ConsensusError>), | ||
|
|
||
| #[error("Multiple consensus errors: {}", join_consensus_errors(.0))] | ||
| ConsensusErrors(Vec<ConsensusError>), | ||
|
|
||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💬 Nitpick: ConsensusErrors plural variant parallels singular ConsensusError without a canonicalization rule
source: ['claude']
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💬 Nitpick: Existing Adding source: ['claude'] |
||
| #[error(transparent)] | ||
| Document(Box<DocumentError>), | ||
|
|
||
|
|
@@ -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 |
|---|---|---|
| @@ -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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: Constructor-only document
source: ['claude-rust-quality', 'codex-rust-quality'] 🤖 Fix this with AI agents |
||
|
|
||
| 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: SDK pre-sign The new source: ['claude-general'] 🤖 Fix this with AI agents |
||
| } | ||
| 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 |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 source: ['claude-rust-quality']
Comment on lines
+42
to
+47
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: Five Each of source: ['claude-general', 'claude-rust-quality', 'codex-rust-quality'] 🤖 Fix this with AI agents |
||
| Ok(transition) | ||
| } | ||
| version => Err(ProtocolError::UnknownVersionMismatch { | ||
| method: "DocumentDeleteTransition::from_document".to_string(), | ||
| known_versions: vec![0], | ||
|
|
||
| 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: 5 of 6 new per-document The PR adds full module scaffolding ( source: ['claude-general', 'codex-general'] 🤖 Fix this with AI agents |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Suggestion: Adding
ProtocolError::ConsensusErrorsis a breaking public-enum changeProtocolErroris a public enum without#[non_exhaustive], so introducing the newConsensusErrors(Vec<ConsensusError>)variant breaks any downstream crate that matchesProtocolErrorexhaustively. 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