Skip to content

Use EventSub instead of Filter/Watch. #89

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Jun 16, 2021
Merged

Use EventSub instead of Filter/Watch. #89

merged 7 commits into from
Jun 16, 2021

Conversation

ggwpez
Copy link
Contributor

@ggwpez ggwpez commented May 31, 2021 •
edited
Loading

Closes #84

@ggwpez ggwpez force-pushed the 84-use-generic-subs branch 4 times, most recently from cf1600c to d4e6825 Compare June 7, 2021 12:56
@ggwpez ggwpez self-assigned this Jun 7, 2021
@ggwpez ggwpez marked this pull request as ready for review June 7, 2021 12:59
@ggwpez ggwpez added this to the Mainnet milestone Jun 8, 2021
matthiasgeihs
matthiasgeihs reviewed Jun 10, 2021
View reviewed changes
Copy link
Contributor

@matthiasgeihs matthiasgeihs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The integration is a bit more complex than I hoped.

Couldn't we just use sub.Read without using sub.ReadPast in many cases?

Hardcoding the event name is not good practice.

@ggwpez ggwpez force-pushed the 84-use-generic-subs branch from d4e6825 to 946ef50 Compare June 14, 2021 08:12
@ggwpez ggwpez requested a review from matthiasgeihs June 14, 2021 13:04
matthiasgeihs
matthiasgeihs suggested changes Jun 15, 2021
View reviewed changes
Copy link
Contributor

@matthiasgeihs matthiasgeihs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address the linter warning. It's easy to fix. For one of them I already added a commit.

There is probably a bug in ensureConcluded. What if the first event is not the Concluded event?

@ggwpez ggwpez requested a review from matthiasgeihs June 15, 2021 09:37
matthiasgeihs
matthiasgeihs previously approved these changes Jun 16, 2021
View reviewed changes
Copy link
Contributor

@matthiasgeihs matthiasgeihs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

ggwpez added 5 commits June 16, 2021 22:08
@ggwpez
♻️ [backend/eth/channel] Add BoundContract to Adjudicator.
fd60a7d 
Signed-off-by: Oliver Tale-Yazdi <oliver@perun.network>
@ggwpez
♻️ [backend/channel/eth] Add sync.Once to Adj.Sub.
b00559f 
This will be needed for the generic EventSub which can only be
closed once.
It also means that a Subscription can not be copied anymore.

Signed-off-by: Oliver Tale-Yazdi <oliver@perun.network>
@ggwpez
♻️ [backend/eth/channel] Remove 'past' field of Adj.Sub.
7b071a6 
This field and function is unused.

Signed-off-by: Oliver Tale-Yazdi <oliver@perun.network>
@ggwpez
♻️ [backend/eth/channel] Use EventSub in the Adj and AdjSub.
0331eb9 
Signed-off-by: Oliver Tale-Yazdi <oliver@perun.network>
@ggwpez
💡 [backend/eth/sub] Add comment about nil log.
217ddc7 
Signed-off-by: Oliver Tale-Yazdi <oliver@perun.network>
@ggwpez ggwpez force-pushed the 84-use-generic-subs branch from eed0f33 to bbdd18c Compare June 16, 2021 20:30
ggwpez added 2 commits June 16, 2021 22:33
@ggwpez
✨ [backend/eth/ch/bindings] Extract event names from ABIs.
a29835f 
Signed-off-by: Oliver Tale-Yazdi <oliver@perun.network>
@ggwpez
✅ [backend/eth/sub] Replace hard coded event names.
69ab9e0 
Signed-off-by: Oliver Tale-Yazdi <oliver@perun.network>
@ggwpez ggwpez force-pushed the 84-use-generic-subs branch from bbdd18c to 69ab9e0 Compare June 16, 2021 20:33
@matthiasgeihs matthiasgeihs merged commit f5f7b3e into hyperledger-labs:dev Jun 16, 2021
@matthiasgeihs matthiasgeihs deleted the 84-use-generic-subs branch June 16, 2021 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthiasgeihs matthiasgeihs matthiasgeihs approved these changes

Assignees

@ggwpez ggwpez

Labels
None yet
Projects
None yet
Milestone
Ethereum Mainnet
Development

Successfully merging this pull request may close these issues.

Use generic EventSubs in eth/channel
2 participants
@ggwpez @matthiasgeihs