MaxGasPrice reads from TOML config; allow disabling price reporting on a lane by matYang · Pull Request #591 · smartcontractkit/ccip

matYang · 2024-03-07T04:12:42Z

Motivation

Step 1 of Batch Gas Price reporting, allows for option of disabling gas and token price reporting on a lane, and make MaxGasPrice same per chain, as opposed to having it potentially different per lane.

matYang · 2024-03-07T04:17:52Z

core/services/ocr2/plugins/ccip/ccipcommit/initializers.go

 	unregisterFuncs := []func() error{
 		func() error {
-			return factory.CloseCommitStoreReader(lggr, versionFinder, params.commitStoreAddress, params.destChain.Client(), params.destChain.LogPoller(), params.sourceChain.GasEstimator(), qopts...)
+			return factory.CloseCommitStoreReader(lggr, versionFinder, params.commitStoreAddress, params.destChain.Client(), params.destChain.LogPoller(), params.sourceChain.GasEstimator(), params.sourceChain.Config().EVM().GasEstimator().PriceMax().ToInt(), qopts...)


confirmed with BIX that we can get PriceMax this way, and not worry about PriceMaxKey, closed core PR that exposes this in estimator interface: https://github.com/smartcontractkit/chainlink/pull/12272/files

connorwstein · 2024-03-08T15:01:03Z

core/services/ocr2/plugins/ccip/ccipcommit/ocr2_test.go


 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			obs, err := validateObservations(ctx, logger.TestLogger(t), destTokens, tc.f, tc.commitObservations)


nit: can avoid this if you keep the function pure and just pass in the bool

connorwstein · 2024-03-08T17:21:05Z

core/services/ocr2/plugins/ccip/internal/ccipdata/factory/commit_store.go

 		return nil, err
 	}

+	lggr.Infow("Initializing CommitStore Reader", "version", version.String(), "masGasPrice", maxGasPrice.String())


huh why doesn't maxGasPrice.String panic in the tests when nil?

Afaik it's by Golang's design, it has a nil check in func (x *Int) Text(base int) string
https://stackoverflow.com/questions/42238624/calling-a-method-on-a-nil-struct-pointer-doesnt-panic-why-not

connorwstein · 2024-03-08T17:22:36Z

core/services/ocr2/plugins/ccip/internal/ccipdata/factory/commit_store.go

 		return nil, err
 	}

+	lggr.Infow("Initializing CommitStore Reader", "version", version.String(), "masGasPrice", maxGasPrice.String())


typo - masGasPrice

connorwstein · 2024-03-08T17:25:26Z

docs/CHANGELOG_CCIP.md


+### Changed
+- `CommitStore` offchain config format changed:
+  - Removed `MaxGasPrice` and `SourceMaxGasPrice` fields. MaxGasPrice will be read from PriceMax in chain TOML.


Maybe for further clarification - the fields MaxGasPrice/SourceMaxGasPrice will be ignored if specified but won't error (non-breaking change)

connorwstein · 2024-03-08T17:25:47Z

docs/CHANGELOG_CCIP.md

 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

+
 ## 1.5.0 - Unreleased


Could potentially be released in 1.4.X right?

Last time Da mentioned 1.5 is slated for April, it's probably more realistic to target that. Given we are already at 1.4.5, cutting this in 1.5 makes sense too.

connorwstein · 2024-03-08T17:27:43Z

core/services/ocr2/plugins/ccip/internal/ccipdata/factory/commit_store.go


-func NewCommitStoreReader(lggr logger.Logger, versionFinder VersionFinder, address cciptypes.Address, ec client.Client, lp logpoller.LogPoller, estimator gas.EvmFeeEstimator, pgOpts ...pg.QOpt) (ccipdata.CommitStoreReader, error) {
-	return initOrCloseCommitStoreReader(lggr, versionFinder, address, ec, lp, estimator, false, pgOpts...)
+func NewCommitStoreReader(lggr logger.Logger, versionFinder VersionFinder, address cciptypes.Address, ec client.Client, lp logpoller.LogPoller, estimator gas.EvmFeeEstimator, maxGasPrice *big.Int, pgOpts ...pg.QOpt) (ccipdata.CommitStoreReader, error) {


lets be super clear this is sourceMaxGasPrice

connorwstein · 2024-03-08T17:28:06Z

core/services/ocr2/plugins/ccip/internal/ccipdata/factory/offramp.go


-func NewOffRampReader(lggr logger.Logger, versionFinder VersionFinder, addr cciptypes.Address, destClient client.Client, lp logpoller.LogPoller, estimator gas.EvmFeeEstimator, registerFilters bool, pgOpts ...pg.QOpt) (ccipdata.OffRampReader, error) {
-	return initOrCloseOffRampReader(lggr, versionFinder, addr, destClient, lp, estimator, false, registerFilters, pgOpts...)
+func NewOffRampReader(lggr logger.Logger, versionFinder VersionFinder, addr cciptypes.Address, destClient client.Client, lp logpoller.LogPoller, estimator gas.EvmFeeEstimator, maxGasPrice *big.Int, registerFilters bool, pgOpts ...pg.QOpt) (ccipdata.OffRampReader, error) {


ditto here - let's be very clear this is destMaxGasPrice

connorwstein · 2024-03-08T23:32:39Z

core/services/ocr2/plugins/ccip/internal/ccipdata/v1_0_0/offramp.go

 }

-func NewOffRamp(lggr logger.Logger, addr common.Address, ec client.Client, lp logpoller.LogPoller, estimator gas.EvmFeeEstimator) (*OffRamp, error) {
+func NewOffRamp(lggr logger.Logger, addr common.Address, ec client.Client, lp logpoller.LogPoller, estimator gas.EvmFeeEstimator, maxGasPrice *big.Int) (*OffRamp, error) {


paranoid - can we name this param destMaxGasPrice too, ditto for 1.2 etc

connorwstein · 2024-03-08T23:32:55Z

core/services/ocr2/plugins/ccip/internal/ccipdata/v1_0_0/commit_store.go

 }

-func NewCommitStore(lggr logger.Logger, addr common.Address, ec client.Client, lp logpoller.LogPoller, estimator gas.EvmFeeEstimator) (*CommitStore, error) {
+func NewCommitStore(lggr logger.Logger, addr common.Address, ec client.Client, lp logpoller.LogPoller, estimator gas.EvmFeeEstimator, maxGasPrice *big.Int) (*CommitStore, error) {


can we name this sourceMaxGasPrice too

…n a lane (#591) ## Motivation Step 1 of Batch Gas Price reporting, allows for option of disabling gas and token price reporting on a lane, and make `MaxGasPrice` same per chain, as opposed to having it potentially different per lane.

matYang added 7 commits February 22, 2024 09:53

adding reporting flag

2e4be23

backwards compatible change with PriceReportingDisabled

7bd5612

Remove MaxGasPrice references

616ddc8

set MaxGasPrice in CommitStore and OffRamp from chain's PriceMax

8c363c9

add logs and decode test

e25355a

skip price reporting when configured

6207f82

pass tests

d961ed5

matYang requested review from connorwstein and mateusz-sekara March 7, 2024 04:12

matYang commented Mar 7, 2024

View reviewed changes

matYang added 2 commits March 6, 2024 23:47

add single test case

3631630

Merge branch 'ccip-develop' into batch-gas-reporting

364fad9

matYang temporarily deployed to sdlc March 7, 2024 04:56 — with GitHub Actions Inactive

add commit obs unit test

459283d

matYang temporarily deployed to sdlc March 7, 2024 16:35 — with GitHub Actions Inactive

matYang added 3 commits March 7, 2024 15:39

complete unit tests

dc95e60

fix missing param

b2d2789

Merge branch 'ccip-develop' into batch-gas-reporting

8f80f67

matYang temporarily deployed to sdlc March 7, 2024 20:58 — with GitHub Actions Inactive

fix broken integ test

e8aa293

matYang temporarily deployed to sdlc March 7, 2024 22:15 — with GitHub Actions Inactive

matYang marked this pull request as ready for review March 7, 2024 22:42

matYang requested review from a team and jasonmci as code owners March 7, 2024 22:42

connorwstein reviewed Mar 8, 2024

View reviewed changes

address comments

bc6dbcd

matYang temporarily deployed to sdlc March 8, 2024 22:16 — with GitHub Actions Inactive

matYang requested a review from connorwstein March 8, 2024 22:18

connorwstein approved these changes Mar 8, 2024

View reviewed changes

matYang added 2 commits March 9, 2024 01:45

address comments

dd0e812

Merge branch 'ccip-develop' into batch-gas-reporting

9da3aa5

AnieeG approved these changes Mar 9, 2024

View reviewed changes

matYang merged commit 86fca85 into ccip-develop Mar 10, 2024

matYang deleted the batch-gas-reporting branch March 10, 2024 01:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MaxGasPrice reads from TOML config; allow disabling price reporting on a lane#591

MaxGasPrice reads from TOML config; allow disabling price reporting on a lane#591
matYang merged 17 commits intoccip-developfrom
batch-gas-reporting

matYang commented Mar 7, 2024

Uh oh!

matYang Mar 7, 2024 •

edited

Loading

Uh oh!

connorwstein Mar 8, 2024

Uh oh!

connorwstein Mar 8, 2024

Uh oh!

matYang Mar 8, 2024 •

edited

Loading

Uh oh!

connorwstein Mar 8, 2024

Uh oh!

connorwstein Mar 8, 2024

Uh oh!

connorwstein Mar 8, 2024

Uh oh!

matYang Mar 8, 2024 •

edited

Loading

Uh oh!

connorwstein Mar 8, 2024

Uh oh!

connorwstein Mar 8, 2024

Uh oh!

connorwstein Mar 8, 2024

Uh oh!

connorwstein Mar 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).


		## 1.5.0 - Unreleased

Conversation

matYang commented Mar 7, 2024

Motivation

Uh oh!

matYang Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matYang Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matYang Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

matYang Mar 7, 2024 •

edited

Loading

matYang Mar 8, 2024 •

edited

Loading

matYang Mar 8, 2024 •

edited

Loading