fix(drive-abci): document purchase on mutable document from different epoch had issue #2420
Conversation
WalkthroughThis pull request involves multiple package updates across the Dash Drive repository, primarily focusing on dependency version upgrades. The changes include updating Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (14)
🔇 Additional comments (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
QuantumExplorer
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/rs-drive-abci/tests/supporting_files/contract/crypto-card-game/crypto-card-game-direct-purchase-documents-mutable.json (1)
13-61: Consider adding maximum bounds for numeric properties.While minimum bounds are set for
attackanddefense, consider adding maximum bounds to prevent potential integer overflow issues."attack": { "type": "integer", "description": "Attack power of the card", "minimum": 0, + "maximum": 65535, "position": 5 }, "defense": { "type": "integer", "description": "Defense level of the card", "minimum": 0, + "maximum": 65535, "position": 6 }packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/documents_batch/mod.rs (3)
Line range hint
6451-6466: Refactor Random Number Generator Initialization for Test IsolationThe tests in the
nft_testsmodule use the same seed for theStdRngrandom number generator. To ensure test isolation and reproducibility, consider initializing the RNG with different seeds in each test or resetting the RNG state within each test.
Line range hint
6451-7186: Reduce Code Duplication Across TestsThe tests
test_document_set_price_and_purchaseandtest_document_set_price_and_purchase_different_epoch_documents_mutableshare significant portions of setup and logic. Refactor shared code into helper functions or fixtures to enhance maintainability and readability.
7730-7731: Clarify Test Name for Better UnderstandingThe test
test_document_set_price_and_purchase_different_epochcould be renamed for clarity. A name liketest_document_set_price_and_purchase_across_epochsmore accurately describes the test's purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
packages/rs-drive-abci/Cargo.toml(1 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/documents_batch/mod.rs(2 hunks)packages/rs-drive-abci/tests/supporting_files/contract/crypto-card-game/crypto-card-game-direct-purchase-documents-mutable.json(1 hunks)packages/rs-drive/Cargo.toml(2 hunks)packages/rs-drive/src/drive/document/estimation_costs/stateless_delete_of_non_tree_for_costs/v0/mod.rs(1 hunks)packages/rs-drive/src/util/grove_operations/mod.rs(1 hunks)packages/rs-platform-version/Cargo.toml(1 hunks)packages/strategy-tests/Cargo.toml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive) / Formatting
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dapi-grpc) / Unused dependencies
- GitHub Check: Rust packages (dapi-grpc) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Linting
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
🔇 Additional comments (13)
packages/rs-platform-version/Cargo.toml (1)
14-14: Verify grovedb-version 2.2.1 changelog for breaking changes.The upgrade from 2.1.0 to 2.2.1 should be reviewed for any breaking changes that might affect the platform versioning functionality.
Run this script to check the changelog and release notes:
✅ Verification successful
✓ grovedb-version 2.2.1 upgrade is safe
The upgrade from 2.1.0 to 2.2.1 contains only bug fixes and optimizations with no breaking changes. The changes include a sub tree prefix fix and state sync optimization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check grovedb-version 2.2.1 release notes and changes # Search for any BREAKING CHANGE markers in commit messages gh api graphql -f query=' { repository(owner: "dashpay", name: "grovedb") { releases(first: 5, orderBy: {field: CREATED_AT, direction: DESC}) { nodes { tagName description } } } }' # Look for any migration guides or upgrade notes rg -i "upgrade|migration|breaking.?change" -g "*.md"Length of output: 17350
packages/rs-drive/Cargo.toml (3)
55-60: Verify consistency of grovedb ecosystem updates.All grovedb-related dependencies have been updated to version 2.2.1, which is good for consistency. However, we should verify the compatibility and changes across the ecosystem.
Run this script to check for any compatibility issues:
✅ Verification successful
GroveDB ecosystem update verification successful
All GroveDB-related dependencies are consistently set to version 2.2.1 in both Cargo.toml and Cargo.lock, with proper integration through feature flags and error handling. No compatibility issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify grovedb ecosystem compatibility # Check if all grovedb dependencies are at the same version rg '"grovedb.*"' -A 1 # Look for any compatibility matrices in documentation rg -i "compatibility|version.?matrix" -g "*.md"Length of output: 8555
52-52: Review intmap's breaking changes from 2.0.0 to 3.0.1.The major version bump (2.0.0 -> 3.0.1) of intmap requires careful review of breaking changes.
Run this script to check the changelog:
107-107: Assess impact of switching from grovedb/full to grovedb/minimal.The server feature now uses
grovedb/minimalinstead ofgrovedb/full. This change could affect functionality and needs to be verified.Run this script to understand the feature differences:
✅ Verification successful
Switching to grovedb/minimal is safe for production code
The change only affects test configuration and does not impact production code. All grovedb imports in the codebase use core functionality that is available in both minimal and full features.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Compare grovedb full vs minimal features # Search for feature documentation rg -i "feature.*full|feature.*minimal" -g "*.md" # Look for any usage of full-specific features that might break ast-grep --pattern 'use grovedb::$_'Length of output: 48870
Script:
#!/bin/bash # Search for grovedb's Cargo.toml and feature definitions fd Cargo.toml | xargs rg -l "grovedb.*dependencies|grovedb.*features|features.*minimal|features.*full" # Search for feature documentation in grovedb source rg -i "feature.*minimal|feature.*full" -g "*.rs"Length of output: 612
packages/strategy-tests/Cargo.toml (1)
49-49: Verify RocksDB version compatibility.The upgrade to RocksDB 0.23.0 should be verified for compatibility with other dependencies and potential performance impacts.
Run this script to check for any compatibility issues or breaking changes:
✅ Verification successful
RocksDB version upgrade verified as safe
Both packages use RocksDB 0.23.0 consistently, and the codebase uses standard, stable RocksDB APIs. No compatibility issues or breaking changes detected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check RocksDB version compatibility and changelog # Test: Look for breaking changes in RocksDB 0.23.0 # Check if other packages are using different RocksDB versions rg -g "Cargo.toml" "rocksdb.*version.*0\.[0-9]+\.[0-9]+" # Check for any RocksDB related test failures in CI logs rg -i "rocksdb.*(?:error|fail|break)" .github/Length of output: 240
Script:
#!/bin/bash # Check for RocksDB usage patterns and imports ast-grep --pattern 'use rocksdb::$_' # Look for RocksDB configuration or setup code rg -g "*.rs" "rocksdb::(?:DB|Options|WriteOptions|ReadOptions)" -A 2Length of output: 363
packages/rs-drive-abci/Cargo.toml (1)
102-102: LGTM! Consistent RocksDB version across packages.The RocksDB version update to 0.23.0 aligns with the version used in strategy-tests, maintaining consistency across packages.
packages/rs-drive-abci/tests/supporting_files/contract/crypto-card-game/crypto-card-game-direct-purchase-documents-mutable.json (3)
8-12: LGTM! Document configuration aligns with purchase requirements.The configuration enables document mutability, deletion, and transfers, which is essential for the document purchase functionality across epochs.
62-121: LGTM! Comprehensive indexing strategy.The indices are well-designed for efficient querying of:
- Ownership tracking
- Card attributes (attack, defense)
- Transfer history with block heights
- Combined queries (owner + transfer time)
123-130: Verify required fields for document purchases.The schema correctly requires transfer-related fields ($transferredAt, $transferredAtBlockHeight, $transferredAtCoreBlockHeight) which are essential for tracking document purchases across epochs.
✅ Verification successful
Transfer-related fields are correctly specified and consistent with the codebase.
The schema follows the established pattern for crypto card game documents, requiring all three transfer-related fields ($transferredAt, $transferredAtBlockHeight, $transferredAtCoreBlockHeight) which are properly supported by the implementation in document_info.rs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that transfer-related fields are consistently used across the codebase # Test: Look for usage of transfer-related fields in document operations # Check for transfer field usage in document operations rg -A 5 '\$transferredAt|\$transferredAtBlockHeight|\$transferredAtCoreBlockHeight' \ --type rust \ packages/rs-drive*/src/Length of output: 3807
Script:
#!/bin/bash # Search for other document purchase schemas fd -e json . packages/rs-drive-abci/tests/supporting_files/contract/ \ --exec grep -l "transferredAt\|transferredAtBlockHeight\|transferredAtCoreBlockHeight" {} \; # Show content of found files fd -e json . packages/rs-drive-abci/tests/supporting_files/contract/ \ --exec grep -A 10 "required" {} \;Length of output: 13164
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/documents_batch/mod.rs (3)
8011-8012:⚠️ Potential issueIncrement Document Revision Before Purchase
In
test_document_set_price_and_purchase_different_epoch, the document's revision should be incremented before making the purchase to reflect the state change. Missing this step may lead to version conflicts or rejection of the state transition.Apply this change to increment the document revision:
document.bump_revision(); +document.bump_revision(); let documents_batch_purchase_transition =Likely invalid or redundant comment.
7197-7199: Verify Correctness of Balance CalculationIn
test_document_set_price_and_purchase, the assertion on line 7198:assert_eq!(buyers_balance, dash_to_credits!(0.9) - 68691480);Ensure that the
buyers_balanceaccurately reflects all fees and credits involved in the transaction, including storage fees, processing fees, and the purchase amount.✅ Verification successful
Balance Calculation is Accurate and Complete
The assertion
buyers_balance = dash_to_credits!(0.9) - 68691480is correct and accounts for:
- Purchase price (0.1 DASH)
- Storage fee (64,611,000 credits)
- Processing fee (4,080,480 credits)
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find the complete test implementation ast-grep --pattern 'fn test_document_set_price_and_purchase($$$) { $$$ }' # Search for similar test cases and fee calculations rg "test_document.*purchase" -A 5 rg "68691480|storage.*fee|processing.*fee" -A 2Length of output: 67310
8088-8090: Assert Ownership Transfer After PurchaseAfter the purchase in
test_document_set_price_and_purchase_different_epoch, verify that the document's ownership has been correctly transferred to the purchaser. This ensures the purchase operation completed successfully.Add the following assertion:
assert_eq!(document.owner_id(), purchaser.id());packages/rs-drive/src/drive/document/estimation_costs/stateless_delete_of_non_tree_for_costs/v0/mod.rs (1)
85-90: Ensure Key Length Fits Withinu16BoundsWhen casting
(key_info_path.len() - 1)tou16, verify that the length does not exceedu16::MAX. If there's a possibility of overflow, implement checks or use a larger integer type to prevent data loss.
...rive-abci/src/execution/validation/state_transition/state_transitions/documents_batch/mod.rs
Show resolved
Hide resolved
...s-drive/src/drive/document/estimation_costs/stateless_delete_of_non_tree_for_costs/v0/mod.rs
Show resolved
Hide resolved
Issue being fixed or feature implemented
If a document was changed at a different epoch, the bought at a different later epoch, this would cause a grovedb error. This has now been fixed.
What was done?
We also took the time to upgrade some dependencies in GroveDB, and reduce some dependencies as well leading to slightly faster compilation time.
How Has This Been Tested?
All tests are passing.
Breaking Changes
None expected.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Dependency Updates
rocksdblibrary version from 0.22.0 to 0.23.0 in multiple packagesgrovedband related dependencies to version 2.2.1intmaplibrary to version 3.0.1grovedb-versionto version 2.2.1 in the platform-version packageNew Test Files
Internal Improvements
Action and Dockerfile Updates