[OBSOLETE] Introduce a chain agnostic generic HEAD type in the core Transaction Manager #8597
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| package types | ||
|
|
||
| import "context" | ||
|
|
||
| // HeadTrackable represents a generic type, implemented by the core txm, | ||
| // to be able to receive head events from any chain. | ||
| // Chain implementations should notify head events to the core txm via this interface. | ||
| // | ||
| //go:generate mockery --quiet --name HeadTrackable --output ../mocks/ --case=underscore | ||
| type HeadTrackable[HEAD any] interface { | ||
| OnNewLongestChain(ctx context.Context, head HeadView[HEAD]) | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,32 @@ | ||||||
| package types | ||||||
|
|
||||||
| import "github.com/ethereum/go-ethereum/common" | ||||||
|
|
||||||
| // HeadView provides minimal access to a chain's head, as needed by the TxManager. | ||||||
| // This is a generic interface which ALL chains will implement. | ||||||
| // | ||||||
| //go:generate mockery --quiet --name HeadView --output ./mocks/ --case=underscore | ||||||
| type HeadView[HEAD any] interface { | ||||||
prashantkumar1982 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| // BlockNumber is the head's block number | ||||||
| BlockNumber() int64 | ||||||
|
|
||||||
| // ChainLength returns the length of the chain followed by recursively looking up parents | ||||||
| ChainLength() uint32 | ||||||
|
|
||||||
| // EarliestInChain traverses through parents until it finds the earliest one | ||||||
| EarliestInChain() HeadView[HEAD] | ||||||
|
There was a problem hiding this comment. Shouldn't this return There was a problem hiding this comment. No these functions are called from within the EthConfirmer, which doesn't understand the native Head type. It only can understand HeadView. There was a problem hiding this comment. But There was a problem hiding this comment. It does hide the native type for EthConfirmer right? There was a problem hiding this comment. Generic types are not hidden in any way. There was a problem hiding this comment. Interfaces and generics serve two separate purposes, and when they are mixed together like this they undermine one another. |
||||||
|
|
||||||
| // Hash is the head's block hash | ||||||
| Hash() common.Hash | ||||||
|
|
||||||
| // Parent is the head's parent block | ||||||
| Parent() HeadView[HEAD] | ||||||
|
|
||||||
| // HashAtHeight returns the hash of the block at the given height, if it is in the chain. | ||||||
| // If not in chain, returns the zero hash | ||||||
| HashAtHeight(blockNum int64) common.Hash | ||||||
|
|
||||||
| // GetNativeHead returns the head in the chain's native type | ||||||
| // Chain specific code can retrieve the native type via this function. | ||||||
| GetNativeHead() HEAD | ||||||
|
There was a problem hiding this comment.
Suggested change
There was a problem hiding this comment. I actually don't fully grok how this is meant to work. If a caller knows the type is There was a problem hiding this comment. Sounds good There was a problem hiding this comment. 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 There was a problem hiding this comment. 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. There was a problem hiding this comment.
What problem does this abstract wrapper solve? All the callers still have to import and reference the native type anyways. There was a problem hiding this comment. Components inside the core Txm don't need to know or import native types. There was a problem hiding this comment.
Then they should use a less complex abstraction that does not impose knowing the native type - i.e. only an interface.
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. There was a problem hiding this comment. 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. 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. There was a problem hiding this comment. That makes sense to me if it can be done. |
||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDTY about simplifying this irregular name while we're changing things here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See this for more context:
chainlink/core/chains/evm/headtracker/head_tracker.go
Lines 203 to 211 in e7527e6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
NewHighestHeadlike in that error message?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.