-
Notifications
You must be signed in to change notification settings - Fork 44
fix: data contract proof doesn't work with new auto fields #2501
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
Conversation
WalkthroughThis PR adds a new public method Changes
Sequence Diagram(s)sequenceDiagram
participant Verifier
participant ContractSerialization
participant ExpectedContract
Verifier->>ContractSerialization: Retrieve serialized contract
Verifier->>ExpectedContract: Retrieve expected data contract
Verifier->>ContractSerialization: Call eq_without_auto_fields(expected data contract)
ContractSerialization-->>Verifier: Return boolean result
alt Equality check passes
Verifier->>Verifier: Handle ProofError::IncorrectProof
else
Verifier->>Verifier: Continue processing state transition
end
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (19)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs (1)
109-111
:⚠️ Potential issueMissing auto fields handling in DataContractUpdate case
While the DataContractCreate case was updated to use
eq_without_auto_fields
, this DataContractUpdate case still uses direct comparison (!=
). This inconsistency could lead to the same auto-generated fields issue in this context.-if &contract_for_serialization != data_contract_update.data_contract() { +if !contract_for_serialization.eq_without_auto_fields(data_contract_update.data_contract()) { return Err(Error::Proof(ProofError::IncorrectProof(format!("proof of state transition execution did not contain exact expected contract after update with id {}", data_contract_update.data_contract().id())))); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rs-dpp/src/data_contract/serialized_version/mod.rs
(1 hunks)packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (wasm-dpp) / Linting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Rust crates security audit
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (1)
packages/rs-dpp/src/data_contract/serialized_version/mod.rs (1)
100-129
: Great addition of theeq_without_auto_fields
methodThe implementation correctly addresses the PR's objective of excluding auto-generated fields during data contract comparisons. The method handles both V0 and V1 format versions appropriately, with V0 using direct equality and V1 explicitly comparing only the necessary fields.
The explicit handling of cross-version comparisons returning false is also a good safeguard.
...-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs
Outdated
Show resolved
Hide resolved
chore: update to latest dash core 37 (#2483) feat(platform)!: token advanced distribution and updates (#2471) fix: token history contract (#2474) Co-authored-by: Ivan Shumkov <ivan@shumkov.ru> Co-authored-by: QuantumExplorer <quantum@dash.org> fix(drive): using new rust dash core methods for reversed quorum hash to maintain backwards compatibility (#2489) feat: more granular integer document property types (#2455) Co-authored-by: Quantum Explorer <quantum@dash.org> docs: update comment for data contract code range (#2476) feat: validate token name localizations (#2468) feat(sdk): get identity by non-unique keys build(deps): update grovedb to current develop test: test identity by non-unique pubkey hashes fix(sdk): dash core client fails to get quorum chore: minor fixes test(drive-abci): identity by non-unique pubkey start after chore: minor changes to verify feat(sdk): token and group queries (#2449) chore: revert limit 1 => limit none chore: add non-unique key to test identities test(sdk): test vectors for test_fetch_identity_by_non_unique_public_keys fix(platform)!: token distribution fixes and tests (#2494) chore(platform): bump to version 2.0.0-dev.1 (#2495) test: update assertion fix(sdk): make some things public (#2496) feat(platform): require token for document actions (#2498) fix: data contract proof doesn't work with new auto fields (#2501)
chore: update to latest dash core 37 (#2483) feat(platform)!: token advanced distribution and updates (#2471) fix: token history contract (#2474) Co-authored-by: Ivan Shumkov <ivan@shumkov.ru> Co-authored-by: QuantumExplorer <quantum@dash.org> fix(drive): using new rust dash core methods for reversed quorum hash to maintain backwards compatibility (#2489) feat: more granular integer document property types (#2455) Co-authored-by: Quantum Explorer <quantum@dash.org> docs: update comment for data contract code range (#2476) feat: validate token name localizations (#2468) feat(sdk): get identity by non-unique keys build(deps): update grovedb to current develop test: test identity by non-unique pubkey hashes fix(sdk): dash core client fails to get quorum chore: minor fixes test(drive-abci): identity by non-unique pubkey start after chore: minor changes to verify feat(sdk): token and group queries (#2449) chore: revert limit 1 => limit none chore: add non-unique key to test identities test(sdk): test vectors for test_fetch_identity_by_non_unique_public_keys fix(platform)!: token distribution fixes and tests (#2494) chore(platform): bump to version 2.0.0-dev.1 (#2495) test: update assertion fix(sdk): make some things public (#2496) feat(platform): require token for document actions (#2498) fix: data contract proof doesn't work with new auto fields (#2501)
Issue being fixed or feature implemented
When we added auto generated fields to data contract, the proof logic that uses full equality doesn't work anymore, because on client side autogenerated fields are empty.
What was done?
How Has This Been Tested?
None
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Refactor