Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add logic to calculate gas deviation based on fitted curve #15977

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
Open

Add logic to calculate gas deviation based on fitted curve #15977

wants to merge 9 commits into from

Conversation

ogtownsend
Copy link
Collaborator

@ogtownsend ogtownsend commented Jan 17, 2025 •
edited
Loading

Description

  • Adds a new function DeviatesOnCurve() to CCIP calc to have a more flexible gas deviation threshold based on a sliding log scale
  • This new gas curve function will output larger deviation thresholds for smaller (in USD terms) gas prices, and smaller deviation thresholds for higher gas prices

Motivation

  • It doesn't provide much value to publish a gas update when the gas price changes from $0.00000001 to $0.00000004, all it does it cause us to spend more on publishing these gas updates

@ogtownsend ogtownsend requested a review from matYang January 17, 2025 22:56
@ogtownsend ogtownsend force-pushed the CCIP-4811-design-and-implement-the-deviation-curve-in-ccip-1-5 branch 2 times, most recently from 5353774 to 492db0a Compare January 21, 2025 02:55
@ogtownsend ogtownsend changed the title Add logic to calculate gas deviation based on fitted curve [Do not merge] Add logic to calculate gas deviation based on fitted curve Jan 21, 2025
@ogtownsend ogtownsend marked this pull request as ready for review January 21, 2025 02:55
@ogtownsend ogtownsend requested review from a team as code owners January 21, 2025 02:55
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 21, 2025 •
edited
Loading

AER Report: CI Core ran successfully ✅

aer_workflow , commit

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

ogtownsend
ogtownsend commented Jan 21, 2025
View reviewed changes
core/services/ocr2/plugins/ccip/internal/ccipcalc/calc.go Outdated
Comment on lines 79 to 82
if ppb != EthereumThresholdGatePPB {
return Deviates(xOld, xNew, ppb)
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though this is technically product-specific logic, I decided to keep this check here in calc.go since it's acting as a gate for this deviation curve feature. Having the gate in a single location seemed safer than to have it implemented in several locations higher up in the multiple gas estimators we have.

core/services/ocr2/plugins/ccip/internal/ccipcalc/calc.go Show resolved Hide resolved
@ogtownsend ogtownsend force-pushed the CCIP-4811-design-and-implement-the-deviation-curve-in-ccip-1-5 branch from d9d81b0 to b3b3a86 Compare January 21, 2025 20:03
@ogtownsend ogtownsend changed the title [Do not merge] Add logic to calculate gas deviation based on fitted curve Add logic to calculate gas deviation based on fitted curve Jan 21, 2025
@ogtownsend ogtownsend force-pushed the CCIP-4811-design-and-implement-the-deviation-curve-in-ccip-1-5 branch from 8ee0986 to 7590c20 Compare January 22, 2025 05:29
matYang
matYang reviewed Jan 22, 2025
View reviewed changes
core/services/ocr2/plugins/ccip/prices/da_price_estimator.go
@@ -134,7 +141,7 @@ func (g DAGasPriceEstimator) Deviates(ctx context.Context, p1, p2 *big.Int) (boo
return execDeviates, nil
}

return ccipcalc.Deviates(p1DAGasPrice, p2DAGasPrice, g.daDeviationPPB), nil
return ccipcalc.DeviatesOnCurve(p1DAGasPrice, p2DAGasPrice, big.NewInt(DANoDeviationThresholdUSD), g.daDeviationPPB), nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we discussed scaling DA deviation by 2x on the curve, is that something still worthwhile to have? If so maybe add a multiplier

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given how aggressive this new curve is and with the existing complexity of DA vs exec "no deviation thresholds", I have a slight preference to keeping things as-is and more simple for this first iteration. After launch, using portions of Abhishek's sim script we should be able to relatively easily query percentage of deviations triggered by exec vs DA. If DA heavily outweighs exec then 2x-ing the DA side of things could be a fast-follow.

core/services/ocr2/plugins/ccip/internal/ccipcalc/calc.go Show resolved Hide resolved
@ogtownsend ogtownsend force-pushed the CCIP-4811-design-and-implement-the-deviation-curve-in-ccip-1-5 branch from 7590c20 to f07e559 Compare January 22, 2025 21:18
matYang
matYang previously approved these changes Jan 22, 2025
View reviewed changes
@ogtownsend ogtownsend force-pushed the CCIP-4811-design-and-implement-the-deviation-curve-in-ccip-1-5 branch from f07e559 to 7df13ab Compare January 23, 2025 21:19
@ogtownsend ogtownsend force-pushed the CCIP-4811-design-and-implement-the-deviation-curve-in-ccip-1-5 branch from 7df13ab to f340a87 Compare January 23, 2025 21:21
@cl-sonarqube-production
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube

@ogtownsend ogtownsend added this pull request to the merge queue Jan 23, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 24, 2025
@ogtownsend ogtownsend added this pull request to the merge queue Jan 24, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krehermann krehermann krehermann approved these changes

@matYang matYang matYang approved these changes

@jmank88 jmank88 jmank88 approved these changes

@patrickhuie19 patrickhuie19 Awaiting requested review from patrickhuie19 patrickhuie19 is a code owner automatically assigned from smartcontractkit/foundations

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ogtownsend @jmank88 @matYang @krehermann