Version abstraction #151

connorwstein · 2023-09-21T21:38:45Z

Motivation

Permit breaking ABI changes without plugin modifications
Step towards non-EVM support
Improve clarity of plugins

Solution

Abstract gethwrappers/logpoller/ethclient behind XXReader's which are version specific and make use of gethwrappers/ethclient/logpoller to satisfy an interface for the plugin's needs
Start with just extracting OnRampReader/USDCReader from Reader as that is required for 1.2 backwards compat

connorwstein · 2023-09-26T13:40:00Z

core/services/ocr2/plugins/ccip/abihelpers/abi_helpers.go

@@ -29,9 +28,8 @@ const (
 	ExecutionStateFailure
 )

+// TODO: Deprecate in favour of version specific types


Intention is all geth/log poller code will live in ccipdata under a reader so specific versions can be supported (not just latest version)

connorwstein · 2023-09-26T13:41:57Z

core/services/ocr2/plugins/ccip/commit_plugin.go


+func jobSpecToCommitPluginConfig(lggr logger.Logger, jb job.Job, pr pipeline.Runner, chainSet evm.LegacyChainContainer) (*CommitPluginConfig, *BackfillArgs, error) {


just to share code with the UnregisterFilter call in DeleteJob. Intention is to rename as Close(...) and just close each reader instead (which internally will unregister filters)

connorwstein · 2023-09-26T13:43:07Z

core/services/ocr2/plugins/ccip/commit_plugin.go

@@ -172,16 +177,6 @@ func CommitReportToEthTxMeta(report []byte) (*txmgr.TxMeta, error) {
 	}, nil
 }

-func getCommitPluginSourceLpFilters(onRamp common.Address) []logpoller.Filter {


The commit plugin's CCIPSend events are tied to a specific onramp address version - so this is now handled in the OnRampReader

connorwstein · 2023-09-26T13:43:48Z

core/services/ocr2/plugins/ccip/commit_plugin.go

-		return err
-	}
-	return unregisterCommitPluginFilters(ctx, sourceChain.LogPoller(), destChain.LogPoller(), commitStore, common.HexToAddress(pluginConfig.OffRamp), qopts...)
+	// TODO: once offramp/commit are abstracted, we can call Close on the offramp/commit readers to unregister filters.


Will follow up with abstracting the rest of these

connorwstein · 2023-09-26T13:44:37Z

core/services/ocr2/plugins/ccip/commit_reporting_plugin.go

-	checkFinalityTags        bool
+	lggr logger.Logger
+	// Source
+	onRampReader        ccipdata.OnRampReader


notice how the source log poller is now completely abstracted in the commit plugin (can't quite abstract it yet in exec since source price reg is not yet abstracted)

connorwstein · 2023-09-26T13:57:28Z

core/services/ocr2/plugins/ccip/testhelpers/ccip_contracts.go

@@ -1475,13 +1476,3 @@ func GetBalance(t *testing.T, chain bind.ContractBackend, tokenAddr common.Addre
 	require.NoError(t, err)
 	return bal
 }
-
-func GenerateCCIPSendLog(t *testing.T, message evm_2_evm_onramp.InternalEVM2EVMMessage) types.Log {


connorwstein · 2023-09-26T13:57:54Z

core/services/ocr2/plugins/ccip/tokendata/reader.go

@@ -18,7 +17,5 @@ var (
 type Reader interface {
 	// ReadTokenData returns the attestation bytes if ready, and throws an error if not ready.
 	ReadTokenData(ctx context.Context, msg internal.EVM2EVMOnRampCCIPSendRequestedWithMeta) (tokenData []byte, err error)
-
-	// GetSourceLogPollerFilters returns the filters that should be used for the source chain log poller
-	GetSourceLogPollerFilters() []logpoller.Filter


ccipdata.USDCReader is now logpoller owner

connorwstein · 2023-09-26T13:58:22Z

core/services/ocr2/plugins/ccip/tokendata/usdc/usdc.go

@@ -37,20 +34,6 @@ const (
 	attestationStatusPending attestationStatus = "pending_confirmations"
 )

-// usdcPayload has to match the onchain event emitted by the USDC message transmitter


log details pushed down to USDCReader

connorwstein · 2023-09-26T13:58:44Z

core/services/ocr2/plugins/ccip/tokendata/usdc/usdc.go

 	if err != nil {
 		return []byte{}, err
 	}
-
-	parsedMsgBody, err := decodeUSDCMessageSent(usdcMessageBody)


GetLastUSDCMessage handles parsing now

connorwstein · 2023-09-26T14:00:03Z

core/services/ocr2/plugins/ccip/tokendata/usdc/usdc_blackbox_test.go

-	require.NoError(t, err)
+	// Message is the bytes itself from MessageSend(bytes message)
+	// i.e. ABI parsed.
+	message := "0x0000000000000001000000020000000000048d71000000000000000000000000eb08f243e5d3fcff26a9e38ae5520a669f4019d000000000000000000000000023a04d5935ed8bc8e3eb78db3541f0abfb001c6e0000000000000000000000006cb3ed9b441eb674b58495c8b3324b59faff5243000000000000000000000000000000005425890298aed601595a70ab815c96711a31bc65000000000000000000000000ab4f961939bfe6a93567cc57c59eed7084ce2131000000000000000000000000000000000000000000000000000000000000271000000000000000000000000035e08285cfed1ef159236728f843286c55fc0861"


making this a true black box test just canned input/output

…ctkit/ccip into feature/backwards-compat-1.2

dimkouv · 2023-09-27T10:17:44Z

core/services/ocr2/plugins/ccip/commit_reporting_plugin.go

-	leafHasher               hashlib.LeafHasherInterface[[32]byte]
-	checkFinalityTags        bool
+	lggr logger.Logger
+	// Source


Having the source/dest dependencies grouped by a comment like this could be improved.
Sometimes it's a bit confusing to just see plugin.onRamp plugin.offRamp, you won't know where this contract exists unless you are familiar with the whole ccip architecture or read the comment (which might also end up being incorrect) in the commitPluginConfig.

I would suggest something like this.

type CommitPluginConfig { source SourceChainDeps dest DestChainDeps } type SourceChainDeps struct { onRampReader ccipdata.OnRampReader } type DestChainDeps struct { offRampReader ccipdata.OffRampReader } func (plugin) Whatever() { plugin.sourceChain.onRampReader.GetWhatever() plugin.destChain.offRampReader.GetWhatever() }

seem like a marginal improvement but would prefer to come back to it after all the versioning abstraction is done

core/services/ocr2/plugins/ccip/commit_reporting_plugin_test.go

core/services/ocr2/plugins/ccip/execution_reporting_plugin_test.go

dimkouv · 2023-09-27T10:27:14Z

core/services/ocr2/plugins/ccip/internal/ccipdata/logpoller.go

@@ -302,9 +184,11 @@ func parseLogs[T any](logs []logpoller.Log, lggr logger.Logger, parseFunc func(l
 		if err == nil {
 			reqs = append(reqs, Event[T]{
 				Data: *data,
-				BlockMeta: BlockMeta{
+				Meta: Meta{


Since we are also moving towards supporting non-evm:

// now type Meta struct { BlockTimestamp time.Time BlockNumber int64 TxHash common.Hash LogIndex uint } // should be something like this since logIndex is evm-specific type Meta struct { BlockTimestamp time.Time BlockNumber int64 TxHash common.Hash EvmMeta *EvmMeta } type EvmMeta struct { LogIndex uint } // or like this... type Meta struct { ChainType string BlockTimestamp time.Time BlockNumber int64 TxHash common.Hash Meta json.RawMessage } // and unmarshal Meta according to the ChainType

yeah still way far out though. Can address that when we get there I'd say out of scope atm

dimkouv · 2023-09-27T10:31:57Z

core/services/ocr2/plugins/ccip/internal/ccipdata/onramp_reader.go

+	Hash           [32]byte
+	// TODO: add more fields as we abstract exec plugin
+	// also this Log can eventually go away with destchain abstractions
+	Log types.Log // Raw event data


just a note: Log (types.Log) is also evm specific

I'm aware 😄

dimkouv · 2023-09-27T10:32:55Z

core/services/ocr2/plugins/ccip/internal/ccipdata/onramp_reader.go

+	GetSendRequestsBetweenSeqNums(ctx context.Context, seqNumMin, seqNumMax uint64, confs int) ([]Event[EVM2EVMMessage], error)
+
+	// Get router configured in the onRamp
+	Router() common.Address


I would suggest renaming this method to RouterAddress() because without reading the method signature I would expect that onramp.Router() returns a router instance that I can use to call Router specific methods.

dimkouv · 2023-09-27T10:37:26Z

core/services/ocr2/plugins/ccip/internal/ccipdata/onramp_reader.go

+	}
+}
+
+func latestFinalizedBlockHash(ctx context.Context, client client.Client) (common.Hash, error) {


This function could be moved under some library package (would be nice to have some unit tests also).

not worth it as we intend to remove it entirely shortly smartcontractkit/chainlink#10762

dimkouv · 2023-09-27T10:39:05Z

core/services/ocr2/plugins/ccip/internal/ccipdata/onramp_v1_0_0.go

+}
+
+func (o *OnRampV1_0_0) GetSendRequestsGteSeqNum(ctx context.Context, seqNum uint64, confs int) ([]Event[EVM2EVMMessage], error) {
+	if !o.finalityTags {


That's the duplicate code I mentioned in the slack thread.
This code is identical for OnRampV1_0_0 and OnRampV1_2_0.

We need to think of some way to reduce this

seems ok to me

1_0_0 code will be removed after the upgrade is complete. Generally true that older versions will be phased out

finalityTags handling will be pushed to lp CCIP-1078 Support for finality tags in LogPoller chainlink#10762 which reduces this to a single lp call

core/services/ocr2/plugins/ccip/internal/ccipdata/onramp_v1_2_0.go

matYang · 2023-09-28T06:07:57Z

core/services/ocr2/plugins/ccip/internal/ccipdata/onramp_reader.go

+	case "1.2.0":
+		return NewOnRampV1_2_0(lggr, sourceSelector, destSelector, onRampAddress, sourceLP, source, finalityTags)
+	default:
+		return nil, errors.Errorf("expected version 1.0.0 got %v", version.String())


nit: maybe just invalid version %v, we aren't strictly expecting a specific version.

connorwstein added 2 commits September 21, 2023 17:36

Demo

2f10752

Demo re-use

ea0eeda

connorwstein requested review from dimkouv and mateusz-sekara September 21, 2023 21:53

connorwstein added 7 commits September 25, 2023 14:23

Pull hasher and source events into onramp reader

4651b30

Cleanup USDC

1e9beaa

Compiling

bf720db

Merge branch 'ccip-develop' into feature/backwards-compat-1.2

0d7efb2

Fix some tests

a8885dd

Fix more tests

53bd56b

More tests

140db0d

connorwstein requested review from RensR and matYang September 26, 2023 00:06

All tests passing

0a3011b

connorwstein commented Sep 26, 2023

View reviewed changes

connorwstein marked this pull request as ready for review September 26, 2023 14:00

connorwstein requested a review from a team as a code owner September 26, 2023 14:00

connorwstein added 3 commits September 26, 2023 10:00

Merge branch 'ccip-develop' into feature/backwards-compat-1.2

647a543

Self review

381e307

Merge branch 'feature/backwards-compat-1.2' of github.com:smartcontra…

dc50a91

…ctkit/ccip into feature/backwards-compat-1.2

dimkouv reviewed Sep 27, 2023

View reviewed changes

connorwstein added 2 commits September 27, 2023 15:17

PR comments

b219382

Merge branch 'ccip-develop' into feature/backwards-compat-1.2

f3d6062

matYang approved these changes Sep 28, 2023

View reviewed changes

dimkouv approved these changes Sep 28, 2023

View reviewed changes

connorwstein merged commit 77ce9d0 into ccip-develop Sep 28, 2023
44 checks passed

connorwstein deleted the feature/backwards-compat-1.2 branch September 28, 2023 13:00

asoliman92 pushed a commit that referenced this pull request Jul 31, 2024

Version abstraction (#151)

5ea19a7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Version abstraction #151

Version abstraction #151

connorwstein commented Sep 21, 2023 •

edited

Loading

connorwstein Sep 26, 2023

connorwstein Sep 26, 2023

connorwstein Sep 26, 2023

connorwstein Sep 26, 2023

connorwstein Sep 26, 2023

connorwstein Sep 26, 2023

connorwstein Sep 26, 2023

connorwstein Sep 26, 2023

connorwstein Sep 26, 2023

connorwstein Sep 26, 2023

dimkouv Sep 27, 2023

connorwstein Sep 27, 2023

dimkouv Sep 27, 2023

connorwstein Sep 27, 2023

dimkouv Sep 27, 2023

connorwstein Sep 27, 2023

dimkouv Sep 27, 2023

dimkouv Sep 27, 2023

connorwstein Sep 27, 2023

dimkouv Sep 27, 2023

dimkouv Sep 27, 2023

connorwstein Sep 27, 2023 •

edited

Loading

matYang Sep 28, 2023


		func jobSpecToCommitPluginConfig(lggr logger.Logger, jb job.Job, pr pipeline.Runner, chainSet evm.LegacyChainContainer) (CommitPluginConfig, BackfillArgs, error) {

Version abstraction #151

Version abstraction #151

Conversation

connorwstein commented Sep 21, 2023 • edited Loading

Motivation

Solution

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

connorwstein Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorwstein commented Sep 21, 2023 •

edited

Loading

connorwstein Sep 27, 2023 •

edited

Loading