Post version abstraction follow up #194

connorwstein · 2023-10-09T20:01:50Z

Motivation

Address TODOs and nits from version abstraction PRs.

Solution

…/ccip into post-versioning-cleanup

connorwstein · 2023-10-11T21:09:28Z

core/chains/evm/logpoller/orm.go

-			AND block_number <= $5
-			ORDER BY (block_number, log_index)`, utils.NewBig(o.chainID), address, eventSig, minBlock, maxBlock)
+			AND (block_number + $4) <= (SELECT COALESCE(block_number, 0) FROM evm.log_poller_blocks WHERE evm_chain_id = $1 ORDER BY block_number DESC LIMIT 1)
+			AND block_timestamp > $5


@mateusz-sekara without this we'll be unable to query for CreatedAt logs which were backfilled so I believe its required. You'll see very strange results running the query after a series of restarts/backfills as random logs will be omitted. Came up testing the price reg reader, can discuss further

Spoke offline - including the fix here, then will merge from core when a new release comes out.

For the reference smartcontractkit/chainlink#10928

core/chains/evm/logpoller/orm_test.go

connorwstein · 2023-10-11T21:14:59Z

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

@@ -1,24 +1,42 @@
-package ccipdata
+package ccipdata_test


blackbox tests are superior generally imo, they are much better at defending against regressions while refactoring and tend to provide long term value with minimal maintenance (eg TestIntegration_CCIP still standing >1 year). We should save whitebox tests solely for highly complex internal helper components/functions

Agree, I was not familiar with that pattern in Go. Also, it incentivize devs to work on better API/interfaces, because tests are kinda enforcing it ;)

connorwstein · 2023-10-11T21:22:48Z

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

+	}
+}
+
+func TestPriceRegistryReader(t *testing.T) {


general testing philosophy:

Blackbox tests against the reader which we run across all versions. Ensures plugin behaviour is unaffected by versioning.

We can obtain high fidelity tests with minimal code by using the geth sim + LogPollerTest. The benefit is that we avoid having to mock out every eth/lp interaction which permits us flexibility to refactor the readers with strong assurances that if the contract remains the same regressions can be avoided. There is a slight perf cost to using LogPollerTest as it requires a db, however it should be possible to in the near future inject a fake memory ORM into the lp (we'd need some equality test between the fake and the db to ensure no loss of fidelity).

core/chains/evm/logpoller/orm.go

connorwstein · 2023-10-12T19:05:22Z

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

@@ -159,3 +175,195 @@ func TestCommitOnchainConfig(t *testing.T) {
 		})
 	}
 }
+
+func TestCommitStoreReaders(t *testing.T) {


Intend to follow with a similar deep test for offramp/onramp readers, but this PR is already large and on/offramp have somewhat lower risk compared to price reg/commit changes.

Maybe a good followup ticket for @jarnaud ? ;)

connorwstein · 2023-10-12T19:05:57Z

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

+
+	for v, cr := range crs {
+		cr := cr
+		t.Run("CommitStoreReader "+v, func(t *testing.T) {


can come back to tidy this up (pull out helpers etc), but want to get the coverage in place first.

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

connorwstein · 2023-10-12T19:24:33Z

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

-		// Unfortunately the v1 price registry doesn't have a method to get the version so assume if it errors
-		// its v1.
-		return NewPriceRegistryV1_0_0(lggr, priceRegistryAddress, lp, cl)
+		if strings.Contains(err.Error(), "execution reverted") {


Want an explicit error to avoid confusing a random RPC failure from indicating 1.0. Spot checked this against a couple testnet RPCs and they all appear to be consistent with "execution reverted". Should only be required until we upgrade everywhere to 1.2.

I thing this could be part of some tooling. I can imagine a call like this

if rpcErrors.IsExecutionReverted(err) {}

matYang · 2023-10-13T06:26:55Z

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

-	report := ExecReport{
-		Messages:          []internal.EVM2EVMMessage{},
+	report := ccipdata.ExecReport{
+		Messages:          []internal.EVM2EVMMessage(nil),


curious why this needs to be explicitly nil as opposed to empty?

@matYang Check this example: https://go.dev/play/p/x4JK1a1-u8b

Out of curiosity, I'd love to prefer empty slices over nil slice. Null is always bad because it's very error-prone and it's easier to forget about null checking, code becomes more brittle. Processing an empty slice is usually similar to processing a nil slice, but doesn't require this additional null checking.

@dimkouv that's kinda the question, I am curious why do we want it to be null after encoding? Is it a specific requirement from a library? Onchain doesn't care, it just checks for array length.

its just because our decode leaves it nil var messages []internal.EVM2EVMMessages. Could change it but seems out of scope for this PR

actually nothing is relying on nil upstream so quick fix, will add

matYang · 2023-10-13T06:35:31Z

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

+		},
+	}
+	gasPriceUpdatesBlock2 := []ccipdata.GasPrice{
+		{


Can we add a multi-gas price scenario in block 2? V1_0_0 should return error, while V1_2_0 should work fine.

not sure how you mean - 1_0_0 contract signature wont allow it?

hmm I see ApplyPriceRegistryUpdateV1_0_0 is only used for testing and only takes the 1st gas price. I was thinking we can add a test case with multiple gas prices, and v1_2_0 can correctly update them, while v1_0_0 throws. Not necessary now, not a blocker.

matYang · 2023-10-13T06:50:34Z

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

-	assertFilterRegistration(t, new(lpmocks.LogPoller), func(lp *lpmocks.LogPoller, addr common.Address) Closer {
-		c, err := NewCommitStoreV1_2_0(logger.TestLogger(t), addr, new(mocks.Client), lp, nil)
+	assertFilterRegistration(t, new(lpmocks.LogPoller), func(lp *lpmocks.LogPoller, addr common.Address) ccipdata.Closer {
+		c, err := ccipdata.NewCommitStoreV1_2_0(logger.TestLogger(t), addr, new(mocks.Client), lp, nil)
 		require.NoError(t, err)
 		return c
 	}, 1)
 }

 func TestCommitOffchainConfig_Encoding(t *testing.T) {


Is this test mostly for testing the validate function? Maybe it could be more succinct and include V1_0_0

yeah just validate. Leaving broader test cleanup out of scope for now

matYang · 2023-10-13T06:54:39Z

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

+	onramp1 := randomAddress()
+	onramp2 := randomAddress()
+	// Report
+	rep := ccipdata.CommitStoreReport{


for large tests like this where variables definitions and usage can be far apart, I think it can make sense to use full words like report, ditto for a number of vars below, like ge and lm

yeah can come back and prettify this

dimkouv

nits

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

dimkouv · 2023-10-13T07:54:43Z

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

-	report := ExecReport{
-		Messages:          []internal.EVM2EVMMessage{},
+	report := ccipdata.ExecReport{
+		Messages:          []internal.EVM2EVMMessage(nil),


@matYang Check this example: https://go.dev/play/p/x4JK1a1-u8b

dimkouv · 2023-10-13T07:57:05Z

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

-
-var (
-	UsdPerUnitGasUpdatedV1_0_0 = abihelpers.MustGetEventID("UsdPerUnitGasUpdated", abihelpers.MustParseABI(price_registry.PriceRegistryABI))
+	ExecPluginLabel      = "exec"


Consts do not follow the same naming convention.
-- CamelCase is the Go preferred one

Don't we have this label already defined somewhere? Or maybe I saw it in @dimkouv PR that is not merged yet

yeah didn't want to change this for backwards compat

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

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

dimkouv · 2023-10-13T08:12:46Z

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

@@ -1,4 +1,4 @@
-package ccipdata
+package ccipdata_test


To avoid import cycle?

to black box test

core/chains/evm/logpoller/orm.go

mateusz-sekara · 2023-10-13T09:45:14Z

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

@@ -89,9 +89,10 @@ func NewCommitStoreReader(lggr logger.Logger, address common.Address, ec client.
 		return nil, errors.Errorf("expected %v got %v", ccipconfig.EVM2EVMOnRamp, contractType)
 	}
 	switch version.String() {
-	case v1_0_0, v1_1_0:


I've noticed that during perf tests, but it seems that we lost some visibility into contract interactions. So far we have used observability/contract_wrapper.go to track the usages of different contract methods with prom metrics. This logic is no longer in use (probably should be moved to the reader layer)

Going to fix it in a separate PR

mateusz-sekara · 2023-10-13T09:49:10Z

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

@@ -159,3 +175,195 @@ func TestCommitOnchainConfig(t *testing.T) {
 		})
 	}
 }
+
+func TestCommitStoreReaders(t *testing.T) {


Maybe a good followup ticket for @jarnaud ? ;)

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

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

mateusz-sekara · 2023-10-13T10:00:22Z

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

-		// Unfortunately the v1 price registry doesn't have a method to get the version so assume if it errors
-		// its v1.
-		return NewPriceRegistryV1_0_0(lggr, priceRegistryAddress, lp, cl)
+		if strings.Contains(err.Error(), "execution reverted") {


I thing this could be part of some tooling. I can imagine a call like this

if rpcErrors.IsExecutionReverted(err) {}

mateusz-sekara · 2023-10-13T10:11:50Z

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

+func setupPriceRegistryReaderTH(t *testing.T) priceRegReaderTH {
+	user, ec := newSim(t)
+	lggr := logger.TestLogger(t)
+	// TODO: We should be able to use an in memory log poller ORM here to speed up the tests.


Did we measure the impact of the db here? I feel that tests are rather fast, even those using Postgres. Switching to in-memory orm gives us the performance boost in running tests but also opens us to not spotting some bugs that would happen with DB test. LogPoller depends on some Postgres internals like constraints and transaction isolation. IMO I would keep it either using a database orm or having the entire LogPoller faked. At this moment, LogPoller's logic is leaked into ORM (because of db TXs for instance, and validating some conditions by database), which means that LogPoller and DbORM kind of should always work together, because lack of proper domain isolation here - they are a separate interfaces and struct, but they are tightly coupled.
I've seen bugs that should have been noticed by unit tests, but they escaped because in-memory db was used in tests and MySQL on real envs (not in CLL ofc)

Haven't measured no, but there's no question it'd be much faster (queries would move from ms to ns). Agreed its a tradeoff between speed/accuracy and would only be worth it if we found a way to maintain high accuracy

What I'm saying is that optimization from ms -> ns might not even be noticeable to us. If we could reduce test execution from 7 minutes to 4 minutes, it would be great, but reducing from 7 minutes to 6 minutes 55 seconds might not be worth making tests less accurate

mateusz-sekara · 2023-10-13T10:13:21Z

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

+	commitAndGetBlockTs(ec) // Deploy these
+	pr10r, err := ccipdata.NewPriceRegistryReader(lggr, addr, lp, ec)
+	require.NoError(t, err)
+	assert.Equal(t, reflect.TypeOf(pr10r).String(), reflect.TypeOf(&ccipdata.PriceRegistryV1_0_0{}).String())


nit, but I see that we keep mixing assert with require and both packages have almost identical set of methods. I would love to have some ground rule that we either use one or another

my rule is just generally to avoid panics so we can maximize data gathered per test run. So usually that means a length / nil check followed by asserts

Ohh, I see, so assert will let tests continue but eventually mark them as failed, but require halts it immediately, right? Nice, I didn't know that

mateusz-sekara · 2023-10-13T10:17:23Z

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

+	gasPriceUpdatesBlock1 := []ccipdata.GasPrice{
+		{
+			DestChainSelector: dest,
+			Value:             big.NewInt(11),


There are a lot of magic numbers in tests. If they are fixed for some reason, then may be worth documenting or using a conts. If they can be anything, then I would suggest starting using random values. In Ruby on Rails there is a great lib faker for generating random values (but still meaningful e.g. name), not sure if it is sth similar for Go. Anyway, random number generation doesn't require 3rd party lib ;)

If we use random values, then it's a clear signal to the dev that we can use anything, and tests are either written in a way that supports it or this value doesn't matter for them. When I see ints passed back and forth I have mixed feelings when reading tests - is it important that some xyzValue is set to 100 or not

agreed, added some comments. Can follow up with further clarification

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

mateusz-sekara

I also noticed that functions from loaders.go are still in use, posing some risk that the wrong contract wrapper will be used in the Commit/Exec plugin (I think it's only for init, but still...). Also we have two solutions in place for versioning right now.

This means that probably everything that is under loaders.go should be replaced with the new Reader layer, right? @connorwstein

connorwstein · 2023-10-13T14:51:03Z

I also noticed that functions from loaders.go are still in use, posing some risk that the wrong contract wrapper will be used in the Commit/Exec plugin (I think it's only for init, but still...). Also we have two solutions in place for versioning right now.

This means that probably everything that is under loaders.go should be replaced with the new Reader layer, right? @connorwstein

Yep.. out of scope for this PR

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

matYang and others added 10 commits October 2, 2023 17:19

multi gas price update in PriceUpdates struct

97fbf5a

update onchain portion

5cb65fa

offchain non-backwards-incompatible changes

8e78369

fix tests

ad5111f

Merge branch 'ccip-develop' into feature/multi-gas-update

e0561fe

pass tests again

9da3c84

update commit store tests

deb8551

update tests

90a03d4

Follow up

66ebc5f

Add price reg 1.0.0 ABI

726ba03

Base automatically changed from feature/multi-gas-update to ccip-develop October 10, 2023 14:01

connorwstein added 4 commits October 10, 2023 14:46

Merge branch 'ccip-develop' into post-versioning-cleanup

774e73b

Black box testing

1bb71d2

Improve test coverage

7d5598d

Finish price reg test

dc7c537

connorwstein marked this pull request as ready for review October 11, 2023 20:24

connorwstein requested a review from a team as a code owner October 11, 2023 20:24

connorwstein added 4 commits October 11, 2023 16:24

Merge branch 'ccip-develop' into post-versioning-cleanup

4f0c930

Fix lp test

48eaafe

Merge branch 'post-versioning-cleanup' of github.com:smartcontractkit…

420cfbe

…/ccip into post-versioning-cleanup

Cleanup

d806fda

connorwstein commented Oct 11, 2023

View reviewed changes

Fix lint

828ff5e

mateusz-sekara reviewed Oct 12, 2023

View reviewed changes

core/chains/evm/logpoller/orm.go Outdated Show resolved Hide resolved

matYang and others added 5 commits October 12, 2023 11:55

Merge branch 'ccip-develop' into post-versioning-cleanup

40c074c

Port lp test and 2 easy TODOs

9eff47a

Improve commit store coverage

840ba66

Test ChangeConfig and fix bug

3843002

Finish commit store sanity

926ce5d

connorwstein commented Oct 12, 2023

View reviewed changes

core/services/ocr2/plugins/ccip/internal/ccipdata/commit_store_v1_0_0.go Outdated Show resolved Hide resolved

Few more todos

6acfd78

connorwstein commented Oct 12, 2023

View reviewed changes

Merge branch 'ccip-develop' into post-versioning-cleanup

92d73fb

matYang reviewed Oct 13, 2023

View reviewed changes

dimkouv reviewed Oct 13, 2023

View reviewed changes

mateusz-sekara reviewed Oct 13, 2023

View reviewed changes

connorwstein added 6 commits October 13, 2023 10:51

Comments

5a24c7a

Merge branch 'ccip-develop' into post-versioning-cleanup

a690d7e

Fmt

0548c01

Merge branch 'ccip-develop' into post-versioning-cleanup

ec80b5b

Fix merge conflicts

8fce785

Fix 100

24ee1f7

matYang approved these changes Oct 13, 2023

View reviewed changes

connorwstein merged commit 9c68d3e into ccip-develop Oct 13, 2023
45 checks passed

connorwstein deleted the post-versioning-cleanup branch October 13, 2023 19:38

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

Post version abstraction follow up (#194)

b727f90

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

Post version abstraction follow up #194

Post version abstraction follow up #194

Conversation

connorwstein commented Oct 9, 2023

Motivation

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorwstein Oct 11, 2023 • edited Loading

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

dimkouv Oct 13, 2023 • edited Loading

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

dimkouv left a comment

Choose a reason for hiding this comment

dimkouv Oct 13, 2023 • edited Loading

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

mateusz-sekara left a comment • edited Loading

Choose a reason for hiding this comment

connorwstein commented Oct 13, 2023

connorwstein Oct 11, 2023 •

edited

Loading

dimkouv Oct 13, 2023 •

edited

Loading

dimkouv Oct 13, 2023 •

edited

Loading

mateusz-sekara left a comment •

edited

Loading