-
Notifications
You must be signed in to change notification settings - Fork 8
feat/aml: locking accounts #66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
let value = "example".to_string(); | ||
let poc = PanicOnClone::from_ref(&value); | ||
assert!(ptr::eq(&**poc, &value)); | ||
assert_eq!(&**poc, &value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No test for panic behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this struct needs some unit tests... too many special cases.
AccountNotFound(AccountId), | ||
|
||
#[error("account '{0}' is locked")] | ||
AccountLocked(AccountId), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly what "locked" means here, I think the proper language is "frozen"... we "freeze" bank accounts.
async fn is_account_locked(&self, account_id: &AccountIdRef) -> anyhow::Result<bool>; | ||
async fn defuse_is_account_locked( | ||
&self, | ||
defuse_contract_id: &AccountId, | ||
account_id: &AccountIdRef, | ||
) -> anyhow::Result<bool>; | ||
|
||
async fn force_lock_account(&self, account_id: &AccountIdRef) -> anyhow::Result<bool>; | ||
async fn defuse_force_lock_account( | ||
&self, | ||
defuse_contract_id: &AccountId, | ||
account_id: &AccountIdRef, | ||
) -> anyhow::Result<bool>; | ||
async fn force_unlock_account(&self, account_id: &AccountIdRef) -> anyhow::Result<bool>; | ||
async fn defuse_force_unlock_account( | ||
&self, | ||
defuse_contract_id: &AccountId, | ||
account_id: &AccountIdRef, | ||
) -> anyhow::Result<bool>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't think it's necessary to duplicate these methods... just one of them is enough, and specifying the smart contracts in the tests is alright.
@@ -31,3 +32,23 @@ pub trait AccountManager { | |||
/// NOTE: MUST attach 1 yⓃ for security purposes. | |||
fn invalidate_nonces(&mut self, nonces: Vec<AsBase64<Nonce>>); | |||
} | |||
|
|||
#[ext_contract(ext_force_account_locker)] | |||
pub trait AccountForceLocker: AccessControllable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides using the word freeze instead of lock, I think this trait's name should be something like AccountLockable. If we agree on using freeze, then it's AccountFreezable.
// which we want to avoid. | ||
// | ||
// So, since we allow locked accounts to receive incoming transfers, it means | ||
// that `mt_transfer_call()` to locked recepients will always result in full |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: recepients
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with the first review round.
Cargo.lock
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cargo.lock is not up to date. Please run a full build then commit it again.
@@ -1,2 +1,3 @@ | |||
pub mod r#as; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we call this something that doesn't use a literal prefix? Maybe ser_as?
} | ||
impl<R> ReadExt for R where R: Read {} | ||
|
||
pub struct TeeReader<R, W> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// A reader that writes everything it reads to the specified writer. Can bee seen as a passthrough.
/// The struct itself does not do any kind of caching. Any apparent caching is possibly done by the inner read and writer.
@@ -48,3 +48,48 @@ pub trait MultiTokenForceWithdrawer: MultiTokenWithdrawer + AccessControllable { | |||
msg: Option<String>, | |||
) -> PromiseOrValue<Vec<U128>>; | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// MultiTokenCore
functions, but a version that bypasses the locked/frozen state of an account.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// All functions must be #[private]
or only authorized callers
/// Attached deposit of 1yN is required for security purposes. | ||
/// | ||
/// NOTE: this still allows for force withdrawals/transfers | ||
fn force_lock_account(&mut self, account_id: AccountId) -> bool; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of returning a bool here (and in unlock), we should probably just panic with a message "already locked".
@@ -21,7 +22,7 @@ pub trait AccountManager { | |||
/// i.e. this key can't be used to make any actions unless it's re-created. | |||
/// | |||
/// NOTE: MUST attach 1 yⓃ for security purposes. | |||
fn remove_public_key(&mut self, public_key: &PublicKey); | |||
fn remove_public_key(&mut self, public_key: PublicKey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we usually prefer passing a reference?
#[inline] | ||
pub fn as_unlocked_mut(&mut self) -> Option<&mut T> { | ||
if self.locked { | ||
pub const fn as_unlocked_mut(&mut self) -> Option<&mut T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a better name for all "as_unlocked*" functions is "get*".
#[inline] | ||
pub fn unlock(&mut self) -> Option<&mut T> { | ||
if !self.locked { | ||
pub const fn as_unlocked_or_mut(&mut self, force: bool) -> Option<&mut T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_force_mut
?
#[inline] | ||
pub const fn is_locked(&self) -> bool { | ||
self.locked | ||
} | ||
|
||
#[must_use] | ||
#[inline] | ||
pub const fn as_locked(&self) -> Option<&T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this function? What's the purpose of a function that gets while asserting the lock is activated? It should be either "I don't care about the lock" or "Get only if unlocked". This feels more like technical jargon to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This applies to all "as_locked" functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This Lock
was initially made when we were writing the very first version of defuse contract, which was implementing "escrow swap" functionality. It turned out that in some particular cases you expect the lock to be locked, because some actions might be permitted only when it's locked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove these now to make the API simpler to understand? If we need these functions in the future, we can assess whether we need them and add them if we do.
impl<T, As> BorshSerializeAs<Lock<T>> for Lock<As> | ||
where | ||
As: BorshSerializeAs<T>, | ||
{ | ||
#[inline] | ||
fn serialize_as<W>(source: &Lock<T>, writer: &mut W) -> io::Result<()> | ||
where | ||
W: io::Write, | ||
{ | ||
Lock { | ||
locked: source.locked, | ||
value: AsWrap::<&T, &As>::new(&source.value), | ||
} | ||
.serialize(writer) | ||
} | ||
} | ||
|
||
impl<T, As> BorshDeserializeAs<Lock<T>> for Lock<As> | ||
where | ||
As: BorshDeserializeAs<T>, | ||
{ | ||
#[inline] | ||
fn deserialize_as<R>(reader: &mut R) -> io::Result<Lock<T>> | ||
where | ||
R: io::Read, | ||
{ | ||
Lock::<AsWrap<T, As>>::deserialize_reader(reader).map(|v| Lock { | ||
locked: v.locked, | ||
value: v.value.into_inner(), | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need some serialization tests to assert the outcome.
/// | ||
/// This is safe to assume that legacy [`Account`] doesn't start with | ||
/// this prefix, since the first 4 bytes in legacy [`Account`] were used | ||
/// to detote the length of `prefix: Box<[u8]>` in [`LookupMap`] for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"denote"?
impl BorshDeserializeAs<Lock<Account>> for MaybeVersionedAccountEntry { | ||
fn deserialize_as<R>(reader: &mut R) -> io::Result<Lock<Account>> | ||
where | ||
R: io::Read, | ||
{ | ||
let mut buf = Vec::new(); | ||
// There will always be 4 bytes for u32: | ||
// * either `VERSIONED_MAGIC_PREFIX`, | ||
// * or u32 for `Account.nonces.prefix` | ||
let prefix = u32::deserialize_reader(&mut reader.tee(&mut buf))?; | ||
|
||
if prefix == VERSIONED_MAGIC_PREFIX { | ||
VersionedAccountEntry::deserialize_reader(reader) | ||
} else { | ||
Account::deserialize_reader( | ||
// prepend already consumed part of the reader | ||
&mut buf.chain(reader), | ||
) | ||
.map(Into::into) | ||
} | ||
.map(Into::into) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're defining the serialization manually anyway, isn't it better to define the serialization schema without PanicOnClone? We're crossing the barrier of "everything is just derived" to "we have to customize it"... let's go all the way and serialize the enum manually, and deserialize it manually as well. We're already doing that. It would be really awesome if we could throw out PanicOnClone.
|
||
#[rstest] | ||
#[test] | ||
fn legacy_upgrade(random_seed: Seed) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an upgrade test. For such complex serialization schemes, we need fixed value tests too (serialized hex values, back and forth in both cases). This helps in avoiding bugs like the Nest thing, where we need that the complicated scheme ended with something we understand.
To illustrate what I see here: Imagine a function f(x) and its inverse g(x). The test here is kind of testing f(g(x)) = 1
. But we need a test for what f(x) alone does, and what g(x) alone does too, because there are trivial cases where f(g(x)) = 1
is always true, but both f and g are broken.
IMHO, this format of testing must exist in any non-standard serialization plan.
#[error("public key already exists")] | ||
PublicKeyExists, | ||
#[error("public key '{1}' already exists for account '{0}'")] | ||
PublicKeyExists(AccountId, PublicKey), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hesitating, but I'll say it. I love this change. I love to always specify the parameters in errors to make debugging easier.
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis update introduces a comprehensive account locking and forced token transfer system across the Defuse smart contract platform. It adds account-level lock state management, enforces lock checks on state mutations, and provides new traits and methods for forcibly locking/unlocking accounts and performing forced token transfers. Error handling is enhanced with contextual information, and serialization logic is updated to support versioned account data. Supporting utilities, traits, and test infrastructure are expanded to facilitate these features. Changes
Sequence Diagram(s)Account Locking and Forced WithdrawalsequenceDiagram
participant Admin as Admin/DAO
participant Contract as Defuse Contract
participant Account as User Account
Admin->>Contract: force_lock_account(account_id)
Contract->>Account: Set locked = true
Contract-->>Admin: Return true if state changed
Account->>Contract: withdraw(...)
Contract->>Account: Check locked state
alt Account is locked and not force
Contract-->>Account: Return AccountLocked error
else force == true
Contract->>Account: Allow withdrawal
end
Admin->>Contract: force_unlock_account(account_id)
Contract->>Account: Set locked = false
Contract-->>Admin: Return true if state changed
Forced Multi-Token TransfersequenceDiagram
participant DAO as DAO/UnrestrictedWithdrawer
participant Contract as Defuse Contract
participant Sender as Sender Account
participant Receiver as Receiver Account
DAO->>Contract: mt_force_batch_transfer(...)
Contract->>Sender: Check locked state (force=true)
alt Sender exists
Contract->>Sender: Subtract tokens
Contract->>Receiver: Add tokens (even if locked)
Contract-->>DAO: Emit transfer event
else Sender missing or locked
Contract-->>DAO: Error (AccountNotFound/AccountLocked)
end
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 10
🔭 Outside diff range comments (2)
core/src/nonce.rs (1)
28-32
:commit
should return a typedResult
, not a bare booleanThe surrounding engine code is moving towards
Result
-based error propagation.commit()
still returns abool
, forcing every caller to translate the semantic (false
= already-used) manually.- pub fn commit(&mut self, n: Nonce) -> bool { - !self.0.set_bit(n) + pub fn commit(&mut self, n: Nonce) -> Result<(), DefuseError> { + if self.0.set_bit(n) { + Err(DefuseError::NonceAlreadyUsed) + } else { + Ok(()) + } + }Doing the conversion here eliminates duplicated checks, keeps the API symmetric with
Engine::commit_nonce
, and removes the need for the external#[must_use]
enforcement.Cargo.toml (1)
12-27
: Workspace lists both original and renamed crates – risk of duplicate package names
members
still contains"randomness"
and"test-utils"
while dependencies reference them via the new aliasesdefuse-randomness
/defuse-test-utils
.If the
randomness/Cargo.toml
internalpackage.name
was also changed todefuse-randomness
,cargo
will see two crates with different package names pointing to the same path, leading to build-graph duplication and longer compile times.
Conversely, if the internal name stayed"randomness"
, the dependency section should includepackage = "randomness"
:defuse-randomness = { path = "randomness", package = "randomness" }Please double-check the inner
Cargo.toml
s and keep either the alias or the original member entry, not both.
♻️ Duplicate comments (3)
borsh-utils/src/lib.rs (1)
1-1
: Module name still uses raw identifierr#as
Previous feedback asked for a less cryptic name (e.g.ser_as
). Has the team decided to keep the raw identifier? If not, renaming now will avoid breaking changes later.defuse/src/contract/tokens/nep245/resolver.rs (1)
66-66
: Typo in comment – “recepients” → “recipients”.Minor wording nit; previous round already flagged this.
defuse/src/contract/tokens/nep245/force.rs (1)
61-83
: Same approval issue in transfer-call variant.Replicate the above fix in
mt_force_transfer_call
to avoid the identical panic path.
🧹 Nitpick comments (20)
io-utils/src/lib.rs (1)
34-38
: Consider EOF optimisationWhen
n == 0
(EOF) you still callwrite_all(&[])
. Harmless but unnecessary; an early return would save one call in hot paths.near-utils/src/lib.rs (1)
5-8
: Publicly re-exportingpanic_on_clone
could create name collisionsBy glob-re-exporting
panic_on_clone::*
you expose every symbol in that module at the crate root alongside the existing helpers.
IfPanicOnClone
is the only type meant to be public, consider an explicit re-export to avoid future clashes:-mod panic_on_clone; +mod panic_on_clone; -pub use self::{cache::*, gas::*, lock::*, panic::*, panic_on_clone::*, prefix::*}; +pub use self::{cache::*, gas::*, lock::*, panic::*, prefix::*}; +pub use panic_on_clone::PanicOnClone;test-utils/src/random.rs (1)
1-1
: Wide re-export could shadow the local crate namespace
pub use defuse_randomness::{self as randomness, …}
re-exports the external crate under the namerandomness
.
Because this file already declares its own sub-modulerandom
(i.e. this file’s parent crate), future additions to the crate root namedrandomness
may collide with the alias. Consider restricting the alias to avoid confusion:-pub use defuse_randomness::{self as randomness, CryptoRng, Rng, SeedableRng, seq::IteratorRandom}; +pub use defuse_randomness::{CryptoRng, Rng, SeedableRng, seq::IteratorRandom};core/src/engine/mod.rs (1)
77-78
: Avoid unnecessaryclone()
onsigner_id
.Unless
commit_nonce
requires an ownedAccountId
, pass a reference to prevent an allocation:- self.state.commit_nonce(signer_id.clone(), nonce)?; + self.state.commit_nonce(&signer_id, nonce)?;If the method does require ownership, ignore this suggestion.
core/src/events.rs (1)
36-42
: Event version might need bumping / docs updateThe new
AccountLocked
andAccountUnlocked
variants share the same"0.2.1"
#[event_version]
as the pre-existing events.
If external indexers or front-ends rely on a strictly monotonic version to detect schema changes, adding new variants without bumping the version could break them silently.Confirm that downstream consumers treat “additive” enum changes as non-breaking, or consider bumping the minor/patch and updating the public changelog accordingly.
No code change required if the contract’s event-compatibility guarantees already allow this.defuse/src/lib.rs (1)
54-60
: Trait list getting long – consider grouping with feature-gatesThe
Defuse
interface now embeds 18 supertraits. Long lists hamper readability and make conditional compilation trickier.A lightweight refactor:
-pub trait Defuse: - Intents - + RelayerKeys - ... - + AccountForceLocker - + Pausable - + ControllerUpgradable - + FullAccessKeys +pub trait Defuse: + CommonIntents + + Governance + + Tokens + + AccountForceLockerwhere
CommonIntents
,Governance
,Tokens
, … are internal regrouping traits re-exported only inside this module.Not blocking, just something to think about for maintainability.
defuse/src/contract/tokens/nep245/enumeration.rs (1)
42-54
:as_inner_unchecked()
bypasses the lock – confirm threat modelEnumeration now calls
account.as_inner_unchecked()
to read balances even when an account is locked.
That’s consistent with the requirement that locking only blocks mutations.Double-check that no mutable references leak through
&TokenBalance
iterators here; otherwise a future refactor could accidentally calliter_mut()
and mutate without passing through lock guards.All good if the invariant “read-only access is always allowed” is documented.
tests/src/tests/defuse/upgrade.rs (1)
21-23
: Un-guardedwith_data()
may still panic on large state dumpsEven though the whole test is
#[ignore]
, importing the full state with.with_data()
is fragile – main-net contract data can easily exceed the 50 KB sandbox limit and cause anError: exceeded limit
.
Consider:- .with_data() // large state results into errors... + // Avoid copying complete contract state – it frequently exceeds the 50 KB limit. + // Remove `.with_data()` or gate it behind an env-flag when you really need it.This keeps the test runnable locally without risking a hard failure every time main-net state grows.
defuse/src/contract/tokens/nep245/withdraw.rs (1)
235-259
: Force-withdraw still enforces 1 yN – consider adding(owner_id != receiver_id)
guard
mt_force_withdraw
allows a privileged role to yank assets from any account, including accidentally from thereceiver_id
itself.
A quick sanity check prevents nonsensical no-op calls:require!(owner_id != receiver_id, "owner_id = receiver_id");Not critical but avoids user error and useless gas spend.
core/src/intents/account.rs (1)
96-99
: Avoid cloningsigner_id
on every iteration
signer_id.to_owned()
is evaluated for each nonce, needlessly reallocating the sameAccountId
. Clone it once and reuse.- self.nonces - .into_iter() - .try_for_each(|n| engine.state.commit_nonce(signer_id.to_owned(), n)) + let account_id = signer_id.to_owned(); + self.nonces + .into_iter() + .try_for_each(|n| engine.state.commit_nonce(account_id.clone(), n))defuse/src/contract/tokens/mod.rs (1)
70-78
: Minor ergonomics – computeowner_id
once
owner_id.to_owned()
is cloned twice. Grabbing it once tightens the code and avoids a duplicate allocation.- let owner = self - .accounts - .get_mut(owner_id) - .ok_or_else(|| DefuseError::AccountNotFound(owner_id.to_owned()))? - .as_unlocked_or_mut(force) - .ok_or_else(|| DefuseError::AccountLocked(owner_id.to_owned()))?; + let account = owner_id.to_owned(); + let owner = self + .accounts + .get_mut(&account) + .ok_or_else(|| DefuseError::AccountNotFound(account.clone()))? + .as_unlocked_or_mut(force) + .ok_or_else(|| DefuseError::AccountLocked(account.clone()))?;defuse/src/tokens/nep245.rs (1)
52-95
: Force-transfer interface added – consider documenting the 1 yⓃ requirementThe new
MultiTokenForceCore
extern trait mirrors the normal MT Core API and addsAccessControllable
, which is great. For consistency with the other force-* APIs, add a doc comment noting that callers must attach exactly 1 yoctoNEAR for security (same expectation as withdraw/lock functions).near-utils/src/panic_on_clone.rs (1)
20-23
: Document theunsafe { transmute }
justification.
from_ref
relies onunsafe { transmute }
. While#[repr(transparent)]
makes this sound, add a short safety comment explaining that both the source and target have identical layout and lifetimes.
This tiny note prevents future refactors from accidentally invalidating the assumption.defuse/src/contract/accounts/account.rs (1)
176-178
: Fix docstring typo.
detote
→denote
.defuse/src/contract/intents/state.rs (1)
125-131
: Unlocked-bypass helper deserves a comment.
as_inner_unchecked_mut()
intentionally sidesteps the lock to admit incoming deposits.
A short comment citing the invariant (“deposits must always succeed, even when the account is locked”) would prevent future reviewers from labelling this as a bug.defuse/src/contract/accounts/mod.rs (1)
148-159
: Minor API inconsistency
force_lock_account
takesAccountId
by value, whereasforce_unlock_account
takes&AccountId
. The external ABI would be cleaner if both used the same ownership pattern (either both by reference or both by value).defuse/src/contract/tokens/nep245/core.rs (1)
182-187
: Repeated map lookup inside the loop
self.accounts.get_mut(sender_id)
is executed for every(token, amount)
pair, creating multiple mutable borrows and needless work.Store the mutable reference once:
let sender_acc = self .accounts .get_mut(sender_id) .ok_or_else(|| DefuseError::AccountNotFound(sender_id.to_owned()))?; for (token_id, amount) in token_ids.iter().zip(amounts.iter().map(|a| a.0)) { … sender_acc .as_unlocked_or_mut(force) .ok_or_else(|| DefuseError::AccountLocked(sender_id.to_owned()))? .token_balances .sub(token_id.clone(), amount) .ok_or(DefuseError::BalanceUnderflow)?; … }Slightly faster and avoids borrow-checker gymnastics.
tests/src/tests/defuse/env.rs (3)
3-3
: Consider MSRV implications ofstd::sync::LazyLock
.
LazyLock
is only available since Rust 1.70. If CI / dev machines are pinned to an older toolchain (quite common in Near projects), compilation will fail. Either bump MSRV inrust-toolchain.toml
or switch back toonce_cell::sync::Lazy
/lazy_static!
.
257-277
: Minor allocation churn in loops inserting roles.
defuse_contract_id.clone()
is allocated per iteration; cloning theAccountId
once into a local would avoid repeatedArc
bumps. Low-priority for tests, but worth keeping in mind.
318-377
: Repeated token-deposit blocks could be DRYed up.
The three nearly identical storage-deposit calls could be collapsed into a helper that takes a slice of token IDs, improving readability and reducing copy-paste errors if the list changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lock
is excluded by!**/*.lock
📒 Files selected for processing (51)
Cargo.toml
(3 hunks)borsh-utils/Cargo.toml
(1 hunks)borsh-utils/src/as.rs
(1 hunks)borsh-utils/src/lib.rs
(1 hunks)core/Cargo.toml
(1 hunks)core/src/engine/mod.rs
(1 hunks)core/src/engine/state/cached.rs
(7 hunks)core/src/engine/state/deltas.rs
(1 hunks)core/src/engine/state/mod.rs
(2 hunks)core/src/error.rs
(3 hunks)core/src/events.rs
(1 hunks)core/src/intents/account.rs
(4 hunks)core/src/intents/tokens.rs
(2 hunks)core/src/nonce.rs
(1 hunks)defuse/Cargo.toml
(2 hunks)defuse/src/accounts.rs
(3 hunks)defuse/src/contract/accounts/account.rs
(2 hunks)defuse/src/contract/accounts/mod.rs
(3 hunks)defuse/src/contract/intents/state.rs
(7 hunks)defuse/src/contract/mod.rs
(1 hunks)defuse/src/contract/tokens/mod.rs
(2 hunks)defuse/src/contract/tokens/nep141/withdraw.rs
(4 hunks)defuse/src/contract/tokens/nep171/withdraw.rs
(4 hunks)defuse/src/contract/tokens/nep245/core.rs
(6 hunks)defuse/src/contract/tokens/nep245/enumeration.rs
(1 hunks)defuse/src/contract/tokens/nep245/force.rs
(1 hunks)defuse/src/contract/tokens/nep245/mod.rs
(1 hunks)defuse/src/contract/tokens/nep245/resolver.rs
(3 hunks)defuse/src/contract/tokens/nep245/withdraw.rs
(4 hunks)defuse/src/lib.rs
(3 hunks)defuse/src/tokens/nep245.rs
(2 hunks)io-utils/Cargo.toml
(1 hunks)io-utils/src/lib.rs
(1 hunks)near-utils/Cargo.toml
(1 hunks)near-utils/src/lib.rs
(1 hunks)near-utils/src/lock.rs
(2 hunks)near-utils/src/panic_on_clone.rs
(1 hunks)randomness/Cargo.toml
(1 hunks)test-utils/Cargo.toml
(2 hunks)test-utils/src/random.rs
(1 hunks)tests/Cargo.toml
(1 hunks)tests/src/tests/defuse/accounts.rs
(4 hunks)tests/src/tests/defuse/env.rs
(10 hunks)tests/src/tests/defuse/intents/ft_withdraw.rs
(1 hunks)tests/src/tests/defuse/intents/mod.rs
(1 hunks)tests/src/tests/defuse/intents/relayers.rs
(1 hunks)tests/src/tests/defuse/intents/token_diff.rs
(1 hunks)tests/src/tests/defuse/storage/mod.rs
(1 hunks)tests/src/tests/defuse/tokens/nep141.rs
(1 hunks)tests/src/tests/defuse/upgrade.rs
(2 hunks)tests/src/tests/utils.rs
(1 hunks)
🔇 Additional comments (43)
core/Cargo.toml (1)
11-13
: Change is harmless but double-check for duplicate dependency entries
defuse-map-utils.workspace = true
was moved upward and its previous duplicate later in the list appears to have been removed.
Please verify there is no second stray entry further down in this file (easy to miss in long dependency lists).
If nothing is duplicated, you’re good.randomness/Cargo.toml (1)
2-2
: Renaming done – scan the workspace for stalerandomness
crate referencesSome crates/tests may still depend on the old
randomness
name and will fail to compile.#!/bin/bash # Find use statements or Cargo manifests still referencing the old crate name rg -n '"randomness"' -g '*.rs' -g 'Cargo.toml'tests/src/tests/defuse/intents/mod.rs (1)
10-10
: Import update looks correct
defuse_randomness
aligns with the crate-rename; no additional changes required here.near-utils/Cargo.toml (1)
8-10
: Double-check that the newly added crates are actually referenced; otherwise they just bloat the WASM.
defuse-borsh-utils
andimpl-tools
were added as direct dependencies ofdefuse-near-utils
, but the diff shows no accompanying code change in this crate.
If the module does not yet use either crate, the extra bytes will be pulled into the final contract artefact for no benefit.Action items
- Grep the crate for
defuse_borsh_utils
/impl_tools
usage.- If nothing is found, postpone adding the deps until the first line of code requires them.
defuse/src/contract/tokens/nep245/mod.rs (1)
4-4
:mod force
inclusion looks fine – just remember the public re-export if callers need it.The addition is harmless, but if external code should use the
force
API directly, consider:mod force; +pub use force::*;
Otherwise, no issues.
tests/src/tests/defuse/tokens/nep141.rs (1)
12-12
: Good namespacing update – verify the workspace manifest mirrors the rename.The test crate now imports
defuse_randomness
. Make sure the workspaceCargo.toml
has:defuse-randomness = { path = "...", version = "..." }and that the old
randomness
entry was removed to avoid duplicate builds.tests/src/tests/defuse/intents/token_diff.rs (1)
13-13
: Consistent rename acknowledged.Same comment as above – ensure the dependency switch is reflected in
Cargo.toml
.defuse/src/contract/mod.rs (1)
46-48
: Adding enum variants after the existing ones is safe, but remember on-chain ACL migration.Because the
Role
enum is#[derive(BorshDeserialize)]
, its discriminant is stored on-chain wherever roles are persisted.
Appending variants keeps the old numeric values intact, so serialization is backwards-compatible. ✅However you still need to:
- Update any off-chain tooling / front-end that constructs role IDs.
- Add the new roles to
RolesConfig
in migrations or genesis so that ACL look-ups don’t panic on missing keys.- Document the upgrade path – contracts that rely on
UnrestrictedAccountLocker
/Unlocker
must be redeployed or migrated.No code change required here, just a heads-up.
tests/src/tests/defuse/intents/relayers.rs (1)
2-2
: Import path changed – double-check that all call-sites were migrated
ResultAssertsExt
now comes fromdefuse_test_utils::asserts
.
Please run a quick search to confirm no tests still reference the oldtest_utils::asserts
path, otherwise those files will fail to compile after the rename.tests/src/tests/utils.rs (1)
2-3
: Renamed utility crates – ensure feature flags match
defuse_randomness
anddefuse_test_utils
may expose slightly different Cargo features than the old crates. Verify that test-only features (e.g.crypto
,seq
) are enabled intests/Cargo.toml
, otherwise some helper traits (likeIteratorRandom
) might be missing in CI.tests/src/tests/defuse/storage/mod.rs (1)
7-9
: Imports renamed – compile-time check only.The switch to
defuse_randomness
anddefuse_test_utils
is consistent with the workspace-wide rename. No functional impact, just double-check that everyCargo.toml
(including downstream examples/integration tests) now lists the new crates, otherwise the tests will fail to compile.test-utils/Cargo.toml (2)
2-2
: Crate rename acknowledged.
name = "defuse-test-utils"
follows the new naming convention. Nothing else to flag.
11-11
: Dependency rename acknowledged.
defuse-randomness.workspace = true
lines up with the rename done in the code.tests/src/tests/defuse/intents/ft_withdraw.rs (1)
12-14
: Updated imports look correct.The new crate names compile under the same public API, so no further changes required in this file.
core/src/engine/mod.rs (1)
71-74
: Improved error context – good addition.Including
signer_id
andpublic_key
inDefuseError::PublicKeyNotExist
will greatly simplify debugging.tests/Cargo.toml (1)
13-14
: Test dependencies updated – looks good.The rename is reflected here; no additional action required.
borsh-utils/Cargo.toml (1)
7-12
: Good addition – workspace-wide lints enabledEnabling the workspace lint group here keeps this crate aligned with the rest of the repo, and the new deps are declared correctly with workspace resolution.
No concerns.defuse/src/lib.rs (1)
24-25
:ext_contract
trait grows: compilation check recommended
MultiTokenForceCore
is now imported just above the trait definition, but the macro-generatedext_defuse
interface ignores supertraits and only cares about method signatures.
This compiles today becausenear_sdk::ext_contract
expands the trait unaltered, yet an upstream change tonear-sdk
could start validating supertraits and reject the declaration.Nothing to fix now, but keep an eye on
near-sdk
release notes.
To be safe, run a quick search to ensure we don’t have otherext_contract
traits with an excessive supertrait list.#!/bin/bash # Count supertrait lists on all traits annotated with `ext_contract` rg -n --after-context 0 '#\[ext_contract' | while read -r line; do file=$(echo "$line" | cut -d: -f1) line_no=$(echo "$line" | cut -d: -f2) echo "=== $file:$line_no ===" sed -n "$line_no, $((line_no+5))p" "$file" donedefuse/Cargo.toml (2)
16-20
: Optional deps wired tocontract
feature – looks correct
defuse-borsh-utils
anddefuse-io-utils
are markedoptional
and immediately enabled by thecontract
feature.
This keeps dev builds (lib only) slim while ensuring the WASM build gets everything it needs.
No issues spotted.
37-41
: 👍 Useful dev-dependencies added
rstest
anddefuse-test-utils
will help keep new account-locking tests readable.core/src/nonce.rs (1)
16-24
: 👍#[must_use]
attribute is a good safety netAdding
#[must_use]
tonew
andis_used
prevents silent logic errors where the return value is accidentally discarded.core/src/intents/tokens.rs (1)
15-16
: Alias improves readability – verify downstream crates are updated
pub type TokenAmounts = Amounts<BTreeMap<TokenId, u128>>;
is clearer and shortens call-sites.
Please run a quick grep to ensure no crate still imports the oldAmounts<BTreeMap<…>>
form directly; mixed imports will compile but defeat the purpose of the alias.Cargo.toml (1)
44-59
: Consistent prefixing is great – don’t forgetio-utils
defuse-io-utils
is declared in[workspace.dependencies]
but the plain"io-utils"
source directory is also listed inmembers
.
Make sure the crate’s internal name isdefuse-io-utils
; otherwise apply the samepackage =
trick mentioned above.defuse/src/contract/tokens/nep245/withdraw.rs (3)
54-56
: Good: default (non-force) path explicitly setsforce = false
This preserves previous behaviour and avoids accidental bypass of account locks.
65-87
:self.withdraw(…, force)
call relies on updated signatureEnsure every other call-site of
withdraw()
inside the contract layer was updated to pass the newforce
flag; otherwise it will now compile-fail or, worse, silently default tofalse
if you introduced an overload.
207-223
: Depositing refunds into locked accounts may fail – confirmdeposit
bypasses lock
mt_resolve_withdraw
always callsself.deposit(...)
even ifsender_id
is currently locked. Your earlier spec states “locked accounts can still receive deposits”, but please verifydeposit
doesn’t reject onis_account_locked(&sender_id) == true
; otherwise refunds disappear.core/src/error.rs (1)
14-19
: New variants look goodExtension with
AccountNotFound
andAccountLocked
enriches context and keeps error messages consistent.defuse/src/contract/tokens/nep171/withdraw.rs (2)
51-52
: Parameter propagation reads clearly
false
literal here clarifies the non-forced path; no issues spotted.
63-78
: Good API surface –force
flag tunnels through correctlyThe internal plumbing passes
force
to the genericwithdraw
helper, aligning with the new locking semantics.core/src/engine/state/mod.rs (1)
36-54
: Trait evolution is consistentAdding
is_account_locked
toStateView
and converting mutation helpers toResult<()>
provides safer error propagation.core/src/intents/account.rs (1)
36-39
: Direct propagation of the newResult
-based API looks goodThe intent now simply forwards to
state.add_public_key
, relying on the richer error information returned from the state layer. ✅core/src/engine/state/deltas.rs (2)
90-94
:is_account_locked
passthrough added correctlyThe new helper is implemented as a straight delegation to the underlying state ‑ aligns with the trait change and keeps
Deltas
a thin wrapper. ✅
101-113
: Updated mutation signatures are correctly forwarded
add_public_key
,remove_public_key
, andcommit_nonce
now returnResult<()>
; forwarding without additional wrapping preserves the richer error information. Looks good.defuse/src/contract/tokens/mod.rs (1)
22-27
: Bypassing the lock withas_inner_unchecked_mut()
– double-check safetyDeposits are now always allowed, even for locked accounts. That matches the feature brief, but using the unchecked accessor completely sidesteps lock invariants. Please confirm that no invariants (e.g. account versioning, upgrade guards) rely on the lock wrapper during deposits, and consider adding an inline comment explaining why it is safe here to silence future reviewers’ concerns.
defuse/src/contract/tokens/nep141/withdraw.rs (2)
51-52
: Default/non-force withdrawal path wired correctlyPassing
false
for the newforce
flag keeps existing behaviour unchanged for regular users. Looks good.
63-76
:force
flag propagated all the way downThe internal helper now accepts the flag and forwards it to
self.withdraw
, enabling forced withdrawals without duplicating logic – clean reuse. ✅near-utils/src/lock.rs (1)
185-216
: Borsh compatibility may be broken after field re-ordering
Lock { locked, value }
reverses the original (value, locked) field order. Borsh serialises structs positionally, so existing on-chain data will deserialise as garbage unless every caller is switched to the new{ AsWrap … }
adapters.Double-check that:
- Every persisted
Lock<T>
is now (de)serialised exclusively through the newBorshSerializeAs / BorshDeserializeAs
wrappers.- Migration of already-stored data has been handled (e.g. one-shot migration task or forward-compatible enum).
If any old direct
BorshDeserialize
remains, this is a hard-forking change.tests/src/tests/defuse/accounts.rs (1)
291-441
: Great behavioural coverageThe test exercises both the happy path and failure modes (idempotent lock/unlock, incoming vs outgoing transfer restrictions, key management). Nice job – this will catch most regressions around the new feature.
core/src/engine/state/cached.rs (4)
64-110
: Correct implementation of read-only access through Lock wrapper.The StateView methods properly use
Lock::as_inner_unchecked
for read-only access to cached accounts, which is appropriate since these operations don't modify state. The newis_account_locked
method correctly checks both the cached lock state and falls back to the underlying view.
117-155
: Well-implemented lock enforcement for public key operations.The transition from boolean returns to
Result<()>
with specific error types improves error handling. Both methods correctly check account lock status before allowing modifications.
173-228
: Verify the asymmetric lock enforcement between add and sub balance operations.
internal_add_balance
bypasses lock checks usingas_inner_unchecked_mut()
whileinternal_sub_balance
enforces them usingas_unlocked_mut()
. This allows deposits to locked accounts while preventing withdrawals, which aligns with the PR objectives but creates an important behavioral asymmetry.Is this asymmetry intentional to allow incoming transfers/deposits to locked accounts while blocking outgoing transfers?
326-334
: Clean implementation of lock state initialization.The
get_or_create
method elegantly handles lock state initialization by accepting a closure that queries the underlying view, ensuring new cached accounts start with the correct lock state.tests/src/tests/defuse/env.rs (1)
205-214
: Builder API promises behaviour that is currently absent.
Becausedeployer_as_admin
/deployer_as_grantee
are never consumed, calling these fluent methods gives the false impression that roles are applied. Until the fix above is merged, consider panicking inside these methods to prevent silent misuse.
Closing in favor of #106 |
See docs for
AccountForceLocker
:Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Refactor
Documentation
Chores