test: cover proof-verifier, batch token actions, identity conversions#3526
Merged
QuantumExplorer merged 6 commits intoApr 23, 2026
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 20 minutes and 30 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR adds comprehensive unit test coverage for identity conversions, proof verification, and token state transition actions. Tests validate serialization/deserialization roundtrips, accessor/mutator behavior, error handling for invalid inputs, and delegation patterns without modifying any public API declarations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
Review GateCommit:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
packages/rs-drive-proof-verifier/src/from_request.rs (1)
1082-1093: Nit: misleading section header.The header at Line 1083 reads
bincode_encode_values: error path, buttest_bincode_decode_mixed_valid_and_invalidactually exercisesbincode_decode_values. Consider updating the comment to avoid confusing future readers.Proposed tweak
- // --------------------------------------------------------------- - // bincode_encode_values: error path - // --------------------------------------------------------------- + // --------------------------------------------------------------- + // bincode_decode_values: mixed valid/invalid error path + // ---------------------------------------------------------------🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-proof-verifier/src/from_request.rs` around lines 1082 - 1093, The section header comment "bincode_encode_values: error path" is misleading for the test that exercises bincode_decode_values; update the comment to accurately reflect that this block tests bincode_decode_values error handling (e.g., change it to "bincode_decode_values: error path" or similar) so readers can locate tests for bincode_decode_values and avoid confusion when inspecting test_bincode_decode_mixed_valid_and_invalid.packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_config_update_transition_action/v0/transformer.rs (1)
237-425: Note: many of these tests exercise onlyOption::unwrap_or/Clone, not the transformer.The
change_note.unwrap_or(public_note)tests, and the per-variantclone()round-trips ofTokenConfigurationChangeItem, are essentially validating std library behavior and derivedCloneimpls rather than the transformer's logic. They add maintenance cost with limited defect-catching value — if either upstream variant layout changes, these need updating without reflecting a real regression in the transformer.Consider consolidating into one or two parameterized assertions, or replacing with a true integration test against
try_from_borrowed_token_config_update_transition_with_contract_lookup. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_config_update_transition_action/v0/transformer.rs` around lines 237 - 425, The test module contains many small unit tests that only verify standard library behavior (Option::unwrap_or) and derived Clone of TokenConfigurationChangeItem (tests like config_update_change_note_* and the per-variant clone round-trips), which bloats maintenance; consolidate these into one or two parameterized tests that cover the change_note merging rule and Clone round-trip, or remove them and add a focused integration test that exercises try_from_borrowed_token_config_update_transition_with_contract_lookup end-to-end to validate the transformer behavior; update the tests in the tests mod (referencing functions/configs named config_update_change_note_*, config_update_borrowed_note_clone_round_trip, and the TokenConfigurationChangeItem clone tests) to either combine cases into a table-driven test or replace them with an integration test that constructs realistic inputs and asserts the transformer's outputs.packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/transformer.rs (2)
412-421: Movebaseinstead of cloning it.Line 420 clones
base, but the original value is not used afterward. Moving it keeps the fixture simpler and avoids a potentialredundant_cloneclippy warning.♻️ Proposed cleanup
let value = TokenDestroyFrozenFundsTransitionV0 { - base: base.clone(), + base, frozen_identity_id: dpp::identifier::Identifier::new([0x07; 32]), public_note: Some("foo".to_string()), };As per coding guidelines, “Follow rustfmt defaults and keep code clippy-clean for Rust modules”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/transformer.rs` around lines 412 - 421, The test fixture creates `base` as a TokenBaseTransition::V0 and then constructs `TokenDestroyFrozenFundsTransitionV0 { base: base.clone(), ... }` which needlessly clones `base`; change the constructor to move `base` instead of calling `clone()` (i.e., pass `base` directly) and remove the unused clone, updating any variable ownership as needed so `TokenBaseTransitionV0`, `TokenBaseTransition::V0`, and `TokenDestroyFrozenFundsTransitionV0` use the moved value.
332-376: Tie these fragment tests to production code paths.These tests currently re-check
Option::unwrap_or, a locally constructed error wrapper, and theu64alias bounds. They can still pass if the transformer’s real note-merge or error-construction code regresses. Consider extracting tiny private helpers for note resolution/error construction and calling those from both the transformer and tests, or use a fixture that invokes the actual branch under test.Also applies to: 378-401, 444-453
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/transformer.rs` around lines 332 - 376, The tests are exercising Option::unwrap_or directly instead of the transformer's real logic; extract a small private helper (e.g. resolve_action_note(change_note: Option<Option<String>>, public_note: Option<String>) -> Option<String>) and/or an error-construction helper used by the transformer in token_destroy_frozen_funds_transition_action::v0::transformer, call that helper from the transform function, and update the unit tests (change_note_some_some_takes_precedence_over_public_note, change_note_some_none_clears_user_note, change_note_none_preserves_user_note, both_none_yields_none, borrowed_branch_clones_public_note and the other referenced tests) to invoke the helper or the real transformer code path instead of calling unwrap_or directly so the tests fail if the production merge/error logic regresses.packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/mod.rs (1)
187-190: Compare note refs as&strto avoid temporaryStringallocations.Line 190 and Line 210 allocate
Strings only for equality checks. Mapping the accessor result to&strkeeps these assertions allocation-free and less likely to trip clippy/style checks.♻️ Proposed cleanup
- assert_eq!(v0.public_note(), Some(&"burning".to_string())); + assert_eq!(v0.public_note().map(String::as_str), Some("burning")); @@ - assert_eq!(v0.public_note(), Some(&"swapped".to_string())); + assert_eq!(v0.public_note().map(String::as_str), Some("swapped"));As per coding guidelines, “Follow rustfmt defaults and keep code clippy-clean for Rust modules”.
Also applies to: 207-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/mod.rs` around lines 187 - 190, The tests v0_public_note_returns_ref_when_set (and the similar test around lines 207-210) call make_v0 and compare v0.public_note() to Some(&"burning".to_string()), causing temporary String allocations; change the assertions to compare as &str by mapping the accessor result to an &str (e.g., assert_eq!(v0.public_note().map(|s| s.as_str()), Some("burning"))) so no temporary String is created; locate uses of make_v0 and the public_note() accessor in those test functions and update both assertions accordingly.packages/rs-dpp/src/identity/identity_public_key/v0/conversion/json.rs (1)
128-130: Avoid the unnecessaryVecallocation in this assertion.The pattern
&vec![0xABu8; 33][..]allocates on the heap unnecessarily. Use an array slice directly to keep the test clippy-clean and idiomatic.Suggested fix
let key: IdentityPublicKeyV0 = s.as_str().try_into().expect("parse succeeds"); assert_eq!(key.id, 7); - assert_eq!(key.data.as_slice(), &vec![0xABu8; 33][..]); + assert_eq!(key.data.as_slice(), [0xABu8; 33].as_slice());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/identity/identity_public_key/v0/conversion/json.rs` around lines 128 - 130, The assertion unnecessarily allocates a Vec for the expected bytes; change the RHS of the assertion comparing key.data.as_slice() (from the test using IdentityPublicKeyV0 and key.id) to use a fixed array slice or slice literal instead of &vec![0xABu8; 33][..], e.g. a reference to a [u8;33] or a byte slice literal, so the comparison is performed against a stack-allocated slice and removes the heap allocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-dpp/src/identity/conversion/platform_value/mod.rs`:
- Around line 99-110: The comment above the tagged_raw_value() helper is
misleading—update it to state that this fixture does NOT exactly mirror
IdentityV0::to_object(): it intentionally encodes nested `data` as a base64
string (because nested deserializer defaults to human-readable), so document
that difference and clarify that this shape is used for testing
try_from_platform_versioned rather than representing the raw to_object() bytes
output.
In `@packages/rs-drive-proof-verifier/src/proof.rs`:
- Around line 5809-5845: The test
broadcast_state_transition_empty_metadata_when_missing currently uses an invalid
state_transition (vec![0xFF;4]) which triggers Error::ProtocolError before
metadata is checked; replace that payload with a valid serialized state
transition (e.g. serialize a default/fixture StateTransition using the same
platform codec used in production) so
StateTransitionProofResult::maybe_from_proof proceeds to the metadata check and
the test can assert Error::EmptyResponseMetadata, or alternatively rename the
test to indicate it verifies deserialization failure; update the assertion to
expect Error::EmptyResponseMetadata when using the valid payload.
In `@packages/rs-drive-proof-verifier/src/proof/token_contract_info.rs`:
- Around line 256-259: The assertion currently accepts both
Error::EmptyResponseMetadata and Error::NoProofInResult which can hide a
regression in validation order; change the assert in token_contract_info.rs so
it only allows Error::EmptyResponseMetadata (remove Error::NoProofInResult from
the matches!), ensuring the test fails if metadata validation no longer
short-circuits before proof extraction.
In
`@packages/rs-drive-proof-verifier/src/proof/token_pre_programmed_distributions.rs`:
- Around line 323-358: The test limit_at_u16_max_does_not_error_on_conversion
currently relies on empty-proof Drive verification; instead call the
request-only conversion/validation path that checks metadata and request fields
(so the limit conversion is exercised) without performing proof/Drive
verification — e.g., invoke the conversion function on
GetTokenPreProgrammedDistributionsRequest directly (the same validation used by
TokenPreProgrammedDistributions conversion) rather than calling
TokenPreProgrammedDistributions::maybe_from_proof with response_with_proof and
UnreachableProvider; assert the conversion succeeds (or returns an error that is
not a RequestError containing "limit exceeds u16") for limit = u16::MAX and keep
references to GetTokenPreProgrammedDistributionsRequest,
TokenPreProgrammedDistributions, and the conversion function you locate (the
request-only validator/try_from/maybe_from_request) so the test no longer
depends on empty-proof Drive behavior.
In `@packages/rs-drive-proof-verifier/src/verify.rs`:
- Around line 501-512: The test around verify_tenderdash_proof should assert the
InvalidPublicKey path only: within the test that calls
verify_tenderdash_proof(&proof, &metadata, &[0u8; 32], &provider) (and captures
result.expect_err(...)), replace the broad assertion that accepts signature
errors with a strict check that the error corresponds to the public-key
validation failure (the PublicKey::try_from([0u8; 48]) failure). Concretely,
assert that err.to_string() (or the error variant) contains "invalid public key"
/ matches the InvalidPublicKey variant and remove the alternative branches
("Could not verify signature digest" and "empty signature") since the zero key
should trigger the PublicKey::try_from failure and the test uses a non-empty
signature.
In
`@packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_emergency_action_transition_action/v0/transformer.rs`:
- Around line 408-419: The test emergency_borrowed_copies_action_via_star_deref
uses the clippy-triggering pattern `*&pause`/`*&resume` which trips
clippy::deref_addrof; replace each `let copied = *&pause;` and `let copied =
*&resume;` with an intermediate reference binding (e.g. `let r = &pause; let
copied: TokenEmergencyAction = *r;`) or remove those redundant assertions
entirely since they only re-check TokenEmergencyAction: Copy; update the test
body in transformer.rs accordingly to avoid the deref_addrof lint while keeping
the same assertions.
In
`@packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_freeze_transition_action/v0/transformer.rs`:
- Around line 423-429: The test
freeze_borrowed_path_dereferences_identity_for_new_action_v0 contains a needless
addrof deref pattern `let copied = *&id;` which triggers clippy::deref_addrof;
fix it by replacing that line with a direct copy `let copied = id;` (or remove
the trivial test entirely if it adds no value), referencing the Identifier type
and the test function name so you locate the code path to change.
In
`@packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_unfreeze_transition_action/v0/transformer.rs`:
- Around line 404-410: The test
unfreeze_borrowed_path_dereferences_frozen_identity_id uses the `*&id` pattern
which triggers clippy::deref_addrof; update the test (or remove it) so it avoids
`*&`—for example, in the test function
unfreeze_borrowed_path_dereferences_frozen_identity_id bind a reference first
(e.g., `let r = &id; let copied: Identifier = *r;`) instead of `let copied =
*&id;`) or delete the test entirely since it only asserts Identifier: Copy and
does not exercise the transformer; ensure references to Identifier, copied and
the mirrored `frozen_identity_id` pattern are adjusted accordingly so the module
is clippy-clean.
---
Nitpick comments:
In `@packages/rs-dpp/src/identity/identity_public_key/v0/conversion/json.rs`:
- Around line 128-130: The assertion unnecessarily allocates a Vec for the
expected bytes; change the RHS of the assertion comparing key.data.as_slice()
(from the test using IdentityPublicKeyV0 and key.id) to use a fixed array slice
or slice literal instead of &vec![0xABu8; 33][..], e.g. a reference to a [u8;33]
or a byte slice literal, so the comparison is performed against a
stack-allocated slice and removes the heap allocation.
In `@packages/rs-drive-proof-verifier/src/from_request.rs`:
- Around line 1082-1093: The section header comment "bincode_encode_values:
error path" is misleading for the test that exercises bincode_decode_values;
update the comment to accurately reflect that this block tests
bincode_decode_values error handling (e.g., change it to "bincode_decode_values:
error path" or similar) so readers can locate tests for bincode_decode_values
and avoid confusion when inspecting test_bincode_decode_mixed_valid_and_invalid.
In
`@packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_config_update_transition_action/v0/transformer.rs`:
- Around line 237-425: The test module contains many small unit tests that only
verify standard library behavior (Option::unwrap_or) and derived Clone of
TokenConfigurationChangeItem (tests like config_update_change_note_* and the
per-variant clone round-trips), which bloats maintenance; consolidate these into
one or two parameterized tests that cover the change_note merging rule and Clone
round-trip, or remove them and add a focused integration test that exercises
try_from_borrowed_token_config_update_transition_with_contract_lookup end-to-end
to validate the transformer behavior; update the tests in the tests mod
(referencing functions/configs named config_update_change_note_*,
config_update_borrowed_note_clone_round_trip, and the
TokenConfigurationChangeItem clone tests) to either combine cases into a
table-driven test or replace them with an integration test that constructs
realistic inputs and asserts the transformer's outputs.
In
`@packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/mod.rs`:
- Around line 187-190: The tests v0_public_note_returns_ref_when_set (and the
similar test around lines 207-210) call make_v0 and compare v0.public_note() to
Some(&"burning".to_string()), causing temporary String allocations; change the
assertions to compare as &str by mapping the accessor result to an &str (e.g.,
assert_eq!(v0.public_note().map(|s| s.as_str()), Some("burning"))) so no
temporary String is created; locate uses of make_v0 and the public_note()
accessor in those test functions and update both assertions accordingly.
In
`@packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/transformer.rs`:
- Around line 412-421: The test fixture creates `base` as a
TokenBaseTransition::V0 and then constructs `TokenDestroyFrozenFundsTransitionV0
{ base: base.clone(), ... }` which needlessly clones `base`; change the
constructor to move `base` instead of calling `clone()` (i.e., pass `base`
directly) and remove the unused clone, updating any variable ownership as needed
so `TokenBaseTransitionV0`, `TokenBaseTransition::V0`, and
`TokenDestroyFrozenFundsTransitionV0` use the moved value.
- Around line 332-376: The tests are exercising Option::unwrap_or directly
instead of the transformer's real logic; extract a small private helper (e.g.
resolve_action_note(change_note: Option<Option<String>>, public_note:
Option<String>) -> Option<String>) and/or an error-construction helper used by
the transformer in
token_destroy_frozen_funds_transition_action::v0::transformer, call that helper
from the transform function, and update the unit tests
(change_note_some_some_takes_precedence_over_public_note,
change_note_some_none_clears_user_note, change_note_none_preserves_user_note,
both_none_yields_none, borrowed_branch_clones_public_note and the other
referenced tests) to invoke the helper or the real transformer code path instead
of calling unwrap_or directly so the tests fail if the production merge/error
logic regresses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb53e2e2-5ff4-4085-9c8c-b21f86d219b9
📒 Files selected for processing (32)
packages/rs-dpp/src/identity/conversion/platform_value/mod.rspackages/rs-dpp/src/identity/identity.rspackages/rs-dpp/src/identity/identity_facade.rspackages/rs-dpp/src/identity/identity_public_key/conversion/json/mod.rspackages/rs-dpp/src/identity/identity_public_key/conversion/platform_value/mod.rspackages/rs-dpp/src/identity/identity_public_key/v0/conversion/json.rspackages/rs-dpp/src/identity/identity_public_key/v0/conversion/platform_value.rspackages/rs-dpp/src/identity/v0/conversion/json.rspackages/rs-dpp/src/identity/v0/conversion/platform_value.rspackages/rs-drive-proof-verifier/src/from_request.rspackages/rs-drive-proof-verifier/src/proof.rspackages/rs-drive-proof-verifier/src/proof/groups.rspackages/rs-drive-proof-verifier/src/proof/identity_token_balance.rspackages/rs-drive-proof-verifier/src/proof/token_contract_info.rspackages/rs-drive-proof-verifier/src/proof/token_info.rspackages/rs-drive-proof-verifier/src/proof/token_perpetual_distribution_last_claim.rspackages/rs-drive-proof-verifier/src/proof/token_pre_programmed_distributions.rspackages/rs-drive-proof-verifier/src/verify.rspackages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_config_update_transition_action/v0/mod.rspackages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_config_update_transition_action/v0/transformer.rspackages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/mod.rspackages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_destroy_frozen_funds_transition_action/v0/transformer.rspackages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_direct_purchase_transition_action/mod.rspackages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_direct_purchase_transition_action/v0/mod.rspackages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_emergency_action_transition_action/mod.rspackages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_emergency_action_transition_action/v0/transformer.rspackages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_freeze_transition_action/mod.rspackages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_freeze_transition_action/v0/transformer.rspackages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_set_price_for_direct_purchase_transition_action/mod.rspackages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_set_price_for_direct_purchase_transition_action/v0/mod.rspackages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_unfreeze_transition_action/mod.rspackages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_unfreeze_transition_action/v0/transformer.rs
Comment on lines
+5809
to
+5845
| #[test] | ||
| fn broadcast_state_transition_empty_metadata_when_missing() { | ||
| // Deserialization of empty state_transition fails first on real data, | ||
| // but in our case we want to ensure the proof branch fires correctly | ||
| // when metadata is missing. | ||
| use platform::wait_for_state_transition_result_response::{ | ||
| wait_for_state_transition_result_response_v0::Result as V0Result, Version, | ||
| WaitForStateTransitionResultResponseV0, | ||
| }; | ||
| // Must use a payload that successfully deserializes - but without | ||
| // a valid one, we instead hit ProtocolError. We accept either | ||
| // ProtocolError (deserialize fail) or EmptyResponseMetadata | ||
| // depending on ordering. Use clearly-invalid payload so decode | ||
| // explicitly errors first and assert ProtocolError. | ||
| let request = platform::BroadcastStateTransitionRequest { | ||
| state_transition: vec![0xFFu8; 4], | ||
| }; | ||
| let response = platform::WaitForStateTransitionResultResponse { | ||
| version: Some(Version::V0(WaitForStateTransitionResultResponseV0 { | ||
| result: Some(V0Result::Proof(Proof::default())), | ||
| metadata: None, // missing | ||
| })), | ||
| }; | ||
| let provider = unreachable_provider(); | ||
| let err = <StateTransitionProofResult as FromProof< | ||
| platform::BroadcastStateTransitionRequest, | ||
| >>::maybe_from_proof( | ||
| request, | ||
| response, | ||
| Network::Testnet, | ||
| default_platform_version(), | ||
| &provider, | ||
| ) | ||
| .unwrap_err(); | ||
| // Order: proof extracted -> state_transition decoded -> metadata | ||
| // checked. ProtocolError triggers on the decode. | ||
| assert!(matches!(err, Error::ProtocolError { .. }), "got: {err:?}"); |
Contributor
There was a problem hiding this comment.
Make this test actually cover missing metadata.
This test is named for the missing-metadata path, but the invalid state_transition payload triggers Error::ProtocolError before Line 1437 reads metadata, so it does not exercise EmptyResponseMetadata. Please use a valid serialized state transition fixture here, or rename the test to reflect that invalid payloads fail before metadata validation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rs-drive-proof-verifier/src/proof.rs` around lines 5809 - 5845, The
test broadcast_state_transition_empty_metadata_when_missing currently uses an
invalid state_transition (vec![0xFF;4]) which triggers Error::ProtocolError
before metadata is checked; replace that payload with a valid serialized state
transition (e.g. serialize a default/fixture StateTransition using the same
platform codec used in production) so
StateTransitionProofResult::maybe_from_proof proceeds to the metadata check and
the test can assert Error::EmptyResponseMetadata, or alternatively rename the
test to indicate it verifies deserialization failure; update the assertion to
expect Error::EmptyResponseMetadata when using the valid payload.
Comment on lines
+423
to
+429
| #[test] | ||
| fn freeze_borrowed_path_dereferences_identity_for_new_action_v0() { | ||
| let id = Identifier::new([0xAB; 32]); | ||
| // Mirror the `identity_to_freeze_id: *identity_to_freeze_id` pattern. | ||
| let copied = *&id; | ||
| assert_eq!(copied, id); | ||
| } |
Contributor
There was a problem hiding this comment.
Clippy deref_addrof will fail the lint gate.
let copied = *&id; triggers clippy::deref_addrof (needless & then *). Since the workspace enforces clippy-clean builds and macOS CI is already reported failing on this PR, this likely contributes. Also, this test doesn't really exercise the transformer — it's just asserting Copy on Identifier. Consider either removing it or rewriting without the needless addrof:
Proposed fix
- #[test]
- fn freeze_borrowed_path_dereferences_identity_for_new_action_v0() {
- let id = Identifier::new([0xAB; 32]);
- // Mirror the `identity_to_freeze_id: *identity_to_freeze_id` pattern.
- let copied = *&id;
- assert_eq!(copied, id);
- }
+ #[test]
+ fn freeze_borrowed_path_dereferences_identity_for_new_action_v0() {
+ let id = Identifier::new([0xAB; 32]);
+ // Mirror the `identity_to_freeze_id: *identity_to_freeze_id` pattern
+ // (relies on `Identifier: Copy`).
+ let r = &id;
+ let copied: Identifier = *r;
+ assert_eq!(copied, id);
+ }As per coding guidelines: "Follow rustfmt defaults and keep code clippy-clean for Rust modules".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[test] | |
| fn freeze_borrowed_path_dereferences_identity_for_new_action_v0() { | |
| let id = Identifier::new([0xAB; 32]); | |
| // Mirror the `identity_to_freeze_id: *identity_to_freeze_id` pattern. | |
| let copied = *&id; | |
| assert_eq!(copied, id); | |
| } | |
| #[test] | |
| fn freeze_borrowed_path_dereferences_identity_for_new_action_v0() { | |
| let id = Identifier::new([0xAB; 32]); | |
| // Mirror the `identity_to_freeze_id: *identity_to_freeze_id` pattern | |
| // (relies on `Identifier: Copy`). | |
| let r = &id; | |
| let copied: Identifier = *r; | |
| assert_eq!(copied, id); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_freeze_transition_action/v0/transformer.rs`
around lines 423 - 429, The test
freeze_borrowed_path_dereferences_identity_for_new_action_v0 contains a needless
addrof deref pattern `let copied = *&id;` which triggers clippy::deref_addrof;
fix it by replacing that line with a direct copy `let copied = id;` (or remove
the trivial test entirely if it adds no value), referencing the Identifier type
and the test function name so you locate the code path to change.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3526 +/- ##
============================================
+ Coverage 87.84% 88.00% +0.16%
============================================
Files 2474 2474
Lines 292800 294440 +1640
============================================
+ Hits 257196 259132 +1936
+ Misses 35604 35308 -296
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Improve code coverage for the three highest-leverage targets remaining after PR #3525:
rs-drive-proof-verifier(45% → higher, was biggest single-file gap)rs-drive/src/state_transition_action/batch/batched_transition/token_transition/(60-75% cov modules)rs-dpp/src/identity/zero-coverage conversion modulesWhat was done
Added 330 new unit tests across ~30 files. All tests are
#[cfg(test)]additions — no production code was modified.rs-drive-proof-verifier/src/proof.rs— 78 teststry_u32_to_u16,u32_to_u16_optboundary behaviorLength/IntoOptiontrait impl edge cases (None, empty, only-None BTreeMap, absence markers)parse_key_request_typenegative/empty/missing branchesFromProofwrapper methods (from_proof_with_metadata,maybe_from_proof) success + NotFound mappingrs-drive-proof-verifier/src/proof/*.rs+from_request.rs+verify.rs— 57 teststoken_contract_info.rs(6),token_perpetual_distribution_last_claim.rs(8),token_pre_programmed_distributions.rs(7),groups.rs(13),identity_token_balance.rs(6),token_info.rs(6)from_request.rs(21) — offset rejection for contested document queries, missing-version, bad identifiers, invalid bincode start_value, populated roundtripverify.rs(2) —ContextProvider::get_quorum_public_keyerror propagation, invalid 48-byte pubkeyrs-drive/src/state_transition_action/batch/batched_transition/token_transition/— 129 testsSeven token action modules (destroy_frozen_funds, set_price_for_direct_purchase, emergency_action, unfreeze, freeze, config_update, direct_purchase), covering:
mod.rschange_note.unwrap_or(public_note)precedence, owned vs borrowed cloning,TokenConfigurationChangeItemvariant clone roundtrips,TokenPricingSchedulevariant cloning, destructure patternsDoes NOT cover full
try_from_*transformer bodies — those require a liveDrive/GroveDB, matching the siblingtoken_mint/token_transferpattern.rs-dpp/src/identity/— 66 testsidentity.rs(10) —default_versioned,new_with_id_and_keys,into_partial_identity_info, hash stability, deterministic ID, empty-inputs erroridentity_facade.rs(13) —new,create,create_chain_asset_lock_proof,create_instant_lock_proof,create_from_buffer, 6 state-transition helpersconversion/platform_value/mod.rs(6) —TryFromPlatformVersioned<Value>happy + garbage,to_cleaned_objectv0/conversion/json.rs(6) +v0/conversion/platform_value.rs(5) — including 2frozen: V0 consensus behaviorlock-ins documenting pre-existing serialization asymmetriesidentity_public_key/conversion/{json,platform_value}/mod.rs(4+5) — delegation to V0, roundtrip, missing-fields erroridentity_public_key/v0/conversion/{json,platform_value}.rs(8+9) — roundtrips, non-map error, contract-bounds roundtrip, canonical JSON, bad base64Frozen V0 behaviors documented (not fixed)
Per project convention, pre-existing V0 serialization asymmetries are locked in with
// frozen: V0 consensus behaviorrather than modified:IdentityV0::to_object→TryFrom<Value>roundtrip fails becauseplatform_value::to_valueemitsBinaryDataasValue::Byteswhile inner deserializers default to human-readable and expect base64 strings.IdentityV0::from_jsoncannot ingest the canonical legacy fixture because it lacks the$formatVersionadjacency tag required by the serde-taggedIdentityPublicKeyenum.How Has This Been Tested?
Breaking Changes
None — tests only. No production code modified.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit