Update reporting plugin factory constructors #20

makramkd · 2024-07-04T10:03:38Z

We need to inject the required dependencies in order for the plugins to correctly function.

makramkd · 2024-07-17T15:41:03Z

pkg/crconsts/consts.go

Renamed package to consts since its not just chain reader stuff.

makramkd · 2024-07-17T15:45:12Z

commit/factory.go

@@ -35,42 +38,88 @@ func (p PluginFactoryConstructor) NewReportingPluginFactory(
 	keyValueStore core.KeyValueStore,
 	relayerSet core.RelayerSet,
 ) (core.OCR3ReportingPluginFactory, error) {
-	return NewPluginFactory(), nil
+	return nil, errors.New("unimplemented")


We'll revisit this when the loopification work starts.

makramkd · 2024-07-17T15:46:23Z

commit/factory.go

+				MaxObservationLength: 20_000, // 20kB
+				MaxOutcomeLength:     10_000, // 10kB
+				MaxReportLength:      10_000, // 10kB


These are ballpark, we'll revisit as we test.

Will this be read from config in the future?

No these will likely be constants, but might not be these values.

makramkd · 2024-07-17T15:46:46Z

commit/plugin.go

@@ -157,7 +157,7 @@ func (p *Plugin) Observation(
 	// If there's no previous outcome (first round ever), we only observe the latest committed sequence numbers.
 	// and on the next round we use those to look for messages.
 	if outctx.PreviousOutcome == nil {
-		p.lggr.Debugw("first round ever, can't observe new messages yet")
+		p.lggr.Infow("first round ever, can't observe new messages yet")


Making these info because turning on debug logs in the CL node brings an avalanche of logs that are not relevant.

but then it will always show even without turning on debug logs, right?

Log levels filter downards, so Debug means Debug and all levels above, Info means Info and all levels above, etc.

dimkouv

🚀 (approving with minor comments)

commit/factory.go

dimkouv · 2024-07-17T15:55:49Z

commit/plugin.go

@@ -167,7 +167,7 @@ func (p *Plugin) Observation(
 	if err != nil {
 		return types.Observation{}, fmt.Errorf("decode commit plugin previous outcome: %w", err)
 	}
-	p.lggr.Debugw("previous outcome decoded", "outcome", prevOutcome.String())
+	p.lggr.Infow("previous outcome decoded", "outcome", prevOutcome.String())


isn't String() used by default? (not sure)

I'm not 100% sure but doesn't hurt. I think depending on the logger it sometimes tries to json encode it.

execute/factory.go

internal/reader/ccip.go

dimkouv · 2024-07-17T15:58:17Z

internal/reader/ccip.go

 	for chain, cfg := range sourceConfigs {
-		if cfg.OnRamp == "" {
+		if len(cfg.OnRamp) == 0 {


nit: maybe cfg.OnRamp == nil ? not sure what's more idiomatic

I think this is more idiomatic, since both nil and the empty slice will have length zero.

dimkouv · 2024-07-17T15:58:41Z

internal/reader/ccip.go

-				Address: cfg.OnRamp,
-				Name:    crconsts.ContractNameOnRamp,
+				// TODO: revisit, this is EVM specific.
+				Address: "0x" + hex.EncodeToString(cfg.OnRamp),


re-use the func to convert bytes to string

commit/factory.go

winder · 2024-07-17T19:36:25Z

commit/plugin.go

@@ -349,7 +349,7 @@ func (p *Plugin) ShouldTransmitAcceptedReport(
 		return false, fmt.Errorf("validate merkle roots state: %w", err)
 	}

-	p.lggr.Debugw("transmitting report",
+	p.lggr.Infow("transmitting report",


Making this one Info level seems fine.... not sure about the others. Making everything Info means there would be an avalanche of Info logs as well

We'll have to revisit these later, for now I think this is slightly better than turning on Debug and getting all the debug logs of the app.

winder

I think you might have gotten a little carried away with Debug -> Info. Other than that, LGTM.

winder · 2024-07-17T20:21:20Z

commit/factory.go

+	// defaultMsgScanBatchSize is the default batch size for sequence number range queries.
+	// Since 256 messages is the limit for a merkle root, this is set to 256.
+	// NOTE: maybe we can also set this in the OCR config, offchainConfig.
+	defaultMsgScanBatchSize = 256


Looks like there's already a constant for this

Suggested change

defaultMsgScanBatchSize = 256

defaultMsgScanBatchSize = merklemulti.MaxNumberTreeLeaves

Fixed, just used that const directly.

github-actions · 2024-07-17T20:23:33Z

Test Coverage

Branch	Coverage
mk/plugin-factory-constructors	74.8%
ccip-develop	75.6%

## Motivation We still need to create the plugin and bootstrap oracles, meaning there needs to be a concrete implementation of the `OracleCreator` interface. ## Solution Implement the `OracleCreator` interface. An end to end Integration test is also included in this PR, which tests commit reports. However, these commit reports do NOT contain merkle roots, some issues need to be resolved in upcoming PRs which involve changes in chainlink-common. ## Notes * This PR is using `legacyevm` instead of relayers because chain writer changes have not yet fully propagated to this repo. Once they have we can update the implementation to use the relayers. ## Requires * smartcontractkit/chainlink-ccip#20 --------- Co-authored-by: Abdelrahman Soliman (Boda) <2677789+asoliman92@users.noreply.github.com> Co-authored-by: asoliman <abdelrahman.soliman@smartcontract.com>

We still need to create the plugin and bootstrap oracles, meaning there needs to be a concrete implementation of the `OracleCreator` interface. Implement the `OracleCreator` interface. An end to end Integration test is also included in this PR, which tests commit reports. However, these commit reports do NOT contain merkle roots, some issues need to be resolved in upcoming PRs which involve changes in chainlink-common. * This PR is using `legacyevm` instead of relayers because chain writer changes have not yet fully propagated to this repo. Once they have we can update the implementation to use the relayers. * smartcontractkit/chainlink-ccip#20 --------- Co-authored-by: Abdelrahman Soliman (Boda) <2677789+asoliman92@users.noreply.github.com> Co-authored-by: asoliman <abdelrahman.soliman@smartcontract.com>

makramkd added 5 commits July 4, 2024 13:02

commit: update factory constructor

31dbb8b

Merge branch 'ccip-develop' into mk/plugin-factory-constructors

709c6e6

Merge branch 'ccip-develop' into mk/plugin-factory-constructors

ce92963

add contract writers, plugin limits

059bfe2

add exec plugin factory constructor

639e06f

makramkd mentioned this pull request Jul 10, 2024

core/services/ccipcapability: oracle creator impls smartcontractkit/ccip#1180

Merged

makramkd added 4 commits July 12, 2024 12:45

add consts for chain writer

99b5944

more method names

b6dfb9d

disambiguate common methods further

a1a9150

add home chain reader consts

923e0ca

makramkd force-pushed the mk/plugin-factory-constructors branch from b7620f0 to 923e0ca Compare July 15, 2024 12:10

makramkd added 11 commits July 15, 2024 15:20

Merge branch 'ccip-develop' into mk/plugin-factory-constructors

e7841aa

use consts in home chain reader

8ce5cf3

remove panic

c4ab63b

hacky hacks

c4748dc

don't fetch source chain cfg for dest

fb15b58

add todo

c210e1f

pass in filled config

e39aca3

info level logs

b40ce4d

cast to ptr

fcace5e

another log

32e475b

Merge branch 'ccip-develop' into mk/plugin-factory-constructors

5c32485

makramkd commented Jul 17, 2024

View reviewed changes

makramkd marked this pull request as ready for review July 17, 2024 15:41

fix lint

2183765

makramkd commented Jul 17, 2024

View reviewed changes

makramkd requested review from dimkouv and asoliman92 July 17, 2024 15:48

makramkd requested review from winder and 0xnogo July 17, 2024 15:48

dimkouv approved these changes Jul 17, 2024

View reviewed changes

makramkd added 2 commits July 17, 2024 21:37

address comments

1bc85fe

Merge branch 'ccip-develop' into mk/plugin-factory-constructors

688d42c

winder reviewed Jul 17, 2024

View reviewed changes

commit/factory.go Outdated Show resolved Hide resolved

winder reviewed Jul 17, 2024

View reviewed changes

winder approved these changes Jul 17, 2024

View reviewed changes

winder reviewed Jul 17, 2024

View reviewed changes

use const from merklemulti

3929cb0

makramkd merged commit 0bcfc8e into ccip-develop Jul 17, 2024
1 check passed

makramkd deleted the mk/plugin-factory-constructors branch July 17, 2024 20:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update reporting plugin factory constructors #20

Update reporting plugin factory constructors #20

makramkd commented Jul 4, 2024

makramkd Jul 17, 2024

makramkd Jul 17, 2024

makramkd Jul 17, 2024

asoliman92 Jul 17, 2024

makramkd Jul 17, 2024

makramkd Jul 17, 2024

asoliman92 Jul 17, 2024

makramkd Jul 17, 2024

dimkouv left a comment

dimkouv Jul 17, 2024

makramkd Jul 17, 2024

dimkouv Jul 17, 2024

makramkd Jul 17, 2024

dimkouv Jul 17, 2024

winder Jul 17, 2024

makramkd Jul 17, 2024

winder left a comment

winder Jul 17, 2024

makramkd Jul 17, 2024

github-actions bot commented Jul 17, 2024

	defaultMsgScanBatchSize = 256
	defaultMsgScanBatchSize = merklemulti.MaxNumberTreeLeaves

Update reporting plugin factory constructors #20

Update reporting plugin factory constructors #20

Conversation

makramkd commented Jul 4, 2024

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

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

winder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 17, 2024

Test Coverage