Skip to content
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

Use EventSub instead of Filter/Watch. #89

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.

backend/ethereum/channel/conclude.go Outdated Show resolved Hide resolved
backend/ethereum/channel/conclude.go Outdated Show resolved Hide resolved
backend/ethereum/channel/conclude.go Outdated Show resolved Hide resolved
backend/ethereum/channel/conclude.go Outdated Show resolved Hide resolved
backend/ethereum/channel/conclude.go Outdated Show resolved Hide resolved
backend/ethereum/channel/subscription.go Outdated Show resolved Hide resolved
backend/ethereum/channel/subscription.go Show resolved Hide resolved
backend/ethereum/channel/withdraw.go Show resolved Hide resolved
backend/ethereum/channel/withdraw.go Outdated Show resolved Hide resolved
backend/ethereum/subscription/eventsub.go Show resolved Hide resolved
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?

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
♻️ [backend/eth/channel] Add BoundContract to Adjudicator.
fd60a7d 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@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 <[email protected]>
@ggwpez
♻️ [backend/eth/channel] Remove 'past' field of Adj.Sub.
7b071a6 
This field and function is unused.

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
♻️ [backend/eth/channel] Use EventSub in the Adj and AdjSub.
0331eb9 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
💡 [backend/eth/sub] Add comment about nil log.
217ddc7 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@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