Add more did validator test#3754
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive test coverage for the DID validator module, focusing on testing various failure scenarios across different envelope types (RawTxHash, BitcoinMessageV0, and WebAuthnV0).
Key Changes:
- Added 15 new test cases covering various error conditions in DID authentication
- Introduced helper functions for creating test payloads and signatures
- Added test-only helper functions to the did_validator module for payload creation
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| frameworks/rooch-framework/sources/tests/did_validator_test.move | Added comprehensive test coverage for DID validator error scenarios including missing DIDs, unauthorized verification methods, invalid signatures, and envelope-specific validation failures |
| frameworks/rooch-framework/sources/auth_validator/did_validator.move | Added two test-only helper functions (create_test_did_auth_payload and create_test_webauthn_envelope) to support test payload construction |
| let i = 0; | ||
| while (i < 64) { | ||
| vector::push_back(&mut signature, 0x00); | ||
| i = i + 1; | ||
| }; |
There was a problem hiding this comment.
Variable i is declared and reused in the same scope on lines 541-545. The first loop (lines 526-530) already declared and used i, but then it's re-declared on line 541. While this may work in Move, it's better practice to reuse the existing variable or use different variable names to avoid confusion. Consider using i = 0; instead of let i = 0; on line 541.
| #[test] | ||
| #[expected_failure(abort_code = DID_ERROR_VERIFICATION_METHOD_NOT_AUTHORIZED, location = rooch_framework::did_validator)] | ||
| fun test_raw_tx_hash_verification_method_not_found() { | ||
| // Test that validation fails when vm_fragment does not exist in DID document |
There was a problem hiding this comment.
The comment on line 218 says "Test that validation fails when vm_fragment does not exist in DID document", but this is actually testing when a verification method exists but is not in the authentication relationship. The scenario where a verification method doesn't exist at all is the same as the scenario where it's not authorized (both fail at the same check). Consider updating the comment to: "Test that validation fails when vm_fragment is not in the authentication relationship (non-existent keys also fail here)".
| // Test that validation fails when vm_fragment does not exist in DID document | |
| // Test that validation fails when vm_fragment is not in the authentication relationship (non-existent keys also fail here) |
| let i = 0; | ||
| while (i < 64) { | ||
| vector::push_back(&mut signature, 0x00); | ||
| i = i + 1; | ||
| }; |
There was a problem hiding this comment.
[nitpick] This code block for creating a 64-byte signature is duplicated many times throughout the test file (lines 94-98, 151-156, 198-203, 231-236, 278-283, 315-320, 348-353, 390-395, 427-432, 454-459, 493-498, 540-545, 594-599, 653-658, 703-708). Consider extracting this into a helper function like create_empty_signature(size: u64): vector<u8> to improve maintainability and reduce code duplication.
| let i = 0; | ||
| while (i < 64) { | ||
| vector::push_back(&mut signature, 0x00); | ||
| i = i + 1; | ||
| }; |
There was a problem hiding this comment.
Variable i is declared and reused in the same scope on lines 391-395. The first loop (lines 382-386) already declared and used i, but then it's re-declared on line 391. While this may work in Move, it's better practice to reuse the existing variable or use different variable names to avoid confusion. Consider using i = 0; instead of let i = 0; on line 391.
Summary
Summary about this PR