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

gas estimator + fee boosting integ test #15208

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

gas estimator + fee boosting integ test #15208

wants to merge 26 commits into from
+296 −66

Conversation

0xnogo
Copy link
Collaborator

@0xnogo 0xnogo commented Nov 12, 2024 •
edited
Loading

  • Introducing a fee boosting test
  • Adding up the DestGasOverhead in the gas estimation
  • Change the FeeQuoter config to fit the contract caclulation
  • Accepting prices as input in order to allow fee estimation below fee paid

@github-actions GitHub Actions
Copy link
Contributor

I see you updated files related to core. Please run pnpm changeset in the root directory to add a changeset as well as in the text include at least one of the following tags:

  • #added For any new functionality added.
  • #breaking_change For any functionality that requires manual action for the node to boot.
  • #bugfix For bug fixes.
  • #changed For any change to the existing functionality.
  • #db_update For any feature that introduces updates to database schema.
  • #deprecation_notice For any upcoming deprecation functionality.
  • #internal For changesets that need to be excluded from the final changelog.
  • #nops For any feature that is NOP facing and needs to be in the official Release Notes for the release.
  • #removed For any functionality/config that is removed.
  • #updated For any functionality that is updated.
  • #wip For any change that is not ready yet and external communication about it should be held off till it is feature complete.

🎖️ No JIRA issue number found in: PR title, commit message, or branch name. Please include the issue ID in one of these.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 12, 2024 •
edited
Loading

AER Report: CI Core ran successfully ✅

aer_workflow , commit

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

@github-actions GitHub Actions
Copy link
Contributor

Flaky Test Detector for deployment/go.mod project has failed ❌

Ran new or updated tests between develop and 2943efd (ng/fee-boosting-integ).

View Flaky Detector Details | Compare Changes

Failed Tests

Ran 15 tests in total for all affected test packages. Below are the tests identified as flaky, with a pass ratio lower than the 100% threshold:

TestPackage                                                      TestName                                                  PassRatio  RunCount   Skipped
---------                                                        ---------                                                 ---------  ---------  ---------
github.com/smartcontractkit/chainlink/deployment/ccip/changeset  Test_Messaging                                            0%         1          false
github.com/smartcontractkit/chainlink/deployment/ccip/changeset  Test_FeeBoosting/boost_needed_due_to_WETH_price_increase  0%         1          false
github.com/smartcontractkit/chainlink/deployment/ccip/changeset  Test_FeeBoosting                                          0%         1          false
github.com/smartcontractkit/chainlink/deployment/ccip/changeset  Test_FeeBoosting/boost_needed_due_to_LINK_price_decrease  0%         1          false
github.com/smartcontractkit/chainlink/deployment/ccip/changeset  Test_FeeBoosting/boost_needed_due_to_gas_price_increase   0%         1          false

@github-actions GitHub Actions
Copy link
Contributor

Flaky Test Detector for deployment/go.mod project has failed ❌

Ran new or updated tests between develop and c0ecf6c (ng/fee-boosting-integ).

View Flaky Detector Details | Compare Changes

Failed Tests

Ran 15 tests in total for all affected test packages. Below are the tests identified as flaky, with a pass ratio lower than the 100% threshold:

TestPackage                                                      TestName                                                  PassRatio  RunCount   Skipped
---------                                                        ---------                                                 ---------  ---------  ---------
github.com/smartcontractkit/chainlink/deployment/ccip/changeset  Test_FeeBoosting                                          0%         1          false
github.com/smartcontractkit/chainlink/deployment/ccip/changeset  Test_Messaging                                            0%         1          false
github.com/smartcontractkit/chainlink/deployment/ccip/changeset  Test_FeeBoosting/boost_needed_due_to_WETH_price_increase  0%         1          false
github.com/smartcontractkit/chainlink/deployment/ccip/changeset  Test_FeeBoosting/boost_needed_due_to_LINK_price_decrease  0%         1          false
github.com/smartcontractkit/chainlink/deployment/ccip/changeset  Test_FeeBoosting/boost_needed_due_to_gas_price_increase   0%         1          false

@github-actions GitHub Actions
Copy link
Contributor

Flaky Test Detector for deployment/go.mod project has failed ❌

Ran new or updated tests between develop and 8b0acc2 (ng/fee-boosting-integ).

View Flaky Detector Details | Compare Changes

Failed Tests

Ran 10 tests in total for all affected test packages. Below are the tests identified as flaky, with a pass ratio lower than the 100% threshold:

TestPackage                                                      TestName                                                 PassRatio  RunCount   Skipped
---------                                                        ---------                                                ---------  ---------  ---------
github.com/smartcontractkit/chainlink/deployment/ccip/changeset  Test_FeeBoosting/boost_needed_due_to_gas_price_increase  0%         1          false
github.com/smartcontractkit/chainlink/deployment/ccip/changeset  Test_FeeBoosting                                         0%         1          false

@0xnogo 0xnogo changed the title fee boosting integ test gas estimator + fee boosting integ test Nov 14, 2024
@0xnogo 0xnogo mentioned this pull request Nov 14, 2024
Merged
makramkd
makramkd reviewed Nov 15, 2024
View reviewed changes
Copy link
Contributor

@makramkd makramkd left a comment

Choose a reason for hiding this comment

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

Still in progress

core/capabilities/ccip/ccipevm/gas_helpers.go Show resolved Hide resolved
core/capabilities/ccip/ccipevm/gas_helpers.go Show resolved Hide resolved
deployment/ccip/add_lane.go Outdated Show resolved Hide resolved
deployment/ccip/add_lane.go Outdated Show resolved Hide resolved
@0xnogo 0xnogo requested a review from a team as a code owner November 15, 2024 08:22
makramkd
makramkd previously approved these changes Nov 15, 2024
View reviewed changes
@lukaszcl
Copy link
Collaborator

Hey @0xnogo! I see one job related to detecting flaky tests is failing. The issue may not be related to your PR. I think you can ignore this for now. I'll try to debug it later today.

asoliman92
asoliman92 reviewed Nov 15, 2024
View reviewed changes
integration-tests/smoke/fee_boosting_test.go Outdated
}

func runFeeboostTestCase(tc feeboostTestCase) {
require.NoError(tc.t, ccipdeployment.AddLane(tc.deployedEnv.Env, tc.onchainState, tc.sourceChain, tc.destChain, tc.initialPrices))
Copy link
Contributor

Choose a reason for hiding this comment

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

should this move to test setup better?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to pass in the input prices. As mentioned offline, this will change when we will fix the bug in the commit plugin which prevent prices from being published when there is no merkle root

Copy link
Contributor

Choose a reason for hiding this comment

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

fixed here: smartcontractkit/chainlink-ccip#316

asoliman92
asoliman92 reviewed Nov 15, 2024
View reviewed changes
integration-tests/smoke/fee_boosting_test.go
Receiver: common.LeftPadBytes(tc.onchainState.Chains[tc.destChain].Receiver.Address().Bytes(), 32),
Data: []byte("message that needs fee boosting"),
TokenAmounts: nil,
FeeToken: common.HexToAddress("0x0"),
Copy link
Contributor

Choose a reason for hiding this comment

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

How's the token address 0x0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

0x0 = Paying in Eth directly

asoliman92
asoliman92 reviewed Nov 15, 2024
View reviewed changes
integration-tests/smoke/fee_boosting_test.go Outdated
ccipdeployment.ReplayLogs(tc.t, tc.deployedEnv.Env.Offchain, replayBlocks)

ccipdeployment.ConfirmCommitForAllWithExpectedSeqNums(tc.t, tc.deployedEnv.Env, tc.onchainState, expectedSeqNum, startBlocks)
ccipdeployment.ConfirmExecWithSeqNrForAll(tc.t, tc.deployedEnv.Env, tc.onchainState, expectedSeqNum, startBlocks)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to assert that the fee was actually boosted with a more accurate measure?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will come in as a second step. We wanted to increase our coverage first. But there is no easy way of doing it.

asoliman92
asoliman92 previously approved these changes Nov 15, 2024
View reviewed changes
Copy link
Contributor

@asoliman92 asoliman92 left a comment

Choose a reason for hiding this comment

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

I don't think that this is enough as it doesn't really assert that fee boosting was run successfully and the recalculated fees. Approving as you mentioned in our conversation that it's the first iteration and that it's waiting a couple of bug fixes on chainlink-ccip side to be able to continue it properly.

@0xnogo 0xnogo added this pull request to the merge queue Nov 15, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 15, 2024
@0xnogo 0xnogo dismissed stale reviews from asoliman92 and makramkd via 29944c4 November 16, 2024 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@makramkd makramkd makramkd left review comments

@0xAustinWang 0xAustinWang 0xAustinWang approved these changes

@asoliman92 asoliman92 asoliman92 left review comments

@winder winder Awaiting requested review from winder winder is a code owner automatically assigned from smartcontractkit/ccip-offchain

@rstout rstout Awaiting requested review from rstout rstout is a code owner automatically assigned from smartcontractkit/ccip-offchain

@dimkouv dimkouv Awaiting requested review from dimkouv dimkouv is a code owner automatically assigned from smartcontractkit/ccip-offchain

@mateusz-sekara mateusz-sekara Awaiting requested review from mateusz-sekara mateusz-sekara is a code owner automatically assigned from smartcontractkit/ccip-offchain

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

At least 2 approving reviews are required to merge this pull request.

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

Successfully merging this pull request may close these issues.
6 participants
@0xnogo @lukaszcl @rstout @asoliman92 @makramkd @0xAustinWang