feat: Include events in simulation output #135
Conversation
WalkthroughCentralizes event handling with an EventSink (Emit/Record), introduces InspectorImpl (replacing drop-based inspectors), adds lifetime-lifting helpers and IntoStaticIntentEvent, broadens many derives (PartialEq, Eq), rewires contract emission to use emit_defuse_event/postpone_defuse_event, and makes simulation return recorded owned events. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Contract
participant Engine
participant Inspector as InspectorImpl
participant Sink as EventSink
Note over Contract,Sink: Execute flow (Emit mode)
Caller->>Contract: execute_intents(payload)
Contract->>Inspector: InspectorImpl::new(contract.event_sink_handle())
Contract->>Engine: Engine::new(..., &mut Inspector)
Engine->>Inspector: on_event(Dip4Event<'_>)
Inspector->>Sink: consume_event(event.into())
Engine->>Inspector: on_intent_executed(signer, hash, nonce)
Inspector->>Inspector: record IntentEvent(...)
Engine-->>Contract: result
alt inspector.intents_executed not empty
Contract->>Sink: consume_event(Dip4Event::IntentsExecuted(...).into())
end
Contract-->>Caller: outcome
Note over Contract,Sink: Simulation flow (Record mode)
Caller->>Contract: simulate_intents(payload)
Contract->>Sink: record_events_instead_of_emitting()
Contract->>Engine: Engine::new(..., &mut Inspector)
loop simulation
Engine->>Inspector: on_event(...)
Inspector->>Sink: consume_event(event.into())
end
Engine-->>Contract: simulation result
Contract->>Caller: SimulationOutput { events: Sink.recorded_events() }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
pityjllk
force-pushed
the
feat/enrich_sim_output
branch
2 times, most recently
from
66ea229 to
f7fdf9b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
defuse/src/contract/fees.rs (1)
41-45: Inconsistent event emission pattern.The
set_fee_collectormethod uses the direct.emit()pattern whileset_fee(lines 24-27) uses the centralizedself.emit_defuse_eventpattern. For consistency and to fully adopt the EventSink mechanism, this should also use the centralized pattern.Apply this diff to align with the centralized event emission pattern:
- FeeCollectorChangedEvent { + self.emit_defuse_event(FeeCollectorChangedEvent { old_fee_collector: fee_collector.into(), new_fee_collector: Cow::Borrowed(self.fees.fee_collector.as_ref()), - } - .emit(); + }.into());core/src/engine/mod.rs (2)
75-75: Update Engine.min_deadline when recording deadlines
In core/src/engine/mod.rs, afterself.inspector.on_deadline(deadline);add
self.min_deadline = self.min_deadline.min(deadline);so that ExecuteIntentsResult.min_deadline reflects the earliest deadline rather than remaining at
MAX.
94-96: Populate Engine.executed from inspector events. The Engine struct’sexecutedfield (core/src/engine/mod.rs:18) is never populated;inspector.intents_executedis updated in on_intent_executed (line 95) but never merged into executed before it’s returned (line 107).
🧹 Nitpick comments (9)
defuse/src/contract/event_sink.rs (1)
1-31: Remove commented-out duplicate EventSinkThis file is a fully commented duplicate of the core EventSink. It adds noise and risks drift. Prefer deleting the file and relying on defuse_core::EventSink.
core/src/intents/token_diff.rs (1)
87-99: Avoid borrow-of-temporary; use Cow::Owned(vec![...]) for clarity/safetyConstructing a slice from an array temporary and wrapping it in Cow::Borrowed is easy to get wrong. Using Owned Vec avoids lifetime footguns and is negligible overhead for one element.
Apply this refactor:
- engine - .inspector - .on_event(DefuseEvent::TokenDiff(Cow::Borrowed( - [IntentEvent::new( - AccountEvent::new(signer_id, event), - intent_hash, - )] - .as_slice(), - ))); + engine + .inspector + .on_event(DefuseEvent::TokenDiff(Cow::Owned(vec![IntentEvent::new( + AccountEvent::new(signer_id, event), + intent_hash, + )])));tests/src/tests/defuse/intents/mod.rs (1)
201-211: Use the computed intent_hash variable for expected eventsYou already compute intent_hash (Line 202); reuse it instead of recomputing via payload.hash() for clarity.
- let intent_hash = result.intents_executed.first().unwrap().intent_hash; - let expected_events = vec![DefuseEvent::Transfer(Cow::Owned(vec![IntentEvent { - intent_hash: transfer_intent_payload.hash(), + let intent_hash = result.intents_executed.first().unwrap().intent_hash; + let expected_events = vec![DefuseEvent::Transfer(Cow::Owned(vec![IntentEvent { + intent_hash, event: AccountEvent { account_id: env.user1.id().clone().into(), event: Cow::Owned(transfer_intent), }, }]))];defuse/src/contract/mod.rs (1)
12-12: Remove commented-out event_sink module/importLeftover comments can confuse future readers. Since you now use defuse_core::EventSink, these comments can be dropped.
Also applies to: 19-19
core/src/lib.rs (1)
22-56: Solid EventSink design; consider adding a drain methodAPI is clear and minimal. Add a take_recorded_events() to transfer ownership without cloning when the caller is done collecting.
impl EventSink { @@ pub fn recorded_events(&self) -> &[events::DefuseEvent<'static>] { &self.events } + + /// Drain and return recorded events. + pub fn take_recorded_events(&mut self) -> Vec<events::DefuseEvent<'static>> { + std::mem::take(&mut self.events) + } }core/src/accounts.rs (1)
28-51: Consider a trait-based approach to reduce duplication.The specialized methods
into_owned_public_keyandinto_owned_token_diffmanually implement ownership conversion for specific event types. This creates code duplication and an awkward API where callers must know which specialized method to use.Consider introducing a trait like:
pub trait IntoOwnedEvent { type Owned: 'static; fn into_owned_event(self) -> Self::Owned; }Then implement it for
PublicKeyEvent,TokenDiffEvent, etc., and add a generic method:impl<'a, T> AccountEvent<'a, T> where T: IntoOwnedEvent, { pub fn into_owned_event(self) -> AccountEvent<'static, T::Owned> { AccountEvent { account_id: Cow::Owned(self.account_id.into_owned()), event: self.event.into_owned_event(), } } }This would eliminate the need for specialized methods like
into_owned_public_keyandinto_owned_token_diff.core/src/intents/mod.rs (1)
167-174: Optional: Extract common IntentEvent wrapping logic.The three implementations share the pattern of wrapping the converted event in a new
IntentEventwith the sameintent_hash. Consider extracting a helper method to reduce duplication:impl<T> IntentEvent<T> { fn map_event<U>(self, f: impl FnOnce(T) -> U) -> IntentEvent<U> { IntentEvent { intent_hash: self.intent_hash, event: f(self.event), } } }Then each implementation could use:
fn into_static(self) -> Self::Output { self.map_event(|event| event.into_owned_token_diff()) }This is a minor improvement and can be deferred if the current explicit approach is preferred.
Also applies to: 184-188, 197-203
defuse/src/contract/intents/state.rs (1)
302-309: Nit: prefer != over ^ for booleansReadability: inequality makes intent clearer than XOR.
- .inspect(|was_enabled| { - if was_enabled ^ enable { + .inspect(|was_enabled| { + if was_enabled != enable { self.emit_defuse_event(DefuseEvent::SetAuthByPredecessorId(AccountEvent::new( Cow::Borrowed(account_id.as_ref()), SetAuthByPredecessorId { enabled: enable }, ))) } })core/src/engine/inspector.rs (1)
31-33: Optional: avoid cloning recorded eventsIf callers can consume events, expose a draining API on EventSink or return a slice to reduce clones.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
core/Cargo.toml(1 hunks)core/src/accounts.rs(4 hunks)core/src/engine/inspector.rs(2 hunks)core/src/engine/mod.rs(3 hunks)core/src/events.rs(3 hunks)core/src/fees.rs(1 hunks)core/src/intents/account.rs(1 hunks)core/src/intents/mod.rs(3 hunks)core/src/intents/token_diff.rs(2 hunks)core/src/intents/tokens.rs(6 hunks)core/src/lib.rs(1 hunks)defuse/src/contract/accounts/account/mod.rs(3 hunks)defuse/src/contract/accounts/lock.rs(2 hunks)defuse/src/contract/event_sink.rs(1 hunks)defuse/src/contract/fees.rs(1 hunks)defuse/src/contract/intents/execute.rs(0 hunks)defuse/src/contract/intents/mod.rs(2 hunks)defuse/src/contract/intents/simulate.rs(0 hunks)defuse/src/contract/intents/state.rs(4 hunks)defuse/src/contract/mod.rs(3 hunks)defuse/src/intents.rs(1 hunks)nep245/src/events.rs(1 hunks)tests/Cargo.toml(1 hunks)tests/src/tests/defuse/intents/mod.rs(2 hunks)
💤 Files with no reviewable changes (2)
- defuse/src/contract/intents/simulate.rs
- defuse/src/contract/intents/execute.rs
🧰 Additional context used
🧬 Code graph analysis (15)
defuse/src/intents.rs (1)
core/src/events.rs (1)
events(81-85)
core/src/engine/mod.rs (5)
core/src/engine/inspector.rs (1)
new(23-29)core/src/accounts.rs (2)
new(55-60)new(96-98)core/src/intents/mod.rs (1)
new(209-211)core/src/deadline.rs (1)
new(18-20)core/src/engine/state/deltas.rs (4)
new(33-38)new(212-214)finalize(41-43)finalize(233-249)
core/src/intents/token_diff.rs (2)
core/src/accounts.rs (2)
new(55-60)new(96-98)core/src/intents/mod.rs (1)
new(209-211)
core/src/engine/inspector.rs (4)
core/src/events.rs (1)
events(81-85)defuse/src/contract/mod.rs (1)
new(93-103)core/src/accounts.rs (2)
new(55-60)new(96-98)core/src/intents/mod.rs (1)
new(209-211)
defuse/src/contract/accounts/lock.rs (1)
defuse/src/contract/mod.rs (1)
new(93-103)
core/src/lib.rs (2)
core/src/events.rs (1)
events(81-85)defuse/src/contract/mod.rs (1)
new(93-103)
core/src/events.rs (1)
core/src/intents/mod.rs (4)
into_static(155-155)into_static(167-175)into_static(183-188)into_static(196-204)
defuse/src/contract/mod.rs (2)
defuse/src/contract/events.rs (1)
new(9-11)core/src/lib.rs (1)
default(33-38)
defuse/src/contract/accounts/account/mod.rs (5)
defuse/src/contract/intents/state.rs (1)
set_auth_by_predecessor_id(288-310)core/src/engine/state/cached.rs (1)
set_auth_by_predecessor_id(333-347)defuse/src/contract/accounts/account/entry/v1.rs (1)
set_auth_by_predecessor_id(199-213)core/src/engine/state/mod.rs (1)
set_auth_by_predecessor_id(113-113)core/src/engine/state/deltas.rs (1)
set_auth_by_predecessor_id(189-191)
defuse/src/contract/intents/state.rs (5)
core/src/engine/inspector.rs (1)
new(23-29)core/src/engine/mod.rs (1)
new(35-42)defuse/src/contract/accounts/account/mod.rs (1)
new(43-60)defuse/src/contract/mod.rs (1)
new(93-103)defuse/src/contract/accounts/mod.rs (1)
new(98-107)
tests/src/tests/defuse/intents/mod.rs (3)
core/src/events.rs (1)
events(81-85)core/src/accounts.rs (2)
new(55-60)new(96-98)core/src/intents/mod.rs (1)
new(209-211)
core/src/fees.rs (1)
core/src/accounts.rs (2)
into_owned(20-25)into_owned(72-76)
core/src/intents/mod.rs (1)
core/src/events.rs (1)
into_static(91-129)
defuse/src/contract/intents/mod.rs (5)
core/src/events.rs (2)
events(81-85)emit(134-136)core/src/engine/inspector.rs (1)
new(23-29)core/src/engine/mod.rs (1)
new(35-42)defuse/src/contract/mod.rs (1)
new(93-103)nep245/src/events.rs (1)
emit(61-63)
core/src/accounts.rs (1)
core/src/fees.rs (1)
into_owned(218-223)
🔇 Additional comments (27)
core/Cargo.toml (1)
36-36: LGTM!The addition of
schemarsas a workspace dependency correctly enables JsonSchema derivation for event types, supporting the broader API improvements in this PR.tests/Cargo.toml (1)
38-38: LGTM!The addition of
defuse-cryptowithserdefeatures as a dev dependency appropriately supports the test infrastructure changes that require serialization/deserialization of Defuse events and related types.core/src/fees.rs (2)
202-202: LGTM!Adding
PartialEqtoFeeChangedEventenables equality comparisons, which is essential for event recording and testing scenarios introduced by the EventSink mechanism.
210-224: LGTM!The addition of
PartialEqand theinto_ownedmethod forFeeCollectorChangedEventcorrectly implements the static lifetime conversion pattern. The implementation properly converts bothCowfields to owned variants.defuse/src/contract/accounts/lock.rs (2)
27-27: LGTM!The change to use
self.emit_defuse_eventcorrectly adopts the centralized event emission pattern introduced with the EventSink mechanism.
42-42: LGTM!The change to use
self.emit_defuse_eventcorrectly adopts the centralized event emission pattern introduced with the EventSink mechanism.defuse/src/contract/fees.rs (1)
24-27: LGTM!The
set_feemethod correctly adopts the centralized event emission pattern usingself.emit_defuse_event.defuse/src/intents.rs (1)
37-38: LGTM!The addition of the
eventsfield toSimulationOutputcorrectly implements the PR's main objective: including emitted events in simulation results. The use ofDefuseEvent<'static>ensures proper ownership semantics for the recorded events.defuse/src/contract/accounts/account/mod.rs (3)
153-161: All direct callers emit SetAuthByPredecessorId eventsBoth in core/src/intents/account.rs (calling .on_event(AccountEvent::new(…))) and in defuse/src/contract/intents/state.rs (self.emit_defuse_event(DefuseEvent::SetAuthByPredecessorId(…))) events are emitted as expected; no further changes required.
64-66:add_public_keyevents are emitted centrally
Theadd_public_keycall in defuse/src/contract/intents/state.rs wraps the state change with anInspectthat emits thePublicKeyAddedevent, covering all contract call sites.
82-84: Event emission verified. Both the entry modules (v0/v1) and the contract intent layer emitPublicKeyRemovedafter removal; the internal no-op implementation is by design.core/src/intents/account.rs (2)
76-76: PartialEq derive on SetAuthByPredecessorId looks goodNo behavior change; enables equality in tests/comparisons.
96-99: Do not wrap SetAuthByPredecessorId in IntentEvent:DefuseEvent::SetAuthByPredecessorIdexpects a plainAccountEvent, so emitting viaengine.inspector.on_event(AccountEvent::new(signer_id, self).into());is correct and intentional.
Likely an incorrect or invalid review comment.
core/src/intents/token_diff.rs (1)
121-121: PartialEq on TokenDiffEvent is appropriateEnables equality checks for tests and comparisons; no behavior change.
core/src/events.rs (1)
1-128: Looks good.
into_staticleverages the new helper cleanly and covers every variant; no issues spotted.core/src/accounts.rs (2)
11-11: LGTM: PartialEq derives enable event comparisons.Adding
PartialEqtoAccountEvent,PublicKeyEvent, andNonceEventis appropriate for testing and event verification in the simulation output feature.Also applies to: 65-65, 88-88
19-26: LGTM: Lifetime bound correctly constrains generic conversion.The change from
impl<T>toimpl<T: 'static>is correct becauseinto_ownedreturnsAccountEvent<'static, T>, which requiresTto have a'staticlifetime.core/src/intents/mod.rs (2)
152-205: LGTM: IntoStaticIntentEvent enables static lifetime conversion.The trait and its implementations correctly handle ownership conversion for different event types, supporting the simulation output feature's need for static-lifetime events.
The three implementations appropriately handle:
- Generic
Cow<'a, T>eventsTokenDiffEventwith its specialized conversionNonceEventwhich doesn't require event conversion
14-14: LGTM: AccountEvent import supports new trait implementations.The public import of
AccountEventis necessary for theIntoStaticIntentEventtrait implementations added below.core/src/intents/tokens.rs (1)
26-26: LGTM: derive PartialEq for intent structsEnables clean equality checks in tests and downstream code. No behavior change.
Also applies to: 74-74, 169-169, 264-264, 366-366, 414-414
defuse/src/contract/intents/state.rs (3)
2-5: LGTM: imports updated to new public pathsAligns with core module reshuffle (accounts/events/intents/token_id).
98-106: LGTM: emit PublicKeyAdded on successEmits only after successful mutation via inspect; avoids false positives.
117-124: LGTM: emit PublicKeyRemoved on successSame safe pattern; event reflects actual state change.
core/src/engine/inspector.rs (4)
22-29: LGTM: InspectorImpl constructorSane defaults; Rc<RefCell> fits single-threaded contract context.
42-44: LGTM: into_static before consumingConverting to 'static prevents lifetime issues from borrowed, ephemeral events.
38-40: LGTM: min-deadline aggregationUses min to track tightest deadline across intents.
53-57: LGTM: executed-intent recordingCorrect composition of IntentEvent<AccountEvent> with owned signer id.
pityjllk
force-pushed
the
feat/enrich_sim_output
branch
2 times, most recently
from
5f80954 to
8e8b647
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
defuse/src/contract/fees.rs (1)
44-48: RouteFeeCollectorChangedEventthrough the sink
set_feenow emits viaself.emit_defuse_event, but the collector update still calls.emit(). When simulations flip the sink into record mode, this direct emit skips the sink and the event never shows up in the recorded output. Please funnel it through the sink as well.- FeeCollectorChangedEvent { - old_fee_collector: fee_collector.into(), - new_fee_collector: Cow::Borrowed(self.fees.fee_collector.as_ref()), - } - .emit(); + self.emit_defuse_event( + FeeCollectorChangedEvent { + old_fee_collector: fee_collector.into(), + new_fee_collector: Cow::Borrowed(self.fees.fee_collector.as_ref()), + } + .into(), + );defuse/src/contract/intents/state.rs (1)
104-118: Fix movedpublic_keywhen emittingPublicKeyAdded
account.add_public_key(&account_id, public_key)takes ownership ofpublic_key, yet the closure and the error constructor reuse the same variable.defuse_core::crypto::PublicKeyis notCopy, so this path does not compile. Clone (or otherwise retain) the key before passing it into the account and reuse the original for the event/error.- self.accounts + let added = self + .accounts .get_or_create(account_id.clone()) .get_mut() .ok_or_else(|| DefuseError::AccountLocked(account_id.clone()))? - .add_public_key(&account_id, public_key) - .then_some(()) - .inspect(|_| { - self.emit_defuse_event(DefuseEvent::PublicKeyAdded(AccountEvent::new( - Cow::Borrowed(account_id.as_ref()), - PublicKeyEvent { - public_key: Cow::Borrowed(&public_key), - }, - ))) - }) - .ok_or(DefuseError::PublicKeyExists(account_id, public_key)) + .add_public_key(&account_id, public_key.clone()); + + if !added { + return Err(DefuseError::PublicKeyExists(account_id, public_key)); + } + + self.emit_defuse_event(DefuseEvent::PublicKeyAdded(AccountEvent::new( + Cow::Borrowed(account_id.as_ref()), + PublicKeyEvent { + public_key: Cow::Borrowed(&public_key), + }, + ))); + + Ok(())Do the same restructuring for the surrounding return so the function still yields
Result<()>.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
core/src/accounts.rs(5 hunks)core/src/engine/inspector.rs(2 hunks)core/src/events.rs(3 hunks)core/src/lib.rs(1 hunks)defuse/src/contract/accounts/account/mod.rs(4 hunks)defuse/src/contract/accounts/force.rs(2 hunks)defuse/src/contract/fees.rs(1 hunks)defuse/src/contract/intents/mod.rs(2 hunks)defuse/src/contract/intents/state.rs(3 hunks)defuse/src/contract/mod.rs(3 hunks)defuse/src/contract/salts.rs(3 hunks)defuse/src/intents.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- defuse/src/intents.rs
- defuse/src/contract/mod.rs
🧰 Additional context used
🧬 Code graph analysis (6)
core/src/engine/inspector.rs (4)
core/src/events.rs (1)
events(84-88)defuse/src/contract/mod.rs (1)
new(124-136)core/src/accounts.rs (2)
new(55-60)new(96-98)core/src/intents/mod.rs (1)
new(209-211)
core/src/lib.rs (2)
core/src/events.rs (1)
events(84-88)defuse/src/contract/mod.rs (1)
new(124-136)
defuse/src/contract/intents/mod.rs (3)
core/src/events.rs (2)
events(84-88)emit(138-140)core/src/engine/inspector.rs (1)
new(28-34)defuse/src/contract/mod.rs (1)
new(124-136)
defuse/src/contract/accounts/account/mod.rs (5)
defuse/src/contract/intents/state.rs (1)
set_auth_by_predecessor_id(296-298)core/src/engine/state/mod.rs (1)
set_auth_by_predecessor_id(116-116)core/src/engine/state/cached.rs (1)
set_auth_by_predecessor_id(333-347)core/src/engine/state/deltas.rs (1)
set_auth_by_predecessor_id(194-196)defuse/src/contract/accounts/account/entry/v1.rs (1)
set_auth_by_predecessor_id(199-213)
core/src/events.rs (1)
core/src/intents/mod.rs (4)
into_static(155-155)into_static(167-175)into_static(183-188)into_static(196-204)
core/src/accounts.rs (1)
core/src/fees.rs (1)
into_owned(218-223)
🪛 GitHub Check: Check
core/src/engine/inspector.rs
[failure] 40-40:
this .into_iter() call is equivalent to .iter() and will not consume the slice
[failure] 28-28:
this could be a const fn
core/src/events.rs
[failure] 76-76:
item in documentation is missing backticks
core/src/accounts.rs
[failure] 11-11:
you are deriving PartialEq and can implement Eq
[failure] 70-70:
the following explicit lifetimes could be elided: 'a
[failure] 65-65:
you are deriving PartialEq and can implement Eq
[failure] 88-88:
you are deriving PartialEq and can implement Eq
[failure] 103-103:
you are deriving PartialEq and can implement Eq
⏰ 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). (6)
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Security Audit - report
pityjllk
force-pushed
the
feat/enrich_sim_output
branch
from
e01a180 to
93fd38c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
defuse/src/contract/mod.rs (1)
84-86: LGTM!The
event_sinkfield with#[borsh(skip)]is correctly excluded from storage. Since theContractis instantiated fresh for each NEAR call, theEventSinkis also fresh, which means callingrecord_events_instead_of_emitting()in simulation paths won't affect subsequent execution calls.defuse/src/contract/intents/mod.rs (1)
44-52: Event recording pattern is correct.The past review comment suggested binding a guard to keep record mode active. However, the current implementation correctly switches the mode on line 46 and maintains it for the simulation duration because:
event_sinkhas#[borsh(skip)]inContract, so it's instantiated fresh per NEAR call- The mode switch persists for the entire
simulate_intentscall- Events are successfully captured via
inspector.get_events()on line 52The pattern works due to NEAR's fresh-contract-per-call semantics.
🧹 Nitpick comments (1)
core/src/intents/mod.rs (1)
179-208: LGTM!Both specialized implementations correctly handle lifetime conversion for their respective event types:
- TokenDiffEvent delegates to the specialized
into_owned_token_diffmethod- NonceEvent only needs to convert the account_id since NonceEvent itself has no lifetime parameters
The clippy allow directives are appropriate for the same reason as the first implementation.
While the current approach is clear and correct, you could potentially reduce duplication in the future by introducing a trait like
IntoOwnedAccountEventthat each nested event type implements, enabling a single blanket implementation. However, this is purely optional and may not be worth the complexity given the small number of implementations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
core/src/accounts.rs(5 hunks)core/src/deadline.rs(1 hunks)core/src/engine/inspector.rs(2 hunks)core/src/events.rs(3 hunks)core/src/fees.rs(1 hunks)core/src/intents/account.rs(1 hunks)core/src/intents/mod.rs(3 hunks)core/src/intents/token_diff.rs(2 hunks)core/src/intents/tokens.rs(6 hunks)core/src/lib.rs(1 hunks)defuse/src/contract/accounts/force.rs(2 hunks)defuse/src/contract/intents/mod.rs(2 hunks)defuse/src/contract/intents/state.rs(3 hunks)defuse/src/contract/mod.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- core/src/intents/account.rs
- core/src/intents/token_diff.rs
- core/src/fees.rs
- defuse/src/contract/intents/state.rs
🧰 Additional context used
🧬 Code graph analysis (8)
defuse/src/contract/mod.rs (2)
core/src/engine/inspector.rs (1)
new(28-34)core/src/lib.rs (1)
default(33-38)
core/src/intents/mod.rs (1)
core/src/events.rs (1)
into_static(94-133)
core/src/events.rs (1)
core/src/intents/mod.rs (4)
into_static(155-155)into_static(168-176)into_static(185-190)into_static(199-207)
core/src/engine/inspector.rs (5)
core/src/events.rs (1)
events(84-88)core/src/accounts.rs (2)
new(55-60)new(96-98)core/src/deadline.rs (1)
new(18-20)core/src/intents/mod.rs (1)
new(212-214)defuse/src/contract/mod.rs (1)
new(124-136)
defuse/src/contract/accounts/force.rs (1)
core/src/accounts.rs (2)
new(55-60)new(96-98)
core/src/accounts.rs (1)
core/src/fees.rs (1)
into_owned(218-223)
core/src/lib.rs (3)
core/src/events.rs (1)
events(84-88)core/src/engine/inspector.rs (1)
new(28-34)defuse/src/contract/mod.rs (1)
new(124-136)
defuse/src/contract/intents/mod.rs (3)
core/src/engine/inspector.rs (1)
new(28-34)defuse/src/contract/mod.rs (1)
new(124-136)core/src/engine/mod.rs (1)
new(27-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared security analysis / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Security Audit - report
🔇 Additional comments (21)
core/src/deadline.rs (1)
25-25: LGTM!Dereferencing
BLOCK_TIMESTAMPis more direct than cloning forCopytypes. The change is correct and slightly more efficient.core/src/intents/tokens.rs (1)
26-26: LGTM!Adding
PartialEqandEqderives enables equality comparisons for these intent types, which is necessary for event storage, testing, and the newEventSinkinfrastructure introduced in this PR.Also applies to: 74-74, 169-169, 264-264, 366-366, 414-414
defuse/src/contract/mod.rs (1)
88-100: LGTM!The three helper methods correctly forward event operations to the underlying
EventSink:
emit_defuse_event: Consumes events in the current mode (Emit or Record)record_events_instead_of_emitting: Switches to Record mode for simulationsevent_sink_handle: Provides shared access forInspectorImplcore/src/lib.rs (1)
22-57: LGTM!The
EventSinkdesign cleanly separates emit and record modes:
- Emit mode: Events are emitted immediately via
emit()- Record mode: Events are converted to
'staticlifetime and stored for later retrievalThe lifetime lifting via
into_static()ensures recorded events can outlive borrowed data, which is essential for simulation output.defuse/src/contract/accounts/force.rs (1)
94-101: LGTM!The event emission correctly fires only when the auth-by-predecessor-id state changes (line 95:
was_enabled ^ enable). UsingCow::Borrowedfor the account reference is efficient and aligns with the event payload pattern used throughout the codebase.core/src/events.rs (2)
19-23: LGTM!Adding
PartialEqandEqenables event comparison, which is useful for testing and validation. The conditionalJsonSchemaderive appropriately restricts schema generation to ABI builds.
75-134: LGTM!The
into_staticimplementation correctly lifts allDefuseEventvariants to'staticlifetime by converting borrowed data to owned. The helper functionconvert_eventscleanly appliesIntoStaticIntentEventto event slices, enabling theEventSinkto store events beyond their original borrow scopes.defuse/src/contract/intents/mod.rs (1)
53-76: LGTM!The refactored simulation output correctly handles both success and invariant-violation cases:
- Both paths populate
eventsfrominspector.get_events()Okreturns withinvariant_violated: NoneInvariantViolatedreturns with the specific violation value- Other errors panic as expected
This consolidates event collection to a single path and eliminates the previous double-execution pattern.
core/src/engine/inspector.rs (2)
21-39: LGTM!
InspectorImplprovides a clean concrete implementation with:
- Public
intents_executedtracking- Shared
event_sinkviaRc<RefCell<>>- Deadline tracking via
min_deadlineget_events()to retrieve recorded eventsThe
Rc<RefCell<>>pattern is safe in NEAR's single-threaded smart contract environment.
41-65: LGTM!The
Inspectortrait implementation correctly:
- Updates
min_deadlineinon_deadline- Converts events to
'staticlifetime before storing inon_event(line 50)- Records intent execution with owned data in
on_intent_executed(line 61:to_owned())All lifetime handling ensures data can outlive borrowed scopes for simulation output.
core/src/accounts.rs (8)
11-11: LGTM!Adding PartialEq and Eq derives enables equality comparison for AccountEvent, which is useful for testing and event inspection.
28-36: LGTM!The specialized implementation correctly handles lifetime conversion for
AccountEvent<PublicKeyEvent>, converting both the account_id and the nested event to owned forms.
38-51: LGTM!The specialized implementation correctly handles lifetime conversion for
AccountEvent<TokenDiffEvent>, converting the account_id and token diff to owned forms while preserving the fees_collected field.
65-65: LGTM!Adding PartialEq and Eq derives enables equality comparison for PublicKeyEvent.
70-77: LGTM!The
into_ownedimplementation correctly converts borrowed PublicKey data to owned form, following the same pattern asFeeCollectorChangedEvent::into_ownedin core/src/fees.rs.
88-88: LGTM!Adding PartialEq and Eq derives enables equality comparison for NonceEvent.
103-103: LGTM!Adding PartialEq and Eq derives enables equality comparison for SaltRotationEvent.
19-26: Verify constraint tightening impact
No direct.into_owned()calls onAccountEventdetected; confirm no callers depend on the former unconstrained impl for non-'staticT.core/src/intents/mod.rs (3)
14-14: LGTM!The AccountEvent import is necessary for the new trait implementations.
143-143: LGTM!Adding PartialEq and Eq derives enables equality comparison for IntentEvent, which is useful for testing and event inspection.
152-177: LGTM!The trait design is clean and the implementation correctly handles lifetime conversion for generic Cow-wrapped event types. The clippy allow directive is appropriate since the Output type has a different lifetime parameter than Self.
pityjllk
force-pushed
the
feat/enrich_sim_output
branch
from
93fd38c to
54d8cce
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
defuse/src/contract/intents/mod.rs (1)
44-77: Clarify relationship to past guard-based review comment.The simulation flow correctly:
- Switches to record mode (line 46)
- Creates inspector with shared event sink (line 48)
- Collects recorded events (line 52)
- Includes events in both success and error paths
However, a past review comment suggests this should use a guard pattern that reverts mode on drop. The current implementation uses a persistent mode change instead. If this design change is intentional, ensure the mode is properly isolated or reset after simulation to prevent subsequent
execute_intentscalls from recording instead of emitting.This concern is related to the verification requested for
defuse/src/contract/mod.rslines 93-95. If the EventSink mode isn't properly isolated between simulate and execute operations, events may not be emitted correctly.
🧹 Nitpick comments (2)
core/src/intents/token_diff.rs (1)
87-99: Borrowed slice/event rely on lifetime lifting; consider Owned vec for safetyYou’re borrowing both the event (diff: Cow::Borrowed(&self)) and a temporary slice ([...] .as_slice()). This is fine only if Inspector.on_event converts to 'static immediately.
If you want a safer pattern with negligible overhead, use Owned Vec:
Apply within this block:
- engine - .inspector - .on_event(DefuseEvent::TokenDiff(Cow::Borrowed( - [IntentEvent::new( - AccountEvent::new(signer_id, event), - intent_hash, - )] - .as_slice(), - ))); + engine + .inspector + .on_event(DefuseEvent::TokenDiff(Cow::Owned(vec![ + IntentEvent::new(AccountEvent::new(signer_id, event), intent_hash), + ])));Please also confirm Inspector/EventSink lift to 'static when receiving borrowed events.
core/src/intents/tokens.rs (1)
53-61: Borrowed slice/event rely on lifting; consider a small helper to DRYThe borrowed slice pattern depends on Inspector.on_event lifting to 'static. If that’s guaranteed, fine; otherwise prefer Cow::Owned(vec![...]).
Also, this emission boilerplate repeats across intents; consider a tiny helper like emit_single_intent_event(engine.inspector, DefuseEvent::Transfer, AccountEvent::new(...), intent_hash) to reduce duplication.
Applies similarly to FtWithdraw, NftWithdraw, MtWithdraw, NativeWithdraw, and StorageDeposit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
core/src/accounts.rs(5 hunks)core/src/deadline.rs(1 hunks)core/src/engine/inspector.rs(2 hunks)core/src/events.rs(3 hunks)core/src/fees.rs(1 hunks)core/src/intents/account.rs(1 hunks)core/src/intents/mod.rs(3 hunks)core/src/intents/token_diff.rs(2 hunks)core/src/intents/tokens.rs(6 hunks)core/src/lib.rs(1 hunks)defuse/src/contract/accounts/force.rs(2 hunks)defuse/src/contract/intents/mod.rs(2 hunks)defuse/src/contract/intents/state.rs(3 hunks)defuse/src/contract/mod.rs(3 hunks)tests/src/tests/defuse/intents/mod.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- core/src/fees.rs
- core/src/engine/inspector.rs
- core/src/lib.rs
- defuse/src/contract/accounts/force.rs
🧰 Additional context used
🧬 Code graph analysis (8)
defuse/src/contract/intents/state.rs (2)
core/src/accounts.rs (2)
new(55-60)new(96-98)defuse/src/contract/mod.rs (1)
new(124-136)
tests/src/tests/defuse/intents/mod.rs (4)
core/src/accounts.rs (2)
new(55-60)new(96-98)core/src/intents/mod.rs (1)
new(212-214)ton-connect/src/lib.rs (2)
hash(103-105)hash(201-203)core/src/payload/multi.rs (1)
hash(62-72)
defuse/src/contract/mod.rs (2)
core/src/engine/inspector.rs (1)
new(28-34)core/src/lib.rs (1)
default(33-38)
core/src/intents/mod.rs (1)
core/src/events.rs (1)
into_static(94-133)
core/src/events.rs (1)
core/src/intents/mod.rs (4)
into_static(155-155)into_static(168-176)into_static(185-190)into_static(199-207)
core/src/intents/token_diff.rs (4)
core/src/accounts.rs (2)
new(55-60)new(96-98)core/src/engine/inspector.rs (1)
new(28-34)core/src/intents/mod.rs (1)
new(212-214)defuse/src/contract/mod.rs (1)
new(124-136)
defuse/src/contract/intents/mod.rs (5)
core/src/events.rs (2)
events(84-88)emit(138-140)core/src/engine/inspector.rs (1)
new(28-34)defuse/src/contract/mod.rs (1)
new(124-136)nep245/src/events.rs (1)
emit(60-62)defuse/src/intents.rs (1)
simulate_intents(20-20)
core/src/accounts.rs (1)
core/src/fees.rs (1)
into_owned(218-223)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build
- GitHub Check: Security Audit - deny
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-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 (28)
core/src/deadline.rs (1)
25-25: LGTM! Efficient use of dereference for Copy type.Replacing
.clone()with*dereference is the idiomatic approach forCopytypes likeDateTime<Utc>, eliminating unnecessary clone overhead.core/src/intents/account.rs (2)
76-76: Derive additions are okayAdding PartialEq/Eq is additive and keeps API consistent with other intents.
96-99: No changes needed for SetAuthByPredecessorId event emission
DefuseEvent::SetAuthByPredecessorId is defined to accept AccountEvent directly; intent_hash and IntentEvent wrapping apply only to token intents.Likely an incorrect or invalid review comment.
defuse/src/contract/intents/state.rs (1)
110-117: Good: emit events only on successEmitting PublicKeyAdded/Removed inside inspect after successful mutation is correct and consistent with the sink pattern.
Confirm emit_defuse_event stores DefuseEvent::into_static() to avoid holding borrows of local account_id/public_key.
Also applies to: 129-136
core/src/intents/token_diff.rs (1)
121-121: Derive additions are okayPartialEq/Eq on TokenDiffEvent is consistent with broader event equality semantics.
tests/src/tests/defuse/intents/mod.rs (1)
179-192: Strong event assertion in simulationPrecomputing a signed transfer and asserting result.events against an explicit DefuseEvent::Transfer payload is clear and robust. Nice coverage.
Also applies to: 201-210
core/src/intents/tokens.rs (1)
26-26: Derive additions are okayPartialEq/Eq across public intent structs aligns with the PR’s event/test needs.
Also applies to: 74-74, 169-169, 265-265, 366-366, 414-414
defuse/src/contract/mod.rs (5)
16-20: LGTM: EventSink integration imports.The imports properly bring in the necessary types for the EventSink integration pattern. Using
Rc<RefCell<...>>for shared interior mutability aligns with theInspectorImplpattern shown in the relevant snippets.
84-86: LGTM: EventSink field declaration.The
event_sinkfield is correctly marked with#[borsh(skip)]since it's runtime-only state that shouldn't be persisted. TheRc<RefCell<EventSink>>wrapper enables shared mutable access across the contract and inspector.
88-91: LGTM: Event forwarding method.The
emit_defuse_eventmethod correctly forwards events to the internal sink viaborrow_mut().consume_event(), centralizing event handling through the EventSink.
97-99: LGTM: EventSink handle accessor.The
event_sink_handlemethod properly returns a clonedRcreference, enabling the inspector and other components to share access to the same EventSink instance.
132-132: LGTM: EventSink initialization.The EventSink is correctly initialized with
EventSink::default()wrapped inRc::new(RefCell::new(...))during contract construction, establishing the shared mutable access pattern.core/src/events.rs (4)
10-10: LGTM: IntoStaticIntentEvent import.The import properly brings in the trait for converting intent events to static lifetimes, enabling the lifetime lifting pattern implemented in
into_static.
19-23: LGTM: Event derives and schema integration.The addition of
PartialEqandEqenables event comparison for testing and verification. The conditionalJsonSchemaderive is properly gated behind the "abi" feature for non-wasm32 targets, aligning with ABI exposure requirements.
76-90: LGTM: Efficient event conversion helper.The
convert_eventshelper method efficiently converts event slices to static lifetimes using theIntoStaticIntentEventtrait. The generic bounds ensure the method works with any event type implementing the trait, reducing code duplication ininto_static.
94-133: LGTM: Comprehensive lifetime lifting.The
into_staticmethod comprehensively handles allDefuseEventvariants, converting borrowed data to owned data with static lifetimes. Each variant is correctly mapped to its static equivalent using the appropriate conversion methods (into_owned_public_key,into_owned_token_diff,convert_events, etc.).defuse/src/contract/intents/mod.rs (1)
24-40: LGTM: Explicit event emission in execute flow.The
execute_intentsimplementation correctly:
- Creates
InspectorImplwith the shared event sink handle- Emits MT events via
as_mt_event()- Explicitly emits
DefuseEvent::IntentsExecutedwhen intents were executed, replacing the prior inspector-drop side-effect patternThe explicit emission approach (lines 35-39) is clearer and more maintainable than relying on drop-time side effects.
core/src/accounts.rs (5)
11-11: LGTM: Event comparison derives.Adding
Debug,Clone,PartialEq, andEqtoAccountEventenables event comparison for testing and verification scenarios, aligning with the broader event equality pattern.
19-26: LGTM: Static lifetime bound and generic into_owned.The
T: 'staticbound ensures event data has a static lifetime, which is appropriate for the genericinto_ownedmethod. This method correctly converts borrowed account IDs to owned while preserving the event data.
28-51: LGTM: Specialized lifetime conversion methods.The specialized
into_owned_public_keyandinto_owned_token_diffmethods correctly handle lifetime lifting for specific event types:
into_owned_public_keydelegates toPublicKeyEvent::into_owned()into_owned_token_diffexplicitly converts theTokenDiffEventby owning the diff and preserving other fieldsBoth methods follow the established pattern for converting
AccountEvent<'a, T>toAccountEvent<'static, T>.
70-77: LGTM: PublicKeyEvent lifetime conversion.The
into_ownedmethod onPublicKeyEventcorrectly converts the borrowedpublic_keyto an ownedCow::Ownedvariant, enabling static lifetime conversion.
88-88: LGTM: Consistent event derives.Adding
PartialEqandEqtoNonceEventandSaltRotationEventmaintains consistency across event types, enabling comparison and testing scenarios.Also applies to: 103-103
core/src/intents/mod.rs (6)
14-14: LGTM: AccountEvent import.The import properly brings in
AccountEventto support the specializedIntoStaticIntentEventimplementations that work withAccountEvent-wrapped intent events.
143-143: LGTM: IntentEvent equality derives.Adding
PartialEqandEqtoIntentEventenables event comparison, supporting testing and verification scenarios. This aligns with the derives added to other event types in this PR.
152-156: LGTM: IntoStaticIntentEvent trait design.The trait provides a clean abstraction for converting borrowed
IntentEventtypes to owned static-lifetime versions. TheOutputassociated type allows different implementations to return different static types.
158-177: LGTM: Generic Cow-wrapped conversion.The implementation for
IntentEvent<AccountEvent<'a, Cow<'a, T>>>correctly:
- Preserves the
intent_hash- Converts
account_idto owned viainto_owned()- Converts the Cow-wrapped event to owned via
into_owned()The
#[allow(clippy::use_self)]annotation is appropriate since theOutputtype has a different lifetime ('static) thanSelf(lifetime'a).
179-191: LGTM: TokenDiffEvent conversion.The implementation for
IntentEvent<AccountEvent<'a, TokenDiffEvent<'a>>>correctly delegates tointo_owned_token_diff()fromcore/src/accounts.rs, which handles the specialized conversion for token diff events. The clippy allow is appropriate for the lifetime difference.
193-208: LGTM: NonceEvent conversion.The implementation for
IntentEvent<AccountEvent<'_, NonceEvent>>correctly:
- Preserves the
intent_hash- Converts
account_idto owned- Reuses the
NonceEventdirectly (since it's already'static)The clippy allow is appropriate for the lifetime difference between
'_and'static.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
core/src/event_sink.rs (4)
3-6: Derive basic traits on EventMode for ergonomicsRecommend deriving Debug, Clone, Copy, PartialEq, Eq for logging, comparisons, and cheap copies.
-pub enum EventMode { +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum EventMode { Emit, Record, }
8-11: Add Debug on EventSink (keep custom Default)Helpful for tests/logging without exposing internals.
-pub struct EventSink { +#[derive(Debug)] +pub struct EventSink { mode: EventMode, events: Vec<events::DefuseEvent<'static>>, }Also applies to: 13-20
34-37: Consider a draining accessor to avoid borrow conflictsRecorded slice is fine; add a take/drain method for ownership without long-lived borrows (useful with Rc<RefCell>).
Outside this hunk:
impl EventSink { pub fn take_recorded_events(&mut self) -> Vec<events::DefuseEvent<'static>> { std::mem::take(&mut self.events) } }
30-32: Removeconstfrom mutating setter
&mut selfin aconst fnadds no benefit here (workspace MSRV 1.86.0 already supportsconst_mut_refs). Replace with:- pub const fn record_only_mode(&mut self) { + pub fn record_only_mode(&mut self) { self.mode = EventMode::Record; }Optionally add a general
set_mode(&mut self, mode: EventMode)and/or builder constructors for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/event_sink.rs(1 hunks)core/src/lib.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/src/event_sink.rs (1)
core/src/events.rs (1)
events(84-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests
🔇 Additional comments (2)
core/src/lib.rs (1)
6-6: Re-export looks goodPublicly exposing event_sink and re-exporting its items is consistent with existing pattern here.
Also applies to: 13-13
core/src/event_sink.rs (1)
22-28: consume_event logic is clearEmits in Emit mode; stores into_static only when recording. Looks correct.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
defuse/src/contract/mod.rs (1)
44-76: Simulation leaves the shared EventSink stuck in record mode
record_events_instead_of_emitting()flips the shared sink toRecord, but this path never restores it. Becauseevent_sink_handle()is reused byexecute_intents, the very first simulation causes every subsequent on-chain execution (includingIntentsExecuted) to be silently queued instead of emitted. On top of that, the recorded queue is never drained, so later simulations re-deliver stale events. Please wrap the recording window in an RAII guard (or at least restoreEventMode::Emitand drain the queue before returning) so the sink always returns to emit mode once the simulator has copied out its events.Apply this patch (relies on the guard added in
core/src/event_sink.rsbelow):- // NOTE: applies only to events routed through Contract::emit_defuse_event - self.record_events_instead_of_emitting(); - - let mut inspector = InspectorImpl::new(self.event_sink_handle()); + // NOTE: applies only to events routed through Contract::emit_defuse_event + let event_sink = self.event_sink_handle(); + let mut recording = event_sink.borrow_mut().enter_record_mode(); + + let mut inspector = InspectorImpl::new(Rc::clone(&event_sink)); let engine = Engine::new(self.cached(), &mut inspector); let result = engine.execute_signed_intents(signed); - let events = inspector.get_events(); + let events = recording.take_recorded_events();
🧹 Nitpick comments (3)
defuse/src/contract/accounts/account/entry/v0.rs (1)
69-73: Clean up unused test importsDip4Event, AccountEvent, and PublicKeyEvent appear unused in this test-only module. Consider removing to avoid warnings.
defuse/src/contract/accounts/force.rs (1)
31-31: Minor: unify AccountEvent account_id passing styleFor consistency/zero-copy, consider using Cow::Borrowed(account_id.as_ref()) in both lock/unlock emissions (as done in internal_set_auth_by_predecessor_id).
Also applies to: 46-46
defuse/src/contract/events.rs (1)
1-26: Remove or gate the commented-out PostponedMtBurnEventsThis file is entirely commented-out now. Prefer deleting it or gating behind cfg(feature)/#[cfg(test)] to avoid drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
core/src/engine/inspector.rs(1 hunks)core/src/event_sink.rs(1 hunks)core/src/events.rs(2 hunks)core/src/intents/token_diff.rs(3 hunks)core/src/intents/tokens.rs(13 hunks)defuse/src/contract/accounts/account/entry/v0.rs(3 hunks)defuse/src/contract/accounts/account/entry/v1.rs(3 hunks)defuse/src/contract/accounts/force.rs(4 hunks)defuse/src/contract/events.rs(1 hunks)defuse/src/contract/fees.rs(3 hunks)defuse/src/contract/intents/mod.rs(2 hunks)defuse/src/contract/intents/state.rs(3 hunks)defuse/src/contract/mod.rs(4 hunks)defuse/src/contract/salts.rs(3 hunks)defuse/src/contract/tokens/mod.rs(2 hunks)defuse/src/contract/tokens/nep245/core.rs(2 hunks)defuse/src/contract/tokens/nep245/resolver.rs(2 hunks)nep245/src/events.rs(0 hunks)tests/src/tests/defuse/intents/mod.rs(2 hunks)
💤 Files with no reviewable changes (1)
- nep245/src/events.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- defuse/src/contract/fees.rs
- defuse/src/contract/intents/state.rs
- core/src/intents/token_diff.rs
🧰 Additional context used
🧬 Code graph analysis (9)
defuse/src/contract/accounts/account/entry/v1.rs (1)
core/src/events.rs (1)
events(121-125)
defuse/src/contract/accounts/force.rs (2)
defuse/src/contract/mod.rs (1)
new(120-131)core/src/accounts.rs (2)
new(55-60)new(96-98)
core/src/engine/inspector.rs (4)
core/src/events.rs (1)
events(121-125)defuse/src/contract/mod.rs (1)
new(120-131)core/src/accounts.rs (2)
new(55-60)new(96-98)core/src/intents/mod.rs (1)
new(212-214)
defuse/src/contract/intents/mod.rs (4)
core/src/events.rs (2)
events(121-125)emit(27-32)core/src/engine/inspector.rs (1)
new(25-31)defuse/src/contract/mod.rs (1)
new(120-131)core/src/engine/mod.rs (1)
new(27-32)
defuse/src/contract/mod.rs (2)
core/src/engine/inspector.rs (1)
new(25-31)core/src/event_sink.rs (1)
default(17-23)
core/src/intents/tokens.rs (1)
core/src/events.rs (1)
events(121-125)
core/src/events.rs (1)
core/src/intents/mod.rs (4)
into_static(155-155)into_static(168-176)into_static(185-190)into_static(199-207)
core/src/event_sink.rs (3)
core/src/events.rs (1)
events(121-125)core/src/engine/inspector.rs (1)
new(25-31)defuse/src/contract/mod.rs (1)
new(120-131)
tests/src/tests/defuse/intents/mod.rs (3)
core/src/accounts.rs (2)
new(55-60)new(96-98)core/src/intents/mod.rs (1)
new(212-214)core/src/payload/multi.rs (1)
hash(62-72)
🪛 GitHub Check: Format
core/src/engine/inspector.rs
[warning] 42-42:
Diff in /home/runner/work/intents/intents/core/src/engine/inspector.rs
[warning] 1-1:
Diff in /home/runner/work/intents/intents/core/src/engine/inspector.rs
core/src/events.rs
[warning] 50-50:
Diff in /home/runner/work/intents/intents/core/src/events.rs
[warning] 27-27:
Diff in /home/runner/work/intents/intents/core/src/events.rs
[warning] 18-18:
Diff in /home/runner/work/intents/intents/core/src/events.rs
[warning] 145-145:
Diff in /home/runner/work/intents/intents/core/src/events.rs
core/src/event_sink.rs
[warning] 33-33:
Diff in /home/runner/work/intents/intents/core/src/event_sink.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared security analysis / Static Analysis (intents-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 (4)
defuse/src/contract/accounts/account/entry/v0.rs (1)
98-101: Confirm inverted return semantics for add/remove public keyBoth methods now return the negation of maybe_* results (true on no-op, false on change). Verify this flip is intentional for test expectations; otherwise, it can silently invert logic for callers.
Also applies to: 116-118
defuse/src/contract/tokens/nep245/core.rs (1)
197-210: LGTM: centralized MtTransfer emission via emit_defuse_eventEmission payload and lifetimes look correct; aligns with the new event sink path.
defuse/src/contract/tokens/nep245/resolver.rs (1)
105-118: LGTM: refund MtTransfer routed through emit_defuse_eventFields (owner swap, token_ids, amounts, memo) are correct for refunds; uses borrowed data to avoid copies.
core/src/events.rs (1)
112-171: Well-structured lifetime conversion implementation.The
into_static()method andconvert_events()helper correctly handle allDip4Eventvariants, properly converting borrowed data to owned data for static lifetime. The implementation leverages theIntoStaticIntentEventtrait effectively and covers all variants comprehensively.
There was a problem hiding this comment.
Fix closure arg type in Result::inspect (was_enabled is &bool, not bool)
This won’t compile as written. Deref or pattern-match the reference.
Apply:
- .inspect(|was_enabled| {
- if was_enabled ^ enable {
+ .inspect(|&was_enabled| {
+ if was_enabled ^ enable {
self.emit_defuse_event(Dip4Event::SetAuthByPredecessorId(AccountEvent::new(
Cow::Borrowed(account_id.as_ref()),
SetAuthByPredecessorId { enabled: enable },
)).into());
}
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .inspect(|was_enabled| { | |
| if was_enabled ^ enable { | |
| self.emit_defuse_event(Dip4Event::SetAuthByPredecessorId(AccountEvent::new( | |
| Cow::Borrowed(account_id.as_ref()), | |
| SetAuthByPredecessorId { enabled: enable }, | |
| )).into()); | |
| } | |
| }) | |
| .inspect(|&was_enabled| { | |
| if was_enabled ^ enable { | |
| self.emit_defuse_event(Dip4Event::SetAuthByPredecessorId(AccountEvent::new( | |
| Cow::Borrowed(account_id.as_ref()), | |
| SetAuthByPredecessorId { enabled: enable }, | |
| )).into()); | |
| } | |
| }) |
🤖 Prompt for AI Agents
In defuse/src/contract/accounts/force.rs around lines 94 to 101, the closure
passed to Result::inspect treats was_enabled as a bool but it is &bool; change
the closure to accept a reference or dereference the value (e.g. use
|&was_enabled| or |was_enabled: &bool| and then use was_enabled, or keep
|was_enabled| and use *was_enabled in the XOR expression) so the comparison
becomes (*was_enabled) ^ enable (or was_enabled ^ enable if pattern-matching),
ensuring the code compiles.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
defuse/src/contract/accounts/force.rs (1)
98-108: Fix closure parameter type mismatch.The closure receives
was_enabledas&boolfromResult::inspect, but Line 99 attempts to use it directly in an XOR operation withenable: bool. This will not compile because the types don't match.Apply this diff:
- .inspect(|was_enabled| { - if was_enabled ^ enable { + .inspect(|&was_enabled| { + if was_enabled ^ enable { self.emit_defuse_event( Dip4Event::SetAuthByPredecessorId(AccountEvent::new( Cow::Borrowed(account_id.as_ref()), SetAuthByPredecessorId { enabled: enable }, )) .into(), ); } })defuse/src/contract/mod.rs (1)
80-82: EventSink mode isolation issue persistsThe shared
EventSink(viaRc) is mutated toRecordmode insimulate_intents(defuse/src/contract/intents/mod.rs:46) but never restored toEmitmode. This causes subsequentexecute_signed_intentscalls to record instead of emit events.The previous review suggested creating a fresh EventSink for simulations or restoring the mode after. Consider implementing an RAII guard in
EventSinkthat:
- Saves the current mode on construction
- Switches to
Recordmode- Restores the original mode on drop
This would require updating both
event_sink.rsto provide the guard andintents/mod.rsto bind it to a local variable.core/src/engine/inspector.rs (1)
36-38: Event accumulation: drain instead of clone
get_events()clones recorded events without clearing them from the shared EventSink, causing accumulation across simulations. Since the sink is shared viaRc<RefCell>, events from previous simulations remain and get duplicated.Switch to draining the events:
pub fn get_events(&self) -> Vec<DefuseEvent<'static>> { - self.event_sink.borrow().recorded_events() + self.event_sink.borrow_mut().drain_recorded_events() }Note: This requires adding a
drain_recorded_events()method to EventSink in core/src/event_sink.rs:pub fn drain_recorded_events(&mut self) -> Vec<events::DefuseEvent<'static>> { self.recorded_events.drain(..).collect() }core/src/event_sink.rs (2)
34-42: Reverse event ordering: use push_back instead of push_frontLine 40 uses
push_frontwhen recording events, which reverses their chronological order. Events should be appended to maintain FIFO ordering.Apply this fix:
EventMode::Record => self.recorded_events.push_front(event.into_owned()), + EventMode::Record => self.recorded_events.push_back(event.into_owned()),
44-46: Missing mode restoration mechanism
record_only_mode()permanently switches toRecordmode with no way to restoreEmitmode. This is why simulations break subsequent execute calls (see defuse/src/contract/mod.rs:80-82 comment).Implement an RAII guard pattern:
-pub const fn record_only_mode(&mut self) { - self.mode = EventMode::Record; -} +pub fn enter_record_mode(&mut self) -> EventRecordingGuard<'_> { + self.recorded_events.clear(); + EventRecordingGuard { + sink: self, + previous_mode: std::mem::replace(&mut self.mode, EventMode::Record), + } +}Add the guard struct:
pub struct EventRecordingGuard<'a> { sink: &'a mut EventSink, previous_mode: EventMode, } impl Drop for EventRecordingGuard<'_> { fn drop(&mut self) { self.sink.mode = self.previous_mode; self.sink.recorded_events.clear(); } }Then update defuse/src/contract/mod.rs to return the guard from
record_events_instead_of_emitting()and defuse/src/contract/intents/mod.rs to bind it:let _guard = self.record_events_instead_of_emitting();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
core/Cargo.toml(2 hunks)core/src/engine/inspector.rs(1 hunks)core/src/event_sink.rs(1 hunks)core/src/events.rs(2 hunks)core/src/intents/tokens.rs(13 hunks)defuse/src/contract/accounts/force.rs(4 hunks)defuse/src/contract/events.rs(0 hunks)defuse/src/contract/fees.rs(3 hunks)defuse/src/contract/intents/mod.rs(2 hunks)defuse/src/contract/intents/state.rs(3 hunks)defuse/src/contract/mod.rs(4 hunks)defuse/src/contract/salts.rs(3 hunks)defuse/src/contract/tokens/nep245/core.rs(1 hunks)nep245/Cargo.toml(1 hunks)nep245/src/events.rs(2 hunks)tests/src/tests/defuse/intents/mod.rs(2 hunks)
💤 Files with no reviewable changes (1)
- defuse/src/contract/events.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- defuse/src/contract/fees.rs
- defuse/src/contract/salts.rs
🧰 Additional context used
🧬 Code graph analysis (10)
tests/src/tests/defuse/intents/mod.rs (4)
core/src/events.rs (1)
events(123-127)core/src/engine/inspector.rs (1)
new(28-34)core/src/accounts.rs (2)
new(55-60)new(96-98)core/src/intents/mod.rs (1)
new(212-214)
core/src/intents/tokens.rs (4)
core/src/events.rs (1)
events(123-127)core/src/engine/inspector.rs (1)
new(28-34)core/src/accounts.rs (2)
new(55-60)new(96-98)core/src/intents/mod.rs (1)
new(212-214)
core/src/event_sink.rs (3)
core/src/events.rs (1)
events(123-127)defuse/src/contract/mod.rs (1)
new(119-130)core/src/engine/inspector.rs (1)
new(28-34)
defuse/src/contract/accounts/force.rs (1)
core/src/accounts.rs (2)
new(55-60)new(96-98)
defuse/src/contract/mod.rs (2)
core/src/engine/inspector.rs (1)
new(28-34)core/src/event_sink.rs (1)
default(17-23)
defuse/src/contract/intents/mod.rs (3)
core/src/events.rs (2)
events(123-127)emit(27-32)defuse/src/contract/mod.rs (1)
new(119-130)core/src/engine/inspector.rs (1)
new(28-34)
defuse/src/contract/intents/state.rs (2)
defuse/src/contract/mod.rs (1)
new(119-130)core/src/accounts.rs (2)
new(55-60)new(96-98)
core/src/events.rs (4)
nep245/src/events.rs (1)
into_owned(62-107)core/src/accounts.rs (2)
into_owned(20-25)into_owned(72-76)core/src/fees.rs (1)
into_owned(218-223)core/src/intents/mod.rs (4)
into_static(155-155)into_static(168-176)into_static(185-190)into_static(199-207)
core/src/engine/inspector.rs (4)
core/src/events.rs (1)
events(123-127)defuse/src/contract/mod.rs (1)
new(119-130)core/src/accounts.rs (2)
new(55-60)new(96-98)core/src/intents/mod.rs (1)
new(212-214)
nep245/src/events.rs (3)
core/src/fees.rs (1)
into_owned(218-223)core/src/events.rs (3)
into_owned(36-41)into_owned(133-166)events(123-127)core/src/accounts.rs (2)
into_owned(20-25)into_owned(72-76)
🪛 GitHub Actions: Contract Security Analysis
nep245/src/events.rs
[error] 60-60: Clippy: explicit lifetimes could be elided. The lifetime 'a' in 'impl<'a> MtEvent<'a>' can be elided to 'impl MtEvent<'_>'.
[error] 61-61: Clippy: item in documentation is missing backticks. Consider changing to: "/// Convert MtEvent to 'static lifetime by converting all borrowed data to owned".
🪛 GitHub Check: Check
nep245/src/events.rs
[failure] 61-61:
item in documentation is missing backticks
[failure] 60-60:
the following explicit lifetimes could be elided: 'a
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Security Audit - report
🔇 Additional comments (20)
defuse/src/contract/tokens/nep245/core.rs (1)
197-211: LGTM! Event emission properly centralized.The change correctly routes the MtTransfer event through the centralized
emit_defuse_eventhandler with proper.into()conversion, aligning with the PR's unified event emission pathway.core/Cargo.toml (1)
36-36: LGTM! Dependency and feature additions support ABI generation.The
schemarsdependency anddefuse-nep245/abifeature inclusion properly enable JsonSchema support for event types when theabifeature is enabled.Also applies to: 45-45
nep245/Cargo.toml (1)
11-11: LGTM! ABI feature properly configured.The optional
schemarsdependency andabifeature are correctly structured to enable JsonSchema support for NEP-245 event types when needed.Also applies to: 13-14
defuse/src/contract/intents/state.rs (2)
110-120: LGTM! PublicKeyAdded event properly emitted.The event emission correctly wraps the public key lifecycle in a
Dip4Event::PublicKeyAddedwith appropriate borrowed references and routes it through the centralized emission handler.
132-142: LGTM! PublicKeyRemoved event properly emitted.The event emission correctly wraps the public key removal in a
Dip4Event::PublicKeyRemovedwith appropriate borrowed references, maintaining consistency with the addition pathway.tests/src/tests/defuse/intents/mod.rs (1)
179-213: LGTM! Test properly validates simulation events.The test correctly:
- Constructs and signs a
Transferintent with proper payload handling- Builds expected
Dip4Event::Transferevents withIntentEventandAccountEventwrappers- Validates that
simulate_intentsreturns the expected events- Confirms view method behavior by checking balances remain unchanged
nep245/src/events.rs (1)
9-12: LGTM! JsonSchema derive properly feature-gated.The conditional derive for
JsonSchemais correctly configured to enable ABI generation only when theabifeature is enabled and not targeting wasm32.defuse/src/contract/accounts/force.rs (2)
31-33: LGTM! AccountLocked event properly emitted.The event emission correctly uses
Dip4Event::AccountLockedwith properAccountEventwrapping, aligning with the centralized event handling pattern.
48-50: LGTM! AccountUnlocked event properly emitted.The event emission correctly uses
Dip4Event::AccountUnlockedwith properAccountEventwrapping, maintaining consistency with the lock pathway.core/src/intents/tokens.rs (2)
26-26: LGTM! PartialEq and Eq derives added consistently.All token intent structs (
Transfer,FtWithdraw,NftWithdraw,MtWithdraw,NativeWithdraw,StorageDeposit) now correctly derivePartialEqandEq, enabling equality comparisons and supporting the broader event validation infrastructure.Also applies to: 72-72, 167-167, 262-262, 364-364, 412-412
53-59: LGTM! Event emissions properly migrated to Dip4Event.All token intent executions correctly emit events through
Dip4Eventvariants with proper:
- Use of
Cow::Borrowedfor borrowed event dataIntentEventwrapping with intent hashAccountEventwrapping with account ID and intent reference- Consistent pattern across all token intent types
Also applies to: 154-160, 249-255, 351-357, 388-394, 438-444
defuse/src/contract/mod.rs (1)
84-100: LGTM! Clean event delegation APIThe four methods provide a clear interface for event emission, postponement, mode switching, and handle access. The implementation correctly delegates to the underlying EventSink.
core/src/engine/inspector.rs (2)
21-25: LGTM! Clean inspector implementationThe struct and constructor properly initialize the inspector with an event sink handle and reasonable defaults (empty intents_executed, Deadline::MAX).
Also applies to: 27-34
41-63: LGTM! Proper event forwardingThe Inspector trait implementation correctly:
- Tracks minimum deadline
- Forwards Dip4Event to the EventSink
- Records intent execution with proper event wrapping
core/src/event_sink.rs (1)
16-24: LGTM! Solid foundationThe Default implementation, postpone_event logic, and Drop implementation for emitting postponed events are all correct.
Also applies to: 27-32, 54-59
defuse/src/contract/intents/mod.rs (2)
25-39: LGTM! Clean inspector integrationThe refactored
execute_intentsproperly:
- Creates InspectorImpl with the shared event sink
- Passes mutable inspector to Engine
- Explicitly emits IntentsExecuted event when intents were processed
- Handles MT events separately
44-77: Simulation flow looks good, modulo the mode isolation issueThe simulation correctly:
- Switches to record mode (line 46)
- Creates inspector with shared sink
- Executes intents on cached state
- Collects events from inspector
- Returns comprehensive SimulationOutput
However, the mode isolation issue (flagged in defuse/src/contract/mod.rs:80-82 and core/src/event_sink.rs:44-46) means the mode isn't restored after simulation completes.
Once the RAII guard is implemented, bind it here:
- self.record_events_instead_of_emitting(); + let _guard = self.record_events_instead_of_emitting();core/src/events.rs (3)
35-42: LGTM! Correct lifetime liftingThe
into_owned()implementation properly converts both DefuseEvent variants to'staticlifetime by delegating to their respectiveinto_owned()methods. This resolves the previousunimplemented!()stub.
18-54: LGTM! Clean event wrapper designThe DefuseEvent wrapper with Dip4Event and Nep241Event variants provides good separation of concerns. The
emit()andinto_owned()methods, along with the From implementations, offer a clean API.
114-167: LGTM! Comprehensive lifetime conversionThe
into_owned()method systematically handles all Dip4Event variants, using theconvert_eventshelper for intent event collections and delegating to type-specificinto_owned()methods for account/fee events. This ensures all borrowed data is properly converted to owned.
| pub fn recorded_events(&self) -> Vec<events::DefuseEvent<'static>> { | ||
| self.recorded_events.iter().cloned().collect() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Pair drain with the guard pattern
Once the RAII guard is implemented (see comment on lines 44-46), add a drain_recorded_events() method and update recorded_events() or remove it entirely:
pub fn drain_recorded_events(&mut self) -> Vec<events::DefuseEvent<'static>> {
self.recorded_events.drain(..).collect()
}This pairs with the inspector change in core/src/engine/inspector.rs:36-38.
🤖 Prompt for AI Agents
In core/src/event_sink.rs around lines 49-51, replace or remove the existing
recorded_events() that clones the vec and instead add a
drain_recorded_events(&mut self) method which drains self.recorded_events
(self.recorded_events.drain(..).collect()) and returns
Vec<events::DefuseEvent<'static>>; update callers to use the mutable drain
method and remove the cloning-based recorded_events() or keep it deprecated,
ensuring this pairs with the RAII guard changes mentioned for the inspector.
fb8ee09 to
6c757c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
core/src/events.rs (1)
18-24: Verify code formatting.A past review comment flagged formatting inconsistencies in this file. Please ensure
cargo fmthas been run on this file.Run the following script to check formatting:
#!/bin/bash # Check if formatting is needed cargo fmt --package core -- --check core/src/events.rs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
core/src/events.rs(2 hunks)core/src/intents/token_diff.rs(4 hunks)defuse/src/contract/accounts/account/entry/v0.rs(3 hunks)defuse/src/contract/accounts/account/entry/v1.rs(4 hunks)defuse/src/contract/tokens/nep141/withdraw.rs(1 hunks)nep245/src/events.rs(5 hunks)tests/src/tests/defuse/intents/ft_withdraw.rs(1 hunks)tests/src/tests/defuse/intents/native_withdraw.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- defuse/src/contract/accounts/account/entry/v0.rs
- nep245/src/events.rs
🧰 Additional context used
🧬 Code graph analysis (5)
core/src/intents/token_diff.rs (3)
core/src/engine/inspector.rs (1)
new(28-34)core/src/accounts.rs (2)
new(55-60)new(96-98)core/src/intents/mod.rs (1)
new(212-214)
tests/src/tests/defuse/intents/native_withdraw.rs (2)
core/src/engine/state/deltas.rs (3)
new(33-38)new(217-219)iter(403-420)core/src/event_sink.rs (1)
default(17-23)
tests/src/tests/defuse/intents/ft_withdraw.rs (3)
core/src/events.rs (3)
events(123-127)from(45-47)from(51-53)tests/src/tests/defuse/env.rs (1)
builder(45-47)tests/src/utils/mt.rs (3)
mt_contract_balance_of(53-58)mt_contract_balance_of(211-226)mt_contract_balance_of(425-434)
defuse/src/contract/accounts/account/entry/v1.rs (3)
defuse/src/contract/intents/state.rs (1)
set_auth_by_predecessor_id(302-304)defuse/src/contract/accounts/account/mod.rs (1)
set_auth_by_predecessor_id(150-158)core/src/engine/state/cached.rs (1)
set_auth_by_predecessor_id(333-347)
core/src/events.rs (4)
nep245/src/events.rs (1)
into_owned(62-107)core/src/accounts.rs (2)
into_owned(20-25)into_owned(72-76)core/src/fees.rs (1)
into_owned(218-223)core/src/intents/mod.rs (4)
into_static(155-155)into_static(168-176)into_static(185-190)into_static(199-207)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build
- GitHub Check: Security Audit - deny
- GitHub Check: Shared security analysis / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Security Audit - report
🔇 Additional comments (8)
defuse/src/contract/tokens/nep141/withdraw.rs (1)
18-18: Import tweak looks goodSwitching to
env::{self}keeps the module in scope and aligns with the broader refactor; no further action needed.defuse/src/contract/accounts/account/entry/v1.rs (4)
62-62: LGTM! Import simplification aligns with event emission removal.The removal of event-related imports is consistent with the broader refactor to centralize event handling, as this legacy V1 implementation no longer emits events directly.
88-90: LGTM! Negation bug fixed.The removal of the negation operator correctly restores standard Rust collection semantics. The method now returns
truewhen a key is successfully added andfalsewhen already present.This addresses the critical issue previously flagged in past reviews.
106-108: LGTM! Negation bug fixed.The removal of the negation operator correctly restores standard Rust collection semantics. The method now returns
truewhen a key is successfully removed andfalsewhen not present.This addresses the critical issue previously flagged in past reviews.
169-177: LGTM! Appropriate use of unused parameter convention.Renaming the parameter to
_meis idiomatic Rust for intentionally unused parameters. This silences compiler warnings while maintaining API signature compatibility with other implementations in the codebase.core/src/intents/token_diff.rs (1)
89-101: LGTM! Event emission refactoring is well-structured.The changes correctly:
- Construct
TokenDiffEventbefore callingon_event- Wrap it in
Dip4Event::TokenDiffwith properIntentEventandAccountEventcontainers- Use
Cow::Borrowedfor efficient memory usageThis aligns well with the broader event handling refactoring in this PR.
core/src/events.rs (2)
114-167: LGTM! Comprehensive lifetime conversion implementation.The
into_owned()method properly handles allDip4Eventvariants by:
- Converting borrowed account IDs and event data to owned versions
- Using the
convert_eventshelper for intent event slices- Leveraging existing
into_ownedmethods on nested event types- Ensuring all returned data has
'staticlifetimeThis implementation is thorough and correctly supports the simulation output feature.
35-42: No remainingDefuseEvent::into_staticusages
Confirmed rename tointo_owned()and absence of any lingeringDefuseEvent::into_staticcalls.
| Dip4Event::IntentsExecuted(_) | ||
| )); | ||
|
|
||
| // NativeWtithdraw emits mt_burn event taht should be posponed |
There was a problem hiding this comment.
Fix typo in comment.
"NativeWtithdraw" should be "NativeWithdraw".
Apply this diff:
- // NativeWtithdraw emits mt_burn event taht should be posponed
+ // NativeWithdraw emits mt_burn event that should be postponed🤖 Prompt for AI Agents
In tests/src/tests/defuse/intents/native_withdraw.rs around line 223, the inline
comment contains typos: "NativeWtithdraw" and "taht" should be corrected to
"NativeWithdraw" and "that" respectively; update the comment text to read
something like "// NativeWithdraw emits mt_burn event that should be postponed"
(fix spelling and casing) so the comment is clear and correct.
| DefuseEvent::PublicKeyRemoved(AccountEvent::new( | ||
| Cow::Borrowed(me), | ||
| PublicKeyEvent { | ||
| public_key: Cow::Borrowed(public_key), | ||
| }, | ||
| )) | ||
| .emit(); |
There was a problem hiding this comment.
moved to intents state
| DefuseEvent::SetAuthByPredecessorId(AccountEvent::new( | ||
| Cow::Borrowed(me), | ||
| SetAuthByPredecessorId { enabled: enable }, | ||
| )) |
There was a problem hiding this comment.
moved to intents state
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/src/tests/defuse/intents/ft_withdraw.rs (1)
482-487: Track the CachedState limitation.The TODO comment indicates that
CachedStateprevents some calls from being forwarded to the actual contract implementation that emits events. This limitation means the burn event assertion is disabled.Consider creating a tracking issue to address the
CachedStateforwarding limitation so that full event coverage can be validated. Do you want me to help draft an issue description for this?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/src/tests/defuse/intents/ft_withdraw.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/src/tests/defuse/intents/ft_withdraw.rs (4)
core/src/intents/mod.rs (1)
new(212-214)tests/src/utils/mt.rs (3)
mt_contract_balance_of(53-58)mt_contract_balance_of(211-226)mt_contract_balance_of(425-434)core/src/payload/multi.rs (1)
hash(62-72)core/src/payload/raw.rs (1)
hash(28-30)
🪛 GitHub Check: Check
tests/src/tests/defuse/intents/ft_withdraw.rs
[failure] 472-472:
the trait bound Cow<'_, FtWithdraw>: From<FtWithdraw> is not satisfied
[failure] 464-464:
unused import: std::borrow::Cow
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Shared / 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-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared security analysis / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Security Audit - report
🔇 Additional comments (2)
tests/src/tests/defuse/intents/ft_withdraw.rs (2)
396-402: Test is no longer ignored.The test declaration looks good. Unlike the previous review's concern, this version is not marked with
#[ignore], suggesting theCachedStateforwarding issues may have been addressed or the test has been adjusted to work around them.
472-472: Remove incorrect trait bound suggestion
Theeventfield is genericT(inferred asFtWithdraw), and Rust’s blanketimpl From<T> for Tmakes.into()valid. No change needed.Likely an incorrect or invalid review comment.
|
|
||
| // Assert that exactly one FtWithdraw event is emitted with the expected fields | ||
| use defuse::core::{accounts::AccountEvent, intents::IntentEvent}; | ||
| use std::borrow::Cow; |
There was a problem hiding this comment.
Remove unused import.
The std::borrow::Cow import is unused and should be removed.
Apply this diff:
- use std::borrow::Cow;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use std::borrow::Cow; |
🧰 Tools
🪛 GitHub Check: Check
[failure] 464-464:
unused import: std::borrow::Cow
🤖 Prompt for AI Agents
In tests/src/tests/defuse/intents/ft_withdraw.rs around line 464, the import
"use std::borrow::Cow;" is unused; remove that import line from the file (and
run cargo check/format if desired) so the unused import warning is eliminated.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/src/tests/defuse/intents/ft_withdraw.rs (1)
464-471: Consider verifying simulation has no side effects.The test validates that events are recorded correctly, but doesn't verify that simulation didn't actually modify state. Consider capturing and asserting that user1's balance remains 1000 after simulation to ensure the simulation is truly read-only.
Add balance verification after simulation:
+ // Verify simulation didn't modify state + assert_eq!( + env.mt_contract_balance_of(env.defuse.id(), env.user1.id(), &ft1.to_string()) + .await + .unwrap(), + 1000, + "Simulation should not modify balances" + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/src/tests/defuse/intents/ft_withdraw.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/src/tests/defuse/intents/ft_withdraw.rs (6)
tests/src/tests/defuse/intents/mod.rs (4)
intents(62-62)simulate_intents(50-53)simulate_intents(116-121)simulate_intents(150-155)core/src/events.rs (3)
events(123-127)from(45-47)from(51-53)tests/src/tests/defuse/env.rs (2)
builder(45-47)new(49-51)tests/src/utils/mt.rs (3)
mt_contract_balance_of(53-58)mt_contract_balance_of(211-226)mt_contract_balance_of(425-434)core/src/payload/multi.rs (1)
hash(62-72)defuse/src/contract/intents/mod.rs (1)
simulate_intents(44-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Security Audit - deny
- GitHub Check: Build
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared / Static Analysis (intents-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-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 (1)
tests/src/tests/defuse/intents/ft_withdraw.rs (1)
473-478: Clarify or remove outdated TODO comment.Past reviews indicate that CachedState forwarding issues were addressed in commit 51289ff, yet this TODO comment remains. Additionally, the commented-out code references
Nep245Event::MtBurn, which appears to be from the old event system before the Dip4Event migration.Please either:
- Remove the TODO if the issue was fully resolved
- Update the TODO to clarify what specific aspect still needs attention
- Remove the commented-out code that references the deprecated event system
| #[trace] | ||
| async fn ft_withdraw_simulate_records_events(#[notrace] mut rng: impl Rng) { | ||
| use defuse::core::events::Dip4Event; | ||
| use defuse_crypto::Payload; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify if Payload import is necessary.
The Payload import appears unused in this test. The test uses MultiPayload (returned by sign_defuse_message) and calls .hash() on it, which doesn't require importing Payload directly.
Run the following script to verify usage:
🏁 Script executed:
#!/bin/bash
# Description: Check if Payload type is used in this test function beyond the import
# Search for Payload usage in the test function
rg -n 'Payload' tests/src/tests/defuse/intents/ft_withdraw.rs -A 3 -B 1Length of output: 138
Inspect the TODO at lines 473–478 to confirm if it’s still relevant:
🏁 Script executed:
sed -n '470,490p' tests/src/tests/defuse/intents/ft_withdraw.rsLength of output: 287
Remove unused import and resolve the TODO.
- Delete
use defuse_crypto::Payload;(unused). - Drop or implement the
// TODO: Due to CachedState…block so it no longer causes confusion. - (Optional) Add assertions that balances remain unchanged and validate other
SimulationOutputfields.
🤖 Prompt for AI Agents
In tests/src/tests/defuse/intents/ft_withdraw.rs around line 403, remove the
unused import "use defuse_crypto::Payload;" and eliminate the confusing "//
TODO: Due to CachedState…" block by either implementing the intended
cached-state handling or more simply deleting that TODO block; then add
assertions to the test that token balances remain unchanged after the simulation
and validate key SimulationOutput fields (status/result, events, and any
returned balances) to make the test deterministic and clear.
47f7e6c to
de3e556
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
tests/src/tests/defuse/intents/native_withdraw.rs (1)
223-223: Typo already flagged.The typos in the comment have already been identified in previous review comments.
defuse/src/contract/mod.rs (1)
93-95: Event sink mode isolation issue previously flagged.The past review comment correctly identifies that
record_events_instead_of_emitting()permanently switches the shared EventSink to Record mode without providing a restoration mechanism, causing subsequentexecute_intentscalls to record instead of emit events.core/src/event_sink.rs (3)
40-40: Event ordering issue previously flagged.The past review comment correctly identifies that
push_frontreverses event chronological order. Events should be appended withpush_backto maintain FIFO ordering.
44-46: Mode restoration issue previously flagged.The past review comment correctly identifies that
record_only_mode()lacks an RAII guard pattern to automatically restore the previous mode, which causes the mode isolation problems seen in defuse/src/contract/mod.rs.
49-51: Event retrieval inefficiency previously flagged.The past review comment correctly suggests adding a
drain_recorded_events()method to avoid cloning the entire event vector.defuse/src/contract/intents/mod.rs (1)
45-46: Event recording mode issue previously flagged.The past review comment about keeping a guard alive is correct in principle, but the actual issue is that
record_events_instead_of_emitting()doesn't return a guard (see core/src/event_sink.rs lines 44-46). This is a symptom of the broader mode isolation problem already flagged in those files.core/src/events.rs (1)
18-24: Apply code formatting.Static analysis has flagged formatting inconsistencies in this file. Please run
cargo fmtto ensure consistent code style throughout the file.#!/bin/bash # Format the file cargo fmt --package core -- core/src/events.rs
🧹 Nitpick comments (1)
core/src/intents/token_diff.rs (1)
89-101: Consider a helper function for event emission ergonomics.The event emission code is quite nested:
engine.inspector.on_event( Dip4Event::TokenDiff( Cow::Borrowed( [IntentEvent::new( AccountEvent::new(signer_id, event), intent_hash, )].as_slice(), ) ) );While this nesting is necessary given the type structure, consider introducing a helper method to improve readability:
// In a utility module or as a method on Engine/Inspector fn emit_token_diff_event<'a>( inspector: &mut impl Inspector, signer_id: &'a AccountIdRef, event: TokenDiffEvent<'a>, intent_hash: CryptoHash, ) { inspector.on_event( Dip4Event::TokenDiff(Cow::Borrowed( [IntentEvent::new(AccountEvent::new(signer_id, event), intent_hash)].as_slice(), )) ); }This would simplify the call site to:
emit_token_diff_event(&mut engine.inspector, signer_id, event, intent_hash);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
core/Cargo.toml(2 hunks)core/src/accounts.rs(5 hunks)core/src/deadline.rs(1 hunks)core/src/engine/inspector.rs(1 hunks)core/src/event_sink.rs(1 hunks)core/src/events.rs(2 hunks)core/src/fees.rs(1 hunks)core/src/intents/account.rs(1 hunks)core/src/intents/mod.rs(3 hunks)core/src/intents/token_diff.rs(4 hunks)core/src/intents/tokens.rs(13 hunks)core/src/lib.rs(1 hunks)defuse/src/contract/accounts/account/entry/v0.rs(3 hunks)defuse/src/contract/accounts/account/entry/v1.rs(4 hunks)defuse/src/contract/accounts/account/mod.rs(4 hunks)defuse/src/contract/accounts/force.rs(4 hunks)defuse/src/contract/events.rs(0 hunks)defuse/src/contract/fees.rs(3 hunks)defuse/src/contract/intents/execute.rs(0 hunks)defuse/src/contract/intents/mod.rs(2 hunks)defuse/src/contract/intents/simulate.rs(0 hunks)defuse/src/contract/intents/state.rs(3 hunks)defuse/src/contract/mod.rs(4 hunks)defuse/src/contract/salts.rs(3 hunks)defuse/src/contract/tokens/mod.rs(2 hunks)defuse/src/contract/tokens/nep141/withdraw.rs(1 hunks)defuse/src/contract/tokens/nep245/core.rs(1 hunks)defuse/src/contract/tokens/nep245/resolver.rs(2 hunks)defuse/src/intents.rs(1 hunks)nep245/Cargo.toml(1 hunks)nep245/src/events.rs(5 hunks)tests/Cargo.toml(1 hunks)tests/src/tests/defuse/intents/ft_withdraw.rs(3 hunks)tests/src/tests/defuse/intents/mod.rs(2 hunks)tests/src/tests/defuse/intents/native_withdraw.rs(2 hunks)
💤 Files with no reviewable changes (3)
- defuse/src/contract/intents/simulate.rs
- defuse/src/contract/intents/execute.rs
- defuse/src/contract/events.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- defuse/src/contract/accounts/force.rs
- defuse/src/contract/accounts/account/entry/v1.rs
- core/src/intents/account.rs
- tests/src/tests/defuse/intents/mod.rs
- core/src/fees.rs
- core/src/engine/inspector.rs
- nep245/Cargo.toml
- tests/src/tests/defuse/intents/ft_withdraw.rs
- tests/Cargo.toml
- core/src/deadline.rs
- core/Cargo.toml
- defuse/src/contract/accounts/account/entry/v0.rs
🧰 Additional context used
🧬 Code graph analysis (11)
tests/src/tests/defuse/intents/native_withdraw.rs (3)
defuse/src/contract/mod.rs (1)
new(119-130)tests/src/tests/defuse/env.rs (1)
new(49-51)core/src/event_sink.rs (1)
default(17-23)
defuse/src/contract/intents/state.rs (3)
defuse/src/contract/accounts/account/entry/v0.rs (1)
new(73-89)defuse/src/contract/accounts/account/mod.rs (1)
new(40-57)defuse/src/contract/mod.rs (1)
new(119-130)
defuse/src/contract/mod.rs (2)
core/src/events.rs (1)
events(123-127)core/src/event_sink.rs (1)
default(17-23)
defuse/src/intents.rs (1)
core/src/events.rs (1)
events(123-127)
core/src/intents/token_diff.rs (3)
core/src/events.rs (1)
events(123-127)core/src/intents/mod.rs (1)
new(212-214)defuse/src/contract/mod.rs (1)
new(119-130)
defuse/src/contract/accounts/account/mod.rs (5)
defuse/src/contract/accounts/account/entry/v1.rs (1)
set_auth_by_predecessor_id(169-177)defuse/src/contract/intents/state.rs (1)
set_auth_by_predecessor_id(302-304)core/src/engine/state/mod.rs (1)
set_auth_by_predecessor_id(116-116)core/src/engine/state/cached.rs (1)
set_auth_by_predecessor_id(333-347)core/src/engine/state/deltas.rs (1)
set_auth_by_predecessor_id(194-196)
core/src/events.rs (4)
core/src/accounts.rs (2)
into_owned(20-25)into_owned(72-76)core/src/fees.rs (1)
into_owned(218-223)nep245/src/events.rs (1)
into_owned(62-107)core/src/intents/mod.rs (4)
into_static(155-155)into_static(168-176)into_static(185-190)into_static(199-207)
nep245/src/events.rs (3)
core/src/fees.rs (1)
into_owned(218-223)core/src/accounts.rs (2)
into_owned(20-25)into_owned(72-76)core/src/events.rs (3)
into_owned(36-41)into_owned(133-166)events(123-127)
core/src/accounts.rs (1)
core/src/events.rs (2)
into_owned(36-41)into_owned(133-166)
defuse/src/contract/intents/mod.rs (5)
core/src/events.rs (2)
events(123-127)emit(27-32)core/src/engine/inspector.rs (1)
new(28-34)defuse/src/contract/mod.rs (1)
new(119-130)core/src/engine/mod.rs (1)
new(27-32)defuse/src/intents.rs (1)
simulate_intents(20-20)
core/src/event_sink.rs (3)
core/src/events.rs (1)
events(123-127)core/src/engine/inspector.rs (1)
new(28-34)defuse/src/contract/mod.rs (1)
new(119-130)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Security Audit - deny
- GitHub Check: Build
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared security analysis / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Security Audit - report
🔇 Additional comments (25)
tests/src/tests/defuse/intents/native_withdraw.rs (1)
138-228: Well-structured event ordering test.The test correctly validates that
MtBurnevents are postponed until after all intent executions and transfers complete. The assertion sequence matches the expected event flow described in the PR objectives.defuse/src/contract/tokens/nep245/resolver.rs (1)
105-118: LGTM: Emission correctly routed through centralized pathway.The refactor properly routes the
MtTransferrefund event throughemit_defuse_event, preserving the payload structure and refund logic while aligning with the new centralized event handling architecture.defuse/src/contract/tokens/mod.rs (2)
64-64: LGTM: Mint event correctly routed.The
MtMintevent is properly routed throughemit_defuse_event, consistent with the centralized event handling refactor.
118-118: LGTM: Burn event correctly postponed.The
MtBurnevent is properly routed throughpostpone_defuse_event, maintaining the intended behavior of emitting burn events at transaction end to avoid confusion with delta-matching during intent execution (as explained in lines 113-116).defuse/src/intents.rs (1)
37-38: LGTM: Enables event capture in simulation.The new
eventsfield directly supports the PR objective of including events in simulation output, using static lifetimes for owned event data that can be returned from the simulation.core/src/lib.rs (1)
6-6: LGTM: EventSink module properly exposed.The new
event_sinkmodule is correctly added and exported, enabling centralized event handling infrastructure to be used throughout the codebase.Also applies to: 13-13
defuse/src/contract/tokens/nep245/core.rs (1)
197-211: LGTM: Transfer emission correctly centralized.The
MtTransferevent is properly routed throughemit_defuse_eventwhile preserving the event payload structure, consistent with the centralized event handling refactor.defuse/src/contract/fees.rs (2)
24-30: LGTM: Fee change event correctly emitted.The
FeeChangedevent is properly wrapped inDip4Eventand routed throughemit_defuse_event, consistent with the centralized event handling refactor.
44-50: LGTM: Fee collector change event correctly emitted.The
FeeCollectorChangedevent is properly wrapped inDip4Eventand routed throughemit_defuse_event, maintaining the event structure while aligning with the new emission architecture.defuse/src/contract/intents/state.rs (2)
110-120: LGTM: Public key addition event emission is correct.The event emission uses
inspect()to emit only on successful public key addition, with appropriate borrowed lifetimes for both the account_id and public_key.
132-142: LGTM: Public key removal event emission is correct.The event emission pattern mirrors the addition logic, correctly emitting only on successful removal using borrowed references.
defuse/src/contract/salts.rs (1)
21-27: LGTM: Salt rotation event emission is consistent.Both emission sites correctly wrap
SaltRotationEventinDip4Event::SaltRotationand route through the centralizedemit_defuse_eventpathway.Also applies to: 45-51
defuse/src/contract/accounts/account/mod.rs (1)
61-63: LGTM: Event emission correctly moved to state layer.The Account methods now delegate to internal helpers, with event emission properly handled at the state layer (defuse/src/contract/intents/state.rs). The
_meparameter prefix indicates it's no longer used at this level.Also applies to: 79-81, 150-158
core/src/intents/tokens.rs (2)
26-26: LGTM: PartialEq and Eq derives added for event comparison.Adding these derives enables intent types to be compared, which is necessary for the EventSink storage and testing infrastructure.
Also applies to: 72-72, 167-167, 262-262, 364-364, 412-412
53-59: LGTM: Event emission consistently migrated to Dip4Event.All intent types now route events through
Dip4Eventvariants, maintaining consistency with the new centralized event handling architecture.Also applies to: 154-160, 249-255, 351-357, 388-394, 438-444
defuse/src/contract/intents/mod.rs (1)
34-39: LGTM: Explicit event emission replaces drop-based pattern.The explicit check and emission of
Dip4Event::IntentsExecutedis clearer than relying on inspector drop side-effects and properly integrates with the new EventSink architecture.nep245/src/events.rs (2)
8-12: LGTM: Derives extended to support event system requirements.The addition of
PartialEq,Eq, and conditionalJsonSchemaderives enablesMtEventto integrate properly with theDefuseEventwrapper and ABI generation.Also applies to: 24-24, 35-35, 48-48
62-107: LGTM: into_owned correctly converts all borrowed data.The implementation properly converts all
Cowfields to owned variants across allMtEventenum variants, maintaining the correct lifetime transformation to'static.core/src/accounts.rs (1)
11-51: LGTM! Well-structured lifetime-lifting support.The added derives (
PartialEq,Eq) enable event comparison for testing and deduplication. The specializedinto_ownedmethods forPublicKeyEventandTokenDiffEventcorrectly handle lifetime conversion when the genericinto_owned(line 19-26) cannot be used due to theT: 'staticconstraint. The pattern is consistent and necessary for the event recording functionality.Also applies to: 65-77, 88-103
core/src/intents/mod.rs (1)
152-208: LGTM! Clean abstraction for lifetime conversions.The
IntoStaticIntentEventtrait provides a uniform interface for converting borrowedIntentEventtypes to owned ('static) versions. The three implementations correctly handle:
- Generic
Cow-wrapped types (lines 159-177)TokenDiffEventwith specialized conversion (lines 180-191)NonceEventwhich is already'static(lines 194-208)The
#[allow(clippy::use_self)]annotations are appropriate sinceSelfcannot be used when the output type has a different lifetime parameter.core/src/events.rs (3)
35-42: LGTM! Theinto_ownedimplementation resolves the previous unimplemented method issue.The method correctly converts borrowed data to owned data for both
Dip4EventandNep245Eventvariants, enabling events to be stored with a'staticlifetime for simulation output.Note: Based on learnings from the past review, this implementation addresses the previous concern about an unimplemented method that would panic at runtime.
133-166: LGTM! Comprehensiveinto_ownedimplementation.The method correctly handles lifetime conversion for all
Dip4Eventvariants:
- Uses specialized helpers for
PublicKeyEventandTokenDiffEvent- Delegates to
convert_eventsfor event slices- Directly uses
into_ownedfor single events with'alifetimes- Preserves events that are already
'static(e.g.,FeeChanged,SaltRotation)
114-129: Reviewconvert_eventsfor allocation overhead
convert_eventsalways allocates a newVecand clones each element, even when given aCow::Ownedinput. It’s invoked in variousDip4Eventvariant mappings (e.g., Transfer, TokenDiff, IntentsExecuted, withdrawals, StorageDeposit). Benchmark its performance in high-throughput or simulation scenarios and consider zero-copy or in-place conversion optimizations if this becomes a hotspot.core/src/intents/token_diff.rs (2)
123-132: LGTM! Event derives and backward-compatible serialization.The addition of
PartialEqandEqderives enables event comparison for testing. The#[serde(default, ...)]attribute onfees_collectedensures backward compatibility by using a default value if the field is missing during deserialization.
29-29: Removal of Borsh serialization is safe –TokenDiffisn’t ever stored via NEAR’s persistent collections or used in cross-contract calls; it’s an internal type and for events only.
fusede
left a comment
fusede
left a comment
There was a problem hiding this comment.
I like how you moved public key events into State implementation, but I still have concerns mainly about InspectorImpl and exposed usage of RefCell, see comments below
| near-account-id.workspace = true | ||
| near-contract-standards.workspace = true | ||
| near-sdk.workspace = true | ||
| schemars.workspace = true |
There was a problem hiding this comment.
This should be already re-exported by near-sdk when its abi feature is enabled
| } | ||
| } | ||
|
|
||
| // For AccountEvent with PublicKeyEvent |
There was a problem hiding this comment.
Can we avoid writing thse type-specific methods and have a separate trait that will be implemented for Cow and all our event structs that have Cow inside of them?
pub trait IntoOwned {
type Owned: 'static;
pub fn into_owned(self) -> Self::Owned;
}PS: Ideally, there should also be a #[derive(IntoOwned)] procedural macro, which will do it automatically, but I'm not sure that it's worth the effort.
| strum.workspace = true | ||
| tokio = { workspace = true, features = ["macros"] } | ||
| tlb-ton = { workspace = true, features = ["arbitrary"] } | ||
| defuse-crypto = { workspace = true, features = ["serde"] } |
There was a problem hiding this comment.
This is re-exported by defuse-core already
| [dependencies] | ||
| derive_more.workspace = true | ||
| near-sdk.workspace = true | ||
| schemars = { workspace = true, optional = true } |
There was a problem hiding this comment.
This is re-exported by near-sdk already
There was a problem hiding this comment.
omg that was pain to figure out but it seems to be a bug in near-sdk
- in their cargo they have:
[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
schemars = { version = "0.8.8", optional = true }
while cargo book explicitally states that
Unlike in your Rust source code, you cannot use [target.'cfg(feature = "fancy-feature")'.dependencies] to add dependencies based on optional features. Use the [features] section
| use near_sdk::{ | ||
| AccountId, Gas, GasWeight, NearToken, Promise, PromiseOrValue, PromiseResult, assert_one_yocto, | ||
| env, | ||
| env::{self}, |
There was a problem hiding this comment.
| env::{self}, | |
| env, |
| fn on_intent_executed(&mut self, signer_id: &AccountIdRef, hash: CryptoHash, nonce: Nonce); | ||
| } | ||
|
|
||
| pub struct InspectorImpl { |
There was a problem hiding this comment.
What is the point of having Inspector as a trait then? This is supposed to be an zero-cost abstraction that handles all of incoming data according to its business logic.
Here is an example:
What if one day we would want to add tracing? With this approach, you'd need to store all the data that might be used by some callers, while discarded by others. So, for intents.near::execute_intents() this would only increase the memory consumption and gas costs since it stores unnecessary things under the hood.
|
|
||
| #[borsh(skip)] | ||
| runtime: Runtime, | ||
| event_sink: Rc<RefCell<EventSink>>, |
There was a problem hiding this comment.
Why do you need event_sink to be a field of a Contract struct? Can it be defined as a global static variable in defuse-core crate similar to how tracing crate works?
| self.event_sink.borrow_mut().record_only_mode(); | ||
| } | ||
|
|
||
| pub fn event_sink_handle(&self) -> Rc<RefCell<EventSink>> { |
There was a problem hiding this comment.
This might become very error-prone due to RefCell dynamic checking rules: after calling this method the caller should ensure he never calls methods above as well ass RefCell::borrow_mut on the returned handle. This puts responsibility of ensuring safe code on the developers instead of Rust compiler. We should avoid these things as much as possible
| pub intents_executed: Vec<IntentEvent<AccountEvent<'static, NonceEvent>>>, | ||
|
|
||
| /// All events that would be emitted during execution | ||
| pub events: Vec<defuse_core::events::DefuseEvent<'static>>, |
There was a problem hiding this comment.
Would it be easier to store it as a list of serde_json::Value or even strings? Take a look at .to_json() method that gets generated by near_sdk::EventMetadata
| Engine::new(self, ExecuteInspector::default()) | ||
| let mut inspector = InspectorImpl::new(self.event_sink_handle()); | ||
|
|
||
| Engine::new(&mut *self, &mut inspector) |
There was a problem hiding this comment.
| Engine::new(&mut *self, &mut inspector) | |
| Engine::new(self, &mut inspector) |
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Chores