Contract: Reject zero WASM hash on upgrade#318
Conversation
|
@midenotch Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughThis PR removes the unused Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Done, Close: #318 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
contracts/account/test_snapshots/test/test_add_session_key.1.json (1)
467-467: Unrelated snapshot modified (trailing newline removed).This test snapshot is for
test_add_session_key, which is unrelated to the upgrade validation feature being added in this PR. The only change is the removal of the trailing newline after the final}.If this was auto-generated by running the test suite, that's expected behavior. However, consider whether including unrelated snapshot changes in this PR adds noise to the review.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/account/test_snapshots/test/test_add_session_key.1.json` at line 467, The snapshot for test_add_session_key was modified only by removal of the trailing newline, which is unrelated to the upgrade validation changes; revert that snapshot change or restore the original trailing newline in the test_add_session_key snapshot (the snapshot named test_add_session_key in the test_snapshots bundle) so the PR only contains relevant changes, or move this cosmetic snapshot change into a separate commit/PR to avoid noisy unrelated diffs.contracts/account/test_snapshots/test/test_execute_session_key_permissions.1.json (1)
18-18: Snapshot values are non-deterministic across runs.Because
test_execute_session_key_permissionsgenerates the session key viaSigningKey::generate(&mut OsRng), both the public-key bytes and the derived signature payload change on every test run. This snapshot will need to be re-committed whenever the test is re-recorded, which defeats the purpose of a golden fixture. Consider seeding the RNG (e.g.,ChaCha20Rng::from_seed([...])) or using a fixed signing key for the test so the snapshot remains stable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/account/test_snapshots/test/test_execute_session_key_permissions.1.json` at line 18, The snapshot is unstable because test_execute_session_key_permissions calls SigningKey::generate(&mut OsRng), producing non-deterministic bytes; replace the use of OsRng with a deterministic RNG (e.g., create a ChaCha20Rng::from_seed([...]) and pass that mutable RNG into SigningKey::generate) or construct a fixed SigningKey from a known byte seed (e.g., SigningKey::from_bytes with a constant array) so the public-key bytes and signature payload are constant; after making the change, re-record the snapshot to the stable expected value.contracts/account/src/lib.rs (1)
295-297: Optional: avoid host-object allocation for the zero-hash comparison.
BytesN::from_array(&env, &[0u8; 32])allocates a new Soroban host object on everyupgradecall just to compare. You can compare against the raw array directly, which is cheaper and slightly clearer:♻️ Proposed refactor
- if new_wasm_hash == BytesN::from_array(&env, &[0u8; 32]) { - return Err(ContractError::InvalidWasmHash); - } + if new_wasm_hash.to_array() == [0u8; 32] { + return Err(ContractError::InvalidWasmHash); + }Note that this check is a narrow sanity guard — it only rejects the all-zero sentinel and does not verify the hash corresponds to an installed/valid WASM, which is in line with issue
#207's scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/account/src/lib.rs` around lines 295 - 297, The check in the upgrade flow creates a host BytesN via BytesN::from_array(&env, &[0u8; 32]) just to test for the zero-hash; replace that allocation by comparing new_wasm_hash against the raw zero array directly (e.g., read new_wasm_hash's raw bytes/array via its existing accessor and compare to [0u8; 32]) and return Err(ContractError::InvalidWasmHash) when equal; locate this logic in the upgrade function where new_wasm_hash and ContractError::InvalidWasmHash are used and change the comparison to avoid allocating a host object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@contracts/account/test_snapshots/test/test_execute_cross_contract_invocation.1.json`:
- Line 18: The snapshot is non-deterministic because the test constructs a
random SigningKey using OsRng; replace that with a deterministic test key or RNG
seed so signatures are stable (e.g., construct SigningKey from fixed seed/bytes
or use a SeedableRng like StdRng::seed_from_u64 to derive the key) in the places
that create the key (references: OsRng, SigningKey) so the produced signature
bytes in the snapshot remain identical across runs.
In
`@contracts/account/test_snapshots/test/test_execute_session_key_expired.1.json`:
- Line 18: The tests currently generate SigningKey and signatures with OsRng
(see SigningKey creation around contracts/account/src/lib.rs lines ~903-904),
producing nondeterministic public-key/signature bytes that break snapshots;
change the key generation to a deterministic source—either load a fixed fixture
key (hardcoded byte array or test-only constant) or instantiate a seeded RNG
(e.g., StdRng::from_seed) and use that to create the SigningKey/signatures used
by the tests (including the places called out in the comment). Update the code
paths that call OsRng (the SigningKey construction and any signature generation
functions) to use the deterministic seed or fixture so snapshots remain stable.
---
Nitpick comments:
In `@contracts/account/src/lib.rs`:
- Around line 295-297: The check in the upgrade flow creates a host BytesN via
BytesN::from_array(&env, &[0u8; 32]) just to test for the zero-hash; replace
that allocation by comparing new_wasm_hash against the raw zero array directly
(e.g., read new_wasm_hash's raw bytes/array via its existing accessor and
compare to [0u8; 32]) and return Err(ContractError::InvalidWasmHash) when equal;
locate this logic in the upgrade function where new_wasm_hash and
ContractError::InvalidWasmHash are used and change the comparison to avoid
allocating a host object.
In `@contracts/account/test_snapshots/test/test_add_session_key.1.json`:
- Line 467: The snapshot for test_add_session_key was modified only by removal
of the trailing newline, which is unrelated to the upgrade validation changes;
revert that snapshot change or restore the original trailing newline in the
test_add_session_key snapshot (the snapshot named test_add_session_key in the
test_snapshots bundle) so the PR only contains relevant changes, or move this
cosmetic snapshot change into a separate commit/PR to avoid noisy unrelated
diffs.
In
`@contracts/account/test_snapshots/test/test_execute_session_key_permissions.1.json`:
- Line 18: The snapshot is unstable because test_execute_session_key_permissions
calls SigningKey::generate(&mut OsRng), producing non-deterministic bytes;
replace the use of OsRng with a deterministic RNG (e.g., create a
ChaCha20Rng::from_seed([...]) and pass that mutable RNG into
SigningKey::generate) or construct a fixed SigningKey from a known byte seed
(e.g., SigningKey::from_bytes with a constant array) so the public-key bytes and
signature payload are constant; after making the change, re-record the snapshot
to the stable expected value.
🪄 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: 3831d731-3dfd-440d-b462-d5afe4df6ad2
📒 Files selected for processing (28)
contracts/account/src/lib.rscontracts/account/test_snapshots/test/test_add_session_key.1.jsoncontracts/account/test_snapshots/test/test_add_session_key_emits_event.1.jsoncontracts/account/test_snapshots/test/test_double_initialize.1.jsoncontracts/account/test_snapshots/test/test_execute_cross_contract_invocation.1.jsoncontracts/account/test_snapshots/test/test_execute_emits_event.1.jsoncontracts/account/test_snapshots/test/test_execute_event_nonce_is_pre_increment.1.jsoncontracts/account/test_snapshots/test/test_execute_failed_no_event.1.jsoncontracts/account/test_snapshots/test/test_execute_failed_no_nonce_increment.1.jsoncontracts/account/test_snapshots/test/test_execute_owner_path_unaffected_by_session_signature_requirements.1.jsoncontracts/account/test_snapshots/test/test_execute_rejects_duplicate_nonce.1.jsoncontracts/account/test_snapshots/test/test_execute_rejects_invalid_nonce.1.jsoncontracts/account/test_snapshots/test/test_execute_session_key_expired.1.jsoncontracts/account/test_snapshots/test/test_execute_session_key_missing_signature_rejected.1.jsoncontracts/account/test_snapshots/test/test_execute_session_key_permissions.1.jsoncontracts/account/test_snapshots/test/test_execute_validates_nonce_then_increments.1.jsoncontracts/account/test_snapshots/test/test_get_owner_before_initialize_returns_not_initialized.1.jsoncontracts/account/test_snapshots/test/test_get_version_defaults_to_zero_before_initialize_for_compatibility.1.jsoncontracts/account/test_snapshots/test/test_has_session_key_absent.1.jsoncontracts/account/test_snapshots/test/test_has_session_key_after_revoke.1.jsoncontracts/account/test_snapshots/test/test_has_session_key_present.1.jsoncontracts/account/test_snapshots/test/test_initialize.1.jsoncontracts/account/test_snapshots/test/test_initialize_emits_event.1.jsoncontracts/account/test_snapshots/test/test_refresh_session_key_ttl.1.jsoncontracts/account/test_snapshots/test/test_refresh_session_key_ttl_unknown_key_returns_session_key_not_found.1.jsoncontracts/account/test_snapshots/test/test_revoke_session_key_emits_event.1.jsoncontracts/account/test_snapshots/test/test_revoke_session_key_removes_session_key_storage_entry.1.jsoncontracts/account/test_snapshots/test/test_upgrade_rejects_zero_hash.1.json
| "args": [ | ||
| { | ||
| "bytes": "3413fa06eaa2fa6544f36ada757b647fbdc9836ba342075c64f17b0c6f2e7e34" | ||
| "bytes": "12cf8f5e2e4cc64e16de978473dc0449666a3a0c6080c6409676c6b707913a5b" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate the test and confirm whether snapshotted session key/signature bytes are generated nondeterministically.
# Expect: test_execute_cross_contract_invocation should not use OsRng/SigningKey::generate when its outputs are snapshotted.
rg -n -C6 --type rust 'fn test_execute_cross_contract_invocation|OsRng|SigningKey::generate\(&mut csprng\)|sign_payload'Repository: ancore-org/ancore
Length of output: 9452
🏁 Script executed:
head -50 contracts/account/test_snapshots/test/test_execute_cross_contract_invocation.1.jsonRepository: ancore-org/ancore
Length of output: 1257
🏁 Script executed:
# Check if there's any seed setting or deterministic test setup in the test module
rg -n -B5 -A10 'fn test_execute_cross_contract_invocation\(\)' contracts/account/src/lib.rsRepository: ancore-org/ancore
Length of output: 599
🏁 Script executed:
# Search for any test setup or seed-related code
rg -n 'seed|Seed|mock_rng|MockRng|deterministic' contracts/account/src/lib.rsRepository: ancore-org/ancore
Length of output: 43
🏁 Script executed:
# Check if there's a snapshot testing library or framework that might handle seeding
rg -n 'insta|snapshot|assert_snapshot' contracts/account/src/lib.rsRepository: ancore-org/ancore
Length of output: 905
🏁 Script executed:
# Look for Cargo.toml to see test dependencies
cat contracts/account/Cargo.toml | grep -A30 '\[dev-dependencies\]'Repository: ancore-org/ancore
Length of output: 337
🏁 Script executed:
# Search for any environment variable or configuration that controls randomness in tests
rg -n 'SOROBAN|TEST|SEED|RNG' contracts/account/src/lib.rsRepository: ancore-org/ancore
Length of output: 43
🏁 Script executed:
# Check if soroban Env::default() or other soroban SDK functions handle deterministic seeding
rg -n 'Env::default|test_utils|deterministic|seed' contracts/account/src/lib.rs | head -20Repository: ancore-org/ancore
Length of output: 820
🏁 Script executed:
# Look at the exact lines where signatures are created and used in the test
sed -n '853,890p' contracts/account/src/lib.rsRepository: ancore-org/ancore
Length of output: 1469
🏁 Script executed:
# Check if there are any recent changes to this test or related configuration
git log --oneline -20 -- contracts/account/src/lib.rs 2>/dev/null || echo "Git history not available"Repository: ancore-org/ancore
Length of output: 111
🏁 Script executed:
# Verify the sign_payload function to confirm it uses the signing_key for signature generation
sed -n '413,450p' contracts/account/src/lib.rsRepository: ancore-org/ancore
Length of output: 1315
Make this snapshot deterministic by using a fixed signing key in the test.
The test uses OsRng to generate a random SigningKey at lines 862–863, which produces different signatures on every run. Since the resulting signature bytes are snapshotted in the JSON file, the test will fail or churn every time the signature changes. Replace OsRng with a fixed seed or deterministic test signing key to ensure reproducible snapshots.
Also applies to: 59-59, 79-79, 111-111, 369-369, 403-403, 462-482
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@contracts/account/test_snapshots/test/test_execute_cross_contract_invocation.1.json`
at line 18, The snapshot is non-deterministic because the test constructs a
random SigningKey using OsRng; replace that with a deterministic test key or RNG
seed so signatures are stable (e.g., construct SigningKey from fixed seed/bytes
or use a SeedableRng like StdRng::seed_from_u64 to derive the key) in the places
that create the key (references: OsRng, SigningKey) so the produced signature
bytes in the snapshot remain identical across runs.
| "args": [ | ||
| { | ||
| "bytes": "d71b38e738107a9e0bddc5ec86f87aa0d20fee5b48a2849cfebc949fc4d7ed0e" | ||
| "bytes": "47c83bd64ca940358db31773bfec8ac999ea3b07c2dfaec43cd6b08dc62c79c3" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm that the expired session-key snapshot is produced from nondeterministic key generation.
rg -n -C5 --type rust 'fn test_execute_session_key_expired|let mut csprng = OsRng|SigningKey::generate\(&mut csprng\)|fn sign_payload'Repository: ancore-org/ancore
Length of output: 4774
🏁 Script executed:
# Check the snapshot file to see its structure and confirm problematic bytes at mentioned lines
head -n 125 contracts/account/test_snapshots/test/test_execute_session_key_expired.1.json | tail -n +15Repository: ancore-org/ancore
Length of output: 3229
🏁 Script executed:
# Look for any test setup/mocking that might affect randomness
rg -n 'mock|seed|deterministic|Mock|Seed' contracts/account/src/lib.rs | head -20Repository: ancore-org/ancore
Length of output: 745
🏁 Script executed:
# Check for snapshot framework configuration or helpers
fd -t f '\.(toml|yaml|json)$' contracts/ | xargs grep -l 'snapshot' 2>/dev/null | head -5Repository: ancore-org/ancore
Length of output: 43
🏁 Script executed:
sed -n '892,930p' contracts/account/src/lib.rsRepository: ancore-org/ancore
Length of output: 1474
🏁 Script executed:
# Check for any test harness setup files
fd -t f 'lib\.rs|main\.rs|mod\.rs' contracts/account/tests/ contracts/account/benches/Repository: ancore-org/ancore
Length of output: 245
Use a fixed fixture key or seeded deterministic RNG instead of OsRng.
These public-key and signature bytes come from an OsRng-generated SigningKey at contracts/account/src/lib.rs:903-904. Since OsRng provides OS entropy without seeding, each test run produces a different key and signature. The snapshot captures one specific random outcome, causing recurring churn or CI failures on subsequent runs.
Also applies to: lines 62-62, 82-82, 117-117, 375-375, 412-412, 471-471, 488-491, 586-586, 603-606
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@contracts/account/test_snapshots/test/test_execute_session_key_expired.1.json`
at line 18, The tests currently generate SigningKey and signatures with OsRng
(see SigningKey creation around contracts/account/src/lib.rs lines ~903-904),
producing nondeterministic public-key/signature bytes that break snapshots;
change the key generation to a deterministic source—either load a fixed fixture
key (hardcoded byte array or test-only constant) or instantiate a seeded RNG
(e.g., StdRng::from_seed) and use that to create the SigningKey/signatures used
by the tests (including the places called out in the comment). Update the code
paths that call OsRng (the SigningKey construction and any signature generation
functions) to use the deterministic seed or fixture so snapshots remain stable.
|
@midenotch resolve conflcits |
Done |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contracts/account/src/lib.rs (2)
1211-1224:⚠️ Potential issue | 🔴 CriticalCritical: Test expects wrong panic code (
#10) — will fail givenInvalidWasmHash = 9.The
#[should_panic]attribute expectsError(Contract,#10), butInvalidWasmHashis discriminant9(see line 49), and the inline comment on line 1222 also says "#9 (InvalidWasmHash)". The attribute and the comment disagree; the attribute is the bug.This also directly contradicts the PR claim of "27 passed; 0 failed".
🛠 Proposed fix
#[test] - #[should_panic(expected = "Error(Contract, `#10`)")] - fn test_upgrade_zero_hash_panics_with_code_10() { + #[should_panic(expected = "Error(Contract, `#9`)")] + fn test_upgrade_zero_hash_panics_with_code_9() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/account/src/lib.rs` around lines 1211 - 1224, The test test_upgrade_zero_hash_panics_with_code_10 is asserting the wrong panic code; change the #[should_panic(expected = "Error(Contract, `#10`)")] to expect "Error(Contract, `#9`)" so it matches the InvalidWasmHash discriminant (InvalidWasmHash = 9) and the inline comment, ensuring client.upgrade(&BytesN::from_array(&env, &[0u8; 32])) triggers the expected contract error code.
31-50:⚠️ Potential issue | 🔴 CriticalCritical: Enum is missing
InvalidSignatureandInvalidExpirationvariants that are still referenced — the crate will not compile.Per the AI summary,
InvalidSignaturewas removed fromContractError. However, call sites still reference it:
- Line 210:
signature.ok_or(ContractError::InvalidSignature)?- Line 211:
signature_payload.ok_or(ContractError::InvalidSignature)?- Line 1135:
matches!(result, Err(Ok(ContractError::InvalidSignature)))- Line 1174:
matches!(result, Err(Ok(ContractError::InvalidSignature)))Additionally, line 250 returns
ContractError::InvalidExpiration, which is also not defined in this enum.Either re-add the variants or update all call sites. Re-adding is the lowest-risk change and preserves existing semantics; renumbering also invalidates the snapshot file
test_upgrade_rejects_zero_hash.1.jsonthat referencescontract: 9.The PR description claims "27 passed; 0 failed", which is inconsistent with the enum as shown — please double-check whether the tree being tested matches what is being merged.
🛠 Suggested fix (re-add the missing variants, preserving discriminant 9 for InvalidWasmHash)
/// Invalid version provided for migration InvalidVersion = 8, /// WASM hash cannot be zero InvalidWasmHash = 9, + /// Session key expiration time is invalid (e.g. zero) + InvalidExpiration = 10, + /// Signature or signature payload is missing or invalid + InvalidSignature = 11, }If the intent was truly to remove
InvalidSignature, lines 210–211 must be replaced with a different error (e.g.,InsufficientPermissionor a new variant) and tests at lines 1135/1174 updated accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/account/src/lib.rs` around lines 31 - 50, The ContractError enum is missing the InvalidSignature and InvalidExpiration variants but call sites still reference ContractError::InvalidSignature and a return of ContractError::InvalidExpiration; re-add these two variants to the ContractError enum (keep existing discriminants so InvalidWasmHash remains = 9) to restore compile-time references, or if you prefer to change call sites instead, update all occurrences of signature.ok_or(ContractError::InvalidSignature), signature_payload.ok_or(ContractError::InvalidSignature) and the matches! checks that expect InvalidSignature to use the new/alternate error variant, and replace the return of ContractError::InvalidExpiration with the chosen existing/new variant; ensure tests and snapshot expectations are updated if discriminants change.
🧹 Nitpick comments (1)
contracts/account/src/lib.rs (1)
1178-1306: Three tests assert the same behavior — consolidate to one.
test_upgrade_rejects_zero_wasm_hash(1178–1191),test_upgrade_rejects_zero_wasm_hash_error_code(1193–1209), and the newly addedtest_upgrade_rejects_zero_hash(1293–1306) all construct an all-zeroBytesN<32>, calltry_upgrade, and assertErr(Ok(ContractError::InvalidWasmHash)). The_error_codevariant adds no new coverage beyond unwrapping the same result.Recommend keeping only the focused test introduced by this PR (
test_upgrade_rejects_zero_hash) — matching the AI summary's stated intent — plus the two genuinely distinct regression checks (test_upgrade_version_not_incremented_on_invalid_hash,test_upgrade_owner_auth_required_even_for_invalid_hash) which cover useful additional properties.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/account/src/lib.rs` around lines 1178 - 1306, Remove the duplicate tests that assert the same behavior and keep the focused one introduced by this PR: delete test_upgrade_rejects_zero_wasm_hash and test_upgrade_rejects_zero_wasm_hash_error_code, leaving test_upgrade_rejects_zero_hash intact; ensure the remaining test still constructs the zero hash with BytesN::from_array, calls AncoreAccountClient::try_upgrade, and asserts Err(Ok(ContractError::InvalidWasmHash)); keep the distinct regression tests test_upgrade_version_not_incremented_on_invalid_hash and test_upgrade_owner_auth_required_even_for_invalid_hash as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@contracts/account/src/lib.rs`:
- Around line 1211-1224: The test test_upgrade_zero_hash_panics_with_code_10 is
asserting the wrong panic code; change the #[should_panic(expected =
"Error(Contract, `#10`)")] to expect "Error(Contract, `#9`)" so it matches the
InvalidWasmHash discriminant (InvalidWasmHash = 9) and the inline comment,
ensuring client.upgrade(&BytesN::from_array(&env, &[0u8; 32])) triggers the
expected contract error code.
- Around line 31-50: The ContractError enum is missing the InvalidSignature and
InvalidExpiration variants but call sites still reference
ContractError::InvalidSignature and a return of
ContractError::InvalidExpiration; re-add these two variants to the ContractError
enum (keep existing discriminants so InvalidWasmHash remains = 9) to restore
compile-time references, or if you prefer to change call sites instead, update
all occurrences of signature.ok_or(ContractError::InvalidSignature),
signature_payload.ok_or(ContractError::InvalidSignature) and the matches! checks
that expect InvalidSignature to use the new/alternate error variant, and replace
the return of ContractError::InvalidExpiration with the chosen existing/new
variant; ensure tests and snapshot expectations are updated if discriminants
change.
---
Nitpick comments:
In `@contracts/account/src/lib.rs`:
- Around line 1178-1306: Remove the duplicate tests that assert the same
behavior and keep the focused one introduced by this PR: delete
test_upgrade_rejects_zero_wasm_hash and
test_upgrade_rejects_zero_wasm_hash_error_code, leaving
test_upgrade_rejects_zero_hash intact; ensure the remaining test still
constructs the zero hash with BytesN::from_array, calls
AncoreAccountClient::try_upgrade, and asserts
Err(Ok(ContractError::InvalidWasmHash)); keep the distinct regression tests
test_upgrade_version_not_incremented_on_invalid_hash and
test_upgrade_owner_auth_required_even_for_invalid_hash as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48088727-c6f3-456a-9609-5f6bf899ca2a
📒 Files selected for processing (2)
contracts/account/src/lib.rscontracts/account/test_snapshots/test/test_execute_session_key_missing_signature_rejected.1.json
✅ Files skipped from review due to trivial changes (1)
- contracts/account/test_snapshots/test/test_execute_session_key_missing_signature_rejected.1.json
|
@midenotch resolve conflicts |
3 participants
Close: #207
Description
I have implemented the validation logic to reject a zero WASM hash in the upgrade() function of the AncoreAccount contract.
Summary of Changes:
Added Error Variant: Defined InvalidWasmHash = 9 in the ContractError enum.
Validation Logic: Updated the upgrade() function in
lib.rs
to check if the new_wasm_hash is all zeros and return InvalidWasmHash if so.
Unit Testing: Added a new test test_upgrade_rejects_zero_hash to verify the rejection logic.
All 27 contract tests passed successfully
Type of Change
Security Impact
Testing
Test Coverage
test test::test_upgrade_rejects_zero_hash ... ok
test result: ok. 27 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.50s
Breaking Changes
Checklist
For High-Security Changes
Related Issues
Closes #207
Related to #
Additional Context
Reviewer Notes
Summary by CodeRabbit
Bug Fixes
Tests