Salted nonces #131
Salted nonces #131
Conversation
WalkthroughAdds versioned, salted, and expirable nonces with salt rotation and validation; verifies nonce salt and deadline before commit; replaces per-nonce cleanup with prefix-based cleanup; introduces SaltManager API/events and SaltRegistry state; migrates contract storage to a versioned form and updates tests and release to v0.3.0. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Client
participant Engine
participant State
participant Nonces
User->>Client: submit signed intent(nonce, intent_deadline)
Client->>Engine: execute_signed_intent(...)
Engine->>Engine: verify_intent_nonce(nonce, intent_deadline)
Engine->>Nonces: VersionedNonce::maybe_from(nonce)
alt V1 (Salted & Expirable)
Engine->>State: is_valid_salt(salt)?
Engine-->>Client: Err(InvalidSalt) if false
Engine->>Engine: check intent_deadline <= nonce.deadline
Engine-->>Client: Err(DeadlineGreaterThanNonce) if violated
Engine->>Engine: check nonce.deadline not expired
Engine-->>Client: Err(NonceExpired) if expired
else Legacy
note right of Engine: legacy nonce — no salt/deadline checks
end
Engine->>State: commit_nonce(nonce)
State->>Nonces: Nonces::commit(...)
Engine-->>Client: Ok
sequenceDiagram
autonumber
actor Admin as DAO/SaltManager
participant Contract
participant Registry as SaltRegistry
participant Events as DefuseEvent
Admin->>Contract: update_current_salt() (1 yocto)
Contract->>Registry: set_new()
Registry-->>Contract: new current salt
Contract->>Events: emit SaltRotationEvent{current, invalidated: {}}
Contract-->>Admin: current_salt
Admin->>Contract: invalidate_salts([s1,s2]) (1 yocto)
loop each salt
Contract->>Registry: is_valid(salt)?
alt valid
Contract->>Registry: invalidate(salt)
else invalid
Contract-->>Admin: Err(InvalidSalt)
end
end
Contract->>Events: emit SaltRotationEvent{current, invalidated}
Contract-->>Admin: current_salt
sequenceDiagram
autonumber
participant Caller
participant Contract
participant State
participant Accounts
Caller->>Contract: cleanup_nonces([(acct, [nonce,...]), ...])
loop per (acct, nonces)
Contract->>Contract: VersionedNonce::maybe_from(nonce)
alt V1 salted && (expired or invalid salt)
Contract->>State: cleanup_nonce_by_prefix(&acct, prefix)
State->>Accounts: account.cleanup_nonce_by_prefix(prefix)
Accounts-->>State: bool (cleared?)
else skip
note right of Contract: not cleanable
end
end
Contract-->>Caller: done
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)tests/src/tests/defuse/accounts/mod.rs (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
🔇 Additional comments (3)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (23)
defuse/src/contract/accounts/account/mod.rs (1)
153-154: Minor grammar issue in documentation.The documentation has a subject-verb agreement error: "whether the nonces was cleared" should be "whether the nonce was cleared" (singular).
Apply this diff to fix the grammar:
- /// Returns whether the nonces was cleared, + /// Returns whether the nonce was cleared,core/src/engine/state/cached.rs (1)
123-129: Forwarders look good; consider adding #[inline] for consistency.Matches the trait changes and mirrors deltas.rs. Add #[inline] as done for other small passthroughs.
- fn verify_intent_nonce(&self, nonce: Nonce, intent_deadline: Deadline) -> Result<()> { + #[inline] + fn verify_intent_nonce(&self, nonce: Nonce, intent_deadline: Deadline) -> Result<()> { self.view.verify_intent_nonce(nonce, intent_deadline) } - fn is_nonce_cleanable(&self, nonce: Nonce) -> Result<bool> { + #[inline] + fn is_nonce_cleanable(&self, nonce: Nonce) -> Result<bool> { self.view.is_nonce_cleanable(nonce) }defuse/src/contract/state/mod.rs (2)
14-14: Remove stale TODO; versioning now exists via ContractStateEntry.This comment is outdated; replace with a brief note pointing to entry::ContractStateEntry.
-// TODO: add versioning +// State versioning implemented via entry::ContractStateEntry (see module `entry`)
33-41: Use prefix.as_slice().nest for storage key construction and add a legacy‐state deserialization test
Storage keys now useprefix.as_slice().nestfor bothtotal_suppliesandsalts, matching other modules’ conventions. Add a unit test that deserializes a v0 state viaContractStateEntryto catch future layout regressions.tests/src/tests/defuse/state/extentions/mod.rs (1)
1-2: Module re-exports are fine; minor naming nit.If feasible later, consider renaming
extentions→extensionsrepo‑wide to avoid typos in paths/imports. Low priority.defuse/src/contract/accounts/mod.rs (1)
57-66: Don’t silently drop errors; add minimal observability and skip empty batches.Swallowing errors from is_nonce_cleanable and cleanup makes diagnosing issues hard. Log or count failures; also avoid calling cleanup with an empty list.
- // TODO: optimize that - let cleanable = nonces + // NOTE: collect to end the shared &self borrow before &mut self below. + let mut errors = 0usize; + let cleanable = nonces .into_iter() .map(AsBase64::into_inner) - .filter(|&n| self.is_nonce_cleanable(n).unwrap_or_default()) + .filter(|&n| { + match self.is_nonce_cleanable(n) { + Ok(v) => v, + Err(_) => { errors += 1; false } + } + }) .collect::<Vec<_>>(); - // NOTE: all errors are omitted - State::cleanup_expired_nonces(self, &account_id, cleanable).ok(); + if !cleanable.is_empty() { + // NOTE: ignore cleanup errors, but surface counts for visibility. + let _ = State::cleanup_expired_nonces(self, &account_id, cleanable); + } + if errors > 0 { + near_sdk::env::log_str(&format!("is_nonce_cleanable errors: {errors}")); + }core/src/events.rs (1)
33-35: Resolve the TODO: clarify event version.The TODO comment questions which version to use for
SaltRotationEvent. The current version "0.1.0" is significantly lower than other recent events (e.g., "0.3.0", "0.3.1"). Consider whether:
- This should be "0.3.0" (or higher) to align with the current event schema version
- "0.1.0" is intentional to indicate a new feature with its own versioning track
Would you like me to search for documentation or patterns around event versioning in this codebase to help determine the appropriate version?
tests/src/tests/defuse/state/salt.rs (2)
17-23: Don’t couple tests to exact error textAsserting on a hard-coded substring ("Insufficient permissions for method") is brittle. Prefer checking an error code/variant or a stable pattern (e.g., status and method name) to avoid false failures on message wording changes.
If there’s a stable error code for ACL failures, switch assertions to match it. Otherwise, at least narrow to a prefix and method name.
Also applies to: 50-56
31-40: Assert event emission to cover integration behaviorBoth rotate_salt and invalidate_salt emit SaltRotationEvent. Add an assertion that the expected event was emitted (old_salt/new_salt) to catch regressions in event wiring.
If you want, I can extend the SaltManagerExt call helpers to return the transaction outcome so logs can be parsed and asserted.
Also applies to: 71-77, 85-95
core/src/nonce/expirable.rs (2)
39-41: Remove unused import; rstest fixture already provides random_bytesThe imported symbol shadows the rstest fixture name and is unused, triggering a warning.
- use defuse_test_utils::random::random_bytes;
35-61: Add a Borsh roundtrip test to pin the 8‑byte deadline encodingGiven the custom adapter As::, add a simple serialize/deserialize roundtrip to guard against accidental format changes.
#[test] fn borsh_roundtrip() { use near_sdk::borsh::{BorshDeserialize, BorshSerialize}; let now = chrono::Utc::now(); let en = ExpirableNonce::new(Deadline::new(now), [7u8; 24]); let data = en.try_to_vec().unwrap(); let de = ExpirableNonce::<[u8; 24]>::try_from_slice(&data).unwrap(); assert_eq!(en, de); }Based on learnings (chrono 0.4.42 APIs and Days usage are stable).
defuse/src/contract/salts.rs (2)
47-55: Mark read-only methods as view for clarity and toolingis_valid_salt and get_current_salt are pure reads. Consider marking them as view (if supported in your #[near] macro version) to make intent explicit and prevent accidental payable calls.
Example:
#[view] fn is_valid_salt(&self, salt: &Salt) -> bool { ... } #[view] fn get_current_salt(&self) -> Salt { ... }
25-45: Invalidate-current path: document the two-step rotation + clear behaviorLogic is correct (rotate if current, then clear_previous on the now-previous salt), but it’s non-obvious. A short comment will prevent accidental reordering that would break correctness.
- if salt == &self.salts.current() { + // If invalidating the current salt, advance to a new one first so `salt` + // becomes the previous entry we can clear below. + if salt == &self.salts.current() { self.salts.set_new(); }tests/src/tests/defuse/state/extentions/fee.rs (1)
31-36: Make view helpers explicit about the target contractConfirmed that all current calls (
env.defuse.fee()and.fee_collector()) are onContractinstances, but sinceFeesManagerExtis also implemented forAccount, invoking these on a non‐contract account would issue a view on the wrong account ID. Mirror the mutators’ signatures by adding adefuse_contract_idparameter:- async fn fee(&self) -> anyhow::Result<Pips>; + async fn fee(&self, defuse_contract_id: &AccountId) -> anyhow::Result<Pips>; - async fn fee_collector(&self) -> anyhow::Result<AccountId>; + async fn fee_collector(&self, defuse_contract_id: &AccountId) -> anyhow::Result<AccountId>;and update both
AccountandContractimpls to forward the provideddefuse_contract_id.core/src/nonce/salted.rs (3)
19-22: Avoid unwrap in contract code paths; prefer explicit invariant checks.
unwrap()will panic if invariants ever change. Replace with an explicit expect or, better, avoid fallible conversion.- // NOTE: it is safe to unwrap here as random_seed_array always returns 32 bytes - env::random_seed_array()[..4].try_into().unwrap() + // random_seed_array() returns 32 bytes on NEAR; take first 4 deterministically + let bytes = env::random_seed_array(); + // If the SDK ever changes, fail with a clear message instead of panicking mid‑serialization + bytes[..4].try_into().expect("random_seed_array must be at least 4 bytes")
36-45: Clarify rotation semantics and bound retained “previous” salts.
set_newinserts every previous salt and leaves it valid until explicitly cleared. Over many rotations this can grow unbounded and keep multiple previous salts valid.
- Option A (policy): keep only the last previous salt; clear older entries inside
set_new.- Option B (API): add
clear_all_previous()orretain_previous<F: Fn(&Salt)->bool>to bound storage and validity windows.
53-56: Minor: constant‑time lookup is fine; consider documenting expected slice size.
is_validis O(1), butis_valid_saltonSaltedNoncedoes a linear scan overexpected_salts. If this is only used in tests, fine; otherwise document the typical size (e.g., 1–2 salts) to set expectations.tests/src/tests/defuse/accounts/nonces.rs (3)
27-37: Helper constructs correct V1 layout; consider asserting encoded length in tests.
VersionedNonce::V1(..).try_into()relies on the encoded layout being exactly 32 bytes. Add an assertion in tests to guard against future changes toDeadlineencoding.let raw: [u8; 32] = VersionedNonce::V1(...).try_into().unwrap(); assert_eq!(raw.len(), 32, "V1 nonce must encode to 32 bytes");
72-88: Error string coupling can be brittle.
assert_err_contains("invalid salt")couples tests to exact messages. Prefer error codes/variants or stable substrings behind helpers to reduce churn.
315-337: Invalidate‑then‑cleanup flow looks good; add a check that legacy/non‑expired nonces remain.You already assert long‑term nonce is cleared post‑invalidation. Add asserts that unaffected nonces (e.g., a different salt or still‑valid deadline) remain to catch over‑eager cleanup.
core/src/nonce/versioned.rs (3)
55-69: Legacy branch: make chaining explicit for readability and type clarity.
prefix.chain(reader)relies on coercions; usingCursorreads cleaner and avoids surprises.- _ => Self::Legacy(Nonce::deserialize_reader(&mut prefix.chain(reader))?), + _ => { + let mut chained = std::io::Cursor::new(prefix).chain(reader); + Self::Legacy(Nonce::deserialize_reader(&mut chained)?) + }
11-16: Document the layout source of truth and add a static size check.The docstring states V1 = 4 (prefix) + 4 (salt) + 8 (deadline) + 16 (nonce) = 32. Add a
const_assert!(or test) to ensureSaltedNonce<ExpirableNonce<[u8; 16]>>serializes to 28 bytes, guarding futureDeadlinechanges.
88-120: Strengthen tests with negative cases and fuzz.
- Add a test that feeds a 32‑byte payload with an unknown prefix and ensures it decodes as Legacy.
- Add a test that encodes V1, flips one byte in the prefix, and roundtrips to Legacy.
- Consider
arbitrary/fuzz to ensure deserializer never panics on random 32‑byte inputs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
core/src/accounts.rs(2 hunks)core/src/engine/mod.rs(2 hunks)core/src/engine/state/cached.rs(2 hunks)core/src/engine/state/deltas.rs(2 hunks)core/src/engine/state/mod.rs(2 hunks)core/src/error.rs(2 hunks)core/src/events.rs(2 hunks)core/src/nonce.rs(0 hunks)core/src/nonce/expirable.rs(1 hunks)core/src/nonce/mod.rs(1 hunks)core/src/nonce/salted.rs(1 hunks)core/src/nonce/versioned.rs(1 hunks)defuse/src/contract/accounts/account/mod.rs(1 hunks)defuse/src/contract/accounts/account/nonces.rs(4 hunks)defuse/src/contract/accounts/mod.rs(1 hunks)defuse/src/contract/intents/state.rs(2 hunks)defuse/src/contract/mod.rs(5 hunks)defuse/src/contract/salts.rs(1 hunks)defuse/src/contract/state/entry.rs(1 hunks)defuse/src/contract/state/mod.rs(4 hunks)defuse/src/contract/state/v0.rs(1 hunks)defuse/src/intents.rs(1 hunks)defuse/src/lib.rs(1 hunks)defuse/src/salts.rs(1 hunks)tests/src/tests/defuse/accounts/nonces.rs(4 hunks)tests/src/tests/defuse/mod.rs(1 hunks)tests/src/tests/defuse/state/extentions/fee.rs(1 hunks)tests/src/tests/defuse/state/extentions/mod.rs(1 hunks)tests/src/tests/defuse/state/extentions/salt.rs(1 hunks)tests/src/tests/defuse/state/fee.rs(1 hunks)tests/src/tests/defuse/state/mod.rs(1 hunks)tests/src/tests/defuse/state/salt.rs(1 hunks)
💤 Files with no reviewable changes (1)
- core/src/nonce.rs
🧰 Additional context used
🧬 Code graph analysis (23)
tests/src/tests/defuse/state/extentions/fee.rs (2)
tests/src/tests/defuse/state/fee.rs (2)
set_fee(14-43)set_fee_collector(47-74)defuse/src/contract/intents/state.rs (2)
fee(31-33)fee_collector(36-38)
core/src/engine/state/mod.rs (3)
core/src/engine/state/cached.rs (2)
verify_intent_nonce(123-125)is_nonce_cleanable(127-129)core/src/engine/state/deltas.rs (2)
verify_intent_nonce(101-103)is_nonce_cleanable(106-108)defuse/src/contract/intents/state.rs (2)
verify_intent_nonce(94-117)is_nonce_cleanable(119-130)
tests/src/tests/defuse/state/extentions/mod.rs (2)
defuse/src/contract/intents/state.rs (1)
fee(31-33)tests/src/tests/defuse/state/extentions/fee.rs (3)
fee(8-8)fee(31-36)fee(68-70)
tests/src/tests/defuse/state/fee.rs (3)
tests/src/tests/defuse/state/extentions/fee.rs (12)
set_fee(6-6)set_fee(18-29)set_fee(64-66)fee(8-8)fee(31-36)fee(68-70)set_fee_collector(9-13)set_fee_collector(38-54)set_fee_collector(72-80)fee_collector(14-14)fee_collector(55-60)fee_collector(82-84)defuse/src/contract/intents/state.rs (2)
fee(31-33)fee_collector(36-38)core/src/fees.rs (1)
from_pips(39-44)
defuse/src/contract/salts.rs (4)
tests/src/tests/defuse/state/salt.rs (2)
rotate_salt(13-41)invalidate_salt(45-96)tests/src/tests/defuse/state/extentions/salt.rs (12)
rotate_salt(6-6)rotate_salt(20-29)rotate_salt(64-66)invalidate_salt(8-12)invalidate_salt(31-45)invalidate_salt(68-76)is_valid_salt(14-14)is_valid_salt(47-53)is_valid_salt(78-80)get_current_salt(16-16)get_current_salt(55-60)get_current_salt(82-84)defuse/src/salts.rs (4)
rotate_salt(8-8)invalidate_salt(12-12)is_valid_salt(15-15)get_current_salt(18-18)core/src/nonce/salted.rs (1)
is_valid_salt(82-84)
defuse/src/contract/intents/state.rs (4)
core/src/engine/state/cached.rs (2)
verify_intent_nonce(123-125)is_nonce_cleanable(127-129)core/src/engine/state/deltas.rs (2)
verify_intent_nonce(101-103)is_nonce_cleanable(106-108)core/src/engine/state/mod.rs (2)
verify_intent_nonce(46-46)is_nonce_cleanable(49-49)core/src/nonce/versioned.rs (2)
try_from(38-40)try_from(46-52)
defuse/src/contract/accounts/account/nonces.rs (2)
defuse/src/contract/accounts/account/mod.rs (1)
new(48-65)test-utils/src/random.rs (2)
new(77-79)random_bytes(143-151)
defuse/src/contract/state/v0.rs (3)
defuse/src/contract/state/entry.rs (4)
from(68-73)from(78-81)from(86-88)new(36-41)defuse/src/contract/mod.rs (1)
new(83-92)defuse/src/contract/state/mod.rs (1)
new(29-42)
core/src/engine/state/deltas.rs (3)
core/src/engine/state/cached.rs (2)
verify_intent_nonce(123-125)is_nonce_cleanable(127-129)core/src/engine/state/mod.rs (2)
verify_intent_nonce(46-46)is_nonce_cleanable(49-49)defuse/src/contract/intents/state.rs (2)
verify_intent_nonce(94-117)is_nonce_cleanable(119-130)
defuse/src/salts.rs (3)
defuse/src/contract/salts.rs (4)
rotate_salt(13-23)invalidate_salt(27-45)is_valid_salt(48-50)get_current_salt(53-55)tests/src/tests/defuse/state/extentions/salt.rs (12)
rotate_salt(6-6)rotate_salt(20-29)rotate_salt(64-66)invalidate_salt(8-12)invalidate_salt(31-45)invalidate_salt(68-76)is_valid_salt(14-14)is_valid_salt(47-53)is_valid_salt(78-80)get_current_salt(16-16)get_current_salt(55-60)get_current_salt(82-84)core/src/nonce/salted.rs (1)
is_valid_salt(82-84)
core/src/nonce/expirable.rs (2)
test-utils/src/random.rs (2)
random(82-84)random_bytes(143-151)core/src/nonce.rs (1)
ExpirableNonce(73-80)
tests/src/tests/defuse/state/extentions/salt.rs (4)
defuse/src/contract/salts.rs (4)
rotate_salt(13-23)invalidate_salt(27-45)is_valid_salt(48-50)get_current_salt(53-55)tests/src/tests/defuse/state/salt.rs (2)
rotate_salt(13-41)invalidate_salt(45-96)defuse/src/salts.rs (4)
rotate_salt(8-8)invalidate_salt(12-12)is_valid_salt(15-15)get_current_salt(18-18)core/src/nonce/salted.rs (1)
is_valid_salt(82-84)
defuse/src/contract/accounts/mod.rs (6)
serde-utils/src/base64.rs (1)
into_inner(77-79)core/src/engine/state/cached.rs (1)
cleanup_expired_nonces(186-202)core/src/engine/state/deltas.rs (1)
cleanup_expired_nonces(131-137)core/src/engine/state/mod.rs (1)
cleanup_expired_nonces(68-72)defuse/src/contract/intents/state.rs (1)
cleanup_expired_nonces(166-182)defuse/src/accounts.rs (1)
cleanup_expired_nonces(35-35)
core/src/nonce/salted.rs (7)
defuse/src/contract/accounts/mod.rs (1)
new(100-109)defuse/src/contract/mod.rs (1)
new(83-92)defuse/src/contract/state/entry.rs (1)
new(36-41)defuse/src/contract/state/mod.rs (1)
new(29-42)defuse/src/contract/salts.rs (1)
is_valid_salt(48-50)defuse/src/salts.rs (1)
is_valid_salt(15-15)test-utils/src/random.rs (3)
s(54-54)random(82-84)random_bytes(143-151)
tests/src/tests/defuse/state/mod.rs (2)
defuse/src/contract/intents/state.rs (1)
fee(31-33)tests/src/tests/defuse/state/extentions/fee.rs (3)
fee(8-8)fee(31-36)fee(68-70)
core/src/nonce/mod.rs (5)
core/src/nonce/salted.rs (2)
new(26-34)new(78-80)core/src/nonce/expirable.rs (1)
new(25-27)defuse/src/contract/accounts/account/mod.rs (1)
new(48-65)defuse/src/contract/accounts/account/nonces.rs (3)
new(31-37)is_used(63-69)commit(48-60)core/src/nonce/versioned.rs (1)
prefix(27-32)
core/src/engine/state/cached.rs (3)
core/src/engine/state/deltas.rs (2)
verify_intent_nonce(101-103)is_nonce_cleanable(106-108)core/src/engine/state/mod.rs (2)
verify_intent_nonce(46-46)is_nonce_cleanable(49-49)defuse/src/contract/intents/state.rs (2)
verify_intent_nonce(94-117)is_nonce_cleanable(119-130)
defuse/src/contract/state/entry.rs (3)
defuse/src/contract/mod.rs (1)
new(83-92)defuse/src/contract/state/mod.rs (1)
new(29-42)defuse/src/contract/state/v0.rs (1)
from(23-40)
tests/src/tests/defuse/state/salt.rs (3)
defuse/src/contract/salts.rs (3)
rotate_salt(13-23)is_valid_salt(48-50)invalidate_salt(27-45)tests/src/tests/defuse/state/extentions/salt.rs (9)
rotate_salt(6-6)rotate_salt(20-29)rotate_salt(64-66)is_valid_salt(14-14)is_valid_salt(47-53)is_valid_salt(78-80)invalidate_salt(8-12)invalidate_salt(31-45)invalidate_salt(68-76)core/src/nonce/salted.rs (1)
is_valid_salt(82-84)
tests/src/tests/defuse/accounts/nonces.rs (3)
core/src/nonce/salted.rs (2)
new(26-34)new(78-80)defuse/src/contract/accounts/mod.rs (2)
new(100-109)is_nonce_used(51-53)tests/src/tests/defuse/accounts/mod.rs (3)
is_nonce_used(44-44)is_nonce_used(145-154)is_nonce_used(232-234)
defuse/src/contract/mod.rs (3)
core/src/nonce/salted.rs (2)
new(26-34)new(78-80)defuse/src/contract/state/entry.rs (1)
new(36-41)defuse/src/contract/state/mod.rs (1)
new(29-42)
core/src/nonce/versioned.rs (4)
core/src/payload/mod.rs (1)
nonce(68-70)test-utils/src/random.rs (2)
random(82-84)random_bytes(143-151)core/src/nonce/salted.rs (2)
new(26-34)new(78-80)core/src/nonce/expirable.rs (1)
new(25-27)
defuse/src/contract/state/mod.rs (4)
core/src/nonce/salted.rs (2)
new(26-34)new(78-80)defuse/src/contract/accounts/mod.rs (1)
new(100-109)defuse/src/contract/mod.rs (1)
new(83-92)defuse/src/contract/state/entry.rs (1)
new(36-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Build
- GitHub Check: Shared / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared security analysis / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Security Audit - report
🔇 Additional comments (48)
tests/src/tests/defuse/mod.rs (1)
4-4: LGTM!The new
statemodule declaration correctly integrates the salt management and nonce-related tests introduced in this PR.core/src/error.rs (1)
58-59: LGTM!The new error variants
InvalidNonceandInvalidSaltare well-positioned and provide clear error messages for the salted nonce validation flow introduced in this PR.Also applies to: 73-74
defuse/src/lib.rs (1)
8-8: LGTM!The public
saltsmodule declaration correctly exposes the new salt management functionality introduced in this PR.core/src/engine/state/mod.rs (1)
5-5: All StateView implementations updated. The methodsverify_intent_nonceandis_nonce_cleanableare implemented in CachedState, Deltas, and the Contract state with correct delegation.defuse/src/contract/state/mod.rs (1)
24-25: Salt storage integration is correct and scoped.Initializing ValidSalts under a dedicated Salts prefix is the right boundary and keeps keys stable. No issues.
Also applies to: 40-41, 49-49
core/src/engine/mod.rs (1)
77-80: Approve: nonce enforcement in EngineDelegates salt/deadline/format checks to
StateView; ordering (verify_intent_nonce→commit_nonce) is correct.• Confirmed a single
StateViewimpl indefuse/src/contract/intents/state.rs; no legacy implementations missingverify_intent_nonce.defuse/src/intents.rs (2)
14-14: LGTM! Import statement is correct.The addition of
SaltManagerto the imports is necessary to support the updated trait bound on line 17.
17-17: AllIntentsimplementors implementSaltManager. The sole implementor (Contract) already provides aSaltManagerimpl (defuse/src/contract/salts.rs:10), so no further action is required.core/src/events.rs (1)
7-7: LGTM! Import statement is correct.The
SaltRotationEventis properly imported from the accounts module.core/src/accounts.rs (2)
7-7: LGTM! Import statement is correct.The
Salttype is properly imported alongsideNonceto support the newSaltRotationEvent.
67-73: LGTM! Well-defined event structure.The
SaltRotationEventis properly structured with:
- Appropriate
must_uselint to ensure events are emitted- Correct
nearserializers annotation- Both
old_saltandnew_saltfields for complete audit trail- Standard derives (
Debug,Clone)tests/src/tests/defuse/state/fee.rs (2)
12-43: LGTM! Comprehensive permission and functionality test.The
set_feetest correctly verifies:
- Permission enforcement (user without role cannot set fee)
- Role-based access (user with FeesManager role can set fee)
- State mutation (fee value changes correctly)
The test structure is clean and follows good practices with clear test phases.
45-74: LGTM! Comprehensive permission and functionality test.The
set_fee_collectortest correctly verifies:
- Permission enforcement (user without role cannot set collector)
- Role-based access (user with FeesManager role can set collector)
- State mutation (collector changes correctly)
The test structure is consistent with
set_feeand follows the same good testing patterns.tests/src/tests/defuse/state/mod.rs (1)
5-5: LGTM! Clean public API surface.The re-exports provide a clean public API by exposing the extension traits at the module root, making them easy to import and use in tests.
Note: This line references the misspelled module name
extentionswhich should be corrected as noted in the comment on line 1.defuse/src/contract/intents/state.rs (3)
2-2: LGTM! Imports align with new nonce verification.The added imports (
Deadline,ExpirableNonce,SaltedNonce,VersionedNonce) are necessary for the new nonce validation logic introduced below.
119-130: Nonce cleanability logic is correct.The method properly determines whether a nonce can be cleaned:
- V1 nonces are cleanable if the salt is invalid or the deadline has expired.
- Legacy nonces cannot be cleaned (preserving backward compatibility until they are fully deprecated).
The comment on line 125 clearly documents the rationale for not cleaning legacy nonces.
94-117: Intent nonce verification logic approved. Legacy and V1 nonces are handled correctly, and all referenced error variants (InvalidSalt, DeadlineGreaterThanNonce, NonceExpired) are present in DefuseError.defuse/src/contract/accounts/account/nonces.rs (6)
63-69: LGTM! Nonce usage check is straightforward.The
is_usedmethod correctly checks both the new nonces map and the legacy map (if present), ensuring backward compatibility.
72-75: Clear expired logic aligns with versioned nonce design.The
clear_expiredmethod only clears nonces from the new map. The comment on line 73 correctly notes that expirable nonces cannot be in the legacy map, which aligns with the broader transition to versioned/salted nonces.
98-99: Test helper simplified correctly.The
random_noncesfixture now generates plain random nonces usingrng.random(), removing the expirable nonce generation logic. This aligns with the removal of expirable nonce support.
132-148: Test updated to use plain random nonces.The
commit_new_noncetest now generates plain random nonces for bothnew_nonceandlegacy_nonce, removing the expirable nonce logic. This simplifies the test and aligns with the broader nonce model changes.
166-176: Duplicate nonce test simplified.The
commit_duplicate_noncetest now uses a plain random nonce, removing the expirable nonce logic. The test logic remains correct.
200-210: Legacy nonces cannot be cleared test updated.The
legacy_nonces_cant_be_clearedtest now uses a plain random nonce. The test correctly verifies that legacy nonces cannot be cleared and remain marked as used.defuse/src/salts.rs (1)
1-19: SaltManager trait is well-defined.The trait provides a clean interface for managing salts:
rotate_salt: Rotates to a new salt while keeping the previous one valid (graceful rotation).invalidate_salt: Invalidates a specific salt and sets a new one if the invalidated salt was current.is_valid_salt: Queries salt validity.get_current_salt: Retrieves the current salt.The comments clearly document each method's behavior. The trait is appropriately exposed via
ext_contractfor cross-contract calls.core/src/engine/state/deltas.rs (2)
2-2: LGTM! Deadline import added.The
Deadlinetype is now imported to support the newverify_intent_noncemethod signature.
100-108: Forwarding methods are correct.Both
verify_intent_nonceandis_nonce_cleanablecorrectly forward calls to the underlying state. The#[inline]attribute is appropriate for these trivial delegation methods, ensuring no overhead from the wrapper.defuse/src/contract/mod.rs (5)
9-9: Salts module added.The new
saltsmodule is added to support the salt management functionality introduced in this PR.
30-30: State type updated to ContractStateEntry.The import changed from
ContractStatetoContractStateEntry, which aligns with the introduction of versioned contract state (as noted in the PR objectives: "State versioning").
49-50: SaltManager role added for access control.The new
SaltManagerrole enables fine-grained access control for salt rotation and invalidation operations, consistent with the existing role-based access control pattern in the contract.
70-70: Contract state field updated.The
statefield type changed fromContractStatetoContractStateEntry, consistent with the import change on line 30.
86-86: Approve: Signatures are consistent
ContractStateEntry::new<T>(prefix: T, wnear_id: AccountId, fees: FeesConfig)matches the originalContractState::new<S>(prefix: S, wnear_id: AccountId, fees: FeesConfig).defuse/src/contract/state/v0.rs (1)
22-40: RefactorFrom<ContractStateV0>to useContractState::newfor salts prefix consistency
Replace the inlineValidSalts::new(...)with:let mut state = ContractState::new(ContractPrefix::State, wnear_id, fees); state.total_supplies = total_supplies; stateThis centralizes prefix logic in
ContractState::newand prevents future drift.core/src/nonce/mod.rs (7)
1-10: LGTM!Clean modular structure with appropriate re-exports for versioned, salted, and expirable nonce types.
18-24: LGTM!The Nonces wrapper over BitMap256 with proper serialization support is a clean abstraction.
30-33: LGTM!Const constructor properly delegates to the underlying BitMap256.
35-38: LGTM!The method correctly checks whether a nonce has been used by delegating to the bitmap.
40-47: LGTM!The commit method correctly prevents nonce reuse by returning an error if the nonce was already set, providing replay protection.
49-54: LGTM!The method correctly extracts the prefix from the nonce (all bytes except the last) and clears all nonces with that prefix, which aligns with the expirable nonce design.
56-63: LGTM!The iterator method properly constrains the type bound and delegates to the underlying bitmap iterator.
defuse/src/contract/state/entry.rs (6)
19-42: LGTM!The transparent wrapper pattern with custom Borsh adapters provides clean version-aware (de)serialization. The constructor properly delegates to the inner type.
44-58: LGTM!The magic prefix approach using
u32::MAXis safe given the assumption that the legacyIterableMaplength will never reach this value. The comment clearly documents the collision avoidance strategy.
60-65: LGTM!The versioned enum with
CowandPanicOnClonewrappers provides efficient serialization while preventing accidental clones.
67-89: LGTM!The conversion paths correctly handle ownership transfer and unwrapping for both deserialization (V0/Latest → ContractState) and serialization (always Latest).
91-115: LGTM!The deserialization logic correctly handles both versioned and legacy formats. The use of
chain()to prepend the consumed prefix bytes back to the reader for legacy deserialization is the right approach.
117-132: LGTM!Serialization always writes the versioned format with the magic prefix, ensuring forward compatibility while maintaining backward compatibility for reads.
tests/src/tests/defuse/state/extentions/salt.rs (3)
5-17: LGTM!The trait provides a clean async interface for salt management operations, with method signatures matching the contract interface.
20-45: LGTM!The mutation methods correctly target the defuse contract, attach the required 1 yoctoNEAR deposit, and use max gas for the cross-contract calls.
63-85: Contract delegation is correct, but inherits view method issue.The Contract implementation correctly delegates all methods to the underlying account. However, it inherits the design issue flagged in the Account implementation regarding view method contract targeting.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/src/nonce/versioned.rs (1)
65-96: LGTM! Serialization logic is correct.The Borsh implementations correctly handle both legacy and V1 formats. The deserialize logic appropriately uses
chainto prepend the prefix back when falling back to legacy format.The TODO on line 86 suggests the serialize implementation could be more concise. Consider using pattern matching to combine the prefix writing and data serialization:
impl BorshSerialize for VersionedNonce { fn serialize<W: io::Write>(&self, writer: &mut W) -> io::Result<()> { - // TODO: make it more concise - if let Some(prefix) = self.prefix() { - writer.write_all(&prefix)?; - } - match self { - Self::Legacy(nonce) => nonce.serialize(writer), - Self::V1(salted) => salted.serialize(writer), + Self::Legacy(nonce) => nonce.serialize(writer), + Self::V1(salted) => { + writer.write_all(&Self::V1_MAGIC_PREFIX)?; + salted.serialize(writer) + } } } }tests/src/tests/defuse/state/extensions/salt.rs (1)
5-21: Consider consistent parameter passing for Salt.The trait takes
Saltby value ininvalidate_salt(line 11) but by reference inis_valid_salt(line 17). For consistency and to avoid unnecessary clones in calling code, consider taking&Saltin both methods.Apply this diff for consistency:
async fn invalidate_salt( &self, defuse_contract_id: &AccountId, - salt: Salt, + salt: &Salt, ) -> anyhow::Result<()>;Then update the implementations accordingly (lines 35-49 and 76-84).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
core/src/nonce/versioned.rs(1 hunks)tests/src/tests/defuse/accounts/nonces.rs(4 hunks)tests/src/tests/defuse/state/extensions/fee.rs(1 hunks)tests/src/tests/defuse/state/extensions/mod.rs(1 hunks)tests/src/tests/defuse/state/extensions/salt.rs(1 hunks)tests/src/tests/defuse/state/fee.rs(1 hunks)tests/src/tests/defuse/state/mod.rs(1 hunks)tests/src/tests/defuse/state/salt.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/src/tests/defuse/state/fee.rs
- tests/src/tests/defuse/state/salt.rs
🧰 Additional context used
🧬 Code graph analysis (6)
tests/src/tests/defuse/state/extensions/mod.rs (1)
tests/src/tests/defuse/state/extensions/fee.rs (3)
fee(8-8)fee(31-36)fee(68-70)
tests/src/tests/defuse/state/extensions/salt.rs (1)
tests/src/tests/defuse/state/salt.rs (2)
rotate_salt(13-46)invalidate_salt(50-107)
tests/src/tests/defuse/state/mod.rs (2)
tests/src/tests/defuse/state/extensions/fee.rs (3)
fee(8-8)fee(31-36)fee(68-70)defuse/src/contract/intents/state.rs (1)
fee(31-33)
tests/src/tests/defuse/state/extensions/fee.rs (1)
tests/src/tests/defuse/state/fee.rs (2)
set_fee(14-43)set_fee_collector(47-74)
core/src/nonce/versioned.rs (2)
core/src/nonce/salted.rs (2)
new(26-34)new(78-80)core/src/nonce/expirable.rs (1)
new(25-27)
tests/src/tests/defuse/accounts/nonces.rs (3)
core/src/nonce/salted.rs (2)
new(26-34)new(78-80)defuse/src/contract/accounts/mod.rs (2)
new(100-109)is_nonce_used(51-53)tests/src/tests/defuse/accounts/mod.rs (3)
is_nonce_used(44-44)is_nonce_used(145-154)is_nonce_used(232-234)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Build
- GitHub Check: Shared security analysis / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Security Audit - report
🔇 Additional comments (16)
tests/src/tests/defuse/state/mod.rs (1)
1-5: LGTM! Spelling issue resolved.The module structure is correct and the spelling of "extensions" has been fixed (previously flagged as "extentions").
tests/src/tests/defuse/state/extensions/mod.rs (1)
1-2: LGTM!The module declarations correctly expose the fee and salt extension modules publicly.
tests/src/tests/defuse/state/extensions/fee.rs (3)
5-15: LGTM!The trait provides a clean API for fee management with symmetric getter/setter pairs for both fee and fee_collector.
17-61: LGTM!The implementation follows NEAR workspaces best practices with appropriate deposit (1 yoctoNEAR) for change methods and view calls for getters.
63-85: LGTM!The Contract implementation correctly delegates all methods to the underlying Account via
as_account().tests/src/tests/defuse/accounts/nonces.rs (5)
1-26: LGTM!The imports correctly include the new salted/versioned nonce types and salt management traits aligned with the PR's objectives.
27-37: LGTM!The helper correctly constructs a V1 salted nonce with the provided salt and deadline. Using
expectis acceptable in test helpers.
39-208: LGTM!Comprehensive test coverage of nonce validation including legacy compatibility, salt validation, deadline checks, expiry, rotation windows, and invalidation. The test correctly verifies both acceptance and rejection paths.
210-338: LGTM!Thorough test of nonce cleanup covering time-based expiry, idempotency, edge cases (unknown users, legacy nonces, not-yet-expired), and salt invalidation cleanup. The test correctly verifies selective cleanup behavior.
340-391: LGTM!Performance test correctly measures gas costs for batch cleanup with parameterized nonce counts. The chunked creation and batch cleanup logic is sound.
core/src/nonce/versioned.rs (4)
11-33: LGTM!The enum structure cleanly distinguishes legacy from V1 salted nonces. The magic prefix approach for version detection is a standard technique and the
prefix()method provides clean access to version metadata.
35-41: LGTM!The conversion correctly delegates to Borsh deserialization which will detect the version based on the prefix.
43-64: LGTM! Past review concern addressed.The implementation now uses checked serialization with explicit length validation, eliminating the panic risk flagged in the previous review. The error handling correctly returns
io::Errorfor unexpected lengths.
98-130: LGTM!The tests correctly verify roundtrip conversions for both legacy and V1 nonce formats, ensuring serialization/deserialization correctness.
tests/src/tests/defuse/state/extensions/salt.rs (2)
23-69: LGTM!The Account implementation follows consistent patterns for both state-changing methods (with 1 yoctoNEAR deposit and max_gas) and view methods. Error handling is appropriate for test utilities.
71-99: LGTM!The Contract implementation correctly delegates all methods to the underlying Account implementation, providing a consistent interface for both Account and Contract types in tests.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
core/src/nonce/versioned.rs (1)
39-55: Good: safe serialization into fixed array with length check.This addresses the prior panic risk from writing into a fixed
[u8; 32]withunwrap(). Length is validated before copying.
🧹 Nitpick comments (4)
CHANGELOG.md (1)
8-8: Clarify salt size phrasing.Consider rephrasing “4 bytes salts” to “4-byte salts” so the byte-width reads naturally in English.
core/src/nonce/salted.rs (2)
49-53: Avoid on-chain storage growth: remove invalidated salts instead of toggling a bool.
clear_previousflips a flag but keeps the key, sopreviousgrows unbounded across rotations. On NEAR this increases state size (and storage staking) over time. Prefer removing the key to reclaim storage. If IterableMap lacksremove, consider switching to a set-like structure for previous valid salts.Proposed change (if
IterableMap::removeis available):- pub fn clear_previous(&mut self, salt: &Salt) -> bool { - self.previous.get_mut(salt).map(|v| *v = false).is_some() - } + pub fn clear_previous(&mut self, salt: &Salt) -> bool { + self.previous.remove(salt).is_some() + }
38-47: API naming/return value clarity for rotation.
set_newreturns the previous salt while also updatingcurrent. Consider renaming torotateand/or returning the new salt (callers can querycurrent()but returning it is more direct). Not a blocker—just polish.tests/src/tests/defuse/accounts/nonces.rs (1)
269-279: Potential time-based flakiness: rely on chain/env time rather than wall clock sleeps.Tests use
Utc::now()andtokio::sleepto drive expiry. On slower CI or different runtimes, deadlines may race with execution. Prefer advancing the contract’s notion of time (e.g., builder/context timestamp or a helper that bumps block timestamp) to deterministically cross deadlines.Can you confirm whether
Envexposes a way to set/block_timestamp for calls? If yes, we can replace sleeps with timestamp bumps for deterministic tests.Also applies to: 286-297, 315-337
📜 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 (11)
CHANGELOG.md(1 hunks)core/src/events.rs(2 hunks)core/src/nonce/salted.rs(1 hunks)core/src/nonce/versioned.rs(1 hunks)defuse/Cargo.toml(1 hunks)defuse/src/contract/state/entry.rs(1 hunks)defuse/src/contract/state/mod.rs(4 hunks)defuse/src/contract/state/v0.rs(1 hunks)tests/src/tests/defuse/accounts/nonces.rs(4 hunks)tests/src/tests/defuse/state/extensions/salt.rs(1 hunks)tests/src/tests/defuse/state/salt.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- defuse/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/src/tests/defuse/state/salt.rs
- core/src/events.rs
- tests/src/tests/defuse/state/extensions/salt.rs
🧰 Additional context used
🧬 Code graph analysis (6)
defuse/src/contract/state/mod.rs (3)
core/src/nonce/salted.rs (2)
new(28-36)new(80-82)defuse/src/contract/state/entry.rs (1)
new(36-41)defuse/src/contract/mod.rs (1)
new(83-92)
core/src/nonce/salted.rs (4)
defuse/src/contract/state/mod.rs (1)
new(28-41)tests/src/tests/defuse/state/extensions/salt.rs (3)
is_valid_salt(14-18)is_valid_salt(51-61)is_valid_salt(86-94)defuse/src/contract/salts.rs (1)
is_valid_salt(48-50)defuse/src/salts.rs (1)
is_valid_salt(15-15)
core/src/nonce/versioned.rs (3)
core/src/nonce/salted.rs (2)
new(28-36)new(80-82)core/src/nonce/expirable.rs (1)
new(25-27)core/src/nonce/mod.rs (1)
new(31-33)
defuse/src/contract/state/v0.rs (3)
defuse/src/contract/state/entry.rs (4)
from(69-74)from(79-82)from(87-89)new(36-41)defuse/src/contract/state/mod.rs (1)
new(28-41)defuse/src/contract/mod.rs (1)
new(83-92)
defuse/src/contract/state/entry.rs (4)
core/src/nonce/versioned.rs (1)
deserialize_reader(59-74)defuse/src/contract/state/mod.rs (1)
new(28-41)defuse/src/contract/mod.rs (1)
new(83-92)defuse/src/contract/state/v0.rs (1)
from(21-38)
tests/src/tests/defuse/accounts/nonces.rs (3)
core/src/nonce/salted.rs (2)
new(28-36)new(80-82)defuse/src/accounts.rs (1)
is_nonce_used(30-30)tests/src/tests/defuse/accounts/mod.rs (3)
is_nonce_used(44-44)is_nonce_used(145-154)is_nonce_used(232-234)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Build
- GitHub Check: Shared security analysis / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Security Audit - report
🔇 Additional comments (12)
defuse/src/contract/state/mod.rs (3)
1-4: LGTM!The module structure correctly introduces versioning support with the v0 legacy module and the new entry wrapper.
6-6: LGTM!The ValidSalts import is correctly added to support the new salts field in ContractState.
48-48: LGTM!The Salts storage prefix variant is correctly added to support the new salts field.
defuse/src/contract/state/v0.rs (2)
1-18: LGTM!ContractStateV0 correctly represents the legacy state format before the salts field was introduced, enabling backward-compatible migration.
20-39: ApproveFrom<ContractStateV0>prefix initialization.
The migration’s salts key construction (ContractPrefix::State.into_storage_key().nest(StatePrefix::Salts)) matchesContractState::new(Prefix::State.as_slice().nest(Prefix::Salts)). No further action required.defuse/src/contract/state/entry.rs (5)
1-42: LGTM!The ContractStateEntry wrapper correctly encapsulates ContractState with custom versioned serialization hooks and provides a convenient constructor.
61-90: LGTM!The versioned state enum and conversion implementations correctly handle both legacy (V0) and current (Latest) formats with appropriate zero-copy optimization using Cow and PanicOnClone.
92-116: LGTM!The custom deserialization correctly handles both versioned and legacy formats by inspecting the magic prefix and either deserializing as versioned state or reconstructing the legacy reader by prepending the consumed prefix bytes.
118-133: LGTM!The custom serialization correctly always writes the versioned format with the magic prefix, ensuring all future states are stored in the versioned format.
46-59: Comment documents magic-prefix assumption; no further action required.The existing comment clearly explains why
u32::MAXis chosen and its safety assumption; static code analysis cannot enforce runtime token counts. Proceed with monitoring token entry growth externally if needed.tests/src/tests/defuse/accounts/nonces.rs (1)
27-37: Nice coverage and scenarios.Good spread: legacy path, invalid salt, mismatched deadline, expired nonce, success, rotation (previous salt valid), and invalidation. This gives strong behavioral confidence.
Also applies to: 72-88, 90-106, 108-124, 126-149, 151-183, 185-207
core/src/nonce/versioned.rs (1)
11-20: Spec and layout are precise.Clear format doc, explicit V1 magic, and round‑trip tests for both Legacy and V1 variants look solid.
Also applies to: 77-86, 89-121
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
core/src/nonce/versioned.rs (2)
1-1: Resolved:size_ofproperly brought into scope.Import at Line 1 fixes the prior compile error where
size_of::<Nonce>()wasn’t qualified. LGTM.Also applies to: 41-41
67-72: Fix: prefix chaining uses[u8; 4]which doesn’t implementRead. Wrap inio::Cursor.
prefix.chain(reader)won’t compile because[u8; 4]is not aRead. Chain aCursoroverprefixwithreader.- _ => Self::Legacy(Nonce::deserialize_reader(&mut prefix.chain(reader))?), + _ => { + let mut chained = io::Cursor::new(prefix).chain(reader); + Self::Legacy(Nonce::deserialize_reader(&mut chained)?) + }
🧹 Nitpick comments (1)
core/src/nonce/versioned.rs (1)
39-57: Optional: Avoid alloc + copy inTryFrom<VersionedNonce> for Nonce.You serialize into a
Vecthen copy to an array. Use a fixed buffer withio::Cursorto eliminate allocation and the extra copy.- let mut buf = Vec::with_capacity(SIZE); - value.serialize(&mut buf)?; - - if buf.len() != SIZE { + let mut out = [0u8; SIZE]; + let mut cursor = io::Cursor::new(&mut out[..]); + value.serialize(&mut cursor)?; + let written = cursor.position() as usize; + debug_assert_eq!(written, SIZE, "VersionedNonce must serialize to 32 bytes"); + if written != SIZE { return Err(io::Error::new( io::ErrorKind::InvalidData, format!( "encoded VersionedNonce has unexpected length: {}", - buf.len() + written ), )); } - let mut result = [0u8; SIZE]; - result.copy_from_slice(&buf); - Ok(result) + Ok(out)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/nonce/versioned.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/src/nonce/versioned.rs (4)
core/src/nonce/salted.rs (2)
new(28-36)new(80-82)core/src/nonce/expirable.rs (1)
new(25-27)core/src/nonce/mod.rs (1)
new(31-33)defuse/src/contract/accounts/account/nonces.rs (1)
new(31-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Build
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared security analysis / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Security Audit - report
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
core/src/engine/state/mod.rs (1)
45-46: Mark salt check as must-use and clarify semantics.
Return value is significant; add #[must_use] and briefly document rotation window semantics (current vs previous salts).- /// Returns whether salt in nonce is valid - fn is_valid_salt(&self, salt: &Salt) -> bool; + /// Returns whether the provided salt is currently accepted (e.g., current or within a rotation window). + #[must_use] + fn is_valid_salt(&self, salt: &Salt) -> bool;defuse/src/accounts.rs (1)
32-35: Update documentation to reflect per-nonce cleanup semantics.The documentation states "Clears all expired nonces" but the implementation (in
defuse/src/contract/accounts/mod.rslines 54-76) performs per-nonce validation that checks both salt validity and expiration. The method now processes each nonce individually with version-specific cleanup logic rather than bulk-clearing expired nonces.Consider updating the doc comment to:
- /// Clears all expired nonces for given accounts. - /// Omitting any errors, e.g. if account doesn't exist or nonces are not expired. + /// Attempts to clean up the provided nonces for given accounts. + /// Each nonce is validated (salt validity, expiration) before cleanup. + /// Omits errors (e.g., account not found, nonce not cleanable). /// NOTE: MUST attach 1 yⓃ for security purposes.defuse/src/contract/accounts/mod.rs (1)
58-61: Don’t revert on malformed nonce; skip and continue.unwrap_or_panic() aborts the whole cleanup if any nonce is malformed. Prefer skipping bad inputs and optionally logging/counting them to make the call robust against mixed-quality batches.
- let versioned = VersionedNonce::try_from(nonce) - .map_err(|_| DefuseError::InvalidNonce) - .unwrap_or_panic(); + let versioned = match VersionedNonce::try_from(nonce) { + Ok(v) => v, + Err(_) => { + // optionally: log/metric for invalid nonce + continue; + } + };defuse/src/contract/salts.rs (1)
30-52: Tighten invalidate_salt error path (minor).The then_some(()).ok_or(...).unwrap_or_panic() chain is fine; a small readability tweak avoids the unit dance.
- self.salts - .clear_previous(salt) - .then_some(()) - .ok_or(DefuseError::InvalidSalt) - .unwrap_or_panic(); + if !self.salts.clear_previous(salt) { + DefuseError::InvalidSalt.panic(); + }tests/src/tests/defuse/accounts/nonces.rs (1)
27-37: Nice helper; consider passing rng by &mut to avoid moves.Tweak signature to accept &mut impl Rng; call sites already pass &mut rng, so no changes elsewhere.
-fn create_random_salted_nonce(salt: [u8; 4], deadline: Deadline, mut rng: impl Rng) -> Nonce { +fn create_random_salted_nonce(salt: [u8; 4], deadline: Deadline, rng: &mut impl Rng) -> Nonce { VersionedNonce::V1(SaltedNonce::new( salt, ExpirableNonce { deadline, nonce: rng.random::<[u8; 16]>(), }, )) .try_into() .expect("unable to create salted nonce") }core/src/nonce/salted.rs (2)
1-9: Missing import forsize_of.Line 22 uses
size_of::<Salt>()butsize_ofis not explicitly imported. While it may be available through the prelude, explicit imports improve clarity.Apply this diff to add the explicit import:
-use core::mem; +use core::mem::{self, size_of};
45-52: Clarify return value:set_new()returns the OLD salt.The method rotates to a new current salt but returns the previous one. While this may be intentional for event emission or tracking, the naming and documentation could be clearer about this behavior.
Consider either:
- Renaming to
rotate_salt()with documentation: "Returns the old salt that was rotated out"- Returning both old and new salts as a tuple:
(Salt, Salt)- Adding a doc comment explaining the return value represents the previous salt
tests/src/tests/defuse/state/extensions/salt.rs (1)
24-49: Consider ifmax_gas()is necessary for salt operations.Both
rotate_saltandinvalidate_saltuse.max_gas(), which attaches the maximum possible gas to the transaction. While this ensures operations won't fail due to gas limits, salt rotation is likely a lightweight operation that doesn't require maximum gas.Consider specifying a reasonable fixed gas amount instead of
max_gas()to:
- Make gas costs more predictable
- Avoid unnecessarily high gas attachment
- Better align with production usage patterns
Example:
- .max_gas() + .gas(NearGas::from_tgas(5)) // Adjust based on actual gas profiling
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
core/src/accounts.rs(2 hunks)core/src/engine/mod.rs(3 hunks)core/src/engine/state/cached.rs(4 hunks)core/src/engine/state/deltas.rs(3 hunks)core/src/engine/state/mod.rs(3 hunks)core/src/events.rs(2 hunks)core/src/nonce/salted.rs(1 hunks)defuse/src/accounts.rs(1 hunks)defuse/src/contract/accounts/account/mod.rs(1 hunks)defuse/src/contract/accounts/mod.rs(2 hunks)defuse/src/contract/intents/state.rs(3 hunks)defuse/src/contract/salts.rs(1 hunks)defuse/src/salts.rs(1 hunks)tests/src/tests/defuse/accounts/mod.rs(4 hunks)tests/src/tests/defuse/accounts/nonces.rs(5 hunks)tests/src/tests/defuse/state/extensions/salt.rs(1 hunks)tests/src/tests/defuse/state/salt.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- defuse/src/salts.rs
- tests/src/tests/defuse/state/salt.rs
- core/src/events.rs
🧰 Additional context used
🧬 Code graph analysis (13)
defuse/src/accounts.rs (2)
defuse/src/contract/accounts/mod.rs (1)
cleanup_nonces(55-77)tests/src/tests/defuse/accounts/mod.rs (3)
cleanup_nonces(26-26)cleanup_nonces(90-112)cleanup_nonces(198-200)
core/src/engine/state/deltas.rs (7)
core/src/engine/state/cached.rs (3)
is_valid_salt(123-125)cleanup_nonce(182-192)cleanup_nonce(418-420)core/src/engine/state/mod.rs (2)
is_valid_salt(46-46)cleanup_nonce(65-65)core/src/nonce/salted.rs (1)
is_valid_salt(89-91)defuse/src/contract/intents/state.rs (2)
is_valid_salt(94-96)cleanup_nonce(132-141)defuse/src/contract/salts.rs (1)
is_valid_salt(55-57)defuse/src/salts.rs (1)
is_valid_salt(15-15)defuse/src/contract/accounts/account/mod.rs (1)
cleanup_nonce(156-158)
defuse/src/contract/intents/state.rs (6)
core/src/engine/state/cached.rs (3)
is_valid_salt(123-125)cleanup_nonce(182-192)cleanup_nonce(418-420)core/src/engine/state/deltas.rs (2)
is_valid_salt(101-103)cleanup_nonce(126-128)core/src/engine/state/mod.rs (2)
is_valid_salt(46-46)cleanup_nonce(65-65)core/src/nonce/salted.rs (1)
is_valid_salt(89-91)defuse/src/salts.rs (1)
is_valid_salt(15-15)defuse/src/contract/accounts/account/mod.rs (1)
cleanup_nonce(156-158)
tests/src/tests/defuse/accounts/nonces.rs (5)
core/src/nonce/salted.rs (2)
new(33-41)new(85-87)defuse/src/contract/state/mod.rs (1)
new(28-41)core/src/nonce/expirable.rs (1)
new(25-27)defuse/src/contract/intents/state.rs (1)
is_nonce_used(65-70)tests/src/tests/defuse/accounts/mod.rs (3)
is_nonce_used(41-41)is_nonce_used(139-148)is_nonce_used(223-225)
core/src/engine/state/cached.rs (5)
core/src/engine/state/deltas.rs (2)
is_valid_salt(101-103)cleanup_nonce(126-128)core/src/engine/state/mod.rs (2)
is_valid_salt(46-46)cleanup_nonce(65-65)core/src/nonce/salted.rs (1)
is_valid_salt(89-91)defuse/src/contract/intents/state.rs (2)
is_valid_salt(94-96)cleanup_nonce(132-141)defuse/src/contract/accounts/account/mod.rs (1)
cleanup_nonce(156-158)
core/src/engine/mod.rs (2)
core/src/payload/mod.rs (1)
nonce(68-70)core/src/nonce/versioned.rs (2)
try_from(31-33)try_from(39-57)
defuse/src/contract/accounts/mod.rs (8)
defuse/src/accounts.rs (1)
cleanup_nonces(35-35)borsh-utils/src/adapters/mod.rs (1)
into_inner(105-107)core/src/nonce/versioned.rs (2)
try_from(31-33)try_from(39-57)core/src/engine/state/cached.rs (2)
cleanup_nonce(182-192)cleanup_nonce(418-420)core/src/engine/state/deltas.rs (1)
cleanup_nonce(126-128)core/src/engine/state/mod.rs (1)
cleanup_nonce(65-65)defuse/src/contract/accounts/account/mod.rs (1)
cleanup_nonce(156-158)defuse/src/contract/intents/state.rs (1)
cleanup_nonce(132-141)
defuse/src/contract/salts.rs (8)
defuse/src/salts.rs (4)
rotate_salt(8-8)invalidate_salt(12-12)is_valid_salt(15-15)get_current_salt(18-18)tests/src/tests/defuse/state/extensions/salt.rs (12)
rotate_salt(6-6)rotate_salt(24-33)rotate_salt(72-74)invalidate_salt(8-12)invalidate_salt(35-49)invalidate_salt(76-84)is_valid_salt(14-18)is_valid_salt(51-61)is_valid_salt(86-94)get_current_salt(20-20)get_current_salt(63-68)get_current_salt(96-98)tests/src/tests/defuse/state/salt.rs (2)
rotate_salt(13-48)invalidate_salt(52-107)core/src/engine/state/cached.rs (1)
is_valid_salt(123-125)core/src/engine/state/deltas.rs (1)
is_valid_salt(101-103)core/src/engine/state/mod.rs (1)
is_valid_salt(46-46)core/src/nonce/salted.rs (1)
is_valid_salt(89-91)defuse/src/contract/intents/state.rs (1)
is_valid_salt(94-96)
tests/src/tests/defuse/accounts/mod.rs (2)
defuse/src/accounts.rs (1)
cleanup_nonces(35-35)defuse/src/contract/accounts/mod.rs (1)
cleanup_nonces(55-77)
tests/src/tests/defuse/state/extensions/salt.rs (3)
defuse/src/contract/salts.rs (4)
rotate_salt(13-26)invalidate_salt(30-52)is_valid_salt(55-57)get_current_salt(60-62)defuse/src/salts.rs (4)
rotate_salt(8-8)invalidate_salt(12-12)is_valid_salt(15-15)get_current_salt(18-18)tests/src/tests/defuse/state/salt.rs (2)
rotate_salt(13-48)invalidate_salt(52-107)
core/src/nonce/salted.rs (7)
core/src/engine/state/cached.rs (3)
new(33-38)new(368-370)is_valid_salt(123-125)core/src/engine/state/deltas.rs (3)
new(33-38)new(213-215)is_valid_salt(101-103)defuse/src/contract/state/mod.rs (1)
new(28-41)core/src/engine/state/mod.rs (1)
is_valid_salt(46-46)defuse/src/contract/intents/state.rs (1)
is_valid_salt(94-96)defuse/src/contract/salts.rs (1)
is_valid_salt(55-57)defuse/src/salts.rs (1)
is_valid_salt(15-15)
core/src/engine/state/mod.rs (7)
core/src/engine/state/cached.rs (3)
is_valid_salt(123-125)cleanup_nonce(182-192)cleanup_nonce(418-420)core/src/engine/state/deltas.rs (2)
is_valid_salt(101-103)cleanup_nonce(126-128)core/src/nonce/salted.rs (1)
is_valid_salt(89-91)defuse/src/contract/intents/state.rs (2)
is_valid_salt(94-96)cleanup_nonce(132-141)defuse/src/contract/salts.rs (1)
is_valid_salt(55-57)defuse/src/salts.rs (1)
is_valid_salt(15-15)defuse/src/contract/accounts/account/mod.rs (1)
cleanup_nonce(156-158)
defuse/src/contract/accounts/account/mod.rs (5)
core/src/engine/state/cached.rs (2)
cleanup_nonce(182-192)cleanup_nonce(418-420)core/src/engine/state/deltas.rs (1)
cleanup_nonce(126-128)core/src/engine/state/mod.rs (1)
cleanup_nonce(65-65)defuse/src/contract/intents/state.rs (1)
cleanup_nonce(132-141)core/src/payload/mod.rs (1)
nonce(68-70)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Build
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared security analysis / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Security Audit - report
🔇 Additional comments (36)
core/src/engine/state/mod.rs (1)
5-5: Import looks correct.
No issues with adding Salt here.core/src/accounts.rs (1)
7-7: Import looks correct.
No issues.core/src/engine/mod.rs (2)
9-12: Imports look good.
Matches usages below.
87-114: Clarify legacy nonce policy and add missing tests
- Document or feature-gate the migration window for accepting
Legacynonces.- Add unit tests covering:
• acceptance of legacy nonces
• invalid salt error
• expired nonce error
•intent_deadline > nonce_deadlineerrordefuse/src/contract/intents/state.rs (3)
2-2: LGTM - Salt import added.Correctly imports
Saltfromdefuse_coreto support the new salt validation functionality.
94-96: LGTM - Salt validation properly delegated.The
is_valid_saltmethod correctly delegates to thesaltsmanager, consistent with theStateViewtrait requirements.
132-140: LGTM - Per-nonce cleanup correctly implemented.The method correctly transitions from batch cleanup to per-nonce processing, calling
account.cleanup_nonce(nonce)for a single nonce as expected by the updated API.defuse/src/contract/accounts/account/mod.rs (1)
152-158: LGTM - Method renamed with updated documentation.The rename from
clear_expired_noncetocleanup_noncealigns with the broader refactoring to per-nonce cleanup semantics. The updated documentation accurately reflects that nonces are cleared if "expired/invalidated," which properly encompasses both expiration and salt invalidation scenarios introduced with versioned nonces.core/src/engine/state/cached.rs (4)
2-2: LGTM - Salt import added.Correctly adds
Saltto the crate imports to support the new salt validation functionality.
123-125: LGTM - Salt validation delegated to view.The
is_valid_saltmethod correctly delegates to the innerview, consistent with theStateViewtrait contract and the forwarding pattern used throughout this cached state implementation.
182-191: LGTM - Per-nonce cleanup correctly implemented.The method correctly transitions from batch
cleanup_expired_noncesto per-noncecleanup_nonce, processing a single nonce and callingaccount.cleanup_nonce(nonce)as expected by the updatedStatetrait.
418-420: LGTM - CachedAccount method renamed consistently.The rename from
clear_expired_noncetocleanup_noncemaintains consistency with the broader API refactoring while preserving the existing behavior through the unchanged delegation toself.nonces.clear_expired(n).core/src/engine/state/deltas.rs (3)
2-2: LGTM - Salt import added.Correctly adds
Saltto the crate imports to support the new salt validation functionality in theStateViewimplementation.
100-103: LGTM - Salt validation delegated to underlying state.The
is_valid_saltmethod correctly delegates toself.state, consistent with theDeltaswrapper pattern that forwards allStateViewoperations to the underlying state.
126-128: LGTM - Per-nonce cleanup correctly delegated.The method correctly transitions from batch cleanup to per-nonce processing, delegating
cleanup_nonceto the underlying state as expected by theDeltaswrapper pattern and the updatedStatetrait.defuse/src/contract/salts.rs (1)
11-26: Rotate salt flow looks solid (ACL + event + return).Please confirm clear_previous in invalidate_salt only permits clearing the previous salt (and not arbitrary historical salts). If broader invalidation is required, we may need a loop/guard in clear_previous implementation.
tests/src/tests/defuse/accounts/mod.rs (3)
26-26: API rename/signature looks consistent.
90-112: Marshalling B64 nonces and contract call LGTM.Correctly converts to Vec<AsBase64> and invokes cleanup_nonces on the defuse contract.
198-200: Wrapper delegation is correct.tests/src/tests/defuse/accounts/nonces.rs (12)
42-71: Legacy nonce acceptance check reads well.Covers the Legacy path and is_nonce_used assertion. Good baseline.
72-88: Invalid salt path exercised.Good negative test for random salt mismatch.
90-106: “Deadline greater than nonce” validation covered.Asserts protocol-level constraint; useful guard.
108-124: Expired nonce rejection covered.
126-149: Happy-path commit with current salt looks good.
151-183: Commit with previous salt after rotation is validated.Great coverage of “previous salt still valid” rule.
185-208: Commit with invalidated current salt rejected.Covers the invalidate_salt control path well.
212-237: Commit setup for cleanup_nonces test is comprehensive.
271-284: Expired nonce cleanup verifies removal.
286-313: Idempotency/skip cases are well covered.
315-337: Cleanup on invalidated salt is validated.Matches contract-side “invalid salt OR expired” cleanable rule.
351-362: Batch cleanup/gas measurement test is useful.Good coverage for chunked commits and a single cleanup batch.
Also applies to: 382-391
defuse/src/contract/accounts/mod.rs (1)
62-70: No action required — invalid-salt nonces are cleared
cleanup_nonceis only invoked when a nonce is expired or has an invalid salt, andclear_expiredunconditionally deletes that nonce. Invalid-salt nonces will therefore be removed as intended.core/src/nonce/salted.rs (2)
94-160: Well-structured test coverage.The test module provides good coverage of the salt lifecycle:
- Salt validity checks
- Rotation behavior (both old and new salts remain valid)
- Invalidation of previous salts
- SaltedNonce validation against multiple expected salts
The use of
rstestfixtures and property-based testing witharbitrarystrengthens the test suite.
89-91: Salt validation API distinctions are intentional.
SaltedNonce::is_valid_salt(&[Salt])checks a nonce’s salt against a list of allowed salts, whereasValidSalts::is_valid(&Salt)(and itsState::is_valid_salt(&Salt)wrappers) validate a single salt against stored valid salts. The differing signatures reflect these distinct use cases.tests/src/tests/defuse/state/extensions/salt.rs (2)
1-21: LGTM: Clean trait definition for test utilities.The
SaltManagerExttrait provides a clear async interface for salt management operations in integration tests, with proper error handling throughanyhow::Result.
71-99: LGTM: Clean delegation pattern.The
Contractimplementation appropriately delegates to theAccountimplementation through.as_account(), avoiding code duplication while providing a uniform interface for test utilities.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
core/src/accounts.rs (1)
67-73: Add serde_as attributes and Base64 encoding to align with NonceEvent pattern.
SaltRotationEventis missing the same serde attributes thatNonceEventuses (lines 45-58). Without these, the salt fields won't encode as Base64 in JSON serialization, causing ABI schema inconsistencies.Apply this diff to add the required attributes:
+#[cfg_attr( + all(feature = "abi", not(target_arch = "wasm32")), + serde_as(schemars = true) +)] +#[cfg_attr( + not(all(feature = "abi", not(target_arch = "wasm32"))), + serde_as(schemars = false) +)] +#[serde_as] #[must_use = "make sure to `.emit()` this event"] #[near(serializers = [json])] #[derive(Debug, Clone)] pub struct SaltRotationEvent { + #[serde_as(as = "Base64")] pub old_salt: Salt, + #[serde_as(as = "Base64")] pub new_salt: Salt, }
🧹 Nitpick comments (4)
defuse/src/contract/accounts/mod.rs (4)
57-64: Drop the unused error mapping; simplify the conversion.You don’t surface DefuseError::InvalidNonce here; map_err is redundant. Simpler and a touch cheaper:
- let Ok(versioned) = - VersionedNonce::try_from(nonce).map_err(|_| DefuseError::InvalidNonce) - else { - continue; - }; + let Ok(versioned) = VersionedNonce::try_from(nonce) else { + // invalid/unknown nonce encoding → skip + continue; + };
65-73: Encapsulate “cleanable” predicate for clarity and unit testing.Extract the match into a small helper (e.g., fn is_cleanable(&self, v: &VersionedNonce) -> bool). This improves readability and lets you unit‑test edge cases (salt invalid but not expired, expired but valid salt, legacy).
Quick check: ensure ExpirableNonce::has_expired() uses the same time base as NEAR env (block_timestamp in ns vs ms), or handles conversion internally.
74-78: Avoid fully swallowing errors; at least log or count unexpected ones.Ignoring all cleanup errors makes diagnosing data issues hard (e.g., AccountNotFound vs storage corruption). Consider counting outcomes and emitting a compact event/log, or only suppressing “benign” cases while noting the rest.
Example shape:
- if cleanable { - State::cleanup_nonce(self, &account_id, nonce).ok(); - } + if cleanable { + if let Err(e) = State::cleanup_nonce(self, &account_id, nonce) { + // Allow benign misses; record others. + if !matches!(e, DefuseError::AccountNotFound(_) | DefuseError::NonceNotFound) { + near_sdk::env::log_str(&format!("cleanup_nonces: failed {:?} for {}", e, account_id)); + } + } + }
55-79: Reduce repeated account lookups and deduplicate inputs to save gas.
- You call State::cleanup_nonce once per nonce, which re‑looks up the account each time. If layering allows, fetch the account lock once per account_id and call account.cleanup_nonce(nonce) in the inner loop.
- Deduplicate nonces per account before looping (HashSet/BTreeSet) to avoid redundant cleanups when callers pass duplicates.
Both are optional but can materially reduce storage reads on large batches.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/src/accounts.rs(2 hunks)core/src/events.rs(2 hunks)defuse/src/contract/accounts/mod.rs(2 hunks)defuse/src/contract/salts.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/events.rs
🧰 Additional context used
🧬 Code graph analysis (2)
defuse/src/contract/accounts/mod.rs (6)
defuse/src/accounts.rs (1)
cleanup_nonces(35-35)tests/src/tests/defuse/accounts/mod.rs (3)
cleanup_nonces(26-26)cleanup_nonces(90-112)cleanup_nonces(198-200)core/src/nonce/versioned.rs (2)
try_from(31-33)try_from(39-57)core/src/engine/state/mod.rs (1)
cleanup_nonce(65-65)core/src/engine/state/cached.rs (2)
cleanup_nonce(182-192)cleanup_nonce(418-420)defuse/src/contract/intents/state.rs (1)
cleanup_nonce(132-141)
defuse/src/contract/salts.rs (1)
defuse/src/salts.rs (4)
rotate_salt(8-8)invalidate_salt(12-12)is_valid_salt(15-15)get_current_salt(18-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests
🔇 Additional comments (3)
defuse/src/contract/salts.rs (2)
54-62: LGTM! Read-only accessors are well-structured.The
is_valid_saltandget_current_saltmethods correctly delegate to the underlying salt storage without mutation or unnecessary complexity. Theinlinehints are appropriate for these simple accessors.
33-41: ProvideValidSaltsmethods for verification.Please share the implementations of
set_new()andclear_previous()fromdefuse/src/contract/state/v0.rsso we can confirm that the old salt is added topreviousbefore being cleared.defuse/src/contract/accounts/mod.rs (1)
10-10: Imports align with versioned/salted nonces — OKThe expanded imports match the new nonce model and are used below. No issues.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
core/src/engine/mod.rs (1)
77-79: Invoke cleanup after committing the nonce to prevent stale state growth.After
commit_nonce, invokecleanup_noncefor the signer to purge expired entries. This prevents unbounded accumulation of expired nonces in storage.Apply this diff:
self.verify_intent_nonce(nonce, deadline)?; self.state.commit_nonce(signer_id.clone(), nonce)?; +self.state.cleanup_nonce(&signer_id, nonce)?;core/src/nonce/salted.rs (1)
19-29: Salt generation is predictable within the same block.
env::random_seed_array()returns the same value for all calls within a single block. Ifset_newis invoked multiple times in one transaction or block, all generated salts will be identical, undermining the rotation mechanism.Consider one of these approaches:
- Combine
random_seedwith additional entropy (e.g.,env::block_timestamp(), account ID, or a per-contract counter)- Document this limitation if the current behavior is acceptable for your threat model
- Use a deterministic counter combined with block hash
Example mixing entropy:
fn random() -> Self { const SIZE: usize = size_of::<Salt>(); - let seed = &env::random_seed_array()[..SIZE]; + let seed = env::random_seed_array(); + let timestamp = env::block_timestamp().to_le_bytes(); + let mut combined = [0u8; 32]; + for i in 0..32 { + combined[i] = seed[i] ^ timestamp[i % 8]; + } let mut result = [0u8; SIZE]; - - result.copy_from_slice(seed); + result.copy_from_slice(&combined[..SIZE]); Self(result) }core/src/accounts.rs (1)
67-81: Add Base64 encoding to salt event fields for ABI consistency.
RotateSaltEventandInvalidateSaltEventshould encode salt fields as Base64, matchingNonceEvent's pattern. This ensures consistent schema generation and serialization.Apply this diff:
+#[cfg_attr(all(feature = "abi", not(target_arch = "wasm32")), serde_as(schemars = true))] +#[cfg_attr(not(all(feature = "abi", not(target_arch = "wasm32"))), serde_as(schemars = false))] +#[serde_as] #[must_use = "make sure to `.emit()` this event"] #[near(serializers = [json])] #[derive(Debug, Clone)] pub struct RotateSaltEvent { + #[serde_as(as = "Base64")] pub old_salt: Salt, + #[serde_as(as = "Base64")] pub new_salt: Salt, } +#[cfg_attr(all(feature = "abi", not(target_arch = "wasm32")), serde_as(schemars = true))] +#[cfg_attr(not(all(feature = "abi", not(target_arch = "wasm32"))), serde_as(schemars = false))] +#[serde_as] #[must_use = "make sure to `.emit()` this event"] #[near(serializers = [json])] #[derive(Debug, Clone)] pub struct InvalidateSaltEvent { + #[serde_as(as = "Base64")] pub current: Salt, + #[serde_as(as = "Base64")] pub invalidated: Salt, }Based on coding guidelines
defuse/src/contract/accounts/mod.rs (2)
58-65: Avoid duplicating nonce validation logic; extract a shared helperLogic to decide cleanable (salt validity/expiry) duplicates engine verification. Extract into a StateView extension/helper and reuse here and in engine.
If you want, I can draft a StateViewExt with is_valid_nonce(...) and update both call sites.
55-73: Add 1‑yocto guard to public state‑changing cleanup_noncesThis method mutates state and should be guarded to prevent accidental cross‑contract calls. Mark #[payable] and assert_one_yocto() at entry.
Apply this diff:
- fn cleanup_nonces(&mut self, nonces: Vec<(AccountId, Vec<AsBase64<Nonce>>)>) { + #[payable] + fn cleanup_nonces(&mut self, nonces: Vec<(AccountId, Vec<AsBase64<Nonce>>)>) { + assert_one_yocto();core/src/nonce/versioned.rs (1)
73-77: Fix legacy deserialization: wrap prefix with Cursor before chaining
[u8; 4]doesn’t implement Read. Use io::Cursor to prepend the consumed bytes.Apply this diff:
- let versioned = if prefix == Self::VERSIONED_MAGIC_PREFIX { - VersionedNonce::deserialize_reader(reader)? - } else { - VersionedNonce::Legacy(Nonce::deserialize_reader(&mut prefix.chain(reader))?) - }; + let versioned = if prefix == Self::VERSIONED_MAGIC_PREFIX { + VersionedNonce::deserialize_reader(reader)? + } else { + let mut chained = io::Cursor::new(prefix).chain(reader); + VersionedNonce::Legacy(Nonce::deserialize_reader(&mut chained)?) + };
🧹 Nitpick comments (4)
core/src/engine/state/cached.rs (1)
182-191: Single‑nonce cleanup implementation is sound; consider surfacing resultLogic mirrors defuse intent state; returns Ok(()) regardless of whether anything changed. Optional: log/metric (or return bool upstream) to aid observability/debugging.
Would you like me to add a debug log counter for successful/failed nonce cleanups?
core/src/nonce/versioned.rs (1)
105-115: Stabilize test to avoid rare magic‑prefix collisionA random Nonce has a 1/2^32 chance to match VERSIONED_MAGIC_PREFIX, flipping Legacy→V1 parsing. Force a non‑matching prefix in the fixture.
Apply this diff:
- let nonce: Nonce = u.arbitrary().unwrap(); + let mut nonce: Nonce = u.arbitrary().unwrap(); + if nonce[..4] == MaybeVersionedNonce::VERSIONED_MAGIC_PREFIX { + nonce[0] ^= 0x01; // ensure it’s not interpreted as V1 + }tests/src/tests/defuse/accounts/nonces.rs (2)
55-56: Consider using more entropy or a fixed seed for test reproducibility.The test generates
SigningStandardusing only 1 byte of randomness (rng.random::<[u8; 1]>()), which limits the variability. For better test coverage, consider using more random bytes or document why 1 byte is sufficient. Alternatively, for reproducibility, consider using a fixed seed or deterministic SigningStandard selection.Also applies to: 81-82, 99-100, 117-118, 135-136, 169-170, 200-201
342-393: LGTM! Appropriate performance test for batch cleanup.The parameterized test correctly validates cleanup scalability by testing with 1, 10, and 100 nonces. The chunked commit approach (CHUNK_SIZE=10) avoids transaction size limits, and gas measurement provides useful performance data.
Consider documenting why
CHUNK_SIZE = 10was chosen (e.g., transaction size limits, gas constraints) for better maintainability:- const CHUNK_SIZE: usize = 10; + // Chunk size chosen to stay within transaction size/gas limits + const CHUNK_SIZE: usize = 10;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
core/src/accounts.rs(2 hunks)core/src/engine/mod.rs(3 hunks)core/src/engine/state/cached.rs(4 hunks)core/src/engine/state/deltas.rs(3 hunks)core/src/engine/state/mod.rs(3 hunks)core/src/events.rs(2 hunks)core/src/nonce/mod.rs(1 hunks)core/src/nonce/salted.rs(1 hunks)core/src/nonce/versioned.rs(1 hunks)defuse/src/contract/accounts/account/mod.rs(1 hunks)defuse/src/contract/accounts/account/nonces.rs(5 hunks)defuse/src/contract/accounts/mod.rs(2 hunks)defuse/src/contract/intents/state.rs(3 hunks)defuse/src/contract/mod.rs(4 hunks)defuse/src/contract/salts.rs(1 hunks)defuse/src/contract/state/mod.rs(4 hunks)defuse/src/contract/state/versioned.rs(1 hunks)defuse/src/salts.rs(1 hunks)tests/src/tests/defuse/accounts/nonces.rs(5 hunks)tests/src/tests/defuse/state/extensions/salt.rs(1 hunks)tests/src/tests/defuse/state/salt.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/src/tests/defuse/state/salt.rs
- core/src/events.rs
- defuse/src/contract/salts.rs
- core/src/engine/state/mod.rs
- core/src/nonce/mod.rs
- defuse/src/contract/intents/state.rs
🧰 Additional context used
🧬 Code graph analysis (13)
defuse/src/salts.rs (2)
defuse/src/contract/salts.rs (4)
rotate_salt(17-30)invalidate_salt(34-52)is_valid_salt(55-57)get_current_salt(60-62)tests/src/tests/defuse/state/extensions/salt.rs (12)
rotate_salt(6-10)rotate_salt(28-42)rotate_salt(81-89)invalidate_salt(12-16)invalidate_salt(44-58)invalidate_salt(91-99)is_valid_salt(18-22)is_valid_salt(60-70)is_valid_salt(101-109)get_current_salt(24-24)get_current_salt(72-77)get_current_salt(111-113)
core/src/engine/state/cached.rs (6)
core/src/engine/state/deltas.rs (2)
is_valid_salt(101-103)cleanup_nonce(126-128)core/src/engine/state/mod.rs (2)
is_valid_salt(46-46)cleanup_nonce(65-65)defuse/src/contract/intents/state.rs (2)
is_valid_salt(94-96)cleanup_nonce(132-141)defuse/src/contract/salts.rs (1)
is_valid_salt(55-57)defuse/src/salts.rs (1)
is_valid_salt(17-17)defuse/src/contract/accounts/account/mod.rs (1)
cleanup_nonce(156-158)
defuse/src/contract/state/mod.rs (4)
core/src/nonce/salted.rs (2)
new(51-59)new(109-111)defuse/src/contract/accounts/account/mod.rs (1)
new(48-65)defuse/src/contract/accounts/mod.rs (1)
new(106-115)defuse/src/contract/mod.rs (1)
new(88-97)
core/src/engine/mod.rs (2)
core/src/nonce/versioned.rs (2)
from(31-33)from(39-55)core/src/nonce/salted.rs (1)
from(32-36)
tests/src/tests/defuse/state/extensions/salt.rs (3)
defuse/src/contract/salts.rs (4)
rotate_salt(17-30)invalidate_salt(34-52)is_valid_salt(55-57)get_current_salt(60-62)defuse/src/salts.rs (4)
rotate_salt(9-9)invalidate_salt(14-14)is_valid_salt(17-17)get_current_salt(20-20)tests/src/tests/defuse/state/salt.rs (2)
rotate_salt(13-69)invalidate_salt(73-128)
core/src/nonce/salted.rs (3)
core/src/nonce/versioned.rs (2)
from(31-33)from(39-55)defuse/src/contract/state/v0.rs (1)
from(21-38)defuse/src/contract/state/mod.rs (1)
new(28-41)
tests/src/tests/defuse/accounts/nonces.rs (4)
core/src/nonce/salted.rs (2)
new(51-59)new(109-111)defuse/src/contract/state/mod.rs (1)
new(28-41)defuse/src/contract/intents/state.rs (1)
is_nonce_used(65-70)tests/src/tests/defuse/accounts/mod.rs (3)
is_nonce_used(41-41)is_nonce_used(139-148)is_nonce_used(223-225)
defuse/src/contract/accounts/mod.rs (6)
defuse/src/accounts.rs (1)
cleanup_nonces(35-35)core/src/nonce/versioned.rs (2)
from(31-33)from(39-55)core/src/nonce/salted.rs (1)
from(32-36)core/src/engine/state/cached.rs (2)
cleanup_nonce(182-192)cleanup_nonce(418-420)core/src/engine/state/deltas.rs (1)
cleanup_nonce(126-128)core/src/engine/state/mod.rs (1)
cleanup_nonce(65-65)
defuse/src/contract/state/versioned.rs (1)
core/src/nonce/versioned.rs (5)
size_of(41-41)from(31-33)from(39-55)deserialize_as(66-80)serialize_as(84-92)
defuse/src/contract/accounts/account/mod.rs (5)
core/src/engine/state/cached.rs (2)
cleanup_nonce(182-192)cleanup_nonce(418-420)core/src/engine/state/deltas.rs (1)
cleanup_nonce(126-128)core/src/engine/state/mod.rs (1)
cleanup_nonce(65-65)defuse/src/contract/intents/state.rs (1)
cleanup_nonce(132-141)core/src/payload/mod.rs (1)
nonce(68-70)
defuse/src/contract/accounts/account/nonces.rs (1)
core/src/nonce/mod.rs (2)
cleanup(50-54)new(31-33)
core/src/nonce/versioned.rs (3)
core/src/nonce/salted.rs (5)
size_of(21-21)from(32-36)random(20-28)new(51-59)new(109-111)defuse/src/contract/state/versioned.rs (6)
size_of(72-72)from(41-46)from(51-54)from(59-61)deserialize_as(65-87)serialize_as(94-104)core/src/nonce/expirable.rs (1)
new(25-27)
core/src/engine/state/deltas.rs (6)
core/src/engine/state/cached.rs (3)
is_valid_salt(123-125)cleanup_nonce(182-192)cleanup_nonce(418-420)core/src/engine/state/mod.rs (2)
is_valid_salt(46-46)cleanup_nonce(65-65)defuse/src/contract/intents/state.rs (2)
is_valid_salt(94-96)cleanup_nonce(132-141)defuse/src/contract/salts.rs (1)
is_valid_salt(55-57)defuse/src/salts.rs (1)
is_valid_salt(17-17)defuse/src/contract/accounts/account/mod.rs (1)
cleanup_nonce(156-158)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Build
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared security analysis / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Security Audit - report
🔇 Additional comments (20)
core/src/engine/mod.rs (1)
87-111: Nonce verification logic looks correct for V1 salted nonces.The pre-commit verification correctly:
- Validates the salt via
is_valid_salt- Ensures intent deadline ≤ nonce deadline
- Checks nonce expiration
- Allows legacy nonces without checks
core/src/nonce/salted.rs (1)
48-93: ValidSalts logic correctly manages current and previous salts.The rotation and invalidation logic is sound:
set_newrotates the current salt and optionally invalidates the previous oneinvalidatehandles both current and previous saltsis_validchecks both current and previous valid saltsdefuse/src/salts.rs (1)
1-21: SaltManager trait API is well-designed.The trait provides a clean public interface for salt lifecycle management with clear method names and documentation. The cross-contract annotation enables proper external contract calls.
tests/src/tests/defuse/state/extensions/salt.rs (1)
1-114: Test extension trait follows established patterns.
SaltManagerExtcorrectly implements the test helper pattern:
- Async methods for all SaltManager operations
- Proper deposit (1 yoctoNear) for state-changing methods
- Max gas allocation for transactions
- Contract implementation delegates to Account
- View methods use
.view()without depositdefuse/src/contract/accounts/account/mod.rs (1)
152-158: Method rename aligns with codebase conventions.Renaming
clear_expired_noncetocleanup_nonceimproves clarity and consistency with the broader cleanup semantics introduced in this PR. The documentation correctly reflects the expanded scope (expired/invalidated nonces).defuse/src/contract/state/mod.rs (1)
1-49: v0 migration prefix nesting is consistent
Confirmed thatValidSalts::new(ContractPrefix::State.into_storage_key().nest(StatePrefix::Salts))mirrors the.as_slice().nest(Prefix::Salts)pattern inmod.rs.core/src/engine/state/cached.rs (2)
122-125: Forwarder for is_valid_salt looks correctMatches StateView trait and delegates to inner view. No issues.
418-420: CachedAccount::cleanup_nonce rename and delegation LGTMRenamed API and delegation to Nonces::cleanup is consistent across layers.
defuse/src/contract/accounts/account/nonces.rs (1)
72-74: Rename to cleanup and test updates are consistentDelegation to Nonces::cleanup and tests (including legacy nonces not cleanable) align with intended semantics.
Also applies to: 132-147, 199-209
core/src/engine/state/deltas.rs (2)
100-103: is_valid_salt delegation LGTMStraightforward forwarder to inner state view.
126-128: cleanup_nonce delegation LGTMShifts to single‑nonce cleanup as per updated trait; correct delegation.
defuse/src/contract/mod.rs (1)
9-9: Versioned state (de)serialization wiring is correctUsing As on Contract.state matches the provided BorshSerializeAs/BorshDeserializeAs impls. Module exposure and SaltManager role addition look good.
Also applies to: 16-16, 30-31, 71-75
tests/src/tests/defuse/accounts/nonces.rs (8)
1-27: LGTM! Imports align with salted nonce testing requirements.The updated imports correctly expose the necessary types (
Salt,SaltedNonce,VersionedNonce,Role) and trait extensions (SaltManagerExt,AclExt) to support the new salted/versioned nonce test scenarios.
29-38: LGTM! Helper function correctly constructs versioned salted nonces.The helper appropriately creates a
VersionedNonce::V1containing aSaltedNoncewith the provided salt, deadline, and a random 15-byte nonce, reducing code duplication across test cases.
153-185: LGTM! Correctly tests salt rotation with old salt preservation.The test appropriately grants
SaltManagerrole, rotates the salt withpreserve_old=false, and verifies that nonces signed with the previous salt remain valid. This validates the salt lifecycle management.
187-209: LGTM! Correctly tests that invalidated salts are rejected.The test appropriately invalidates the current salt and verifies that nonces signed with the invalidated salt are rejected with an "invalid salt" error.
273-286: LGTM! Correctly tests cleanup of expired nonces.The test appropriately waits for the deadline to pass, calls
cleanup_nonces, and verifies the expired nonce is no longer marked as used.
288-315: LGTM! Comprehensive edge case coverage for cleanup skipping.The test correctly verifies that
cleanup_noncesskips:
- Already cleaned nonces (reusing
expirable_noncefrom the previous block)- Legacy nonces (non-expirable)
- Non-expired nonces
- Nonces for non-existent users
This ensures the cleanup logic is robust and handles various edge cases gracefully.
73-90: Salt construction is correct. Salt wraps a fixed[u8; 4]and itsFrom<&[u8]>impl copies exactly four bytes, so providing a 4-byte slice is safe and won’t fail.
317-339: cleanup_nonces intentionally clears nonces with invalidated salts. Implementation uses!is_valid_salt(salt) || deadline.has_expired(), so removing non-expired nonces for invalid salts is intended and this test is correct.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
defuse/src/contract/accounts/mod.rs (1)
55-55: Enforce the 1 yocto guard oncleanup_nonces_by_prefix.This public, state-changing method must be marked
#[payable]and begin withassert_one_yocto()to prevent accidental cross-contract calls. This was flagged as a critical issue in a previous review and remains unresolved.Apply this diff to add the guard:
+ #[payable] fn cleanup_nonces_by_prefix(&mut self, nonces: Vec<(AccountId, Vec<AsBase64<Nonce>>)>) { + assert_one_yocto(); for (account_id, nonces) in nonces {Additionally, update all test call sites in
tests/src/tests/defuse/accounts/…to attach a 1 yocto deposit when callingcleanup_nonces_by_prefix.
🧹 Nitpick comments (6)
defuse/src/contract/accounts/mod.rs (2)
58-65: Consider extracting nonce validation logic into a reusable trait method.The cleanability check duplicates logic found in
Engine::verify_intent_nonce(). A previous reviewer suggested unifying this via aStateViewExttrait with anis_valid_noncemethod that both locations can call.Consider this pattern:
pub trait StateViewExt: StateView { fn is_valid_nonce(&self, nonce: Nonce) -> bool { match VersionedNonce::from(nonce) { VersionedNonce::V1(SaltedNonce { salt, nonce: ExpirableNonce { deadline, .. }, }) => !deadline.has_expired() && self.is_valid_salt(salt), VersionedNonce::Legacy(_) => true, } } } impl<T> StateViewExt for T where T: StateView {}Then simplify this code to:
let cleanable = !self.is_valid_nonce(nonce);
67-74: Document why errors are intentionally suppressed.The comment "all errors are omitted" (line 71) without explanation may confuse maintainers. Clarify which error cases are expected (e.g.,
AccountNotFound, already-cleaned nonces) and why silent failure is acceptable here.Consider expanding the comment:
- // NOTE: all errors are omitted + // NOTE: Silently ignore errors (AccountNotFound, already-cleaned nonces, etc.) + // as this is a permissionless cleanup method that should not fail on partial successdefuse/src/contract/salts.rs (2)
33-36: Silent filtering of invalid salts may hide caller mistakes.The
filteron line 35 silently drops salts that are already invalid or don't exist. A caller expecting all provided salts to be invalidated won't receive feedback if some were already gone.If callers need to know which salts failed, consider one of these approaches:
- Return a tuple
(Salt, Vec<Salt>)where the second element lists successfully invalidated salts- Log a warning for salts that couldn't be invalidated
- Add a separate query method to check salt validity before invalidation
Example for approach 1:
- fn invalidate_salts(&mut self, salts: Vec<Salt>) -> Salt; + fn invalidate_salts(&mut self, salts: Vec<Salt>) -> (Salt, Vec<Salt>);let invalidated = salts .into_iter() .filter(|s| self.salts.invalidate(*s)) .collect(); let current = self.salts.current(); SaltRotationEvent { current, invalidated, } .emit(); - current + (current, invalidated)
17-18: Document the return value semantics ofset_new.According to a previous comment,
set_newreturns the previous salt, but the return value is discarded here. This could confuse readers who expect it to return the new salt.Add a comment to clarify:
- self.salts.set_new(false); + self.salts.set_new(false); // Returns previous salt (unused here) let current = self.salts.current();Or if the return value is never needed in this context, consider requesting a
set_newvariant that returns nothing to make the intent clearer.defuse/src/contract/accounts/account/mod.rs (1)
152-154: Fix grammar in comment.The comment contains a grammatical error: "the all nonces" should be "all nonces".
Apply this diff:
- /// Clears the all nonces with corresponding prefix if it was expired/invalidated. + /// Clears all nonces with the corresponding prefix if they were expired/invalidated.core/src/nonce/versioned.rs (1)
92-102: Stabilize test: handle rare V1 prefix collision.A random 32-byte
Noncehas a ~1/2^32 chance of starting withVERSIONED_MAGIC_PREFIX, which would cause the test to expectVersionedNonce::V1instead ofLegacy. While extremely rare, this could make the test flaky.Apply this diff to ensure the generated nonce doesn't match the V1 prefix:
fn legacy_roundtrip_layout(random_bytes: Vec<u8>) { let mut u = Unstructured::new(&random_bytes); - let nonce: Nonce = u.arbitrary().unwrap(); + let mut nonce: Nonce = u.arbitrary().unwrap(); + // Ensure nonce doesn't start with V1_MAGIC_PREFIX to avoid rare collision + if nonce[..4] == MaybeVersionedNonce::VERSIONED_MAGIC_PREFIX { + nonce[0] ^= 0x01; + } let expected = VersionedNonce::from(nonce); assert_eq!(expected, VersionedNonce::Legacy(nonce));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
bitmap/src/lib.rs(1 hunks)core/src/engine/state/cached.rs(4 hunks)core/src/engine/state/deltas.rs(3 hunks)core/src/engine/state/mod.rs(3 hunks)core/src/nonce/mod.rs(1 hunks)core/src/nonce/salted.rs(1 hunks)core/src/nonce/versioned.rs(1 hunks)defuse/src/contract/accounts/account/mod.rs(2 hunks)defuse/src/contract/accounts/account/nonces.rs(6 hunks)defuse/src/contract/accounts/mod.rs(3 hunks)defuse/src/contract/intents/state.rs(3 hunks)defuse/src/contract/salts.rs(1 hunks)defuse/src/contract/state/mod.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- defuse/src/contract/intents/state.rs
- core/src/nonce/salted.rs
- core/src/nonce/mod.rs
🧰 Additional context used
🧬 Code graph analysis (10)
defuse/src/contract/accounts/account/mod.rs (4)
core/src/engine/state/cached.rs (2)
cleanup_nonce_by_prefix(182-196)cleanup_nonce_by_prefix(422-424)core/src/engine/state/deltas.rs (1)
cleanup_nonce_by_prefix(126-132)core/src/engine/state/mod.rs (1)
cleanup_nonce_by_prefix(65-69)defuse/src/contract/intents/state.rs (1)
cleanup_nonce_by_prefix(132-145)
defuse/src/contract/state/mod.rs (3)
core/src/nonce/salted.rs (2)
new(105-113)new(163-165)defuse/src/contract/mod.rs (1)
new(99-108)defuse/src/contract/state/v0.rs (1)
new(50-62)
core/src/engine/state/deltas.rs (6)
core/src/engine/state/cached.rs (3)
is_valid_salt(123-125)cleanup_nonce_by_prefix(182-196)cleanup_nonce_by_prefix(422-424)core/src/engine/state/mod.rs (2)
is_valid_salt(46-46)cleanup_nonce_by_prefix(65-69)defuse/src/contract/intents/state.rs (2)
is_valid_salt(94-96)cleanup_nonce_by_prefix(132-145)defuse/src/contract/salts.rs (1)
is_valid_salt(50-52)defuse/src/salts.rs (1)
is_valid_salt(17-17)defuse/src/contract/accounts/account/mod.rs (1)
cleanup_nonce_by_prefix(156-158)
bitmap/src/lib.rs (2)
core/src/nonce/mod.rs (1)
cleanup_by_prefix(51-53)defuse/src/contract/accounts/account/nonces.rs (1)
cleanup_by_prefix(72-74)
core/src/engine/state/mod.rs (7)
core/src/engine/state/cached.rs (3)
is_valid_salt(123-125)cleanup_nonce_by_prefix(182-196)cleanup_nonce_by_prefix(422-424)core/src/engine/state/deltas.rs (2)
is_valid_salt(101-103)cleanup_nonce_by_prefix(126-132)defuse/src/contract/intents/state.rs (2)
is_valid_salt(94-96)cleanup_nonce_by_prefix(132-145)defuse/src/contract/salts.rs (1)
is_valid_salt(50-52)tests/src/tests/defuse/state/extensions/salt.rs (3)
is_valid_salt(14-18)is_valid_salt(51-61)is_valid_salt(88-96)defuse/src/salts.rs (1)
is_valid_salt(17-17)defuse/src/contract/accounts/account/mod.rs (1)
cleanup_nonce_by_prefix(156-158)
core/src/engine/state/cached.rs (6)
core/src/engine/state/deltas.rs (2)
is_valid_salt(101-103)cleanup_nonce_by_prefix(126-132)core/src/engine/state/mod.rs (2)
is_valid_salt(46-46)cleanup_nonce_by_prefix(65-69)defuse/src/contract/intents/state.rs (2)
is_valid_salt(94-96)cleanup_nonce_by_prefix(132-145)defuse/src/contract/salts.rs (1)
is_valid_salt(50-52)defuse/src/salts.rs (1)
is_valid_salt(17-17)defuse/src/contract/accounts/account/mod.rs (1)
cleanup_nonce_by_prefix(156-158)
defuse/src/contract/salts.rs (3)
tests/src/tests/defuse/state/extensions/salt.rs (12)
update_current_salt(6-6)update_current_salt(24-33)update_current_salt(72-76)invalidate_salts(8-12)invalidate_salts(35-49)invalidate_salts(78-86)is_valid_salt(14-18)is_valid_salt(51-61)is_valid_salt(88-96)current_salt(20-20)current_salt(63-68)current_salt(98-100)defuse/src/salts.rs (4)
update_current_salt(9-9)invalidate_salts(14-14)is_valid_salt(17-17)current_salt(20-20)core/src/nonce/salted.rs (3)
current(144-146)new(105-113)new(163-165)
core/src/nonce/versioned.rs (2)
core/src/nonce/salted.rs (5)
size_of(27-27)from(179-183)random(26-34)new(105-113)new(163-165)core/src/nonce/expirable.rs (1)
new(25-27)
defuse/src/contract/accounts/account/nonces.rs (2)
bitmap/src/lib.rs (1)
cleanup_by_prefix(45-47)core/src/nonce/mod.rs (1)
cleanup_by_prefix(51-53)
defuse/src/contract/accounts/mod.rs (9)
defuse/src/accounts.rs (1)
cleanup_nonces_by_prefix(35-35)tests/src/tests/defuse/accounts/mod.rs (3)
cleanup_nonces_by_prefix(26-29)cleanup_nonces_by_prefix(93-118)cleanup_nonces_by_prefix(204-209)core/src/nonce/salted.rs (1)
from(179-183)core/src/nonce/versioned.rs (2)
from(28-30)from(36-44)core/src/engine/state/cached.rs (2)
cleanup_nonce_by_prefix(182-196)cleanup_nonce_by_prefix(422-424)core/src/engine/state/deltas.rs (1)
cleanup_nonce_by_prefix(126-132)core/src/engine/state/mod.rs (1)
cleanup_nonce_by_prefix(65-69)defuse/src/contract/accounts/account/mod.rs (1)
cleanup_nonce_by_prefix(156-158)defuse/src/contract/intents/state.rs (1)
cleanup_nonce_by_prefix(132-145)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Security Audit - deny
- GitHub Check: Build
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared security analysis / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Security Audit - report
🔇 Additional comments (14)
bitmap/src/lib.rs (1)
45-47: LGTM—internal API usage is consistent. Ranrg -n "clear_by_prefix": no results; all callers usecleanup_by_prefix. As this is a public API change, ensure any external/downstream crates are updated to call the renamed method.defuse/src/contract/salts.rs (2)
14-27: Clarify thatupdate_current_saltdoes not invalidate the previous salt.The method calls
set_new(false)and emits an event with an emptyinvalidatedset, meaning the previous salt remains valid. This is correct behavior, but the method name alone doesn't convey this nuance.Consider adding a doc comment to make the behavior explicit:
/// Rotates to a new current salt without invalidating the previous one. /// /// Returns the newly generated salt. #[access_control_any(roles(Role::DAO, Role::SaltManager))] #[payable] fn update_current_salt(&mut self) -> Salt {Note: A previous reviewer suggested adding an
invalidate_current: boolparameter to allow optional invalidation. If this flexibility is needed in the future, consider that enhancement.
40-44: LGTM: Event emission correctly reflects the operation.The
SaltRotationEventwithcurrentandinvalidatedfields accurately represents the state change frominvalidate_salts. This addresses the concern from a previous review about unclear event semantics.defuse/src/contract/accounts/mod.rs (1)
72-73: Nonce prefix extraction is correct. The pattern[prefix @ .., _]yields a 31-byte slice, matchingNoncePrefix = U248, andcleanup_nonce_by_prefixoperates on the expected prefix length.defuse/src/contract/accounts/account/nonces.rs (1)
72-74: LGTM!The method signature change is consistent with the broader prefix-based cleanup refactor, and the implementation correctly delegates to the underlying nonces cleanup method.
core/src/nonce/versioned.rs (1)
26-30: Verify unreachable assumptions hold.The
unreachable!()assumes that deserializing any 32-byteNonceas aVersionedNoncenever fails. This is safe because:
- Legacy path: any 32 bytes not starting with the magic prefix deserialize as
Legacy(Nonce)- Versioned path: bytes starting with the magic prefix deserialize as the versioned format
However, ensure that deserialization errors from malformed versioned nonces are properly handled elsewhere in the codebase, as this conversion would panic on such input.
core/src/engine/state/mod.rs (2)
45-46: LGTM!The addition of
is_valid_saltto theStateViewtrait is consistent with the salted nonce implementation and provides the necessary validation interface.
65-69: Engine-levelcommit_noncedefers cleanup to the contract layer
Cleanup is invoked in defuse/src/contract/accounts/mod.rs:73 immediately aftercommit_nonce, so expired nonces are purged per transaction.Likely an incorrect or invalid review comment.
defuse/src/contract/state/mod.rs (1)
39-39: Salt prefix nesting is consistent. Both v0 migration and the new state use.nest(Prefix::Salts)to initializeSaltRegistry, so no changes are required.core/src/engine/state/deltas.rs (2)
100-103: LGTM!The delegation to the underlying state is correct for salt validation in the wrapper pattern.
126-131: LGTM!The refactored single-prefix cleanup approach aligns with the PR's nonce versioning objectives. The delegation is straightforward and correct.
core/src/engine/state/cached.rs (3)
123-125: LGTM!Correct delegation through the view layer for salt validation.
182-193: LGTM!The refactored cleanup method correctly delegates to the account level, and the use of
to_owned()on line 190 is idiomatic for borrowed types.
422-423: LGTM!The method signature now uses
NoncePrefixfor explicit typing, addressing previous review feedback. The delegation to the nonces collection is correct.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
core/src/nonce/salted.rs (4)
29-42: Compile fix: qualify size_of.size_of isn’t in scope. Use mem::size_of::().
Apply:
- const SIZE: usize = size_of::<Salt>(); + const SIZE: usize = mem::size_of::<Salt>();
216-222: Tests: set testing env before SaltRegistry::new to prevent panic.Seed the VM before constructing SaltRegistry.
- fn contains_salt_test(random_bytes: Vec<u8>) { - let random_salt: Salt = Unstructured::new(&random_bytes).arbitrary().unwrap(); - let salts = SaltRegistry::new(random_bytes); + fn contains_salt_test(random_bytes: Vec<u8>, mut rng: impl Rng) { + let random_salt: Salt = Unstructured::new(&random_bytes).arbitrary().unwrap(); + let _ = set_random_seed(&mut rng); + let salts = SaltRegistry::new(random_bytes);
239-261: Tests: seed VM before constructing registry (reset_salt_test).Same panic risk; move seeding first.
- fn reset_salt_test(random_bytes: Vec<u8>, mut rng: impl Rng) { - let mut salts = SaltRegistry::new(random_bytes); + fn reset_salt_test(random_bytes: Vec<u8>, mut rng: impl Rng) { + let _ = set_random_seed(&mut rng); + let mut salts = SaltRegistry::new(random_bytes);
225-237: Tests: seed VM before constructing registry.Avoid early env access panics by moving set_random_seed earlier.
- fn update_current_salt_test(random_bytes: Vec<u8>, mut rng: impl Rng) { - let mut salts = SaltRegistry::new(random_bytes); - - let current = set_random_seed(&mut rng); + fn update_current_salt_test(random_bytes: Vec<u8>, mut rng: impl Rng) { + let current = set_random_seed(&mut rng); + let mut salts = SaltRegistry::new(random_bytes);defuse/src/contract/accounts/mod.rs (1)
55-55: Enforce a 1 yocto guard oncleanup_nonces.This public, state-changing method must be marked
#[payable]and begin withassert_one_yocto()to prevent accidental cross-contract calls or malicious invocations without attached deposits.Apply this diff:
+ #[payable] fn cleanup_nonces(&mut self, nonces: Vec<(AccountId, Vec<AsBase64<Nonce>>)>) { + assert_one_yocto(); for (account_id, nonces) in nonces {
🧹 Nitpick comments (3)
core/src/nonce/salted.rs (1)
32-39: Salt predictability and collision bounds; add entropy/counter.Derivation uses only block seed + 1-byte attempt; predictable within a block and capped to 256 candidates. Consider mixing a persisted rotation counter and/or timestamp/caller into the hash to improve uniqueness and unpredictability, e.g., sha256(seed || counter || attempt) with counter stored in state.
If desired, I can draft an update using a u64 counter persisted alongside current/previous salts.
defuse/src/contract/salts.rs (1)
32-49: Graceful invalidation with event emission is good; minor note on unwrap_or_panic.Filtering by is_ok() is fine. Consider propagating SaltGenerationFailed instead of unwrap_or_panic in update_current_salt for resilience, if you expect extreme rotations.
defuse/src/contract/accounts/mod.rs (1)
58-65: Simplify pattern matching for legacy nonces.The
falseassignment for legacy nonces followed by anif !cleanable { continue; }check is verbose. Applying the early-continue pattern directly in the match arm improves clarity.Based on learnings from past reviews, apply this diff:
let cleanable = match VersionedNonce::from(nonce) { VersionedNonce::V1(SaltedNonce { salt, nonce: ExpirableNonce { deadline, .. }, - }) => deadline.has_expired() || !self.is_valid_salt(salt), - // NOTE: legacy nonces can't be cleared before a complete prohibition on its usage - VersionedNonce::Legacy(_) => false, + }) => deadline.has_expired() || !self.is_valid_salt(salt), + VersionedNonce::Legacy(_) => continue, }; - if !cleanable { - continue; - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.md(1 hunks)core/src/error.rs(2 hunks)core/src/nonce/salted.rs(1 hunks)defuse/src/accounts.rs(1 hunks)defuse/src/contract/accounts/mod.rs(3 hunks)defuse/src/contract/salts.rs(1 hunks)tests/src/tests/defuse/accounts/mod.rs(4 hunks)tests/src/tests/defuse/accounts/nonces.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
🧬 Code graph analysis (6)
tests/src/tests/defuse/accounts/mod.rs (2)
defuse/src/accounts.rs (1)
cleanup_nonces(35-35)defuse/src/contract/accounts/mod.rs (1)
cleanup_nonces(55-76)
defuse/src/contract/salts.rs (4)
defuse/src/salts.rs (4)
update_current_salt(9-9)invalidate_salts(14-14)is_valid_salt(17-17)current_salt(20-20)core/src/nonce/salted.rs (3)
current(158-160)new(113-121)new(177-179)defuse/src/contract/state/mod.rs (1)
new(27-41)defuse/src/contract/intents/state.rs (1)
is_valid_salt(94-96)
tests/src/tests/defuse/accounts/nonces.rs (4)
test-utils/src/random.rs (3)
random(82-84)random_bytes(143-151)rng(138-140)defuse/src/contract/salts.rs (1)
current_salt(58-60)tests/src/tests/defuse/state/extensions/salt.rs (3)
current_salt(20-20)current_salt(63-68)current_salt(98-100)defuse/src/salts.rs (1)
current_salt(20-20)
defuse/src/contract/accounts/mod.rs (7)
tests/src/tests/defuse/accounts/mod.rs (3)
cleanup_nonces(26-26)cleanup_nonces(90-112)cleanup_nonces(198-200)defuse/src/accounts.rs (1)
cleanup_nonces(35-35)core/src/engine/state/mod.rs (1)
cleanup_nonce_by_prefix(65-69)core/src/engine/state/cached.rs (2)
cleanup_nonce_by_prefix(182-196)cleanup_nonce_by_prefix(422-424)defuse/src/contract/accounts/account/mod.rs (1)
cleanup_nonce_by_prefix(156-158)defuse/src/contract/intents/state.rs (1)
cleanup_nonce_by_prefix(132-145)core/src/engine/state/deltas.rs (1)
cleanup_nonce_by_prefix(126-132)
core/src/nonce/salted.rs (2)
defuse/src/contract/state/mod.rs (1)
new(27-41)defuse/src/contract/mod.rs (1)
new(99-108)
defuse/src/accounts.rs (2)
tests/src/tests/defuse/accounts/mod.rs (3)
cleanup_nonces(26-26)cleanup_nonces(90-112)cleanup_nonces(198-200)defuse/src/contract/accounts/mod.rs (1)
cleanup_nonces(55-76)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Build
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared security analysis / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Security Audit - report
🔇 Additional comments (10)
core/src/nonce/salted.rs (1)
101-121: Construction reads env; document/guard in tests.SaltRegistry::new calls Salt::derive(0) → env::random_seed_array(); requires testing_env in unit tests. Ensure tests set the VM context before constructing.
defuse/src/contract/salts.rs (1)
15-28: Rotation flow looks correct.assert_one_yocto + role guard + set_new + event emission + return current are consistent.
tests/src/tests/defuse/accounts/mod.rs (3)
26-26: Trait rename aligns with contract changes.
90-112: Call target updated to cleanup_nonces; looks good.
198-200: Forwarder method rename consistent.core/src/error.rs (1)
58-60: Verify exhaustive matches and serialization compatibilityUpdate every downstream
matchonDefuseErrorto include the new variants (InvalidNonce,InvalidSalt,SaltGenerationFailed), and confirm external/ABI consumers tolerate these additions.defuse/src/accounts.rs (1)
32-35: API rename complete; no stale callers found.tests/src/tests/defuse/accounts/nonces.rs (3)
29-38: LGTM! Well-designed test helper.The
create_random_salted_noncehelper cleanly encapsulates V1 nonce creation with salt, deadline, and random bytes. This promotes DRY principles and makes test intent clear.
42-203: LGTM! Comprehensive nonce lifecycle coverage.The test scenarios thoroughly validate:
- Legacy nonce backward compatibility (lines 50-71)
- Invalid salt rejection (lines 73-89)
- Deadline ordering constraints (lines 92-106)
- Expiration enforcement (lines 108-123)
- Successful commit flow (lines 126-147)
- Salt rotation with old salt acceptance (lines 149-180)
- Invalidated salt rejection (lines 182-203)
This provides excellent coverage of the salted nonce state machine.
208-334: LGTM! Thorough cleanup edge case validation.The test effectively verifies cleanup behavior for:
- Expired nonce removal (lines 268-280)
- Legacy nonce preservation (line 289)
- Non-expired nonce preservation (line 290)
- Unknown user handling (line 291)
- Invalid salt cleanup (lines 312-333)
This ensures cleanup_nonces behaves correctly across boundary conditions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
defuse/src/contract/accounts/mod.rs (2)
55-55: Add 1 yocto deposit guard to prevent accidental calls.This public state-changing method must be marked
#[payable]and callassert_one_yocto()at the start to prevent accidental cross-contract invocations.Apply this diff:
+ #[payable] fn cleanup_nonces(&mut self, nonces: Vec<(AccountId, Vec<AsBase64<Nonce>>)>) { + assert_one_yocto(); for (account_id, nonces) in nonces {Based on past review comments, ensure all test files (e.g.,
tests/src/tests/defuse/accounts/…) are updated to attach 1 yocto when callingcleanup_nonces.
72-73: Guard against empty nonce panic.Line 72 uses
let [prefix @ .., _] = nonce;which will panic ifnonceis a zero-length array. Add a defensive check before prefix extraction.Apply this diff:
// NOTE: all errors are omitted + if nonce.is_empty() { + continue; + } let [prefix @ .., _] = nonce; let _ = State::cleanup_nonce_by_prefix(self, &account_id, prefix);This prevents runtime panics from malformed or empty nonces.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/src/engine/mod.rs(3 hunks)core/src/nonce/versioned.rs(1 hunks)defuse/src/contract/accounts/mod.rs(3 hunks)defuse/src/contract/mod.rs(5 hunks)defuse/src/contract/tokens/mod.rs(5 hunks)defuse/src/contract/versioned/v0.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- defuse/src/contract/tokens/mod.rs
🧰 Additional context used
🧬 Code graph analysis (5)
defuse/src/contract/accounts/mod.rs (7)
defuse/src/accounts.rs (1)
cleanup_nonces(35-35)tests/src/tests/defuse/accounts/mod.rs (3)
cleanup_nonces(26-26)cleanup_nonces(90-112)cleanup_nonces(198-200)serde-utils/src/base64.rs (1)
into_inner(77-79)core/src/nonce/versioned.rs (1)
maybe_from(25-28)defuse/src/contract/intents/state.rs (1)
cleanup_nonce_by_prefix(132-144)core/src/engine/state/mod.rs (1)
cleanup_nonce_by_prefix(65-69)defuse/src/contract/accounts/account/mod.rs (1)
cleanup_nonce_by_prefix(156-158)
defuse/src/contract/mod.rs (4)
core/src/nonce/salted.rs (3)
derive(29-42)new(113-121)new(179-181)defuse/src/contract/accounts/mod.rs (1)
new(109-119)defuse/src/contract/state/mod.rs (1)
new(27-41)defuse/src/contract/state/v0.rs (2)
new(50-62)migrate(21-38)
defuse/src/contract/versioned/v0.rs (3)
defuse/src/contract/versioned/mod.rs (3)
from(32-39)from(44-47)from(52-54)defuse/src/contract/mod.rs (1)
migrate(146-148)defuse/src/contract/state/v0.rs (1)
migrate(21-38)
core/src/engine/mod.rs (2)
core/src/payload/mod.rs (1)
nonce(68-70)core/src/nonce/versioned.rs (1)
maybe_from(25-28)
core/src/nonce/versioned.rs (2)
core/src/nonce/salted.rs (4)
derive(29-42)new(113-121)new(179-181)from(195-199)core/src/nonce/expirable.rs (1)
new(25-27)
🪛 GitHub Check: Check
core/src/nonce/versioned.rs
[failure] 66-66:
std::convert::Fromnonce::versioned::VersionedNonce` is not satisfied
[failure] 62-62:
mismatched types
[failure] 61-61:
std::convert::Fromnonce::versioned::VersionedNonce` is not satisfied
[failure] 49-49:
std::convert::Fromnonce::versioned::VersionedNonce` is not satisfied
[failure] 47-47:
no variant or associated item named Legacy found for enum nonce::versioned::VersionedNonce in the current scope
[failure] 46-46:
mismatched types
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared security analysis / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Security Audit - report
🔇 Additional comments (8)
defuse/src/contract/versioned/v0.rs (2)
11-24: LGTM!The V0 storage structure is correctly set up for migration. The
_postponed_burnsfield is appropriately marked with#[borsh(skip)]to indicate it's runtime-only and won't be persisted, which aligns with the architectural decision to keep it unversioned.
26-41: LGTM!The migration implementation correctly transforms V0 storage to the current version. The state migration via
ContractState::migrate(state, Prefix::State)will add the newsaltsfield (as shown in the relevant code snippet fromdefuse/src/contract/state/v0.rs), while preservingaccountsandrelayer_keys. Dropping_postponed_burnsis appropriate since it's runtime-only.defuse/src/contract/accounts/mod.rs (1)
57-69: LGTM!The versioned nonce handling logic is correct:
- V1 salted nonces are cleanable if expired or their salt is invalid.
- Legacy nonces (non-versioned) are preserved until their usage is completely prohibited, as documented in the inline comment.
defuse/src/contract/mod.rs (5)
9-13: LGTM!The new
saltsandversionedmodules are correctly declared to support the salt management and storage versioning features introduced in this PR.
69-80: LGTM!The Contract structure refactoring correctly enables versioned storage:
- Custom (de)serialization via
MaybeVersionedContractStoragehandles version detection and automatic migration from V0 to the latest version.postponed_burnsis correctly marked as runtime-only with#[borsh(skip)], consistent with the design inContractStorageV0.Deref/DerefMutimplementations provide ergonomic access to the inner storage and state.
82-92: LGTM!The
ContractStoragestructure is correctly defined as the core storage primitive, containing the essential state components (accounts,state,relayer_keys). TheDeref/DerefMutimplementations provide convenient access to the nested state.
145-149: LGTM!The
MigrateStorageWithPrefixtrait provides a clean, prefix-aware migration API. This pattern is correctly applied inContractState::migrate(as shown in relevant code snippets) to add the newsaltsfield while preserving existing state during V0→current migration.
100-110: Verify ContractState::new salt registry initialization
Implementation ofContractState::newwasn’t located indefuse/src/contract/state; ensure thesaltsfield is initialized withSaltRegistry::new(prefix.nest(Prefix::Salts)).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
defuse/src/contract/runtime.rs (2)
5-8: Clean encapsulation.The struct design is sound with proper encapsulation (private field, public API). The
Defaultderive aligns well with the removal of manual constructors mentioned in the AI summary.Consider adding documentation to describe the Runtime's purpose and its role in managing postponed burn events:
+/// Manages runtime state for the contract, including postponed multi-token burn events. #[derive(Debug, Default)] pub struct Runtime { postponed_burns: PostponedMtBurnEvents, }
10-14: Straightforward delegation.The
mt_burnmethod correctly delegates to the internalpostponed_burnsfield. The'staticlifetime constraint onMtBurnEventis appropriate for events that need to be stored beyond the current call stack.Consider adding documentation to clarify the method's purpose:
impl Runtime { + /// Records a multi-token burn event to be processed later. pub fn mt_burn(&mut self, event: MtBurnEvent<'static>) { self.postponed_burns.mt_burn(event); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
defuse/src/contract/runtime.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Security Audit - deny
- GitHub Check: Build
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared security analysis / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Security Audit - report
🔇 Additional comments (1)
defuse/src/contract/runtime.rs (1)
1-3: LGTM!The imports are correctly structured and necessary for the Runtime implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
core/src/nonce/salted.rs (4)
30-30: Compilation error: qualifysize_of.
size_ofis not in scope. The import at line 1 only brings in thememmodule, not thesize_offunction directly.Apply this diff:
- const SIZE: usize = size_of::<Salt>(); + const SIZE: usize = mem::size_of::<Salt>();
218-224: Tests will panic without VM context setup.
SaltRegistry::new()callsSalt::derive(0)which usesenv::random_seed_array(), requiring a VM testing context. Line 220 constructs the registry before any context is set up, which will cause a panic.Apply this diff:
#[rstest] fn contains_salt_test(random_bytes: Vec<u8>) { + let mut rng = rng(); + set_random_seed(&mut rng); let random_salt: Salt = Unstructured::new(&random_bytes).arbitrary().unwrap(); let salts = SaltRegistry::new(random_bytes);Based on learnings from past reviews.
227-239: Initialize VM context before constructingSaltRegistry.Similar to
contains_salt_test, line 228 constructs the registry before setting up the VM context. Theset_random_seedcall on line 230 comes too late.Apply this diff:
#[rstest] fn update_current_salt_test(random_bytes: Vec<u8>, mut rng: impl Rng) { + set_random_seed(&mut rng); let mut salts = SaltRegistry::new(random_bytes); - let current = set_random_seed(&mut rng); + let current = rng.random(); + let context = VMContextBuilder::new().random_seed(current).build(); + testing_env!(context); let previous = salts.set_new().expect("should set new salt");Based on learnings from past reviews.
242-263: Initialize VM context before constructingSaltRegistry.Line 243 constructs the registry before any VM context is set up. Move the
set_random_seedcall beforeSaltRegistry::new().Apply this diff:
#[rstest] fn reset_salt_test(random_bytes: Vec<u8>, mut rng: impl Rng) { + set_random_seed(&mut rng); let mut salts = SaltRegistry::new(random_bytes); let random_salt = rng.random::<[u8; 4]>().as_slice().into(); - let seed = set_random_seed(&mut rng); + let seed = rng.random(); + let context = VMContextBuilder::new().random_seed(seed).build(); + testing_env!(context); let current = seed_to_salt(&seed, 0);Based on learnings from past reviews.
🧹 Nitpick comments (1)
defuse/src/contract/mod.rs (1)
149-153: Unify migration trait usage across versionsConsider implementing
MigrateStorageWithPrefix<T>for each versioned state (e.g., v0→current) and using it inside the versioned deserializer. Improves cohesion and discoverability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/nonce/salted.rs(1 hunks)defuse/src/contract/mod.rs(5 hunks)defuse/src/contract/tokens/mod.rs(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
core/src/nonce/salted.rs (3)
defuse/src/contract/mod.rs (1)
new(103-114)defuse/src/contract/state/mod.rs (1)
new(27-41)defuse/src/contract/state/v0.rs (1)
new(50-62)
defuse/src/contract/mod.rs (4)
core/src/nonce/salted.rs (3)
derive(29-42)new(113-121)new(179-181)defuse/src/contract/accounts/mod.rs (1)
new(108-118)defuse/src/contract/state/mod.rs (1)
new(27-41)defuse/src/contract/state/v0.rs (2)
new(50-62)migrate(21-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Security Audit - deny
- GitHub Check: Build
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared security analysis / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Security Audit - report
🔇 Additional comments (10)
core/src/nonce/salted.rs (5)
29-42: Verify salt predictability is acceptable for your threat model.
Salt::deriveusesenv::random_seed_array()which is constant within a block and publicly derivable from the block hash. While thenumparameter prevents identical salts within the same rotation sequence, an observer who knows the block hash and current valid salts can predict the next salt. Consider whether this predictability is acceptable for your use case, or if additional entropy sources (e.g., a persisted counter, timestamp, or account ID) should be mixed in.Based on learnings from past reviews.
45-62: LGTM!The Display/Debug implementations correctly use hex encoding, and the
FromStrimplementation properly delegates toFromHex::from_hex, handling errors appropriately.
65-99: LGTM!The JsonSchema implementation correctly represents Salt as a hex-encoded string with appropriate schema metadata, and is properly feature-gated.
123-163: LGTM!The salt rotation logic is sound:
derive_next_saltcorrectly uses the inclusive range0..=u8::MAXto find an unused saltset_newproperly rotates the salt and maintains the previous salt as validinvalidatehandles the edge case of invalidating the current salt by rotating firstis_validcorrectly checks both current and previous salts
165-183: LGTM!The
SaltedNoncestructure is clean and straightforward, with appropriate Borsh serialization support and a simple constructor.defuse/src/contract/mod.rs (4)
93-96: Runtime separation for postponed burns looks goodKeeping
postponed_burnsinRuntimewith#[borsh(skip)]avoids persisting transient data. LGTM.
105-111: Initialization aligns with new storage layoutAccounts/State/RelayerKeys prefixes are correct; runtime defaults are clear. LGTM.
68-79: Contract state layout change: ensure backward-compatible deserialization fully consumes old bytesYou collapsed top-level fields into
storageand movedpostponed_burnstoruntime(skipped). This changes the Borsh layout of the contract state. The customdeserialize_with = As::<MaybeVersionedContractStorage>::deserializemust:
- Correctly read previous versions where
accounts,state,relayer_keys, and possiblypostponed_burnswere serialized at the top level, and- Fully consume any trailing bytes (e.g., old
postponed_burns) sotry_from_slicedoesn’t fail on leftover data.If
MaybeVersionedContractStorage::deserializeonly reads the new storage triple and leaves old bytes unread, on-chain upgrade will break.Action:
- Confirm that the deserializer accounts for the old top-level layout and drains obsolete fields.
- Add/ensure an integration test that loads a v0 snapshot (with
postponed_burns) and successfully deserializes into the newContract.
52-53: Add SaltManager role usage checksSaltManager added. Verify that salt management APIs are gated with this role and that RolesConfig/tests include it.
defuse/src/contract/tokens/mod.rs (1)
21-25: Access path refactor to storage/runtime is correct
- Accounts/state routed via
self.storage.*.- Burn scheduling via
self.runtime.postponed_burns.No behavioral change detected. LGTM.
Also applies to: 42-47, 78-83, 106-108, 118-119
fusede
left a comment
fusede
left a comment
There was a problem hiding this comment.
Very well done! This is a final batch of comments, most of them are just nitpicking, so it's up to you. But please do add Role::GarbageCollector part for sure
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
core/src/nonce/salted.rs (1)
212-219: Critical: Test will panic without VM context.
SaltRegistry::new(random_bytes)callsSalt::derive(0)which usesenv::random_seed_array()and will panic unless a testing context is set up first. Callset_random_seed(&mut rng())before line 215.Apply this diff:
#[rstest] fn contains_salt_test(random_bytes: Vec<u8>) { let random_salt: Salt = Unstructured::new(&random_bytes).arbitrary().unwrap(); + let mut entropy = rng(); + set_random_seed(&mut entropy); let salts = SaltRegistry::new(random_bytes);Based on past review comments that flagged this exact issue.
core/src/nonce/versioned.rs (1)
31-42: Critical: Unqualifiedsize_ofwill not compile.Line 33 uses
size_of::<Nonce>()without qualification or import, which will cause a compilation error.Apply this diff:
impl From<VersionedNonce> for Nonce { fn from(value: VersionedNonce) -> Self { - const SIZE: usize = size_of::<Nonce>(); + const SIZE: usize = core::mem::size_of::<Nonce>(); let mut result = [0u8; SIZE];Based on past review comments that flagged this issue.
defuse/src/contract/accounts/mod.rs (1)
56-57: Critical: Add #[payable] guard to state-changing method.While access control restricts who can call this method, it should still follow the contract's pattern of requiring a 1 yocto deposit for all state-changing operations (see lines 38-40, 45-47, 76-77 for examples).
Apply this diff:
#[access_control_any(roles(Role::DAO, Role::GarbageCollector))] + #[payable] fn cleanup_nonces(&mut self, nonces: Vec<(AccountId, Vec<AsBase64<Nonce>>)>) { + assert_one_yocto();Based on past review comments and consistency with other state-changing methods in this file.
🧹 Nitpick comments (9)
core/src/nonce/versioned.rs (1)
44-73: Consider making test more robust against prefix collision.The test is functionally correct, but could be more explicit by ensuring the random
legacy_noncedoesn't accidentally start withVERSIONED_MAGIC_PREFIX. While the probability is 1/2^32 and deserialization would likely fail anyway, forcing the prefix to differ makes the test more deterministic.Optional refinement:
#[rstest] fn maybe_from_test(random_bytes: Vec<u8>) { let mut u = Unstructured::new(&random_bytes); - let legacy_nonce: Nonce = u.arbitrary().unwrap(); + let mut legacy_nonce: Nonce = u.arbitrary().unwrap(); + // Ensure it's not accidentally treated as versioned + if legacy_nonce[..4] == VersionedNonce::VERSIONED_MAGIC_PREFIX { + legacy_nonce[0] ^= 0x01; + }Based on past review suggestions about handling rare prefix collisions.
defuse/src/contract/accounts/mod.rs (2)
58-69: Per-nonce prefix cleanup is correct.The implementation iterates through nonces, validates cleanability, and performs prefix-based cleanup. The pattern
let [prefix @ .., _] = nonce;is safe for fixed-sizeNoncearrays (always 32 bytes) and extracts all bytes except the last.Optional: Consider logging ignored errors for observability:
// NOTE: all errors are omitted let [prefix @ .., _] = nonce; - let _ = State::cleanup_nonce_by_prefix(self, &account_id, prefix); + if let Err(e) = State::cleanup_nonce_by_prefix(self, &account_id, prefix) { + near_sdk::log!("Cleanup failed for account {}: {:?}", account_id, e); + }
84-102: Nonce cleanability logic is sound.The method correctly identifies nonces that should be cleaned up:
- Legacy nonces are preserved (not cleanable)
- V1 nonces are cleanable if expired OR have invalid salt
Optional: Simplify the double-negative for readability:
VersionedNonce::V1(SaltedNonce { salt, nonce: ExpirableNonce { deadline, .. }, }) => { - if !deadline.has_expired() && self.is_valid_salt(salt) { - return false; - } + deadline.has_expired() || !self.is_valid_salt(salt) } } - - truetests/src/tests/defuse/accounts/nonces.rs (5)
73-90: Avoid rare flake: ensure “invalid salt” is not equal to current/previousArbitrary salt could (extremely rarely) match the current (or previous) salt. Guard to ensure it’s truly invalid.
- let random_salt = Salt::arbitrary(u).unwrap(); + // Ensure we pick a salt that is not the current one (and unlikely to be previous) + let random_salt = loop { + let s = Salt::arbitrary(u).unwrap(); + if s != current_salt { + break s; + } + };
91-106: Naming nit: this nonce isn’t “expired”The variable expired_nonce is misleading here (it’s only inconsistent vs provided deadline). Consider a clearer name.
- let expired_nonce = create_random_salted_nonce(current_salt, deadline, &mut rng); + let nonce_with_smaller_deadline = + create_random_salted_nonce(current_salt, deadline, &mut rng); ... - expired_nonce, + nonce_with_smaller_deadline,
235-260: Reduce Arbitrary() decode risk: feed more than 1 byteUsing only 1 byte for Unstructured can cause Arbitrary to fail for some types. Feed a small random slice to reduce flakiness.
- env.user1.sign_defuse_message( - SigningStandard::arbitrary(&mut Unstructured::new(&rng.random::<[u8; 1]>())) - .unwrap(), + env.user1.sign_defuse_message( + SigningStandard::arbitrary( + &mut Unstructured::new(&random_bytes(8..64, &mut rng)), + ) + .unwrap(), ... - env.user1.sign_defuse_message( - SigningStandard::arbitrary(&mut Unstructured::new(&rng.random::<[u8; 1]>())) - .unwrap(), + env.user1.sign_defuse_message( + SigningStandard::arbitrary( + &mut Unstructured::new(&random_bytes(8..64, &mut rng)), + ) + .unwrap(), ... - env.user1.sign_defuse_message( - SigningStandard::arbitrary(&mut Unstructured::new(&rng.random::<[u8; 1]>())) - .unwrap(), + env.user1.sign_defuse_message( + SigningStandard::arbitrary( + &mut Unstructured::new(&random_bytes(8..64, &mut rng)), + ) + .unwrap(),Based on learnings
338-342: Fix log message: invalidation vs rotationThe expect message says “rotate” but the action is invalidate_salts. Adjust to avoid confusion.
- .expect("unable to rotate salt"); + .expect("unable to invalidate salt");
265-265: Add expiry margin to avoid timing flakinessSleeping exactly WAITING_TIME can be borderline for the last committed nonce(s). Add a small margin.
- sleep(Duration::from_secs_f64(WAITING_TIME.as_seconds_f64())).await; + sleep(Duration::from_secs_f64(WAITING_TIME.as_seconds_f64() + 0.5)).await;Also applies to: 402-402
defuse/src/contract/mod.rs (1)
151-155: Document migration trait intent and usageConsider a brief doc comment on MigrateStorageWithPrefix to clarify it’s for moving legacy on-chain collections under a new prefix during deserialization/upgrades. Helps future maintainers align implementations across versioned modules.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.md(1 hunks)core/src/engine/mod.rs(3 hunks)core/src/nonce/salted.rs(1 hunks)core/src/nonce/versioned.rs(1 hunks)defuse/src/contract/accounts/mod.rs(4 hunks)defuse/src/contract/intents/mod.rs(1 hunks)defuse/src/contract/mod.rs(5 hunks)defuse/src/intents.rs(3 hunks)tests/src/tests/defuse/accounts/mod.rs(4 hunks)tests/src/tests/defuse/accounts/nonces.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
core/src/nonce/salted.rs (3)
defuse/src/contract/accounts/mod.rs (1)
new(122-132)defuse/src/contract/state/mod.rs (1)
new(27-41)defuse/src/contract/state/v0.rs (1)
new(50-62)
tests/src/tests/defuse/accounts/nonces.rs (6)
test-utils/src/random.rs (3)
random(82-84)random_bytes(143-151)rng(138-140)core/src/nonce/salted.rs (3)
rng(239-239)new(108-116)new(174-176)defuse/src/contract/mod.rs (1)
new(105-116)defuse/src/contract/salts.rs (1)
current_salt(58-60)tests/src/tests/defuse/state/extensions/salt.rs (3)
current_salt(20-20)current_salt(63-68)current_salt(98-100)tests/src/tests/defuse/accounts/mod.rs (3)
is_nonce_used(45-45)is_nonce_used(147-156)is_nonce_used(237-239)
core/src/engine/mod.rs (2)
core/src/payload/mod.rs (1)
nonce(68-70)core/src/nonce/versioned.rs (1)
maybe_from(25-28)
core/src/nonce/versioned.rs (3)
core/src/payload/mod.rs (1)
nonce(68-70)core/src/nonce/salted.rs (4)
derive(24-37)from(190-194)new(108-116)new(174-176)core/src/nonce/expirable.rs (1)
new(25-27)
defuse/src/contract/mod.rs (4)
core/src/nonce/salted.rs (3)
derive(24-37)new(108-116)new(174-176)defuse/src/contract/accounts/mod.rs (1)
new(122-132)defuse/src/contract/state/mod.rs (1)
new(27-41)defuse/src/contract/state/v0.rs (2)
new(50-62)migrate(21-38)
tests/src/tests/defuse/accounts/mod.rs (2)
defuse/src/contract/accounts/mod.rs (1)
cleanup_nonces(57-69)defuse/src/accounts.rs (1)
cleanup_nonces(35-35)
defuse/src/contract/accounts/mod.rs (8)
tests/src/tests/defuse/accounts/mod.rs (3)
cleanup_nonces(26-30)cleanup_nonces(94-120)cleanup_nonces(206-214)defuse/src/accounts.rs (1)
cleanup_nonces(35-35)core/src/engine/state/mod.rs (1)
cleanup_nonce_by_prefix(65-69)core/src/engine/state/cached.rs (2)
cleanup_nonce_by_prefix(182-194)cleanup_nonce_by_prefix(420-422)defuse/src/contract/intents/state.rs (1)
cleanup_nonce_by_prefix(132-144)core/src/engine/state/deltas.rs (1)
cleanup_nonce_by_prefix(126-132)defuse/src/contract/accounts/account/mod.rs (1)
cleanup_nonce_by_prefix(156-158)core/src/nonce/versioned.rs (1)
maybe_from(25-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Build
- GitHub Check: Shared security analysis / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Security Audit - report
🔇 Additional comments (31)
CHANGELOG.md (1)
1-22: LGTM! Clear and comprehensive changelog.The changelog follows standard conventions and clearly documents all major features and changes in version 0.3.0, including salt management, nonce versioning, and cleanup functionality.
core/src/nonce/salted.rs (14)
1-21: LGTM! Clean imports and type definition.The Salt struct is well-defined with appropriate serialization support and optional fuzzing integration via the arbitrary feature.
23-38: Salt derivation strategy is sound.The implementation combines the block seed with an attempt number (
num), which provides sufficient entropy when called fromderive_next_salt(lines 118-123) that iterates through 0..=255. This addresses the block-collision concern raised in past reviews.
40-50: LGTM! Hex encoding for display.The Debug and Display implementations correctly encode the 4-byte salt as hex for human-readable output.
52-58: LGTM! Safe hex parsing.The
FromHex::from_heximplementation validates the decoded length and returns an error if the input doesn't decode to exactly 4 bytes, preventing the panic risk mentioned in past reviews.
60-94: LGTM! Proper JSON schema for ABI.The JsonSchema implementation correctly defines Salt as a hex-encoded string with appropriate content encoding metadata.
96-116: LGTM! Well-structured salt registry.The SaltRegistry maintains current and previous salts with validity tracking. Initializing with
Salt::derive(0)provides a deterministic starting point.
118-123: LGTM! Exhaustive salt generation.The method correctly tries all 256 possible salt values and returns the first unused one. The error case handles the (unlikely) scenario where all salts are already registered.
125-134: LGTM! Clean salt rotation logic.The rotation correctly moves the current salt to previous (keeping it valid), generates a new current salt, and returns the rotated salt for event emission.
136-147: LGTM! Safe invalidation with automatic rotation.The method prevents directly invalidating the current salt by rotating first, which ensures there's always a valid salt available for new operations.
149-158: LGTM! Clean accessor methods.The validation and accessor methods are straightforward and correct.
160-177: LGTM! Simple and effective wrapper.The SaltedNonce struct cleanly wraps any nonce type with a salt, maintaining generic flexibility.
179-210: LGTM! Comprehensive test utilities.The test helpers properly set up VM context (addressing past review concerns) and provide useful utilities for deterministic salt generation in tests.
221-234: LGTM! Proper test setup and comprehensive rotation verification.The test correctly initializes the VM context before creating the registry and thoroughly validates the salt rotation mechanism.
236-258: LGTM! Thorough invalidation testing.The test covers both invalidation scenarios (previous and current salts) and properly sets up VM context throughout.
defuse/src/contract/intents/mod.rs (1)
53-56: LGTM! Current salt now exposed in simulation output.The addition of
current_salttoStateOutputaligns with theSaltManagertrait integration and provides necessary salt information to clients.defuse/src/intents.rs (3)
1-14: LGTM! Clean salt-related imports.The imports correctly bring in
Salttype andSaltManagertrait needed for the trait bound and struct field changes.
17-17: LGTM! Trait bound reflects salt management dependency.Adding
SaltManageras a supertrait ensures implementers provide salt management capabilities required by the intents system.
58-64: LGTM! StateOutput extended with salt information.The
current_saltfield provides clients with the necessary information to construct valid salted nonces, completing the salt management API surface.core/src/nonce/versioned.rs (2)
1-19: LGTM! Well-structured versioned nonce enum.The V1 variant correctly composes
SaltedNonceandExpirableNoncewith 15 bytes for the inner nonce, which totals to 32 bytes when serialized with the magic prefix (4) + version (1) + salt (4) + deadline (8) + nonce (15).
21-29: LGTM! Safe versioned nonce detection.The
maybe_frommethod correctly handles both versioned and legacy nonces by checking for the magic prefix, ensuring backward compatibility.core/src/engine/mod.rs (3)
9-9: LGTM! Complete imports for nonce verification.All necessary types are imported to support the new versioned nonce validation logic.
77-79: LGTM! Pre-commit validation prevents invalid nonces.Calling
verify_intent_noncebeforecommit_nonceensures that only valid, unexpired nonces with correct salts are stored in state.
87-113: LGTM! Comprehensive nonce validation logic.The verification method correctly handles:
- Legacy nonce compatibility (early return)
- Salt validity checking
- Deadline ordering (intent ≤ nonce deadline)
- Expiration validation
The defensive checks prevent multiple attack vectors including replay attacks with rotated salts and deadline manipulation.
defuse/src/contract/accounts/mod.rs (2)
9-10: LGTM! Complete imports for access control and nonce validation.The imports properly support both the access control decorator and the versioned nonce handling in the cleanup logic.
Also applies to: 17-17, 25-25
122-131: LGTM! Minor formatting improvement.The added blank line improves code readability without any functional changes.
tests/src/tests/defuse/accounts/mod.rs (3)
26-30: LGTM! Test helper signature matches contract API.The trait method correctly reflects the renamed
cleanup_noncesAPI with the requireddefuse_contract_idparameter.
94-120: LGTM! Test implementation correctly calls renamed method.The implementation properly updates the contract call from
cleanup_expired_noncestocleanup_nonces, maintaining all other test helper functionality.
206-214: LGTM! Contract implementation correctly delegates to Account.The forwarding implementation maintains the clean delegation pattern for the renamed method.
tests/src/tests/defuse/accounts/nonces.rs (1)
29-38: Helper looks good; confirm inner nonce length expectationcreate_random_salted_nonce is clear and uses host RNG (good). Please confirm that [u8; 15] matches the expected inner nonce size across validation paths (e.g., is_nonce_used, signature domain). If a fixed size is required (e.g., 16 or 32), align here to avoid accidental acceptance/rejection edge cases.
defuse/src/contract/mod.rs (1)
70-81: Storage versioning wiring looks correctUsing As:: for both serialize_with and deserialize_with is the right pattern; runtime is borsh-skipped, avoiding on-chain format changes. No issues spotted.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
defuse/src/contract/accounts/mod.rs (1)
61-72: Guard against empty nonce panic in prefix extraction.Line 68's destructuring
let [prefix @ .., _] = nonce;will panic if the nonce array is empty. WhileNoncemay be non-empty by construction, defensive validation prevents runtime panics from malformed input or future type changes.Apply this diff to add a length check:
for nonce in nonces.into_iter().map(AsBase64::into_inner) { if !self.is_nonce_cleanable(nonce) { continue; } // NOTE: all errors are omitted + if nonce.is_empty() { + continue; + } let [prefix @ .., _] = nonce; let _ = State::cleanup_nonce_by_prefix(self, &account_id, prefix); }
🧹 Nitpick comments (1)
defuse/src/contract/accounts/mod.rs (1)
87-105: Consider unifying nonce validation logic.This logic partially duplicates nonce verification in
Engine::verify_intent_nonce(). Consider extracting shared validation into aStateViewExttrait method as suggested in previous reviews to improve maintainability.Based on learnings from past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
defuse/src/contract/accounts/mod.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
defuse/src/contract/accounts/mod.rs (7)
tests/src/tests/defuse/accounts/mod.rs (3)
cleanup_nonces(26-30)cleanup_nonces(94-120)cleanup_nonces(206-214)defuse/src/accounts.rs (1)
cleanup_nonces(35-35)core/src/engine/state/mod.rs (1)
cleanup_nonce_by_prefix(65-69)core/src/engine/state/cached.rs (2)
cleanup_nonce_by_prefix(182-194)cleanup_nonce_by_prefix(420-422)defuse/src/contract/intents/state.rs (1)
cleanup_nonce_by_prefix(132-144)defuse/src/contract/accounts/account/mod.rs (1)
cleanup_nonce_by_prefix(156-158)core/src/nonce/versioned.rs (1)
maybe_from(25-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Build
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared security analysis / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Security Audit - report
🔇 Additional comments (1)
defuse/src/contract/accounts/mod.rs (1)
56-59: Ensure test calls include exactly 1 yocto deposit.Verify that every
cleanup_noncestest invocation attaches a 1 yocto deposit (e.g., via.deposit(1)or.deposit_yocto(1)); otherwiseassert_one_yocto()will fail.
Summary by CodeRabbit
New Features
Changes
Tests
Documentation
Chores