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.

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

refactor mercury #11137

Merged
merged 5 commits into from
Nov 16, 2023
Merged

refactor mercury #11137

merged 5 commits into from
Nov 16, 2023

Conversation

shileiwill
Copy link
Contributor

@shileiwill shileiwill commented Nov 1, 2023

AUTO-6853 AUTO-6327

Background

As said in the above tickets, The streamslookup code is not very modular and is difficult to import by other utilities (chaincli and observability). Refactor this codebase into a standalone package and remove the code duplication from chaincli.

Given that folks are actively working on mercury related improvements, I want to surface this PR early to get alignment and avoid missing important feature improvements (retry etc), and avoid conflicts.

This refactoring PR is on top of the core codebase as of 11/2. Some in-flight changes on mercury are not included, will need to add them manually, e.g. PR

As the saying goes, naming is one of the most difficult thing in programming, please suggest better names for folders and files if you have good names!

Shout out to @ferglor who laid the foundation for the big refactoring, thank you Fergal!

Description

Same as background

Changes

Break the streams_lookup.go to multiple components so it can be used as a lib.

Test Plans

PR is just to refactor current code without logic changes. All unit tests and integration tests should pass.
I ran smoke tests and unit tests locally to ensure they pass. Will wait for CI signals, should be all green.

Copy link
Contributor

github-actions bot commented Nov 1, 2023

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

@shileiwill shileiwill force-pushed the refactor_mercury branch 5 times, most recently from 5b63dd7 to 81f9647 Compare November 3, 2023 07:33
@shileiwill shileiwill marked this pull request as ready for review November 3, 2023 07:48
@shileiwill shileiwill requested review from a team, jmank88 and krehermann as code owners November 3, 2023 07:48
@shileiwill shileiwill requested review from a team and removed request for a team November 3, 2023 07:48
@@ -122,14 +127,26 @@ var upkeepStateEvents = []common.Hash{
}

type MercuryConfig struct {
cred *models.MercuryCredentials
abi abi.ABI
Cred *models.MercuryCredentials
Copy link
Contributor

Choose a reason for hiding this comment

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

If there is an accessor function, does this value need to be exported on the struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is right, the Credentials() is more than enough, will update.

AllowListCache *cache.Cache
}

func (c *MercuryConfig) Credentials() mercury.MercuryCredentials {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you can, try not to return interfaces. It makes the code more brittle. The ideal Go pattern is to consume interfaces and return structs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a trick used to avoid circular dependency. We have this models.MercuryCredentials struct defined in models package.

type MercuryCredentials struct {
	LegacyURL string
	URL       string
	Username  string
	Password  string
}

We then defined this mercury.MercuryCredentials interface under mercury package. models.MercuryCredentials implements mercury.MercuryCredentials so we can use mercury in all mercury related references, while keep the other non-mercury references unchanged.

I inherited this from Fergal. Happy to learn more on how we can improve.

@@ -101,7 +101,7 @@ func (r *EvmRegistry) getBlockHash(blockNumber *big.Int) (common.Hash, error) {
}

// verifyCheckBlock checks that the check block and hash are valid, returns the pipeline execution state and retryable
func (r *EvmRegistry) verifyCheckBlock(_ context.Context, checkBlock, upkeepId *big.Int, checkHash common.Hash) (state encoding.PipelineExecutionState, retryable bool) {
func (r *EvmRegistry) verifyCheckBlock(_ context.Context, checkBlock, upkeepId *big.Int, checkHash common.Hash) (state uint8, retryable bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and many other places, a transition away from defined types to base types was done. Was there a reason for this? We lose some value of strong types in static code analysis when we do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is also because of circular dependencies. The types are defined in encoding package, and we dont want to reference them from mercury packages. The UpkeepFailureReason is used by both mercury and non-mercury. Happy to hear other approaches.

type UpkeepFailureReason uint8
type PipelineExecutionState uint8

Copy link
Collaborator

Choose a reason for hiding this comment

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

As part of the spike, it looked like the defined types were always explicitly cast to their underlying types before use, and we didn't seem to define any custom functions on the new types so it didn't feel necessary to keep the defined types at the time, but it's not something I feel strongly about either

Copy link
Contributor

@infiloop2 infiloop2 Nov 29, 2023

Choose a reason for hiding this comment

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

+1 we should fix this, there's nothing stopping this function from returning a uint8 which is not a valid execution state. Plus as currently implemented there is duplication of definitions between mercury files and encoding files which is just a recipe for bugs

What's the correct dependency structure to do this properly?

c.lggr.Debugf("at block %s upkeep %s received status code %d from mercury v0.2 with BODY=%s", sl.Time.String(), sl.UpkeepId.String(), resp.StatusCode, hexutil.Encode(body))

var m MercuryV02Response
if err := json.Unmarshal(body, &m); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

since we are refactoring now, maybe let's use err1 here to avoid shadow?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer shadowing err over err1, err2 etc

Copy link
Contributor

Choose a reason for hiding this comment

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

The golangci-lint configuration insists that you do not shadow. Is it failing to detect this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jmank88 I dont see lint issues. Can I run golangci-lint locally? if so, i can try to repro.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

There is make golangci-lint for the core module. Not sure about the others but it could be adapted easily


ts := time.Now().UTC().UnixMilli()
signature := mercury.GenerateHMACFn(http.MethodGet, mercuryBatchPathV03+params, []byte{}, c.mercuryConfig.Credentials().GetUsername(), c.mercuryConfig.Credentials().GetPassword(), ts)
req.Header.Set("Content-Type", "application/json")
Copy link
Contributor

Choose a reason for hiding this comment

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

should we put these in some consts since they won't change?

ocr2keepers "github.com/smartcontractkit/ocr2keepers/pkg/v3/types"

"github.com/smartcontractkit/chainlink/v2/core/gethwrappers/generated/automation_utils_2_1"
iregistry21 "github.com/smartcontractkit/chainlink/v2/core/gethwrappers/generated/i_keeper_registry_master_wrapper_2_1"
)

type UpkeepFailureReason uint8
type PipelineExecutionState uint8
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dug more on this. The root cause of circular dependency is the Packer interface. The old Packer interface is very convoluted. For mercury, we just need a subset of the functions, so we defined a smaller Packer for mercury in mercury.go.

type Packer interface {
	UnpackCheckCallbackResult(callbackResp []byte) (uint8, bool, []byte, uint8, *big.Int, error)
	PackGetUpkeepPrivilegeConfig(upkeepId *big.Int) ([]byte, error)
	UnpackGetUpkeepPrivilegeConfig(resp []byte) ([]byte, error)
	DecodeStreamsLookupRequest(data []byte) (*StreamsLookupError, error)
}

The first return of UnpackCheckCallbackResult is a PipelineExecutionState. If we keep using this encoding. PipelineExecutionState, then mercury package will have encoding and encoding package will also need mercury.

type Packer interface {
	UnpackCheckCallbackResult(callbackResp []byte) (uint8, bool, []byte, uint8, *big.Int, error)
...

Copy link
Contributor

Choose a reason for hiding this comment

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

what's the plan to address this? We are causing a regression in typing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to extract Mercury as a standalone lib in this PR, we changed the return type of UnpackCheckCallbackResult func from encoding. PipelineExecutionState to uint8, so as to avoid circular dependency btw encoding and mercury.

IMHO, this encoding package can be refactored further. Those failure reasons and states can be in a standalone package, while packer in its own package. Packer has one-way invocation to the failure reason/states package.

@shileiwill shileiwill force-pushed the refactor_mercury branch 7 times, most recently from b8051e8 to a73e8eb Compare November 8, 2023 21:33
Copy link
Contributor

@FelixFan1992 FelixFan1992 left a comment

Choose a reason for hiding this comment

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

from streams lookup logic perspective, i think everything looks OK so far.

// if the core node has retried totalMediumPluginRetries times, do not set retry interval and plugin will use
// the default interval
} else {
ri = 1 * time.Second
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of this else block, you could change line 44 to beri := 1 * time.Second? Could we also rename ri to interval or retryInterval?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i renamed the variable. this else check should be here bc ok means contains or not...

}

for _, tc := range testCases {
t.Run("", func(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we replace the empty string here with a tc.name field that describes what the test is doing?


if failureReason == uint8(mercury.MercuryUpkeepFailureReasonMercuryCallbackReverted) {
checkResults[i].IneligibilityReason = uint8(mercury.MercuryUpkeepFailureReasonMercuryCallbackReverted)
s.lggr.Debugf("at block %d upkeep %s mercury callback reverts", lookup.Block, lookup.UpkeepId)
Copy link
Contributor

Choose a reason for hiding this comment

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

include time


unpackCallBackState, needed, performData, failureReason, _, err := s.packer.UnpackCheckCallbackResult(mercuryBytes)
if err != nil {
s.lggr.Errorf("at block %d upkeep %s UnpackCheckCallbackResult err: %s", lookup.Block, lookup.UpkeepId, err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

include time


if !needed {
checkResults[i].IneligibilityReason = uint8(mercury.MercuryUpkeepFailureReasonUpkeepNotNeeded)
s.lggr.Debugf("at block %d upkeep %s callback reports upkeep not needed", lookup.Block, lookup.UpkeepId)
Copy link
Contributor

Choose a reason for hiding this comment

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

include time

@shileiwill shileiwill force-pushed the refactor_mercury branch 2 times, most recently from fd1a4e0 to 880e7be Compare November 15, 2023 17:35
@@ -227,6 +229,13 @@ func (k *Keeper) Debug(ctx context.Context, args []string) {
message(fmt.Sprintf("checkUpkeep failed with UpkeepFailureReason %d", checkResult.UpkeepFailureReason))
}
if checkResult.UpkeepFailureReason == uint8(encoding.UpkeepFailureReasonTargetCheckReverted) {
// TODO use the new streams lookup lib
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this TODO still relevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is still relevant, we are not actually using the new lib yet, this is gonna be the next PR.

secret: "yourSecret",
ts: 1234567890,
expected: "17b0bb6b14f7b48ef9d24f941ff8f33ad2d5e94ac343380be02c2f1ca32fdbd8",
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we add more test cases here?

@cl-sonarqube-production
Copy link

@shileiwill shileiwill added this pull request to the merge queue Nov 16, 2023
Merged via the queue into develop with commit e030073 Nov 16, 2023
88 of 89 checks passed
@shileiwill shileiwill deleted the refactor_mercury branch November 16, 2023 18:26
Comment on lines +120 to +122
func (l *StreamsLookup) IsMercuryUsingBatchPathV03() bool {
return l.TimeParamKey == BlockNumber
}
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this also have a check for v03? (l.FeedParamKey == FeedIDs) since the time param key can be for v02 also

Copy link
Contributor

Choose a reason for hiding this comment

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

reading more, the context in which function is being is only for v0.3 so it doesn't cause any issue but function can be named better to reduce confusion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I am thinking of IsMercuryV03UsingBatchPath, let me know if you have a better name.

Copy link
Contributor

Choose a reason for hiding this comment

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

batch path is used in both v0.3 timestamp and block number. looking at the context https://github.com/smartcontractkit/chainlink/blob/develop/core/services/ocr2/plugins/ocr2keeper/evmregistry/v21/mercury/v03/request.go#L103-L105 maybe IsMercuryV03UsingBlockNumber ?

Comment on lines +6 to +14
// upkeep failure onchain reasons
MercuryUpkeepFailureReasonNone MercuryUpkeepFailureReason = 0
MercuryUpkeepFailureReasonTargetCheckReverted MercuryUpkeepFailureReason = 3
MercuryUpkeepFailureReasonUpkeepNotNeeded MercuryUpkeepFailureReason = 4
MercuryUpkeepFailureReasonMercuryCallbackReverted MercuryUpkeepFailureReason = 7
// leaving a gap here for more onchain failure reasons in the future
// upkeep failure offchain reasons
MercuryUpkeepFailureReasonMercuryAccessNotAllowed MercuryUpkeepFailureReason = 32
MercuryUpkeepFailureReasonInvalidRevertDataInput MercuryUpkeepFailureReason = 34
Copy link
Contributor

Choose a reason for hiding this comment

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

how can we remove this duplication. This is just adding surface area for bugs when this file goes out of sync with interface file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about we move the states/errors in interface file to a separate (standalone and light) package, so we can share states/errors across different services, mercury is one of the service referencing the states/errors?

Copy link
Contributor

Choose a reason for hiding this comment

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

possibly. looking at interface package, the circular dependency is because of DecodeStreamsLookupRequest(data []byte) (*mercury.StreamsLookupError, error)

Not sure if we can move to some mercury package to remove the cyclic dependency

Comment on lines +5 to +14
const (
NoPipelineError MercuryUpkeepState = 0
RpcFlakyFailure MercuryUpkeepState = 3
MercuryFlakyFailure MercuryUpkeepState = 4
PackUnpackDecodeFailed MercuryUpkeepState = 5
MercuryUnmarshalError MercuryUpkeepState = 6
InvalidMercuryRequest MercuryUpkeepState = 7
InvalidMercuryResponse MercuryUpkeepState = 8 // this will only happen if Mercury server sends bad responses
UpkeepNotAuthorized MercuryUpkeepState = 9
)
Copy link
Contributor

Choose a reason for hiding this comment

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

same question as above

Comment on lines +108 to +110
func (l *StreamsLookup) IsMercuryVersionUnkown() bool {
return l.FeedParamKey != FeedIDs
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this function is a bit confusing FeedParamKey can be FeedIdHex and not have unknown version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I think we can remove this function. Looking at the usage, what we really need is to determine it is not v02 and not v03 (We can just use not IsMercuryV03())

Copy link
Contributor

Choose a reason for hiding this comment

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

yep sounds good

// buildResult checks if the upkeep is allowed by Mercury and builds a streams lookup request from the check result
func (s *streams) buildResult(ctx context.Context, i int, checkResult ocr2keepers.CheckResult, checkResults []ocr2keepers.CheckResult, lookups map[int]*mercury.StreamsLookup) {
lookupLggr := s.lggr.With("where", "StreamsLookup")
if checkResult.IneligibilityReason != uint8(mercury.MercuryUpkeepFailureReasonTargetCheckReverted) {
Copy link
Contributor

@infiloop2 infiloop2 Nov 30, 2023

Choose a reason for hiding this comment

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

this is a confusing constant, the ineligibility reason is generic target check reverted and not specific to mercury.

the revert data is what defines whether this revert was for mercury or not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I guess a better representation could be "if checkResult.IneligibilityReason != uint8(encoding.UpkeepFailureReasonTargetCheckReverted)". This goes back to the package name discussion, where to hold the states and failureReasons.

checkResults[i].IneligibilityReason = uint8(mercury.MercuryUpkeepFailureReasonMercuryAccessNotAllowed)
return
}
} else if streamsLookupResponse.IsMercuryVersionUnkown() {
Copy link
Contributor

Choose a reason for hiding this comment

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

as per current implementation this is not really an unknown check but a non v0.3 check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, true, will update.

Comment on lines +60 to +63
func (bs *BlockSubscriber) LatestBlock() *ocr2keepers.BlockKey {
return bs.latestBlock.Load()
}

Copy link
Contributor

Choose a reason for hiding this comment

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

i'm not sure why this change was needed. was this moved from somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This blockSubscriber is used in StreamsLookup, to be more precise, StreamsLookup uses only the latestBlock from blockSubscriber. This blockSubscriber is heavy and from a heavy package. we dont want to carry this to the new streams lib.

So, in this refactoring, streams takes a latestBlockProvider interface and requires implementation of LatestBlock function. Check streams.go L#71

Would like to hear if there is better way to achieve this.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good

@@ -227,6 +229,13 @@ func (k *Keeper) Debug(ctx context.Context, args []string) {
message(fmt.Sprintf("checkUpkeep failed with UpkeepFailureReason %d", checkResult.UpkeepFailureReason))
}
if checkResult.UpkeepFailureReason == uint8(encoding.UpkeepFailureReasonTargetCheckReverted) {
// TODO use the new streams lookup lib
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is still relevant, we are not actually using the new lib yet, this is gonna be the next PR.

ocr2keepers "github.com/smartcontractkit/ocr2keepers/pkg/v3/types"

"github.com/smartcontractkit/chainlink/v2/core/gethwrappers/generated/automation_utils_2_1"
iregistry21 "github.com/smartcontractkit/chainlink/v2/core/gethwrappers/generated/i_keeper_registry_master_wrapper_2_1"
)

type UpkeepFailureReason uint8
type PipelineExecutionState uint8
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to extract Mercury as a standalone lib in this PR, we changed the return type of UnpackCheckCallbackResult func from encoding. PipelineExecutionState to uint8, so as to avoid circular dependency btw encoding and mercury.

IMHO, this encoding package can be refactored further. Those failure reasons and states can be in a standalone package, while packer in its own package. Packer has one-way invocation to the failure reason/states package.

Comment on lines +120 to +122
func (l *StreamsLookup) IsMercuryUsingBatchPathV03() bool {
return l.TimeParamKey == BlockNumber
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I am thinking of IsMercuryV03UsingBatchPath, let me know if you have a better name.

Comment on lines +6 to +14
// upkeep failure onchain reasons
MercuryUpkeepFailureReasonNone MercuryUpkeepFailureReason = 0
MercuryUpkeepFailureReasonTargetCheckReverted MercuryUpkeepFailureReason = 3
MercuryUpkeepFailureReasonUpkeepNotNeeded MercuryUpkeepFailureReason = 4
MercuryUpkeepFailureReasonMercuryCallbackReverted MercuryUpkeepFailureReason = 7
// leaving a gap here for more onchain failure reasons in the future
// upkeep failure offchain reasons
MercuryUpkeepFailureReasonMercuryAccessNotAllowed MercuryUpkeepFailureReason = 32
MercuryUpkeepFailureReasonInvalidRevertDataInput MercuryUpkeepFailureReason = 34
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about we move the states/errors in interface file to a separate (standalone and light) package, so we can share states/errors across different services, mercury is one of the service referencing the states/errors?

Comment on lines +108 to +110
func (l *StreamsLookup) IsMercuryVersionUnkown() bool {
return l.FeedParamKey != FeedIDs
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I think we can remove this function. Looking at the usage, what we really need is to determine it is not v02 and not v03 (We can just use not IsMercuryV03())

checkResults[i].IneligibilityReason = uint8(mercury.MercuryUpkeepFailureReasonMercuryAccessNotAllowed)
return
}
} else if streamsLookupResponse.IsMercuryVersionUnkown() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, true, will update.

}

if block == nil || block.Int64() == 0 {
if latestBlock := s.blockSubscriber.LatestBlock(); latestBlock != nil && latestBlock.Number != 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

LatestBlock() is used in this buildCallOpts function. This is part of blockSubscriber.

}

if block == nil || block.Int64() == 0 {
if latestBlock := s.blockSubscriber.LatestBlock(); latestBlock != nil && latestBlock.Number != 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

LatestBlock() is used in buildCallOpts(). This is part of blockSubscriber.

func NewStreamsLookup(
packer mercury.Packer,
mercuryConfig mercury.MercuryConfigProvider,
blockSubscriber latestBlockProvider,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To get rid of the dependency of blockSubscriber, we created this latestBlockProvider interface, which requires latestBlock() func. This latestBlock func is the only func we need in streamsLookup.

Also refer to streams.go L#310

Comment on lines +60 to +63
func (bs *BlockSubscriber) LatestBlock() *ocr2keepers.BlockKey {
return bs.latestBlock.Load()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This blockSubscriber is used in StreamsLookup, to be more precise, StreamsLookup uses only the latestBlock from blockSubscriber. This blockSubscriber is heavy and from a heavy package. we dont want to carry this to the new streams lib.

So, in this refactoring, streams takes a latestBlockProvider interface and requires implementation of LatestBlock function. Check streams.go L#71

Would like to hear if there is better way to achieve this.

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.

7 participants