-
Notifications
You must be signed in to change notification settings - Fork 84
Conversation
Hi @DiegoB1911, thank you it is the right approach overall. However, here are some indications on how to make it "better":
let contract_events = EventsFilterBuilderTrait::from_events(@spy.get_events())
.with_contract_address(test_address())
.build();
for event in evm_events {
let starknet_event = EventIntoStarknetEvent::into(event);
contract_events.assert_emitted(@starknet_event);
};
impl EventIntoStarknetEvent of Into<Event, StarknetEvent> {
fn into(self: Event) -> StarknetEvent {
let mut serialized_keys = array![];
let mut serialized_data = array![];
Serde::<Array<u256>>::serialize(@self.keys, ref serialized_keys);
Serde::<Array<u8>>::serialize(@self.data, ref serialized_data);
StarknetEvent { keys: serialized_keys, data: serialized_data }
}
} (see usage in example above) Here is a full git diff of the changes I would make. As you see the logic is roughly the same. I also modified a file under snforge_utils to avoid passing ownership when using |
]; | ||
|
||
// Serialize events and store in mock_events | ||
serialize_mock_events(events.clone(), ref mock_events); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer the implementation of Into
, see main message (but same underlying idea)
#[test] | ||
fn test_emit_events() { | ||
let mut state: State = Default::default(); | ||
let mut mock_events = ArrayTrait::<(ContractAddress, MockEvent)>::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let mut mock_events = ArrayTrait::<(ContractAddress, MockEvent)>::new(); | |
let mut mock_events = ArrayTrait::<(ContractAddress, MockEvent)>::new(); |
no longer required
Co-authored-by: Mathieu <60658558+enitrat@users.noreply.github.com>
Co-authored-by: Mathieu <60658558+enitrat@users.noreply.github.com>
@enitrat Thank you for your review and explanation. I hadn’t considered implementing the Into trait in that way, and I also thought about importing snforge_std::Event as you did ("snforge_std::Event as StarknetEvent"), but I wasn’t sure it was possible to rename imported elements like in other languages, which is why I created the mock event. Thanks for showing me that approach. I'm still learning how traits and implementations work, and how Cairo functions in general, so I really appreciate your help. I’ve made the requested changes. Please let me know if there’s anything else I need to adjust. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, great job 💪
This PR implements the unit test for the emit_events function.
Pull Request type
Please check the type of change your PR introduces:
What is the current behavior?
The emit_events function doesn't have any unit tests.
Resolves: #952
What is the new behavior?
Does this introduce a breaking change?
This change is