Skip to content

Use EventSub instead of Filter/Watch.#89

Merged
matthiasgeihs merged 7 commits intohyperledger-labs:devfrom
perun-network:84-use-generic-subs
Jun 16, 2021
Merged

Use EventSub instead of Filter/Watch.#89
matthiasgeihs merged 7 commits intohyperledger-labs:devfrom
perun-network:84-use-generic-subs

Conversation

@ggwpez
Copy link
Copy Markdown
Contributor

@ggwpez ggwpez commented May 31, 2021

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
Copy link
Copy Markdown
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.

Comment thread backend/ethereum/channel/conclude.go Outdated
Comment thread backend/ethereum/channel/conclude.go Outdated
Comment thread backend/ethereum/channel/conclude.go Outdated
Comment thread backend/ethereum/channel/conclude.go Outdated
Comment thread backend/ethereum/channel/conclude.go Outdated
Comment thread backend/ethereum/channel/subscription.go Outdated
Comment thread backend/ethereum/channel/subscription.go
Comment thread backend/ethereum/channel/withdraw.go
Comment thread backend/ethereum/channel/withdraw.go Outdated
Comment thread backend/ethereum/subscription/eventsub.go
@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
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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
Signed-off-by: Oliver Tale-Yazdi <oliver@perun.network>
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>
This field and function is unused.

Signed-off-by: Oliver Tale-Yazdi <oliver@perun.network>
Signed-off-by: Oliver Tale-Yazdi <oliver@perun.network>
Signed-off-by: Oliver Tale-Yazdi <oliver@perun.network>
ggwpez added 2 commits June 16, 2021 22:33
Signed-off-by: Oliver Tale-Yazdi <oliver@perun.network>
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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use generic EventSubs in eth/channel

2 participants