test: unify identity versioned cost coverage #2416
Conversation
QuantumExplorer
added
enhancement
WalkthroughThis pull request introduces comprehensive modifications to the test suites across multiple modules in the Rust Drive implementation. The changes primarily focus on enhancing test coverage by adding version-specific test cases for identity and document insertion, balance updates, and key management. A key pattern emerges where tests are now separated into first and latest platform version scenarios, with helper functions introduced to reduce code duplication and improve test modularity. The modifications aim to provide more granular testing of fee estimation and application processes across different platform versions. Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/rs-drive/src/drive/identity/update/mod.rs (2)
Line range hint
73-142: Add parameter documentation to the helper function.While the function is well-structured, consider adding documentation for the parameters to improve maintainability:
+ /// Helper function to test adding balance to an identity + /// + /// # Arguments + /// * `apply` - If true, commits the transaction. If false, only estimates costs + /// * `platform_version` - The platform version to use for the test + /// * `expected_fee_result` - The expected fee calculation result fn do_should_add_to_balance( apply: bool, platform_version: &PlatformVersion, expected_fee_result: FeeResult, ) {
275-383: Enhance assertion messages for better debugging.Consider adding more descriptive messages to assertions to make test failures more informative:
- assert_eq!(updated_balance.expect("balance should present"), 0); + assert_eq!( + updated_balance.expect("balance should be present after update"), + 0, + "Balance should be zero after debt reduction" + ); - assert_eq!(updated_negative_balance, negative_amount - added_balance); + assert_eq!( + updated_negative_balance, + negative_amount - added_balance, + "Negative balance should be reduced by the added balance amount" + );packages/rs-drive/src/drive/document/insert/mod.rs (2)
318-379: Document the fee calculation logic.The tests show complex fee calculations with significant differences between apply and estimate scenarios. Consider adding documentation explaining:
- How storage fees are calculated using epoch-based costs
- Why processing fees vary so much between apply and estimate
- The business logic behind these fee structures
Add a doc comment explaining the fee calculation:
/// # Fee Calculation Logic /// /// Storage fees are calculated as: /// * Base storage cost (1305) * Epoch-specific cost per byte /// /// Processing fees vary by operation: /// * Apply: 900,400 credits /// * Estimate: 73,253,660 credits /// /// The higher estimation cost accounts for...
Line range hint
381-444: LGTM! Well-documented helper function.The function is well-structured with clear documentation and logical separation of concerns. Consider extracting the document setup into a separate helper function for reuse in other tests.
/// Extract document setup into a reusable function fn setup_dashpay_profile_document( drive: &Drive, random_owner_id: [u8; 32], platform_version: &PlatformVersion, ) -> (DataContract, Document) { let contract = setup_contract( drive, "tests/supporting_files/contract/dashpay/dashpay-contract.json", None, None, ); let document_type = contract .document_type_for_name("profile") .expect("expected to get document type"); let document = json_document_to_document( "tests/supporting_files/contract/dashpay/profile0.json", Some(random_owner_id.into()), document_type, platform_version, ) .expect("expected to get cbor document"); (contract, document) }packages/rs-drive/src/drive/identity/balance/update.rs (1)
24-71: Consider documenting the fee estimation pattern.This file follows the same pattern seen across other files where estimation costs are significantly higher than apply costs. This appears to be a consistent design decision across the codebase.
Consider adding a central documentation (e.g., in a README or module-level doc) explaining:
- Why estimation costs are consistently higher
- The rationale behind this design decision
- How these values were determined
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/rs-drive/src/drive/document/insert/mod.rs(4 hunks)packages/rs-drive/src/drive/identity/balance/update.rs(7 hunks)packages/rs-drive/src/drive/identity/insert/add_new_identity/mod.rs(1 hunks)packages/rs-drive/src/drive/identity/insert/add_new_identity/v0/mod.rs(0 hunks)packages/rs-drive/src/drive/identity/update/mod.rs(9 hunks)
💤 Files with no reviewable changes (1)
- packages/rs-drive/src/drive/identity/insert/add_new_identity/v0/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive-abci) / Linting
- 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: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
🔇 Additional comments (4)
packages/rs-drive/src/drive/identity/insert/add_new_identity/mod.rs (3)
136-146: LGTM! Well-structured test for first platform version.The test effectively validates both storage and processing fees for identity insertion.
148-158: Verify if fee values should be identical across versions.The test uses identical fee values for both first and latest platform versions. Please confirm if this is intentional or if the latest version should have different fee calculations.
184-223: LGTM! Well-designed helper function.The helper function effectively:
- Encapsulates common test logic
- Handles both apply and non-apply scenarios
- Includes proper transaction management
- Has appropriate assertions
packages/rs-drive/src/drive/identity/update/mod.rs (1)
24-77: Clarify the significant difference in processing fees.The processing fees show a large variance between apply and estimate scenarios:
- Apply: 174,660
- Estimate: 4,278,840
Please explain why estimation costs are significantly higher (~24x) than actual application costs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/rs-drive/src/drive/identity/update/mod.rs (4)
134-141: Complete the root hash verification.The code calculates the root hash but doesn't verify it. Consider adding an assertion to ensure the state remains unchanged when
applyis false.- // Not applying -> no commit. We can check root hash if we want - let app_hash_after = drive - .grove - .root_hash(None, &platform_version.drive.grove_version) - .unwrap() - .expect("should return app hash"); - // Or any other logic to ensure no state changes actually took effect - let _ = app_hash_after; + // Verify state remains unchanged when not applying + let app_hash_before = drive + .grove + .root_hash(None, &platform_version.drive.grove_version) + .unwrap() + .expect("should return app hash"); + drive.grove.rollback_transaction(db_transaction).unwrap(); + let app_hash_after = drive + .grove + .root_hash(None, &platform_version.drive.grove_version) + .unwrap() + .expect("should return app hash"); + assert_eq!(app_hash_before, app_hash_after, "State should remain unchanged when not applying");
703-704: Consider testing intermediate revision values.The test jumps from revision 0 to 2 directly. Consider adding test cases for:
- Incrementing revision by 1
- Attempting to decrease revision
- Attempting to skip multiple revisions
739-741: Enhance the comment for clarity.The comment could be more descriptive about the expected state.
- // No commit => no changes + // When not applying changes: + // 1. Transaction should be rolled back + // 2. Identity revision should remain at 0 + // 3. State should be unchanged
Line range hint
1-742: Overall excellent test structure and coverage!The implementation demonstrates:
- Consistent test patterns across different operations
- Effective use of helper functions to reduce duplication
- Comprehensive coverage of platform versions and scenarios
Consider adding error case tests for:
- Invalid identity IDs
- Unauthorized key operations
- Concurrent modifications
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive/src/drive/identity/update/mod.rs(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Formatting
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive-abci) / Formatting
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (2)
packages/rs-drive/src/drive/identity/update/mod.rs (2)
Line range hint
1-25: Well-structured module organization!The code follows Rust best practices with clear separation of concerns and logical grouping of imports.
Line range hint
400-614: Excellent test coverage for key disabling functionality!The tests thoroughly cover:
- Both first and latest platform versions
- Both apply and estimate scenarios
- Fee consistency verification
|
Merging this in as it's only test improvements with no regressions. @shumkov if you wish to review you may though on Monday. |
Issue being fixed or feature implemented
This pull request enhances test coverage for identity operations by unifying and expanding the versioned cost tests. It adds both “apply” and “no apply” test scenarios for first/latest versions, eliminating repetitive code and ensuring comprehensive validation of fee calculations.
What was done?
How Has This Been Tested?
Breaking Changes
No breaking changes introduced. Existing functionality remains intact, though tests have been significantly reorganized and expanded.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Tests
Refactor