Version abstraction part 2 #180

connorwstein · 2023-10-02T15:28:57Z

Motivation

Same as part 1, support breaking ABI changes with minimal plugin impact

Solution

In part 2 we introduce CommitStoreReader, PriceRegistryReader and OffRampReaders which are version agnostic dependencies injected into the plugin. They abstract away all gethwrappers and lp filter handling. Goal is for the version type switch to only exist in the construction of the version agnostic reader.
Notes
- abihelpers is now version agnostic
- Type switch for which type of gas estimator to use is handled in the ConfigChanged impl

connorwstein · 2023-10-03T18:41:45Z

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

 	if err != nil {
 		return nil, nil, err
 	}
-	priceGetterObject, err := pricegetter.NewPipelineGetter(pluginConfig.TokenPricesUSDPipeline, pr, jb.ID, jb.ExternalJobID, jb.Name.ValueOrZero(), lggr)
+
+	// Load all the readers relevant for this plugin.


should be possible to push all contract interactions and initial validations into the readers, but can be done in a follow up

connorwstein · 2023-10-03T18:44:28Z

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

@@ -88,66 +80,3 @@ func TestGetCommitPluginFilterNamesFromSpec(t *testing.T) {
 	}

 }
-
-func TestGetCommitPluginFilterNames(t *testing.T) {


covered by the assertFilterRegistration tests in ccipdata (isolated filter tests per reader)

connorwstein · 2023-10-03T18:48:04Z

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

@@ -59,7 +50,7 @@ type update struct {
 	value     *big.Int
 }

-type CommitPluginConfig struct {
+type CommitPluginStaticConfig struct {


the factory object contains some static config that persists in memory across instances. Naming that static config so thats clear. Then when a new reporting plugin is created, we copy the static config into an instance object CommitReportPlugin (true factory pattern). Can do the same to the exec plugin in a follow up

Nice, I see what you did, ignore my previous comment

connorwstein · 2023-10-03T18:50:22Z

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

+	// between plugin instances (ie between SetConfigs onchain)
+	config CommitPluginStaticConfig
+
+	// Dynamic readers


following the same pattern that was here prior except with readers instead of filters. There is likely a cleaner overall solution that involves using oracle.Close() for cleanup (called in both config change and job spec delete), but leaving that out of scope for now as it likely also depends on resolving the feeds transactionality issue (see comment in UnregisterFiltersXX)

connorwstein · 2023-10-03T18:51:57Z

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

+
+// NewReportingPlugin returns the ccip CommitReportingPlugin and satisfies the ReportingPluginFactory interface.
+func (rf *CommitReportingPluginFactory) NewReportingPlugin(config types.ReportingPluginConfig) (types.ReportingPlugin, types.ReportingPluginInfo, error) {
+	destPriceReg, err := rf.config.commitStore.ConfigChanged(config.OnchainConfig, config.OffchainConfig)


passing the config as blogs to the version specific commit store minimizes version switches as the version specific reader knows how to parse its own config

connorwstein · 2023-10-03T18:57:17Z

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

 	execLggr := lggr.Named("CCIPExecution").With(
 		"sourceChain", ChainName(int64(chainId)),
 		"destChain", ChainName(destChainID))
+	// TODO: we don't support onramp source registry changes without a reboot yet?


important follow up needed

connorwstein · 2023-10-03T19:00:35Z

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

+	}
+}
+
+// Backwards compat for tests


quick follow up is to always use readers in tests, not too difficult but involves mocking a call to TypeAndVersion. Leaving out of scope for now and just enc/dec v1 reports

connorwstein · 2023-10-03T19:01:25Z

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

@@ -38,145 +36,10 @@ func NewLogPollerReader(lp logpoller.LogPoller, lggr logger.Logger, client evmcl
 	}
 }

-func (c *LogPollerReader) GetTokenPriceUpdatesCreatedAfter(ctx context.Context, priceRegistryAddress common.Address, ts time.Time, confs int) ([]Event[price_registry.PriceRegistryUsdPerTokenUpdated], error) {


LogPollerReader is broken down into PriceRegistryReader,CommitStoreReader and OffRampReader. Only element that remains is the LatestBlock

connorwstein · 2023-10-03T19:02:09Z

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

+
+// Do not change the JSON format of this struct without consulting with
+// the RDD people first.
+type ExecOffchainConfig struct {


this has not changed across versions yet, leaving as a common struct until it breaks

connorwstein · 2023-10-03T19:14:49Z

core/services/relay/evm/ccip.go

+	if err != nil {
+		return nil, err
+	}
+	fn, err := ccip.CommitReportToEthTxMeta(typ, ver)


required as reports are version dependent

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

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

…ractkit/ccip into feature/backwards-compat-part2

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

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

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

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

connorwstein · 2023-10-05T18:51:13Z

Followed up with @dimkouv offline to clarify a few things. Some follow ups planned:

Further unit testing of readers
Investigate pushing caching and all RPC handling into readers as well as reader composition

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

matYang · 2023-10-05T21:04:38Z

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

@@ -26,6 +26,7 @@ type FakeCommitStore struct {

 func NewFakeCommitStore(t *testing.T, nextSeqNum uint64) (*FakeCommitStore, common.Address) {
 	addr := utils.RandomAddress()
+	//mockCommitStore := mock_contracts.NewCommitStoreInterface(t)


nit: duplicate

matYang · 2023-10-05T21:10:00Z

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

+var (
+	_                                OffRampReader = &OffRampV1_0_0{}
+	ExecutionStateChangedEventV1_0_0               = abihelpers.MustGetEventID("ExecutionStateChanged", abihelpers.MustParseABI(evm_2_evm_offramp.EVM2EVMOffRampABI))
+	ExecutionStateChangedSeqNrV1_0_0               = 1


nit: would prefer ExecutionStateChangedSeqNrIndexV1_0_0 since this is the index, SeqNr might be misleading

matYang · 2023-10-05T21:16:06Z

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

+			}
+			return commitReportToEthTxMeta(commitReport)
+		}, nil
+		// TODO: 1.2 will split


note to self

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

Spoke offline

Co-authored-by: Matt Yang <[email protected]> Co-authored-by: dimkouv <[email protected]>

connorwstein added 13 commits September 28, 2023 10:26

Price reg wip

ef17ad6

Wip

5229810

Wip

d114b00

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

4bd69cb

Progress on config versioning

5c27967

Report encoding

9a9d094

More cleanup

b444c0a

Regen mocks

c2751bf

TODOs

30076f3

Filter testing

b877f2a

Fix a test, generate mocks

02b1958

Compiling

1ae17d2

Fix some tests and lints

e08837f

connorwstein commented Oct 3, 2023

View reviewed changes

First pass self review

461170b

connorwstein requested review from matYang and dimkouv October 3, 2023 19:22

matYang and others added 2 commits October 4, 2023 02:04

fix commit plugin observation test case

629a789

fix da tests

28421b3

dimkouv reviewed Oct 4, 2023

View reviewed changes

core/services/ocr2/plugins/ccip/abihelpers/abi_helpers.go Show resolved Hide resolved

core/services/ocr2/plugins/ccip/execution_batch_building.go Outdated Show resolved Hide resolved

matYang and others added 2 commits October 4, 2023 11:39

tiny commit plugin test prettify

ea2b720

Fix lint and metatx

d4a8544

connorwstein marked this pull request as ready for review October 4, 2023 15:39

connorwstein requested a review from a team as a code owner October 4, 2023 15:39

connorwstein added 3 commits October 4, 2023 11:40

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

87aea9b

Merge branch 'feature/backwards-compat-part2' of github.com:smartcont…

437df74

…ractkit/ccip into feature/backwards-compat-part2

Fix several bugs

6d071cf

connorwstein requested a review from a team as a code owner October 4, 2023 18:05

connorwstein added 2 commits October 4, 2023 14:33

Fix commit reader encoding

6565148

Add lock for dynamic config

7b393f1

connorwstein added 4 commits October 4, 2023 15:32

Fix exec encoding

7c3abd6

Fix meta test

a9198c6

Fix test

c20ec9b

Permit empty gas prices

f280c55

dimkouv previously requested changes Oct 5, 2023

View reviewed changes

PR comments

0274a89

connorwstein added 3 commits October 5, 2023 15:02

Fix tests and regen

f8de232

Fix tests

57da55f

Update mock

4432504

matYang reviewed Oct 5, 2023

View reviewed changes

core/services/ocr2/plugins/ccip/commit_reporting_plugin.go Outdated Show resolved Hide resolved

core/services/ocr2/plugins/ccip/commit_reporting_plugin.go Show resolved Hide resolved

connorwstein added 2 commits October 5, 2023 16:43

Comments

b7869ea

Fix tests

1f6878e

matYang reviewed Oct 5, 2023

View reviewed changes

matYang approved these changes Oct 5, 2023

View reviewed changes

connorwstein merged commit 8e9719d into ccip-develop Oct 5, 2023
44 checks passed

connorwstein deleted the feature/backwards-compat-part2 branch October 5, 2023 22:03

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

Version abstraction part 2 (#180)

0df8f98

Co-authored-by: Matt Yang <[email protected]> Co-authored-by: dimkouv <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Version abstraction part 2 #180

Version abstraction part 2 #180

connorwstein commented Oct 2, 2023 •

edited

Loading

connorwstein Oct 3, 2023

connorwstein Oct 3, 2023

connorwstein Oct 3, 2023

dimkouv Oct 5, 2023

connorwstein Oct 3, 2023

connorwstein Oct 3, 2023

connorwstein Oct 3, 2023

connorwstein Oct 3, 2023

connorwstein Oct 3, 2023

connorwstein Oct 3, 2023

connorwstein Oct 3, 2023

connorwstein commented Oct 5, 2023

matYang Oct 5, 2023

matYang Oct 5, 2023

matYang Oct 5, 2023

Version abstraction part 2 #180

Version abstraction part 2 #180

Conversation

connorwstein commented Oct 2, 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

connorwstein commented Oct 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorwstein commented Oct 2, 2023 •

edited

Loading