feat: imt_mint/imt_burn intents
#193
Conversation
📝 WalkthroughWalkthroughAdds Intent-Mintable Token (IMT) support: new State trait methods Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Engine as Engine
participant State as State
participant Events as Events
participant Receiver as Receiver
Client->>Engine: submit ImtMint / ImtBurn intent (signed)
Engine->>Engine: verify signature & compute intent_hash
Engine->>State: mint(owner, tokens, memo) / burn(signer, tokens, memo)
State->>State: internal_add_balance / internal_sub_balance
State-->>Engine: Result (Ok / Err)
Engine->>Events: emit MtMint/MtBurn and ImtMint/ImtBurn events
alt notification requested
Engine->>Receiver: call NotifyOnTransfer (gas capped)
Receiver-->>Engine: callback result
end
Engine-->>Client: execution result (events + status)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 4
🤖 Fix all issues with AI agents
In @core/src/intents/tokens.rs:
- Around line 566-575: The doc comment on the MtBurn struct is incorrect: it
currently says "Burn a set of tokens from the signer to a specified account id"
(copied from MtMint) even though burning does not transfer tokens to another
account; update the comment for MtBurn to accurately describe its behavior
(e.g., "Burn a set of tokens from the signer, reducing total supply" or
similar), ensuring you edit the doc comment immediately above the MtBurn struct
definition and keep serde/derive attributes unchanged.
In @defuse/src/contract/account_sync.rs:
- Around line 27-39: The loop moves `public_key` into State::add_public_key,
then attempts to borrow it for the event, causing a use-after-move; fix by
ensuring the event uses an owned or cloned value (or by passing a reference into
add_public_key) so `public_key` is not consumed before emission: either call
DefuseEvent::PublicKeyAdded(...) before State::add_public_key, or call
State::add_public_key with a reference (e.g. &public_key) or pass
public_key.clone() into add_public_key and keep the original for
Cow::Borrowed(&public_key); update the uses around State::add_public_key,
DefuseEvent::PublicKeyAdded, AccountEvent::new, and PublicKeyEvent to avoid
moving `public_key` prior to creating the event.
In @tests/src/tests/defuse/accounts/account_sync.rs:
- Around line 185-193: The assertion message is inverted: the test asserts the
public key does NOT exist via assert!(!env.defuse.has_public_key(account_id,
public_key).await.unwrap(), ...), but the message says "not found"; update the
message to reflect the unexpected presence of the key (e.g., "Public key
{public_key:?} unexpectedly found for account {account_id}") so it accurately
describes the failure when env.defuse.has_public_key returns true; locate the
assertion inside the loop over public_keys where has_public_key(account_id,
public_key) is called.
🧹 Nitpick comments (6)
defuse/src/lib.rs (1)
16-17: Commented-out feature gate leavesaccount_syncunconditionally exposed.This aligns with the observation in
Cargo.toml— thefarfeature flag exists but isn't being used. If this is intentional for the current release, consider removing the comment entirely to avoid confusion. If conditional compilation is intended, uncomment the#[cfg(feature = "far")]attribute.core/src/intents/mod.rs (1)
75-80: Inconsistent doc comment style.Other
Intentenum variants (lines 43-74) use///for documentation comments, butMtMintandMtBurnuse//. This means the doc comments won't be generated for these variants.📝 Proposed fix
- // See [`MtMint`] + /// See [`MtMint`] MtMint(MtMint), - // See [`MtBurn`] + /// See [`MtBurn`] MtBurn(MtBurn),tests/src/tests/defuse/intents/mt_burn.rs (1)
26-113: Well-structured test coverage for MtBurn intent.The test correctly validates the mint-then-burn flow and verifies both the balance state and emitted events. The negative path tests properly ensure only the minter can burn tokens.
However, lines 100-112 duplicate the MtBurn event assertion already present in lines 76-98. Consider removing this redundant check.
♻️ Remove duplicate assertion
- assert_a_contains_b!( - a: result.logs().clone(), - b: [ - MtEvent::MtBurn(Cow::Owned(vec![MtBurnEvent { - owner_id: user.id().into(), - token_ids: vec![mt_id.to_string()].into(), - amounts: vec![U128::from(amount)].into(), - memo: Some(memo.into()), - authorized_id: None, - }])) - .to_nep297_event() - .to_event_log(),] - ); }defuse/src/account_sync.rs (2)
4-16: Parameter naming inconsistency with implementation.The trait defines parameter name as
public_keys, but the implementation indefuse/src/contract/account_sync.rsusesentries. While this doesn't affect functionality (NEAR ABI uses the impl's parameter names for JSON serialization), consistent naming improves maintainability.♻️ Align parameter names with implementation
pub trait AccountSyncManager { /// Registers or re-activates `public_key` under the user account_id. /// /// NOTE: MUST attach 1 yⓃ for security purposes. - fn force_add_public_keys(&mut self, public_keys: Vec<(AccountId, Vec<PublicKey>)>); + fn force_add_public_keys(&mut self, entries: Vec<(AccountId, Vec<PublicKey>)>); /// Deactivate `public_key` from the user account_id, /// 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 force_remove_public_keys(&mut self, public_keys: Vec<(AccountId, Vec<PublicKey>)>); + fn force_remove_public_keys(&mut self, entries: Vec<(AccountId, Vec<PublicKey>)>); }
4-4: Address TODO for locked accounts synchronization.The TODO comment indicates that locked accounts and accounts with predecessor ID disabled should also be synchronized. This could be a security or consistency concern if not addressed.
Do you want me to open a new issue to track this task?
tests/src/tests/defuse/accounts/account_sync.rs (1)
29-38: Edge case: random key count can be zero.
rng.random_range(0..10)can generate 0, resulting in empty key vectors. While this tests the empty-input case implicitly, it may cause test flakiness if no assertions are triggered. Consider using1..10for more deterministic coverage, or explicitly add a test for the empty case.Also applies to: 113-122
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
core/src/engine/state/cached.rscore/src/engine/state/deltas.rscore/src/engine/state/mod.rscore/src/events.rscore/src/intents/mod.rscore/src/intents/tokens.rsdefuse/Cargo.tomldefuse/src/account_sync.rsdefuse/src/contract/account_sync.rsdefuse/src/contract/intents/state.rsdefuse/src/contract/mod.rsdefuse/src/lib.rstests/src/tests/defuse/accounts/account_sync.rstests/src/tests/defuse/accounts/manage_public_keys.rstests/src/tests/defuse/accounts/mod.rstests/src/tests/defuse/intents/mod.rstests/src/tests/defuse/intents/mt_burn.rstests/src/tests/defuse/intents/mt_mint.rstests/src/tests/defuse/intents/simulate.rs
🧰 Additional context used
🧬 Code graph analysis (7)
defuse/src/account_sync.rs (1)
defuse/src/contract/account_sync.rs (2)
force_add_public_keys(24-40)force_remove_public_keys(44-60)
defuse/src/contract/account_sync.rs (2)
defuse/src/account_sync.rs (2)
force_add_public_keys(10-10)force_remove_public_keys(16-16)tests/src/utils/fixtures.rs (1)
public_key(7-12)
defuse/src/contract/intents/state.rs (3)
core/src/engine/state/cached.rs (2)
mt_mint(379-386)mt_burn(389-396)core/src/engine/state/deltas.rs (2)
mt_mint(218-225)mt_burn(228-235)core/src/engine/state/mod.rs (2)
mt_mint(131-132)mt_burn(134-139)
core/src/intents/tokens.rs (2)
core/src/intents/mod.rs (4)
execute_intent(84-92)execute_intent(96-110)execute_intent(114-141)new(157-159)core/src/amounts.rs (1)
new(23-25)
tests/src/tests/defuse/intents/mod.rs (5)
core/src/engine/state/cached.rs (2)
mt_burn(389-396)mt_mint(379-386)core/src/engine/state/deltas.rs (2)
mt_burn(228-235)mt_mint(218-225)core/src/engine/state/mod.rs (2)
mt_burn(134-139)mt_mint(131-132)defuse/src/contract/intents/state.rs (2)
mt_burn(359-366)mt_mint(349-356)defuse/src/contract/events.rs (1)
mt_burn(9-11)
core/src/engine/state/deltas.rs (4)
core/src/engine/state/cached.rs (2)
mt_mint(379-386)mt_burn(389-396)core/src/engine/state/mod.rs (2)
mt_mint(131-132)mt_burn(134-139)defuse/src/contract/intents/state.rs (2)
mt_mint(349-356)mt_burn(359-366)poa-factory/src/contract.rs (1)
tokens(183-191)
core/src/engine/state/cached.rs (3)
core/src/engine/state/deltas.rs (2)
mt_mint(218-225)mt_burn(228-235)core/src/engine/state/mod.rs (2)
mt_mint(131-132)mt_burn(134-139)defuse/src/contract/intents/state.rs (2)
mt_mint(349-356)mt_burn(359-366)
⏰ 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: Check
- 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: 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 (24)
defuse/Cargo.toml (1)
69-69: Feature flagfaris defined but not used for conditional compilation.The
farfeature flag is empty and the corresponding#[cfg(feature = "far")]attributes are commented out in bothdefuse/src/lib.rs(line 16) anddefuse/src/contract/mod.rs(line 58). This meansaccount_syncandPubKeySynchronizerare unconditionally included regardless of whether this feature is enabled.Either:
- Remove the commented
#[cfg]attributes if the feature gating is no longer intended, or- Uncomment the
#[cfg(feature = "far")]attributes to properly gate the functionalitytests/src/tests/defuse/accounts/manage_public_keys.rs (2)
43-46: LGTM!The updated unwrap chain correctly handles the nested result structure from
exec_transaction(), where the outerunwrap()handles execution errors andinto_result().unwrap()extracts the contract's result.
96-100: LGTM!Consistent application of the same result handling pattern as in
test_add_public_key.core/src/events.rs (2)
59-63: LGTM!The
MtMintandMtBurnevent variants follow the established pattern for token events, with consistent versioning (0.3.0) and type signatures matchingFtWithdraw,NftWithdraw,MtWithdraw, etc.
13-15: LGTM!Import correctly extended to include the new
MtMintandMtBurntypes.tests/src/tests/defuse/intents/mod.rs (1)
47-48: LGTM!Test modules for
mt_burnandmt_mintare properly wired into the test suite, corresponding to the new MtMint/MtBurn intent functionality.tests/src/tests/defuse/accounts/mod.rs (1)
1-1: LGTM!The
account_synctest module is properly added to cover the newAccountSyncManagerfunctionality andPubKeySynchronizerrole permissions.defuse/src/contract/mod.rs (2)
57-59: Commented-out feature gate forPubKeySynchronizerrole.Similar to the other files, the
#[cfg(feature = "far")]is commented out, makingPubKeySynchronizerunconditionally available in theRoleenum. This role will be usable for access control regardless of whether thefarfeature is enabled.If conditional compilation is intended, note that adding/removing enum variants behind feature flags can cause serialization compatibility issues with stored ACL data when the feature is toggled. Consider whether this role should always exist (even if unused) to maintain storage compatibility.
3-3: LGTM!The
account_syncmodule is appropriately added as a private module for internal contract implementation.tests/src/tests/defuse/intents/simulate.rs (3)
23-24: LGTM!Imports for
MtBurnandMtMintare correctly added to support the new simulate tests.
909-958: LGTM!The
simulate_mint_intenttest is well-structured and consistent with other simulate tests in this file. It correctly verifies that the simulation produces the expectedMtMintevent in the logs.
960-1023: LGTM!The
simulate_burn_intenttest correctly mints tokens first to establish a balance before simulating the burn. This ensures the burn operation has tokens to burn. The event assertions follow the established pattern.core/src/engine/state/mod.rs (1)
130-139: LGTM!The new
mt_mintandmt_burntrait methods follow the established conventions in theStatetrait:
mt_minttakesowner_id: AccountIdby value (consistent withadd_public_key,internal_add_balance)mt_burntakesowner_id: &AccountIdRefby reference (consistent withinternal_sub_balance,ft_withdraw)- Both use
Amountsfor multi-token operations andOption<String>for optional memocore/src/intents/mod.rs (1)
138-139: LGTM!The
ExecutableIntentdispatch forMtMintandMtBurnfollows the established pattern used by other intent variants.tests/src/tests/defuse/intents/mt_mint.rs (2)
26-82: LGTM!The test comprehensively verifies the
MtMintintent flow:
- Creates and signs an
MtMintintent- Executes via
simulate_and_execute_intents- Verifies the resulting MT balance
- Confirms both the NEP-297
MtMintEventandDefuseEvent::MtMintare emitted
12-12: Bothdefuse_escrow_swap::token_id::nep245::Nep245TokenIdanddefuse::core::token_id::nep245::Nep245TokenIdresolve to the same underlying type fromdefuse-token-id. The difference in import paths is due to re-exports in each crate, not different types. Using the escrow-swap re-export in the MT tests is consistent with their purpose.Likely an incorrect or invalid review comment.
defuse/src/contract/intents/state.rs (1)
358-366: No action needed. Bothmt_mintandmt_burnare correctly implemented. Thewithdrawmethod acceptsmemo: Option<impl Into<String>>, which means any type implementingInto<String>is valid. BothOption<&str>(frommemo.as_deref()) andOption<String>(frommemodirectly) satisfy this bound, so the code compiles without issues. The stylistic difference in how memo is passed does not indicate a type mismatch.Likely an incorrect or invalid review comment.
tests/src/tests/defuse/intents/mt_burn.rs (1)
115-199: LGTM!The negative path tests correctly validate that only the original minter can burn tokens, including the scenario where tokens are transferred to another user who then attempts to burn them.
core/src/engine/state/cached.rs (2)
377-396: LGTM!The
mt_mintandmt_burnimplementations correctly delegate to the existinginternal_add_balanceandinternal_sub_balancemethods. The#[inline]annotations are appropriate for these thin wrapper methods.
388-396: The code is correct. Noselfwithdrawtypo exists indefuse/src/contract/intents/state.rs. Themt_burnfunction properly callsself.withdraw(owner_id, tokens, memo, false).Likely an incorrect or invalid review comment.
tests/src/tests/defuse/accounts/account_sync.rs (1)
21-103: LGTM!The test thoroughly validates permission checks and event emissions for the
force_add_public_keysfunctionality. The pattern of first testing unauthorized access, then granting role and verifying success is well-structured.core/src/intents/tokens.rs (2)
520-564: LGTM!The MtMint implementation correctly:
- Emits the event before state mutation (consistent with other intents)
- Transforms token IDs by prefixing with the owner_id, establishing minter ownership
- Delegates to
engine.state.mt_mintfor the actual balance update
577-609: LGTM!The MtBurn implementation mirrors MtMint appropriately. The token ID transformation ensures that only tokens minted by the signer (prefixed with their account ID) can be burned.
core/src/engine/state/deltas.rs (1)
216-235: LGTM!The
mt_mintandmt_burnmethods correctly delegate to the inner state without updating theTransferMatcher. This is consistent with other bypass methods likeft_withdrawandnft_withdraw, which also don't participate in delta tracking since they represent external operations rather than internal transfers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/src/tests/defuse/intents/mt_burn.rs (1)
77-113: Consider removing duplicate assertion.The second
assert_a_contains_b!macro call (lines 101-113) checks forMtEvent::MtBurnwhich is already verified in the first assertion (lines 77-99). This appears to be redundant.♻️ Proposed fix to remove duplicate assertion
assert_a_contains_b!( a: result.logs().clone(), b: [ MtEvent::MtBurn(Cow::Owned(vec![MtBurnEvent { owner_id: user.id().into(), token_ids: vec![mt_id.to_string()].into(), amounts: vec![U128::from(amount)].into(), memo: Some(memo.into()), authorized_id: None, }])) .to_nep297_event() .to_event_log(), DefuseEvent::MtBurn(Cow::Owned(vec![IntentEvent { intent_hash: burn_payload.hash(), event: AccountEvent { account_id: user.id().clone().into(), event: Cow::Owned(intent), }, }])) .to_nep297_event() .to_event_log(), ] ); - - assert_a_contains_b!( - a: result.logs().clone(), - b: [ - MtEvent::MtBurn(Cow::Owned(vec![MtBurnEvent { - owner_id: user.id().into(), - token_ids: vec![mt_id.to_string()].into(), - amounts: vec![U128::from(amount)].into(), - memo: Some(memo.into()), - authorized_id: None, - }])) - .to_nep297_event() - .to_event_log(),] - ); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/src/intents/tokens.rstests/src/tests/defuse/intents/mt_burn.rstests/src/tests/defuse/intents/mt_mint.rstests/src/tests/defuse/intents/simulate.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/src/tests/defuse/intents/mt_mint.rs
🧰 Additional context used
🧬 Code graph analysis (2)
tests/src/tests/defuse/intents/simulate.rs (1)
tests/src/tests/defuse/intents/mod.rs (2)
new(25-32)into_event(34-42)
core/src/intents/tokens.rs (2)
core/src/intents/mod.rs (4)
execute_intent(84-92)execute_intent(96-110)execute_intent(114-141)new(157-159)core/src/amounts.rs (1)
new(23-25)
⏰ 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). (10)
- GitHub Check: Build Reproducible
- GitHub Check: Security Audit - deny
- 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-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 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 (10)
core/src/intents/tokens.rs (5)
3-3: LGTM!Import correctly added for the new
Nep245TokenIdusage in the mint/burn implementations.
521-532: LGTM!The
MtMintstruct follows the established pattern used byTransferand other intents, with appropriate serde attributes for serialization.
534-566: LGTM!The implementation correctly:
- Emits the
MtMintevent before state mutation- Transforms tokens to NEP-245 format with the signer as the minter namespace
- Delegates to the state layer for the actual mint operation
The design allows users to mint tokens namespaced under their account ID, which provides proper isolation.
568-577: LGTM!The
MtBurnstruct appropriately omitsreceiver_idsince burns are scoped to tokens minted by the signer (owner_id).
579-611: LGTM!The implementation correctly enforces that only the original minter can burn their minted tokens by:
- Using
owner_id(the signer) in theNep245TokenIdconstruction- Passing
owner_idtomt_burnas the account to deduct balance fromThis ensures proper authorization for burn operations.
tests/src/tests/defuse/intents/simulate.rs (3)
23-24: LGTM!Imports correctly added for the new
MtMintandMtBurnintents used in the simulation tests.
909-959: LGTM!The test follows the established simulation test pattern, correctly verifying that the
MtMintintent produces the expectedDefuseEvent::MtMintevent in the simulation logs.
961-1025: LGTM!The test correctly sets up the precondition (minting tokens first via
simulate_and_execute_intents) before simulating the burn intent. The assertion structure matches other simulation tests in this file.tests/src/tests/defuse/intents/mt_burn.rs (2)
116-184: LGTM!The test comprehensively covers two important negative scenarios:
- Lines 129-145: Attempting to burn tokens that were never minted (non-existent balance)
- Lines 147-183: Attempting to burn tokens minted by another user (authorization check)
Both correctly use
unwrap_err()to verify the operations fail as expected, validating that only the original minter can burn their minted tokens.
1-22: No action required—the imports are consistent.Both
defuse_escrow_swap::token_id::nep245::Nep245TokenId(line 12) anddefuse::core::token_id::nep245::Nep245TokenIdare re-exports of the same underlying typedefuse_token_id::nep245::Nep245TokenId. Theescrow-swap/src/lib.rsandcore/src/lib.rsboth declarepub use defuse_token_id as token_id;, making both import paths equivalent. There is no type mismatch concern.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Line 74: Update the pinned cargo-near version used in the CI step that runs
"cargo install cargo-make cargo-near@0.17.0 --locked" (and the identical
invocation later) to the latest stable 0.18.0, or if you must keep 0.17.0, add
an inline comment near those "cargo install ... cargo-near@0.17.0 --locked"
lines documenting the reason for pinning (e.g., compatibility or test
reproducibility) so reviewers understand why an older release is used.
🧹 Nitpick comments (2)
tests/src/tests/defuse/accounts/account_sync.rs (2)
29-38: Test may pass vacuously when zero keys are generated.
rng.random_range(0..10)can produce 0 for both users, meaning no keys are actually added or verified. Consider using1..10to ensure at least one key per user is tested, or add an assertion thatpublic_keysis non-empty.Suggested fix
let public_keys = vec![&user1, &user2] .into_iter() .map(|u| { - let pubkeys = (0..rng.random_range(0..10)) + let pubkeys = (0..rng.random_range(1..10)) .map(|_| public_key(&mut rng)) .collect(); (u.account().id(), pubkeys) }) .collect::<Vec<(_, Vec<PublicKey>)>>();
113-122: Same zero-keys edge case as above.This has the same
0..10range that could produce empty key lists. Consider using1..10for consistency and to ensure actual functionality is exercised.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.ymltests/src/tests/defuse/accounts/account_sync.rs
🧰 Additional context used
🧬 Code graph analysis (1)
tests/src/tests/defuse/accounts/account_sync.rs (2)
tests/src/utils/fixtures.rs (1)
public_key(7-12)tests/src/tests/defuse/env/mod.rs (1)
create_user(140-151)
⏰ 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). (10)
- GitHub Check: Security Audit - deny
- GitHub Check: Build Reproducible
- 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-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 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: Security Audit - report
🔇 Additional comments (2)
tests/src/tests/defuse/accounts/account_sync.rs (1)
105-209: Well-structured test for add/remove flow.Good coverage of the permission model: user1 adds keys, user2's unauthorized attempt fails, then user2 is granted role and successfully removes keys. The assertions verify both state (
has_public_key) and event emissions..github/workflows/ci.yml (1)
107-107: Consistent version pinning across jobs.Good that both Build and Build Reproducible jobs use the same cargo-near version for consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
165-165: Version inconsistency between audit jobs.This job pins
cargo-auditto^0.22, but thesecurity_audit_denyjob (line 189) installs it without a version specifier. This inconsistency could lead to different audit results between the two jobs if crates.io has a newer major version.Consider aligning both jobs to use the same version constraint.
♻️ Suggested fix for line 189
- name: Install Cargo Plugins - run: cargo install cargo-audit --locked + run: cargo install cargo-audit --version "^0.22" --locked
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml
⏰ 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). (10)
- GitHub Check: Build
- GitHub Check: Build Reproducible
- GitHub Check: Security Audit - deny
- 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 (1)
.github/workflows/ci.yml (1)
74-74: Pinningcargo-nearto0.17.0improves build reproducibility.This is a good practice for CI stability. Version 0.17.0 is the latest release, so the pinning uses a current version. Be aware that this will require maintenance updates when newer versions are released.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
core/src/intents/tokens.rs (2)
534-572: Consider adding validation for empty tokens.The
Transferintent (line 99-100) validates that tokens are not empty and returnsDefuseError::InvalidIntent. TheMtMintimplementation lacks this validation, which could allow no-op intents that still consume gas and emit events.💡 Suggested validation
impl ExecutableIntent for MtMint { #[inline] fn execute_intent<S, I>( self, owner_id: &AccountIdRef, engine: &mut Engine<S, I>, intent_hash: CryptoHash, ) -> Result<()> where S: State, I: Inspector, { + if self.tokens.is_empty() { + return Err(DefuseError::InvalidIntent); + } + let tokens = Amounts::new(
585-623: Same empty tokens validation consideration applies to MtBurn.For consistency with
Transferand the suggestedMtMintfix, consider adding empty tokens validation here as well.💡 Suggested validation
impl ExecutableIntent for MtBurn { #[inline] fn execute_intent<S, I>( self, owner_id: &AccountIdRef, engine: &mut Engine<S, I>, intent_hash: CryptoHash, ) -> Result<()> where S: State, I: Inspector, { + if self.tokens.is_empty() { + return Err(DefuseError::InvalidIntent); + } + let tokens = Amounts::new(tests/src/tests/defuse/accounts/account_sync.rs (1)
32-41: Random key count could result in empty test scenarios.The random range
0..10(lines 35, 123) can generate 0 keys, causing the test to pass without actually testing any key operations. Consider using1..10to ensure at least one key is tested.💡 Suggested fix
let public_keys = vec![&user1, &user2] .into_iter() .map(|u| { - let pubkeys = (0..rng.random_range(0..10)) + let pubkeys = (0..rng.random_range(1..10)) .map(|_| public_key(&mut rng)) .collect::<HashSet<_>>(); (u.account().id(), pubkeys) }) .collect::<HashMap<_, HashSet<PublicKey>>>();Also applies to: 120-129
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
core/src/intents/tokens.rsdefuse/src/accounts.rsdefuse/src/contract/accounts/force.rsdefuse/src/contract/accounts/mod.rsdefuse/src/contract/mod.rstests/src/tests/defuse/accounts/account_sync.rstests/src/tests/defuse/intents/mt_burn.rstests/src/tests/defuse/intents/mt_mint.rstests/src/tests/defuse/intents/simulate.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- defuse/src/contract/mod.rs
- tests/src/tests/defuse/intents/mt_burn.rs
- tests/src/tests/defuse/intents/mt_mint.rs
🧰 Additional context used
🧬 Code graph analysis (5)
defuse/src/accounts.rs (1)
defuse/src/contract/accounts/force.rs (2)
force_add_public_keys(92-100)force_remove_public_keys(104-112)
defuse/src/contract/accounts/mod.rs (6)
core/src/engine/state/deltas.rs (2)
remove_public_key(118-120)add_public_key(113-115)core/src/engine/state/cached.rs (2)
remove_public_key(154-172)add_public_key(134-152)defuse/src/contract/intents/state.rs (2)
remove_public_key(115-123)add_public_key(104-112)defuse/src/contract/accounts/account/mod.rs (2)
remove_public_key(79-87)add_public_key(67-75)defuse/src/contract/accounts/account/entry/v0.rs (2)
remove_public_key(128-142)add_public_key(98-112)defuse/src/contract/accounts/account/entry/v1.rs (2)
remove_public_key(124-138)add_public_key(94-108)
tests/src/tests/defuse/accounts/account_sync.rs (3)
tests/src/utils/fixtures.rs (1)
public_key(7-12)tests/src/tests/defuse/env/mod.rs (1)
create_user(140-151)defuse/src/accounts.rs (1)
has_public_key(11-11)
core/src/intents/tokens.rs (1)
core/src/amounts.rs (1)
new(23-25)
defuse/src/contract/accounts/force.rs (1)
defuse/src/accounts.rs (2)
force_add_public_keys(89-89)force_remove_public_keys(95-95)
⏰ 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). (10)
- GitHub Check: Build Reproducible
- GitHub Check: Security Audit - deny
- 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 (12)
core/src/intents/tokens.rs (1)
520-532: MtMint struct is well-defined.The struct correctly captures the necessary fields for minting tokens with proper serialization attributes. The
#[serde_as]forAmountswithBTreeMapandDisplayFromStraligns with the existingTransferpattern.tests/src/tests/defuse/intents/simulate.rs (3)
23-24: Import additions are appropriate.The new imports for
MtBurn,MtMint, andAccountIdare correctly added to support the new simulation tests.Also applies to: 45-45
909-965: Test correctly verifies MtMint event transformation.The test properly validates that:
- The mint intent is created with a base token ID
- The event contains the transformed
Nep245TokenIdwith the user's account as the issuer prefix- The nonce event is emitted
967-1037: Test properly validates MtBurn flow with prerequisite mint.The test correctly:
- First mints tokens using
simulate_and_execute_intentsto persist state- Then creates and simulates a burn intent
- Verifies the burn event contains the expected transformed token ID
defuse/src/accounts.rs (1)
85-95: New batch public key management methods are well-defined.The trait extension adds
force_add_public_keysandforce_remove_public_keyswith appropriate:
- Batch operation signature using
HashMap<AccountId, HashSet<PublicKey>>- Documentation noting 1 yoctoNEAR security requirement
- Consistent naming with existing force_* methods
defuse/src/contract/accounts/force.rs (3)
22-26: Access control consistently extended with UnrestrictedAccountManager role.The existing force methods now include
Role::UnrestrictedAccountManageralongside the appropriate locker/unlocker roles, providing a unified role for comprehensive account management capabilities.Also applies to: 41-45, 60-64, 75-79
90-100: Implementation correctly delegates to Contract method for event emission.The method properly:
- Requires DAO or UnrestrictedAccountManager role (intentionally more restrictive than lock/unlock)
- Enforces 1 yoctoNEAR deposit
- Delegates to
self.add_public_keywhich handles both state mutation and event emission
102-112: force_remove_public_keys follows the same correct pattern.Consistent implementation with
force_add_public_keys, properly delegating toself.remove_public_key.tests/src/tests/defuse/accounts/account_sync.rs (2)
44-59: Permission boundary test correctly validates access control.The test properly verifies that unauthorized users receive "Insufficient permissions for method" error before being granted the
UnrestrictedAccountManagerrole.
86-108: Comprehensive verification of both state and event emission.The tests properly verify:
- State changes via
has_public_keyassertions- Event emission via
assert_a_contains_b!for each key operationThis provides good coverage of the force operations' correctness.
Also applies to: 200-222
defuse/src/contract/accounts/mod.rs (2)
40-46: AccountManager methods correctly delegate to Contract methods.The
add_public_keyandremove_public_keytrait implementations now delegate to the newContractmethods, centralizing the logic for state mutation and event emission.Also applies to: 48-54
82-104: Contract methods correctly centralize public key management with event emission.The implementation:
- Performs state mutation via
State::add_public_key/State::remove_public_key, which returnResult<()>- Uses
unwrap_or_panic()to fail fast on invalid operations (duplicate add returnsPublicKeyExistserror, missing remove returnsPublicKeyNotExisterror)- Emits the appropriate
DefuseEventafter successful state change
PublicKeyderivesCopy, so passing it by value to the State method and then borrowing it for the event is correct.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 191-192: The CI is ignoring advisory RUSTSEC-2026-0002 for the lru
crate; instead of masking it, update the lru dependency to >= 0.16.3 by running
`cargo update -p lru` if it's transitive via near-sdk or by adding an explicit
override/patch for the lru crate in Cargo.toml to force version 0.16.3+ (or
replace near-sdk's dependency if necessary), then remove the --ignore
RUSTSEC-2026-0002 flag from the cargo audit invocation so the advisory is no
longer bypassed.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml
⏰ 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). (10)
- GitHub Check: Security Audit - deny
- GitHub Check: Build
- GitHub Check: Build Reproducible
- 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-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: Security Audit - report
🔇 Additional comments (3)
.github/workflows/ci.yml (3)
107-107: LGTM!Consistent version pinning with the Build job ensures both build paths use identical tooling.
74-74: Good: Pinningcargo-nearto a specific version.Pinning the version ensures reproducible builds and prevents unexpected breakages from upstream releases.
165-165: Add cargo-audit version constraint to security checks.Installing
cargo-auditversion 0.22.0 (via^0.22) in the new security audit check job is appropriate. The version exists and is not yanked.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
token-id/src/lib.rs (1)
12-24: Updatecompile_error!message to include "dip5".The
cfgcondition now includesfeature = "dip5", but the error message still only listsnep141,nep171, andnep245. Add"dip5"to the message for consistency.📝 Proposed fix
compile_error!( r#"At least one of these features should be enabled: - "nep141" - "nep171" - "nep245" +- "dip5" "# );
🧹 Nitpick comments (3)
token-id/src/dip5.rs (1)
13-16: Consider aligning field naming with other token ID types.Other token ID types in this crate use
contract_id(e.g.,Nep245TokenId.contract_id,Nep171TokenId.contract_id) and descriptive token field names (mt_token_id,nft_token_id). Usingminter_idand generictoken_iddiverges from this pattern. Consider renaming tocontract_idanddip5_token_idfor consistency.core/src/intents/tokens.rs (2)
534-545: Consider validating thattokensis non-empty.Unlike
Transfer(line 97),MtMintdoesn't validate thatself.tokensis non-empty. An empty mint would be a no-op that wastes gas. Consider adding validation similar toTransfer.📝 Proposed fix
fn execute_intent<S, I>( mut self, owner_id: &AccountIdRef, engine: &mut Engine<S, I>, intent_hash: CryptoHash, ) -> Result<()> where S: State, I: Inspector, { + if self.tokens.is_empty() { + return Err(DefuseError::InvalidIntent); + } + self.tokens = Amounts::new(
564-575: Minor: Avoid double clone oftokenswhen notification is present.
self.tokens.clone()is called at line 566 formt_mintand again at line 572 fornotify_on_transfer. Consider restructuring to reduce cloning.♻️ Proposed optimization
- engine - .state - .mt_mint(self.receiver_id.clone(), self.tokens.clone(), self.memo)?; - - if let Some(notification) = self.notification { - engine.state.notify_on_transfer( - owner_id, - self.receiver_id, - self.tokens.clone(), - notification, - ); - } + let tokens = self.tokens; + engine + .state + .mt_mint(self.receiver_id.clone(), tokens.clone(), self.memo)?; + + if let Some(notification) = self.notification { + engine.state.notify_on_transfer( + owner_id, + self.receiver_id, + tokens, + notification, + ); + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
core/Cargo.tomlcore/src/intents/token_diff.rscore/src/intents/tokens.rsdefuse/src/contract/intents/state.rsdefuse/src/contract/tokens/mod.rsdefuse/src/contract/tokens/nep245/enumeration.rstoken-id/Cargo.tomltoken-id/src/dip5.rstoken-id/src/lib.rs
🧰 Additional context used
🧬 Code graph analysis (3)
token-id/src/lib.rs (4)
token-id/src/dip5.rs (1)
new(20-25)token-id/src/nep245.rs (1)
new(20-25)token-id/src/nep141.rs (1)
new(16-20)token-id/src/nep171.rs (1)
new(20-25)
token-id/src/dip5.rs (1)
token-id/src/lib.rs (4)
fmt(82-101)fmt(106-108)from_str(115-129)display_from_str_roundtrip(231-235)
core/src/intents/tokens.rs (2)
core/src/intents/token_diff.rs (1)
execute_intent(40-102)core/src/intents/mod.rs (4)
execute_intent(77-85)execute_intent(89-103)execute_intent(107-133)new(149-151)
⏰ 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). (10)
- GitHub Check: Security Audit - deny
- GitHub Check: Build
- GitHub Check: Build Reproducible
- 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-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: Security Audit - report
🔇 Additional comments (19)
token-id/Cargo.toml (1)
24-29: LGTM!The
dip5feature follows the same pattern as existing token type features (nep141,nep171,nep245) and is correctly added to the default feature set for consistent behavior.defuse/src/contract/tokens/nep245/enumeration.rs (1)
53-56: LGTM!Dip5 is correctly grouped with Nep141 and Nep245 for ownership resolution, returning
Noneforowner_id. This is appropriate since Dip5 tokens, like fungible tokens, don't have unique per-token ownership semantics like NFTs (Nep171).defuse/src/contract/intents/state.rs (2)
250-257: LGTM!The gas normalization logic correctly ensures
min_gasis always at leastMT_ON_TRANSFER_GAS_MINwhile respecting user-provided values when they exceed the minimum. Theunwrap_orfallback toMT_ON_TRANSFER_GAS_DEFAULTfollowed by.max(MT_ON_TRANSFER_GAS_MIN)is a safe pattern.
354-363: LGTM!The
mt_mintimplementation correctly delegates todepositwith proper type conversion (memo.as_deref()). The#[inline]annotation is appropriate for this simple delegation method.defuse/src/contract/tokens/mod.rs (1)
50-57: LGTM!Dip5 is correctly excluded from the NFT uniqueness check (
total_supply > 1). This allows Dip5 tokens to be deposited multiple times like other fungible token types (Nep141, Nep245), which is the expected behavior.core/src/intents/token_diff.rs (1)
206-214: LGTM!The fee-exemption logic correctly adds Dip5 to the zero-fee tier. Dip5 tokens are unconditionally exempt from fees, aligning with the comment that treats them similarly to NFTs, while Nep245 tokens only get fee exemption when
amount <= 1.core/Cargo.toml (1)
23-28: LGTM!The addition of the
dip5feature todefuse-token-idis consistent with the broader Dip5 integration across the codebase.token-id/src/dip5.rs (2)
42-51: LGTM!The
FromStrimplementation correctly parses theminter_id:token_idformat and handles errors appropriately.
60-73: LGTM!The roundtrip test follows the established pattern in this crate and validates display/parse symmetry.
token-id/src/lib.rs (5)
76-77: LGTM!The
Dip5variant addition follows the established pattern with correct discriminant assignment.
96-99: LGTM!The
Debugimplementation forDip5is consistent with other variants.
126-127: LGTM!The
FromStrimplementation forDip5follows the existing pattern.
167-171: LGTM!The
JsonSchemaexample forDip5is appropriately feature-gated and follows the pattern.
205-208: LGTM!The test case for
dip5roundtrip is correctly feature-gated and follows the established test pattern.core/src/intents/tokens.rs (5)
3-3: LGTM!Import of
Nep245TokenIdis necessary for the token transformation inMtMint.
42-44: LGTM!Moving the gas constants to
NotifyOnTransfercentralizes them for reuse by bothTransferandMtMint.
125-129: LGTM!The simplified notification handling delegates gas management to
notify_on_transfer, as per the refactoring approach.
512-532: LGTM!The
MtMintstruct follows the established pattern fromTransfer, with appropriate serde attributes for thetokensandnotificationfields.
546-554: LGTM!The token transformation correctly wraps each token with
Nep245TokenIdusing the signer as the minter, producing properly prefixed token IDs (e.g.,nep245:<owner_id>:<token_id>).
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/intents/tokens.rs (1)
124-128: Confirm min-gas defaults must be enforced innotify_on_transfercreation logic.The concern is valid. The
NotifyOnTransferstruct definesMT_ON_TRANSFER_GAS_DEFAULT(30 Tgas) andMT_ON_TRANSFER_GAS_MIN(5 Tgas), but the promise creation indefuse/src/contract/tokens/nep245/core.rsuses.with_static_gas(notify.min_gas.unwrap_or_default()), which allocatesGas::default()(0 Tgas) whenmin_gasisNone.Since
NotifyOnTransfer::new(msg)leavesmin_gas: None, callers receive zero gas formt_on_transferinstead of the intended 30 Tgas default. Apply the default and validate the minimum:let min_gas = notify.min_gas .unwrap_or(NotifyOnTransfer::MT_ON_TRANSFER_GAS_DEFAULT) .max(NotifyOnTransfer::MT_ON_TRANSFER_GAS_MIN); .with_static_gas(min_gas)
♻️ Duplicate comments (1)
core/src/intents/mod.rs (1)
73-80: Fix doc comments forImtMintandImtBurnvariants.The comments use
//instead of///(doc comments), and both referenceImtMintinstead of their respective types.📝 Proposed fix
- // See [`ImtMint`] + /// See [`ImtMint`] #[cfg(feature = "imt")] ImtMint(crate::intents::imt::ImtMint), - // See [`ImtBurn`] + /// See [`ImtBurn`] #[cfg(feature = "imt")] ImtBurn(crate::intents::imt::ImtBurn),
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@core/src/intents/tokens.rs`:
- Around line 123-126: The code forwards self.notification directly to
engine.state.notify_on_transfer but the docs and earlier logic expect min_gas
normalization (default 30TGas, minimum 5TGas); update the call site in tokens.rs
so you normalize the gas value on the notification before forwarding: read the
gas field from self.notification, if absent set to the default (30TGas), and
enforce a floor of 5TGas, then construct/replace the notification with the
normalized gas and pass that to engine.state.notify_on_transfer(sender_id,
self.receiver_id, self.tokens, notification). Ensure you modify the same symbols
(self.notification and the argument to engine.state.notify_on_transfer) so
behavior matches the documented default/min rules.
🧹 Nitpick comments (1)
tests/src/tests/defuse/intents/mt_burn.rs (1)
78-114: Duplicate MtBurn assertion can be removed.
The firstassert_a_contains_b!already checks the MtBurn event; the second repeats it.♻️ Suggested cleanup
- assert_a_contains_b!( - a: result.logs().clone(), - b: [ - MtEvent::MtBurn(Cow::Owned(vec![MtBurnEvent { - owner_id: user.id().into(), - token_ids: vec![mt_id.to_string()].into(), - amounts: vec![U128::from(amount)].into(), - memo: Some(memo.into()), - authorized_id: None, - }])) - .to_nep297_event() - .to_event_log(),] - ); + // Duplicate MtBurn assertion removed (already covered above)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@core/src/intents/imt.rs`:
- Around line 94-123: ImtBurn::execute_intent is missing a guard against an
empty tokens collection; add the same empty-token validation used by ImtMint at
the start of execute_intent (check self.tokens.is_empty()) and return the same
error variant that ImtMint uses for empty collections (use the existing
DefuseError variant from ImtMint) before the TokenId::Imt-only check, so empty
burns are rejected early.
♻️ Duplicate comments (3)
core/src/intents/tokens.rs (1)
75-79: Doc claimsmin_gasadjustment but code passes notification unchanged.Lines 77-78 document that
min_gasis adjusted with a minimum of 5TGas, but at line 122 the notification is forwarded directly tonotify_on_transferwithout any normalization. Either restore the min_gas enforcement or update the documentation to reflect the current behavior.Also applies to: 122-126
core/src/intents/imt.rs (2)
42-77: Add validation for empty tokens and length limit.Compared to
Transfer(which rejects empty tokens at line 94 in tokens.rs),ImtMintlacks input validation:
- Empty tokens should be rejected
- Token count should be limited (consistent with deposit limits)
📝 Proposed fix
) -> Result<()> where S: State, I: Inspector, { + if self.tokens.is_empty() { + return Err(DefuseError::InvalidIntent); + } + engine .inspector .on_event(DefuseEvent::ImtMint(Cow::Borrowed(
128-140: Useinto_iter()sinceselfis consumed.The method takes ownership of
selfbut then uses.iter()which borrows. Using.into_iter()would be more idiomatic and avoids the dereference:📝 Proposed fix
fn into_generic_tokens(self, minter_id: &AccountIdRef) -> Amounts<BTreeMap<TokenId, u128>> { Amounts::new( - self.iter() - .map(|(token_id, amount)| { + self.into_iter() + .map(|(token_id, amount)| { let token = defuse_token_id::imt::ImtTokenId::new(minter_id, token_id.to_string()) .into(); - (token, *amount) + (token, amount) }) .collect(), ) }
🧹 Nitpick comments (1)
core/src/intents/imt.rs (1)
80-92: Clarify doc comment forImtBurn.The doc says "Burn a set of tokens minted by signer" but the implementation allows burning any IMT tokens from the signer's balance, regardless of who the original minter was. Consider rewording to avoid confusion:
#[near(serializers = [borsh, json])] #[derive(Debug, Clone)] -/// Burn a set of tokens minted by signer, within the intents contract. +/// Burn IMT tokens from the signer's balance, within the intents contract. pub struct ImtBurn {
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@core/src/intents/tokens.rs`:
- Around line 671-673: ImtBurn::execute_intent builds IMT token IDs using the
signer as the prefix, which is wrong when minter_id differs; change the token-ID
construction call from using signer_id to using self.minter_id (i.e., call
self.tokens.into_generic_tokens(self.minter_id)) so the IMT series is identified
by the actual minter, and keep passing signer_id as the owner to
engine.state.imt_burn(signer_id, tokens, self.memo).
In `@tests/src/tests/defuse/intents/imt_burn.rs`:
- Around line 120-145: The test failed_to_burn_tokens currently calls
env.simulate_and_execute_intents(...).await.unwrap_err() which accepts any
error; instead capture the error into a variable (e.g., let err =
env.simulate_and_execute_intents(...).await.unwrap_err();) and assert the error
matches the IMT-only restriction: check the error message or variant contains
the expected text/enum for "only minted imt tokens can be burned" (or whatever
your runtime returns) to ensure the failure is for ImtBurn rejection of non-IMT
TokenId; update the assertion to use the captured err and a targeted
match/assert (referencing failed_to_burn_tokens, ImtBurn, TokenId/Nep141TokenId,
and env.simulate_and_execute_intents).
♻️ Duplicate comments (1)
core/src/intents/tokens.rs (1)
586-609: Reject empty token sets inImtMintfor consistency withTransfer.
Transferrejects empty token sets, butImtMintstill allows them, resulting in a silent no-op. Consider adding the same guard.🛠️ Proposed fix
{ + if self.tokens.is_empty() { + return Err(DefuseError::InvalidIntent); + } + engine .inspector .on_event(DefuseEvent::ImtMint(Cow::Borrowed(
🧹 Nitpick comments (1)
tests/src/tests/defuse/intents/imt_burn.rs (1)
78-114: Deduplicate MtBurn log assertion.
The secondassert_a_contains_b!repeats the MtBurn event already asserted above. Consider removing it or extracting a shared expected event to reduce noise.♻️ Proposed simplification
- assert_a_contains_b!( - a: result.logs().clone(), - b: [ - MtEvent::MtBurn(Cow::Owned(vec![MtBurnEvent { - owner_id: user.id().into(), - token_ids: vec![mt_id.to_string()].into(), - amounts: vec![U128::from(amount)].into(), - memo: Some(memo.into()), - authorized_id: None, - }])) - .to_nep297_event() - .to_event_log(),] - );
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@core/src/intents/tokens.rs`:
- Around line 651-677: The ImtBurn.execute_intent path currently calls
self.tokens.into_generic_tokens(...) and then engine.state.imt_burn(...) without
rejecting an empty token list; add the same empty-tokens validation used by
Transfer and ImtMint: after computing tokens in execute_intent (the variable
`tokens` from self.tokens.into_generic_tokens(&self.minter_id)?), check
tokens.is_empty() and return the same error used elsewhere for empty token lists
(mirror the Transfer/ImtMint behavior) instead of proceeding to
engine.state.imt_burn.
In `@tests/src/tests/defuse/intents/imt_burn.rs`:
- Around line 102-114: The test asserts an MtBurnEvent with owner_id set to
user.id() but the tokens were minted to and burned by other_user; update the
assertion in the assert_a_contains_b! block so MtBurnEvent.owner_id uses
other_user.id().into() (or remove the assertion if redundant) and ensure
token_ids/amounts/memo still reference mt_id, amount, and memo variables; verify
this change in the assert against result.logs() where MtEvent::MtBurn and
.to_nep297_event().to_event_log() are used.
- Around line 70-76: The assertion checks the wrong account: after minting to
other_user the balance assertion calls env.defuse.mt_balance_of(user.id(),
&mt_id.to_string()) — change the call to use other_user.id() instead so you
assert env.defuse.mt_balance_of(other_user.id(),
&mt_id.to_string()).await.unwrap() == 0; keep mt_id and env.defuse as-is and
only replace user.id() with other_user.id().
♻️ Duplicate comments (2)
core/src/intents/tokens.rs (1)
588-631: Add validation to reject empty tokens inImtMint.
Transferexplicitly validates that tokens are non-empty (line 96), butImtMintdoes not. For consistency and to prevent unintended no-op minting, add the same validation:) -> Result<()> where S: State, I: Inspector, { + if self.tokens.is_empty() { + return Err(DefuseError::InvalidIntent); + } + engine .inspectortests/src/tests/defuse/intents/imt_burn.rs (1)
143-145: Assert the failure reason, not just failure.
unwrap_err()without checking the error type allows false positives. Capture and assert the specific error to ensure the test validates the intended IMT-only restriction.Proposed fix
- env.simulate_and_execute_intents(env.defuse.id(), [withdraw_payload]) - .await - .unwrap_err(); + let err = env + .simulate_and_execute_intents(env.defuse.id(), [withdraw_payload]) + .await + .unwrap_err(); + assert!( + err.to_string().contains("only IMT tokens can be burned") + || err.to_string().contains("InvalidTokenId"), + "unexpected error: {err}" + );
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@core/src/intents/tokens.rs`:
- Around line 606-608: Remove the stray trailing semicolons after the `if` guard
blocks that return `Err(DefuseError::InvalidIntent)` in
core/src/intents/tokens.rs — specifically the `if self.tokens.is_empty() {
return Err(DefuseError::InvalidIntent); };` and the similar guard later — by
deleting the final `;` after the closing `}` so the guard ends with `}` only;
locate these checks by searching for `self.tokens.is_empty()` and the other
`return Err(DefuseError::InvalidIntent)` guard and remove the extra semicolons
to satisfy the linter.
In `@tests/src/tests/defuse/intents/imt_mint.rs`:
- Around line 92-117: The test failed_imt_mint_intent constructs token with
exactly MAX_TOKEN_ID_LEN chars so it doesn't trigger the validation which checks
token_id.len() > MAX_TOKEN_ID_LEN; update the token generation in
failed_imt_mint_intent to create a string longer than MAX_TOKEN_ID_LEN (e.g.,
MAX_TOKEN_ID_LEN + 1) so the token exceeds the allowed length and the
env.simulate_and_execute_intents(...).assert_err_contains("token ID is too
large") assertion will pass.
| } | ||
|
|
||
| #[cfg(feature = "imt")] | ||
| pub mod imt { |
There was a problem hiding this comment.
consider moving to separate module?
| if token_id.len() > MAX_TOKEN_ID_LEN { | ||
| return Err(DefuseError::TokenIdTooLarge(token_id.len())); | ||
| } | ||
|
|
||
| let token = defuse_token_id::imt::ImtTokenId::new(minter_id, token_id).into(); | ||
|
|
||
| Ok((token, amount)) | ||
| }) |
There was a problem hiding this comment.
maybe move
if token_id.len() > MAX_TOKEN_ID_LEN {
return Err(DefuseError::TokenIdTooLarge(token_id.len()));
}
into ImtTokenId::new ?
There was a problem hiding this comment.
token_id length limits are verifier-specific, since TokenId is reused in other crates, i.e. escrow-swap
| #[near] | ||
| impl Contract { | ||
| pub(crate) const DO_NATIVE_WITHDRAW_GAS: Gas = Gas::from_tgas(10); | ||
| pub(crate) const DO_NATIVE_WITHDRAW_GAS: Gas = Gas::from_tgas(12); |
There was a problem hiding this comment.
is it really required? was the previous estimation imprecise?
There was a problem hiding this comment.
Due to the increase in contract size, runtime now requires more gas for its load - 11 is sufficient, but we are reserving 1 morе
| "non-reproducible-wasm", | ||
| "--locked", | ||
| "--features=abi,contract", | ||
| "--features=abi,contract,imt", |
There was a problem hiding this comment.
shouldnt we make it default feature instead?
There was a problem hiding this comment.
For now, this feature will only be available on FAR, so we are not making it the default
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.