Runlog service refactor tier 1 #139

boxhock · 2021-04-19T12:56:21Z

No description provided.

boxhock

This PR is pretty big. The most important changes happen within subscriber and the ETH/EVM implementations in blockchain.

boxhock · 2021-04-21T10:30:24Z

blockchain/binance-smart-chain.go

@@ -1,176 +0,0 @@
-package blockchain


Deleted because the current ETH implementation would replace this now, so no need to maintain two exact identical implementations.

If there ever is a need to change the implementation for BSC, we can bring it back again.

boxhock · 2021-04-21T10:31:14Z

blockchain/blockchain.go

+	addresses := params.Addresses
+	if len(params.Addresses) == 0 && params.Address != "" {
+		addresses = []string{params.Address}
+	}


Helper to select a single address if possible

What about just always appending params.Address to addresses if it is not empty? Also, what is the case of having 2 separate fields?

We originally started by just having Addresses, but on the CL side it makes more sense to only define a single address: Address. Having support for both fields is more for backwards compatibility. Ideally, for more safety, I'd prefer users use the single address field.

boxhock · 2021-04-21T10:31:41Z

blockchain/cfx.go

@@ -5,7 +5,10 @@ import (
 	"errors"


Not included in this tier, so can be ignored

boxhock · 2021-04-21T10:33:05Z

blockchain/common/common.go

-	Request(t string) (response interface{}, err error)
-	Subscribe(ctx context.Context, t string, ch chan<- interface{}) (err error)
-	CreateJobRun(t string, v interface{}) (map[string]interface{}, error)
+	Stop()
+}
+
+type FluxMonitorManager interface {
+	Manager
+	GetState(ctx context.Context) (*FluxAggregatorState, error)
+	SubscribeEvents(ctx context.Context, ch chan<- interface{}) error
+	CreateJobRun(roundId uint32) map[string]interface{}
+}
+
+type RunlogManager interface {
+	Manager
+	SubscribeEvents(ctx context.Context, ch chan<- RunlogRequest) error
+	CreateJobRun(request RunlogRequest) map[string]interface{}


Instead of having a very generic interface with a lot of interface{} types, I've split runlog and FM into two different interfaces, so they can be implemented more precisely and gives us more type safety.

boxhock · 2021-04-21T10:34:11Z

blockchain/evm/evm.go

@@ -1,20 +1,18 @@
-package blockchain
+package evm


Since a lot of blockchains are EVM based, having an EVM package allows other blockchains to import the common functionality that they need.

boxhock · 2021-04-21T10:35:27Z

blockchain/evm/evm.go

+	"github.com/smartcontractkit/external-initiator/blockchain/common"
+
+	eth "github.com/ethereum/go-ethereum/common"


After going down a rabbit hole online, I've learned that dot-imports (which I had planned for our internal common package) is not a good thing to use, and should only ever be used in tests.

Instead, the GETH common package is imported as eth.

ebarakos · 2021-04-22T08:20:33Z

blockchain/blockchain.go

+	addresses := params.Addresses
+	if len(params.Addresses) == 0 && params.Address != "" {
+		addresses = []string{params.Address}
+	}


What about just always appending params.Address to addresses if it is not empty? Also, what is the case of having 2 separate fields?

ebarakos · 2021-04-22T08:24:38Z

blockchain/common/common.go

-	RequestId        string
-	CallbackFunction string
-}
+type RunlogRequest map[string]interface{}


Why making it generic?

Because all fields here ended up in what was previously the Params field anyways, so with the previous struct the values went a lot back-and-forth, and for new blockchain implementations, this list would just grow never ending.

ebarakos · 2021-04-22T08:35:55Z

blockchain/keeper.go

 	"math/big"
 	"net/url"
 	"strings"
 	"time"

+	common2 "github.com/smartcontractkit/external-initiator/blockchain/common"


Should this be renamed or maybe rename the geth package to eth as above?

This will be fixed once we reach this part of the migration

ebarakos · 2021-04-22T15:10:18Z

client/web.go

 	js := &store.JobSpec{
 		Job:  req.JobID,
-		Spec: req.Params.FluxMonitor,
+		Spec: spec,


Nice. So we are storing the whole actual JobSpec here, right? Both for RL and FM.

ebarakos · 2021-04-22T15:11:31Z

services/fluxmonitor.go

@@ -40,32 +40,32 @@ type FluxMonitorConfig struct {
 func ParseFMSpec(jsonSpec json.RawMessage, runtimeConfig store.RuntimeConfig) (FluxMonitorConfig, error) {
 	var fmConfig FluxMonitorConfig

-	res := gjson.GetBytes(jsonSpec, "feeds.#.url")
+	res := gjson.GetBytes(jsonSpec, "fluxmonitor.feeds.#.url")


Maybe keep only the fluxmonitor part before starting parsing? In order to not duplicate the fluxmonitor. below

I initially wanted to do that, but wasn't able to get what I was looking for. Do you know how to do this with gjson?

Something like

jsonSpec = []byte(gjson.GetBytes(jsonSpec, "fluxmonitor").Raw)

or rename it to fmSpec

ebarakos · 2021-04-22T15:12:42Z

services/fluxmonitor.go

@@ -130,22 +130,16 @@ func NewFluxMonitor(job string, config FluxMonitorConfig, triggerJobRun chan sub

 	FAEvents := make(chan interface{})

-	state, err := fm.blockchain.Request(common.FMRequestState)
+	state, err := fm.blockchain.GetState(context.TODO())


How do you envision this context.TODO() to be used?

We'll define the request timeouts here, instead of having the blockchains define them. I'll make an issue for this.

ebarakos · 2021-04-22T15:21:42Z

client/service.go

 	js, err := srv.store.LoadJobSpec(sub.Job)
-	if err != nil || js == nil {
+	if err != nil || gjson.GetBytes(js.Spec, "fluxmonitor").Raw == "null" {
 		startService = srv.subscribeRunlog


Why not calling service, err := srv.subscribeRunlog(sub, triggerJobRun, js) here directly?

Instead of implementing Starter type etc.

Good question, I'll change this.

ebarakos · 2021-04-26T08:36:49Z

services/fluxmonitor.go

-	fmConfig.Multiply = int32(gjson.GetBytes(jsonSpec, "fluxmonitor.precision").Int())
-	fmConfig.Threshold = gjson.GetBytes(jsonSpec, "fluxmonitor.threshold").Float()
-	fmConfig.AbsoluteThreshold = gjson.GetBytes(jsonSpec, "fluxmonitor.absoluteThreshold").Float()
+	fmConfig.RequestData = fmSpec.Get("requestData").Raw


boxhock force-pushed the runlog-service-refactor-tier-1 branch 2 times, most recently from 31d5675 to cc7a275 Compare April 20, 2021 15:21

boxhock marked this pull request as ready for review April 21, 2021 10:29

boxhock commented Apr 21, 2021

View reviewed changes

boxhock requested a review from ebarakos April 21, 2021 10:42

ebarakos suggested changes Apr 22, 2021

View reviewed changes

boxhock mentioned this pull request Apr 23, 2021

FM service should implement contexts #141

Open

boxhock added 13 commits April 23, 2021 11:01

Return Substrate runlog service

2d62566

Revert the change of the Event type

f40d15d

Cleanup

a152358

Refactor subscriber and BM managers

265d3f5

Modify other implementations to make project compile

bb56260

More refactoring, mock client changes

ccae7ca

Remove outdated tests

e70cd83

Fix failing tests

213b544

Prevent exhaustive cases

24f44cc

Avoid use of dot-imports

6bec528

Cleanup

c855dc7

Increase test coverage

d383717

Start service directly instead of making a starter type

7495a92

boxhock force-pushed the runlog-service-refactor-tier-1 branch from f6a5102 to 7495a92 Compare April 23, 2021 09:04

Make updated FM validation use the new spec path

7a8dcf6

boxhock requested a review from ebarakos April 23, 2021 09:14

ebarakos approved these changes Apr 26, 2021

View reviewed changes

Only use fluxmonitor config object when parsing it

6a5799b

boxhock requested a review from ebarakos April 26, 2021 08:34

ebarakos reviewed Apr 26, 2021

View reviewed changes

ebarakos approved these changes Apr 26, 2021

View reviewed changes

boxhock merged commit 4c6c7a8 into feature/fluxmonitor Apr 26, 2021

boxhock deleted the runlog-service-refactor-tier-1 branch April 26, 2021 08:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Runlog service refactor tier 1 #139

Runlog service refactor tier 1 #139

boxhock commented Apr 19, 2021

boxhock left a comment

boxhock Apr 21, 2021

boxhock Apr 21, 2021

ebarakos Apr 22, 2021

boxhock Apr 23, 2021

boxhock Apr 21, 2021

boxhock Apr 21, 2021

boxhock Apr 21, 2021

boxhock Apr 21, 2021

ebarakos Apr 22, 2021

ebarakos Apr 22, 2021

boxhock Apr 23, 2021

ebarakos Apr 22, 2021

boxhock Apr 23, 2021

ebarakos Apr 22, 2021

boxhock Apr 23, 2021

ebarakos Apr 22, 2021

boxhock Apr 23, 2021

ebarakos Apr 23, 2021

ebarakos Apr 22, 2021

boxhock Apr 23, 2021

boxhock Apr 23, 2021

ebarakos Apr 22, 2021

boxhock Apr 23, 2021

ebarakos Apr 26, 2021

		"github.com/smartcontractkit/external-initiator/blockchain/common"

		eth "github.com/ethereum/go-ethereum/common"

Runlog service refactor tier 1 #139

Runlog service refactor tier 1 #139

Conversation

boxhock commented Apr 19, 2021

boxhock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment