[OBSOLETE] Introduce a chain agnostic generic HEAD type in the core Transaction Manager #8597

prashantkumar1982 · 2023-03-01T01:42:12Z

This PR is now obsolete in favor of #8674

… into headview

github-actions · 2023-03-01T01:42:37Z

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

… into headview2

augustbleeds · 2023-03-02T17:11:45Z

core/chains/evm/txmgr_wrapper.go

+var _ httypes.HeadTrackable = &txmWrapper{}
+
+// EVM specific wrapper to hold the core TxMgr object underneath
+type txmWrapper struct {


What's the reasoning behind introducing this wrapper in this PR? Is it necessary to make Head Tracker generic?

The TXM, after the generic HeadTracking changes implements the following trackable:
OnNewLongestChain(ctx context.Context, head txmgrtypes.HeadView)

Earlier, the last parameter was *evmtypes.Head. It was evm specific, but now is generic.
However, the HeadBroadcaster, which calls this method, still expects the evmtypes.Head param. Rather than change the broadcaster, and thus all subscribers across the codebase, I created a txmWrapper, which accepts evmtypes.Head, and then converts that to HeadView, and then calls the Txm's implementation.
Thus an easy hack to make the TXM generic

Awesome, thanks for explaining

can we call this struct EvmTxm since it will become EVM specific?

and then it would be NewEvmTxManager. we will need to use it to construct the chain-specific gas estimators, etc before passing it to the chain-agnostic TXM

and then the struct would be something like

type EvmTxm struct { txmgr.TxManager httypes.HeadTrackable } func (txm *EvmTxm) OnNewLongestChain(ctx context.Context, evmHead *evmtypes.Head) { ... }

this way you don't need to redefine a bunch of interfaces because the TxManager interface already has them, and all you need is to wrap a function

aalu1418

looking good! there's a few changes that would be useful to see and would setup nicely for extracting other components :)

aalu1418 · 2023-03-06T21:26:55Z

core/chains/evm/txmgr_wrapper.go

+var _ httypes.HeadTrackable = &txmWrapper{}
+
+// EVM specific wrapper to hold the core TxMgr object underneath
+type txmWrapper struct {


can we call this struct EvmTxm since it will become EVM specific?

and then it would be NewEvmTxManager. we will need to use it to construct the chain-specific gas estimators, etc before passing it to the chain-agnostic TXM

and then the struct would be something like

type EvmTxm struct { txmgr.TxManager httypes.HeadTrackable } func (txm *EvmTxm) OnNewLongestChain(ctx context.Context, evmHead *evmtypes.Head) { ... }

this way you don't need to redefine a bunch of interfaces because the TxManager interface already has them, and all you need is to wrap a function

common/txmgr/types/head_view.go

… into headview2

jmank88 · 2023-03-09T23:45:22Z

common/txmgr/types/head_trackable.go

+//
+//go:generate mockery --quiet --name HeadTrackable --output ../mocks/ --case=underscore
+type HeadTrackable[HEAD any] interface {
+	OnNewLongestChain(ctx context.Context, head HeadView[HEAD])


WDTY about simplifying this irregular name while we're changing things here?

Suggested change

OnNewLongestChain(ctx context.Context, head HeadView[HEAD])

NewHead(ctx context.Context, head HeadView[HEAD])

This is indicating that we found a head which creates a new longest chain, not just any head.
For example, if we had 2 forks, A and B, like this:
A: 4,5,6,7
B:4,5

Now if we see a new head #6 on B fork, it won't trigger this callback. Only trigger on A8, or if we reach B8 before A8.

See this for more context:

chainlink/core/chains/evm/headtracker/head_tracker.go

Lines 203 to 211 in e7527e6

if prevHead == nil || head.Number > prevHead.Number {

promCurrentHead.WithLabelValues(ht.chainID.String()).Set(float64(head.Number))

headWithChain := ht.headSaver.Chain(head.Hash)

if headWithChain == nil {

return errors.Errorf("HeadTracker#handleNewHighestHead headWithChain was unexpectedly nil")

}

ht.backfillMB.Deliver(headWithChain)

ht.broadcastMB.Deliver(headWithChain)

How about NewHighestHead like in that error message?

Suggested change

OnNewLongestChain(ctx context.Context, head HeadView[HEAD])

NewHighestHead(ctx context.Context, head HeadView[HEAD])

Yes, I would prefer that name. However note that OnLongestChain name is used in all places that subscribe to HeadBroadcaster, which is way more places than just TxMgr.
If we want to do that, I would prefer to do a repo wide rename in a dedicated PR.

common/txmgr/types/head_view.go

jmank88 · 2023-03-09T23:47:59Z

common/txmgr/types/head_view.go

+
+	// GetNativeHead returns the head in the chain's native type
+	// Chain specific code can retrieve the native type via this function.
+	GetNativeHead() HEAD


Suggested change

GetNativeHead() HEAD

Native() HEAD

I actually don't fully grok how this is meant to work. If a caller knows the type is HEAD, then why aren't they just using that directly in the first place?

Sounds good

I'm also not sure about this call. The chain-specific txm implements the interface to comply with the minimum required functionality based on the HEAD type which is known to it.

The HeadView was a generic interface with a few functions that all chains implement. But this Native() will return the complete Head object in chain's native type. This is being passed to GasEstimators, which need the whole chain specific head object.

The HeadView was a generic interface with a few functions that all chains implement. But this Native() will return the complete Head object in chain's native type. This is being passed to GasEstimators, which need the whole chain specific head object.

What problem does this abstract wrapper solve? All the callers still have to import and reference the native type anyways.

Components inside the core Txm don't need to know or import native types.
For example, look at the eth_confirmer.go file.
It uses the methods in the Head interface, without knowing the native type.

Components inside the core Txm don't need to know or import native types.

Then they should use a less complex abstraction that does not impose knowing the native type - i.e. only an interface.

It uses the methods in the Head interface, without knowing the native type.

It does know the type. It is leaking everywhere. If the generic type is not being used, then it should be eliminated from that code scope with a stronger abstraction that assumes less.

I think there may be another way, which you might prefer.

I create a generic Head interface, similar to the one I currently have here.
Then, the native evmtypes.Head should implement it, and extend it with its own native head fields. Similarly, all chains will implement it, and extend with their own fields.

The txmgr will only use the generic Head interface, and pass it around. Chain-specific code can keep using native Head type.

I didn't do it this way earlier, as I didn't want to change the existing evmtypes.Head, as its being used in a lot of places.

That makes sense to me if it can be done.

jmank88 · 2023-03-10T00:00:27Z

core/chains/evm/txmgr/eth_broadcaster.go

-type EthBroadcaster struct {
+//
+// The type HEAD represents the chain's native head datatype.
+type EthBroadcaster[HEAD any] struct {


Where is this generic type HEAD used? Is it only to propagate to the estimator?

Yes. Is there a better way to handle that without using this generic on the EthBroadcaster level?

I didn't see one 🤷

One stylistic thing that could help limit some of the noise might be to use:

Suggested change

type EthBroadcaster[HEAD any] struct {

type EthBroadcaster[H Head] struct {

With type Head any defined and documented elsewhere.

Does that help?
"Head any" or "H Head" are equally puzzling for the puzzled, no?

Generics are always hard to read and understand, imo. Can't really solve that i think.

It objectively reduces the spam elsewhere through the files, and using single letters is standard, idiomatic Go. We absolutely can and should be working our hardest to make these APIs readable for users.

Are you saying generally, wherever we use generics, instead of saying [XYZ any], we should say [X XYZ]?
I'm not against using idiomatic go, if this is an established best practice.

any is perfectly fine to use given the right circumstances. I still don't fully understand what we are trying to capture in our case, but aliasing something in for any was just meant as a way to document your expectations clearly on a type.

Single letter generic types are the only examples I have seen.

sdrug · 2023-03-10T01:07:16Z

common/txmgr/types/head_view.go

+	ChainLength() uint32
+
+	// EarliestInChain traverses through parents until it finds the earliest one
+	EarliestInChain() HeadView[HEAD]


Shouldn't this return HEAD type as well? Same is for Parent() down below

No these functions are called from within the EthConfirmer, which doesn't understand the native Head type. It only can understand HeadView.

But HeadView does not hide the head type, it just parameterizes it so it can be swapped out.

It does hide the native type for EthConfirmer right?
The EthConfirmer needs to be generic, without importing any native types. But it also needs to call EarliestInChain().

Generic types are not hidden in any way.

Interfaces and generics serve two separate purposes, and when they are mixed together like this they undermine one another.

core/chains/evm/txmgr/eth_broadcaster.go

core/chains/evm/chain.go

cl-sonarqube-production · 2023-03-10T02:19:59Z

SonarQube Quality Gate

56.9% Coverage on New Code (is less than 90%)

See analysis details on SonarQube

augustbleeds · 2023-03-10T14:10:51Z

Does this PR cover all changes in the ticket?

augustbleeds · 2023-03-10T14:14:03Z

common/txmgr/types/head.go

+	EarliestInChain() Head[NATIVE]
+
+	// Hash is the head's block hash
+	Hash() common.Hash


this should be generic right (especially since this is geth's version of a hash)

augustbleeds · 2023-03-10T14:25:31Z

core/chains/evm/txmgr/txmgr.go

@@ -93,7 +96,7 @@ type reset struct {
 	done chan error
 }

-type Txm struct {
+type Txm[HEAD any] struct {


❗ this is a huge step. first generic introduced into the txm service :)

samsondav

What problem does this solve?

augustbleeds · 2023-03-10T14:31:23Z

core/chains/evm/evm_txm.go

+
+// evmTxm is an evm wrapper over the generic TxManager interface
+type evmTxm struct {
+	httypes.HeadTrackable


~~let's use txmgrtypes.Head[*evmtypes.Head] here instead of httypes.HeadTracakable. (just tried locally and it works) so new structs use generic interfaces~~

~~Actually, txmgrtypes.TxManager already requires txmgrtypes.HeadTrackable[HEAD] so I think httypes.HeadTracakable is redundant?~~

Nvm, disregard above. I see what you're trying to accomplish by making the evmTxm compatible with the eth-specific code

Any reasons we don't just change functions that expect httypes.HeadTrackable to expect txmgrtypes.Head[*evmtypes.Head]?

augustbleeds · 2023-03-10T14:45:20Z

What problem does this solve?

It's part of the larger initiative to generalize the Transaction manager to be chain-agnostic so that the bulk of logic needed to integrate with new chains is already written. We have a bunch of tickets to generalize different components, starting with their types first.

jmank88 · 2023-03-10T14:58:29Z

What problem does this solve?

It's part of the larger initiative to generalize the Transaction manager to be chain-agnostic so that the bulk of logic needed to integrate with new chains is already written. We have a bunch of tickets to generalize different components, starting with their types first.

But generic types are not the only way to generalize and should not be taken as a maxim. Each case needs to be considered separately, and many cases here look like they could simply use an interface.

github-actions · 2023-05-21T00:12:59Z

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

prashantkumar1982 added 5 commits February 27, 2023 15:45

Initial changes

53a01fd

Resolving merge conflicts

4422700

midpoint

8cb32ca

Merge branch 'develop' of https://github.com/smartcontractkit/chainlink…

248a535

… into headview

Initial changes with generic HeadView interface

6b4bcc5

test fixes

f4a1437

aalu1418 self-requested a review March 1, 2023 15:30

prashantkumar1982 added 7 commits March 1, 2023 16:33

Merge issues

97aadd4

test fixes

08715d7

Merge branch 'develop' of https://github.com/smartcontractkit/chainlink…

7c95323

… into headview2

docker changes testing

ba27396

mocks

89c5571

mocks fix

cf8bbfe

test fixes

8166f75

augustbleeds reviewed Mar 2, 2023

View reviewed changes

aalu1418 requested changes Mar 6, 2023

View reviewed changes

prashantkumar1982 added 13 commits March 7, 2023 16:34

Merge

d03175f

Merge branch 'develop' of https://github.com/smartcontractkit/chainlink…

bdc3f1a

… into headview2

merging

1e3573c

Merge conflicts and Generic types

23d0be7

Merge branch 'develop' of https://github.com/smartcontractkit/chainlink…

13928c6

… into headview2

Small fixes

58796ad

Small fixes

eae70df

Merge branch 'develop' of https://github.com/smartcontractkit/chainlink…

bea18e1

… into headview2

Build fixes

8471df7

build fixes

15df148

mocks

510ceef

Merge branch 'develop' of https://github.com/smartcontractkit/chainlink…

b10e528

… into headview2

Import fixes

a6d24dc

prashantkumar1982 marked this pull request as ready for review March 9, 2023 23:40

prashantkumar1982 requested review from jkongie, jmank88, samsondav, a team, connorwstein and PiotrTrzpil as code owners March 9, 2023 23:40

jmank88 reviewed Mar 9, 2023

View reviewed changes

common/txmgr/types/head_view.go Outdated Show resolved Hide resolved

jmank88 reviewed Mar 9, 2023

View reviewed changes

jmank88 reviewed Mar 10, 2023

View reviewed changes

sdrug reviewed Mar 10, 2023

View reviewed changes

Response to comments

1390b5e

sdrug reviewed Mar 10, 2023

View reviewed changes

core/chains/evm/txmgr/eth_broadcaster.go Show resolved Hide resolved

sdrug reviewed Mar 10, 2023

View reviewed changes

core/chains/evm/chain.go Outdated Show resolved Hide resolved

nits

3f3e4dd

augustbleeds reviewed Mar 10, 2023

View reviewed changes

samsondav requested changes Mar 10, 2023

View reviewed changes

augustbleeds reviewed Mar 10, 2023

View reviewed changes

prashantkumar1982 mentioned this pull request Mar 10, 2023

Introduce a chain agnostic Head interface in the core Transaction Manager #8674

Merged

prashantkumar1982 changed the title ~~Introduce a chain agnostic generic HEAD type in the core Transaction Manager~~ [OBSOLETE] Introduce a chain agnostic generic HEAD type in the core Transaction Manager Mar 10, 2023

github-actions bot added the Stale label May 21, 2023

github-actions bot closed this May 28, 2023

	OnNewLongestChain(ctx context.Context, head HeadView[HEAD])
	NewHead(ctx context.Context, head HeadView[HEAD])

	if prevHead == nil \|\| head.Number > prevHead.Number {
	promCurrentHead.WithLabelValues(ht.chainID.String()).Set(float64(head.Number))

	headWithChain := ht.headSaver.Chain(head.Hash)
	if headWithChain == nil {
	return errors.Errorf("HeadTracker#handleNewHighestHead headWithChain was unexpectedly nil")
	}
	ht.backfillMB.Deliver(headWithChain)
	ht.broadcastMB.Deliver(headWithChain)

	OnNewLongestChain(ctx context.Context, head HeadView[HEAD])
	NewHighestHead(ctx context.Context, head HeadView[HEAD])

	type EthBroadcaster[HEAD any] struct {
	type EthBroadcaster[H Head] struct {

[OBSOLETE] Introduce a chain agnostic generic HEAD type in the core Transaction Manager #8597

[OBSOLETE] Introduce a chain agnostic generic HEAD type in the core Transaction Manager #8597

Conversation

prashantkumar1982 commented Mar 1, 2023 • edited Loading

github-actions bot commented Mar 1, 2023

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

aalu1418 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

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

cl-sonarqube-production bot commented Mar 10, 2023

augustbleeds commented Mar 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samsondav left a comment

Choose a reason for hiding this comment

augustbleeds Mar 10, 2023 • edited Loading

Choose a reason for hiding this comment

augustbleeds Mar 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

augustbleeds commented Mar 10, 2023

jmank88 commented Mar 10, 2023

github-actions bot commented May 21, 2023

prashantkumar1982 commented Mar 1, 2023 •

edited

Loading

augustbleeds Mar 10, 2023 •

edited

Loading

augustbleeds Mar 10, 2023 •

edited

Loading