Generic events sub #36

ggwpez · 2021-04-06T14:52:25Z

Add generic event subs in preparation for #19.

matthiasgeihs

Here is my first review. Please do not force push new changes as long as the review is in progress and the code is not stable yet.

You may consider integrating into this PR already some of the places where the subscription will be used. Otherwise it could happen that we only realize necessary changes later.

backend/ethereum/bindings/peruntoken/PerunToken.go

matthiasgeihs · 2021-05-20T14:02:04Z

backend/ethereum/glue/doc.go

+
+// Package glue glues together go-perun and go-ethereum.
+// It contains generic functionality and can he used even without go-perun.
+package glue // import "perun.network/go-perun/backend/ethereum/glue"


The package name "glue" is very vague. Use package name "subscription" instead?

Renamed it to subscription for now. I also wanted to put the reorg-resistant TX stuff in here, then the name subscription would not be enough.

backend/ethereum/glue/eventsub.go

backend/ethereum/glue/eventsub_test.go

ggwpez · 2021-05-20T16:42:03Z

You may consider integrating into this PR already some of the places where the subscription will be used. Otherwise it could happen that we only realize necessary changes later.

Ok i will try it out at some spots where we use it.

ggwpez · 2021-05-20T17:14:14Z

While using the EventSub in the Funder, it seems that the start, end *uint64 pointers that specify the range to query are not easy to use.
I will try to change it just to pastBlocks which is then a relative offset into the past.

ggwpez · 2021-05-24T15:39:03Z

Since the EventSub is using geth functions, we need to check the ChainNotReachableError for every returned error.
The function isChainNotReachableError is in [backend/eth/channel] (and private), which prevents me from importing it, since it would create a cyclic dependency.

So we either make an addictional [backend/eth/channel/error] package or merge the glue package into [backend/eth/channel].
Any other ideas @matthiasgeihs ?

matthiasgeihs · 2021-05-25T13:59:51Z

Since the EventSub is using geth functions, we need to check the ChainNotReachableError for every returned error.
The function isChainNotReachableError is in [backend/eth/channel] (and private), which prevents me from importing it, since it would create a cyclic dependency.

So we either make an addictional [backend/eth/channel/error] package or merge the glue package into [backend/eth/channel].
Any other ideas @matthiasgeihs ?

I don't understand. Why do we need to check for ChainNotReachableError because of using geth functions? Isn't the error type specific to go-perun?

Anyways, adding a package seems cleaner to me. Or could we maybe check for the error without using a dedicated function?

ggwpez · 2021-05-25T14:07:22Z

I don't understand. Why do we need to check for ChainNotReachableError because of using geth functions? Isn't the error type specific to go-perun?

Ok i meant we need to use checkIsChainNotReachableError in order to make sure that the error is correctly wrapped in the go-perun error type.

Anyways, adding a package seems cleaner to me. Or could we maybe check for the error without using a dedicated function?

The error checking function is like 5 lines of boilerplate code that i would need in 4-5 places.
I will opt for the new [backend/eth/channel/error] package to keep the code more readable.

matthiasgeihs

Only general comments. Looks good.

matthiasgeihs · 2021-06-01T09:14:25Z

backend/ethereum/channel/funder.go

@@ -184,7 +184,7 @@ func (f *Funder) sendFundingTx(ctx context.Context, request channel.FundingReq,
 	// nolint: gocritic
 	if bal == nil || bal.Sign() <= 0 {
 		f.log.WithFields(log.Fields{"channel": request.Params.ID(), "idx": request.Idx}).Debug("Skipped zero funding.")
-	} else if alreadyFunded, err := checkFunded(ctx, bal, contract, fundingID); err != nil {
+	} else if alreadyFunded, err := f.checkFunded(ctx, bal, contract, fundingID); err != nil {


Not sure why we check the funding here before we actually do it, but this should probably be not the subject of the current PR.

The framework could be restarted after sending the deposit transactions, in which case we do not want to send them twice.
It is still a race condition in that case, but should mitigate most of it. This is what the docu means with It is idempotent..
@matthiasgeihs

matthiasgeihs · 2021-06-01T09:17:23Z

backend/ethereum/channel/funder.go

+			Filter: [][]interface{}{filter},
+		}
+	}
+	sub, err := subscription.NewEventSub(ctx, f, contract, event, startBlockOffset)


There is a difference in functionality here compared to before. Before, we were starting from block 1. Here we are starting from current block - offset. I think it's OK because we also do it in other places but in general this is an open issue how to choose the startBlockOffset correctly.

I will open an issue for brainstorming it.
PS: Done in #91

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

ggwpez mentioned this pull request Apr 6, 2021

☂️ Reorg resistance of Ethereum backend #19

Closed

17 tasks

ggwpez force-pushed the generic-events-sub branch 2 times, most recently from ed7b697 to d0b692a Compare April 6, 2021 15:03

ggwpez linked an issue Apr 8, 2021 that may be closed by this pull request

Generic Event Sub #39

Closed

ggwpez force-pushed the generic-events-sub branch 3 times, most recently from 8aad187 to 27c6d19 Compare May 19, 2021 09:51

ggwpez marked this pull request as ready for review May 19, 2021 09:54

ggwpez requested a review from matthiasgeihs May 19, 2021 09:54

matthiasgeihs suggested changes May 20, 2021

View reviewed changes

ggwpez requested a review from matthiasgeihs May 25, 2021 10:18

ggwpez marked this pull request as draft May 28, 2021 08:04

ggwpez force-pushed the generic-events-sub branch 2 times, most recently from ae9c7db to 7734441 Compare May 28, 2021 12:03

ggwpez marked this pull request as ready for review May 28, 2021 12:06

ggwpez mentioned this pull request May 31, 2021

Use EventSub instead of Filter/Watch. #89

Merged

matthiasgeihs previously approved these changes Jun 1, 2021

View reviewed changes

ggwpez added 2 commits June 1, 2021 11:31

✨ Create [backend/eth/subscription].

ad037d0

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

🎨 [backend/eth/channel] Use EventSub in Funder.

a325916

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

ggwpez dismissed matthiasgeihs’s stale review via a325916 June 1, 2021 09:31

ggwpez force-pushed the generic-events-sub branch from 7734441 to a325916 Compare June 1, 2021 09:31

matthiasgeihs approved these changes Jun 2, 2021

View reviewed changes

matthiasgeihs merged commit e16052e into hyperledger-labs:dev Jun 2, 2021

matthiasgeihs deleted the generic-events-sub branch June 2, 2021 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic events sub #36

Generic events sub #36

ggwpez commented Apr 6, 2021

matthiasgeihs left a comment •

edited

Loading

matthiasgeihs May 20, 2021

ggwpez May 25, 2021

ggwpez commented May 20, 2021

ggwpez commented May 20, 2021

ggwpez commented May 24, 2021

matthiasgeihs commented May 25, 2021

ggwpez commented May 25, 2021

matthiasgeihs left a comment

matthiasgeihs Jun 1, 2021

ggwpez Jun 1, 2021

matthiasgeihs Jun 1, 2021

ggwpez Jun 1, 2021 •

edited

Loading

Generic events sub #36

Generic events sub #36

Conversation

ggwpez commented Apr 6, 2021

matthiasgeihs left a comment • edited Loading

Choose a reason for hiding this comment

matthiasgeihs May 20, 2021

Choose a reason for hiding this comment

ggwpez May 25, 2021

Choose a reason for hiding this comment

ggwpez commented May 20, 2021

ggwpez commented May 20, 2021

ggwpez commented May 24, 2021

matthiasgeihs commented May 25, 2021

ggwpez commented May 25, 2021

matthiasgeihs left a comment

Choose a reason for hiding this comment

matthiasgeihs Jun 1, 2021

Choose a reason for hiding this comment

ggwpez Jun 1, 2021

Choose a reason for hiding this comment

matthiasgeihs Jun 1, 2021

Choose a reason for hiding this comment

ggwpez Jun 1, 2021 • edited Loading

Choose a reason for hiding this comment

matthiasgeihs left a comment •

edited

Loading

ggwpez Jun 1, 2021 •

edited

Loading