-
Notifications
You must be signed in to change notification settings - Fork 77
Conversation
@@ -52,8 +52,10 @@ def __get_actual_fee(self) -> int: | |||
|
|||
def __get_events(self) -> List[Event]: | |||
"""Returns the events""" | |||
contract_address = self.execution_info.call_info.contract_address | |||
return [Event.create(event_content=e, emitting_contract_address=contract_address) for e in self.execution_info.call_info.events] | |||
if isinstance(self.execution_info, StarknetTransactionExecutionInfo): |
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.
Which other class can it be (other than StarknetTransactionExecutionInfo
)? And when is it so? Why doesn't that other class support using raw_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.
What other type it can be is defined by in the execution_info
type. It can be either TransactionExecutionInfo
for invokes or StarknetTransactionExecutionInfo
for deploys
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.
Right, forgot about that. That type is actually not specified correctly: or
shouldn't be used for parameter types, typing.Union
should (in other places in the project as well). An even better solution would be not to have all the if
s in the code, but one class for each transaction type.
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.
That's the only if
. We could transform TransactionExecutionInfo
to StarknetTransactionExecutionInfo
but I made a priority of fixing this instead. Transforming it to StarknetTransactionExecutionInfo
is not trivial because of the 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.
Usage related changes
Development related changes
Checklist: