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

SHIP-1754 Consolidate FinalityDepth config #13702

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

SHIP-1754 Consolidate FinalityDepth config #13702

wants to merge 31 commits into from

Conversation

adityavadhavkar1215
Copy link

@adityavadhavkar1215 adityavadhavkar1215 commented Jun 26, 2024 •
edited by jira bot
Loading

SHIP-1754

Requires Dependencies

No

Resolves Dependencies

No

@adityavadhavkar1215 adityavadhavkar1215 requested a review from a team June 26, 2024 20:31
@adityavadhavkar1215 adityavadhavkar1215 requested a review from a team as a code owner June 26, 2024 20:31
@adityavadhavkar1215 adityavadhavkar1215 requested a review from a team as a code owner July 5, 2024 14:13
@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

simsonraj
simsonraj reviewed Jul 17, 2024
View reviewed changes
core/chains/evm/config/toml/defaults/OKX_Testnet.toml
@@ -1 +1,2 @@
ChainID = '65'
FinalityDepth = 50
Copy link
Contributor

Choose a reason for hiding this comment

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

@friedemannf is this right for OKX?

Copy link
Contributor

Choose a reason for hiding this comment

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

This value isn't defined at CCIP, can we get a check on this please?
@friedemannf

@Madalosso
Copy link
Contributor

Blocked by smartcontractkit/ccip#1151
The new value defined on CCIP PR should be replicated over this repo on ccip/config/evm/*.toml

@Madalosso
Copy link
Contributor

This PR Blocks https://github.com/smartcontractkit/infra-k8s/pull/18775
As these values need to be merged/deployed before we get rid of the overrides at infra-k8s

@Madalosso
Copy link
Contributor

Madalosso commented Sep 30, 2024 •
edited
Loading

Sum up and steps to get every PR merged:

  1. We need to get CCIP PR reviewed/merged
  2. Replicate changes from CCIP PR into this repo ccip/config/evm/*.toml
  3. Review/merge this PR
  4. Deploy/merge (whatever is necessary to get these changes to affect infra-k8s output)
  5. Merge infra-k8s PR, , which removes the overrides (by them, not necessary)

@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

@Madalosso Madalosso requested a review from a team as a code owner November 6, 2024 18:15
Madalosso
Madalosso reviewed Nov 6, 2024
View reviewed changes
core/chains/evm/config/toml/defaults/zkSync_Sepolia.toml
@@ -1,18 +1,32 @@
ChainID = '300'
ChainType = 'zksync'
FinalityDepth = 10
# 200block ~ 20min concurrent with the l1_committed tag
Copy link
Contributor

Choose a reason for hiding this comment

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

These are recent config changes added by @stackman27 4 months ago.
(same for mainnet)

Madalosso
Madalosso reviewed Nov 6, 2024
View reviewed changes
core/chains/evm/config/toml/defaults/Base_Sepolia.toml
@@ -2,6 +2,8 @@ ChainID = '84532'
ChainType = 'optimismBedrock'
FinalityDepth = 200
FinalityTagEnabled = true
# TODO: Check! Not sure if necessary
Copy link
Contributor

Choose a reason for hiding this comment

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

@simsonraj please help

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 6, 2024

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

Ran new or updated tests between develop and 6c262e6 (SHIP-1754-Consolidate-all-FinalityDepth-settings).

View Flaky Detector Details | Compare Changes

Failed Tests

Ran 6 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/v2/core/config/docs  TestDoc      0%         1          false
github.com/smartcontractkit/chainlink/v2/core/config/docs  TestDoc/EVM  0%         1          false

@github-actions GitHub Actions
Copy link
Contributor

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

AER Report: CI Core

aer_workflow , commit , Detect Changes , Clean Go Tidy & Generate , Scheduled Run Frequency , Find New Flaky Tests In Root Project / Find Tests To Run , lint , Core Tests (go_core_tests) , Core Tests (go_core_tests_integration) , Find New Flaky Tests In Deployment Project , Core Tests (go_core_ccip_deployment_tests) , Core Tests (go_core_race_tests) , Core Tests (go_core_fuzz) , Find New Flaky Tests In Root Project / Run Tests , Find New Flaky Tests In Root Project / Report , Flakey Test Detection , SonarQube Scan

1. Tests failed: go_core_tests

Source of Error: 
Run tests	2024-11-08T17:48:30.1094996Z Encountered test failures.
Run tests	2024-11-08T17:48:30.1095528Z go_core_tests exiting with code 1
Run tests	2024-11-08T17:48:30.1115339Z ##[error]Process completed with exit code 1.

Why: The error indicates that the Go core tests encountered failures during their execution, which caused the process to exit with a non-zero status code (1).

Suggested fix: Review the test output logs to identify which specific tests failed and address the underlying issues in the code or test cases. Ensure all dependencies and configurations are correctly set up.

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

@Madalosso Madalosso self-requested a review November 7, 2024 19:55
@Madalosso Madalosso force-pushed the SHIP-1754-Consolidate-all-FinalityDepth-settings branch from 8d911f7 to 9132246 Compare November 8, 2024 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Madalosso Madalosso Madalosso approved these changes

@prashantkumar1982 prashantkumar1982 Awaiting requested review from prashantkumar1982

@simsonraj simsonraj Awaiting requested review from simsonraj

@dhaidashenko dhaidashenko Awaiting requested review from dhaidashenko

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.
5 participants
@adityavadhavkar1215 @Madalosso @simsonraj @dhaidashenko @prashantkumar1982