-
Notifications
You must be signed in to change notification settings - Fork 158
Abstract events into an event listener #137
Abstract events into an event listener #137
Conversation
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.
@kfichter Looks good, left a few comments for you.
plasma/child_chain/child_chain.py
Outdated
# Register for deposit event listener | ||
deposit_filter = self.root_chain.on('Deposit') | ||
deposit_filter.watch(self.apply_deposit) | ||
self.event_listener = RootEventListener(root_chain, finality=1) |
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.
How do you feel about changing finality
to confirmations
as it's more descriptive?
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.
Agreed.
event_filter = self.root_chain.eventFilter(event_name, { | ||
'fromBlock': current_block['number'] - (self.finality * 2), | ||
'toBlock': current_block['number'] - self.finality | ||
}) |
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.
How did you decide upon the (-finality, -finality * 2) gap for the filter? Do you think if we just set it to -finality events might be missed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought events might be missed if the client lost connection temporarily. Maybe it's better to just update some internal variable that stores the latest block the client has polled instead.
return child_chain | ||
yield child_chain | ||
|
||
child_chain.event_listener.stop_all() |
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.
Shouldn't stopping the events that the child chain is instantiated with come before yield
?
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.
yield
returns the child chain immediately (for use in tests), which starts event listeners. The stop_all()
statement is just teardown after the tests have completed.
child_chain.apply_exit(sample_event) | ||
|
||
# Transaction now marked spent | ||
assert child_chain.blocks[blknum].transaction_set[txindex].spent1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add an assert verifying that the output that's now spent was not before apply_exit
?
Removed my hack - this should be ready to go. |
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.
Nice, let's merge it. 👍
Abstracts all events into
RootEventListener
, which emits events only once they're "finalized" in the root chain. Should avoid a lot of issues with reorgs. Needs to be integration tested, waiting on testing lang.