BCF-2492 Remove evm default chain id #10490

ilija42 · 2023-09-05T14:47:36Z

Remove all usage of default chainID:

Add job spec validation in job orm CreateJob that checks if evmChainID is defined for every job type that has EVMChainID in its spec
All pipeline vars now pass evmChainID inside of the jobSpec vars
Add default value for parsing pipeline evmChainID var for EstimateGasLimitTask, ETHCallTask and ETHTxTask
Remove keystore migrations from v1 to v2. Migrations used default ChainID, and it turned out that the migrations from v1 to v2 are obsolete, so it was easier just to remove it completely

For all job specs evmChainID field got injected with the default chain ID on job creation so all existing jobs would be valid. But if NOPs externally stored some old job specs, they won't have evmChainID defined in them and after these changes get merged evmChainID won't be implicitly injected anymore. I talked to Brendon and this should be fine.

Also a lot of tests relied on DefaultChainID, so most of the changes are to pass evmChainID where needed in tests.

- Remove default chain id from LegacyChainContainer - Add alternate function for default chainID in tests

github-actions · 2023-09-05T14:47:58Z

I see that you haven't updated any README files. Would it make sense to do so?

github-actions · 2023-09-05T14:47:58Z

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

- Remove default chain id from LoopRelayAdapter - Remove default chain id use from common.go getChain - Remove default chain id use from evm relayer init

- Remove code that relied on default chainID

- Fix evm transfer controller tests - Fix integration test helpers - Fix head tracker tests - Fix evm transaction commands test

…fault-evm-chainID # Conflicts: # core/internal/cltest/mocks.go # core/services/chainlink/relayer_chain_interoperators.go # core/services/relay/evm/mocks/loop_relay_adapter.go # core/services/relay/evm/relayer_extender.go

github-actions · 2023-09-07T11:14:25Z

Running downstream job at https://github.com/smartcontractkit/operator-ui/actions/runs/6108992813

github-actions · 2023-09-07T12:24:33Z

Running downstream job at https://github.com/smartcontractkit/operator-ui/actions/runs/6109652266

…se it

github-actions · 2023-09-08T11:28:44Z

Running downstream job at https://github.com/smartcontractkit/operator-ui/actions/runs/6121157137

samsondav

Very good! I've been wanting to see this gone for years

core/chains/evm/chain.go

core/cmd/shell_local.go

core/services/keystore/master.go

jmank88 · 2023-09-14T13:22:10Z

These changes require all jobs that need evmChainID to provide it and aren't backwards comaptible.

Normally we would provide a migration to auto-upgrade the jobs so that this is a non-issue. Was this considered?

…fault-evm-chainID # Conflicts: # core/cmd/shell_remote_test.go # core/web/common.go

ilija42 · 2023-09-14T13:49:20Z

These changes require all jobs that need evmChainID to provide it and aren't backwards comaptible.

Normally we would provide a migration to auto-upgrade the jobs so that this is a non-issue. Was this considered?

I think I didn't explain the issue properly, there wouldn't be any issue with existing created jobs, just with old job specs that don't have evmChainID defined, depending on how NOPs usually store jobspecs.
For eg. if NOP stored a jobspec in some txt file, on job creation this jobspecs evmChainID field gets injected with defaultChainID and gets saved to DB(so all existing jobs would be valid). But There still exists a copy of that jobspec in the txt file without injected evmChainID and it wouldn't work if evmChainID wasn't manually added to it.

RyanRHall

I only looked at keystore migration code, but removing all of that looks wonderful to me :)

krehermann

let's add something to the change log

cl-sonarqube-production · 2023-09-14T16:25:15Z

SonarQube Quality Gate

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

95.3% Coverage
2.3% Duplication

Remove evm default chain id usage

2de282d

- Remove default chain id from LegacyChainContainer - Add alternate function for default chainID in tests

ilija42 added 20 commits September 5, 2023 20:54

Remove the rest of evm default chain id usage

63d8a6f

- Remove default chain id from LoopRelayAdapter - Remove default chain id use from common.go getChain - Remove default chain id use from evm relayer init

Enforce evmChainID to be defined in all jobspecs where present

99096ab

Remove setdefault chain id from NewLegacyChainsWithChain() mock

48ca938

Change cltest NewApplicationWithConfig to init evmopts if evm enabled

6e1d5a0

Fix pipeline & eth_keys & jobs controller tests to encode evmChainID

6fbea6b

Add evmChainID to test tomls and to VRFSpecParams testspec struct

58df0cf

Add evmChainID to vrf submit_tx pipeline vars and testspec source

587c0d8

Add evmChainID to testspecs job specs where missing

fbcd0b6

Add evmChainID flags to shell local tests

ecca443

Change TestORM_CreateJob_OCR_DuplicatedContractAddress

6d04fc6

- Remove code that relied on default chainID

Change orm validation missing evmChainID error message

f8c55ea

Change tests that relied on default chainID to use replacement func

37790d8

- Fix evm transfer controller tests - Fix integration test helpers - Fix head tracker tests - Fix evm transaction commands test

Add deprecated tag to default chainID helper funcs and update err msg

1b79c95

comment out test case in TestShell_ReplayBlock

5881147

minor tests fix

b4236d3

Add evmchainID to spec to keepers and flux integration tests

b887874

Add evmChainID to vrfv2 pipeline vars

8868ada

Fix evmChainID parsing in estimategas pipeline task

fff8fc6

Add evmChainID to estimate gas and eth call pipeline tasks vars spec

598b341

ilija42 added 2 commits September 7, 2023 14:22

Fix/Change cmds tests to use evmChainID

4e3f8b4

Fix vrf integration tests and helpers to use evmChainID

096706b

ilija42 added 2 commits September 8, 2023 13:27

Add evmChainID parsing string as default val to pipeline tasks that u…

e133425

…se it

Add evmChainID to specs in internal features_test

93b392c

ilija42 requested review from a team, connorwstein, prashantkumar1982, RyanRHall, PiotrTrzpil and pinebit as code owners September 13, 2023 19:38

samsondav previously approved these changes Sep 13, 2023

View reviewed changes

krehermann reviewed Sep 13, 2023

View reviewed changes

core/chains/evm/chain.go Outdated Show resolved Hide resolved

krehermann reviewed Sep 13, 2023

View reviewed changes

core/cmd/shell_local.go Show resolved Hide resolved

krehermann reviewed Sep 13, 2023

View reviewed changes

core/services/keystore/master.go Show resolved Hide resolved

ilija42 added 2 commits September 14, 2023 14:58

Remove keystore migrate as all v1 keys should have been migrated by now

4982867

Use %q in (c *LegacyChains) Get() for better error msg output

24556cb

ilija42 dismissed samsondav’s stale review via 24556cb September 14, 2023 13:01

Merge remote-tracking branch 'origin/develop' into BCF-2492-remove-de…

575b15b

…fault-evm-chainID # Conflicts: # core/cmd/shell_remote_test.go # core/web/common.go

Fix TestShell_ReplayBlocks test by fixing err message assert

164bb39

RyanRHall previously approved these changes Sep 14, 2023

View reviewed changes

ilija42 requested a review from krehermann September 14, 2023 15:30

krehermann approved these changes Sep 14, 2023

View reviewed changes

krehermann requested changes Sep 14, 2023

View reviewed changes

Add changes to CHANGELOG.md

ba4ac9e

ilija42 dismissed RyanRHall’s stale review via ba4ac9e September 14, 2023 16:10

krehermann approved these changes Sep 14, 2023

View reviewed changes

ilija42 added this pull request to the merge queue Sep 14, 2023

Merged via the queue into develop with commit c9e080b Sep 14, 2023
98 checks passed

ilija42 deleted the BCF-2492-remove-default-evm-chainID branch September 14, 2023 16:47

snehaagni mentioned this pull request Oct 18, 2023

release/2.6.0 -> master #10997

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BCF-2492 Remove evm default chain id #10490

BCF-2492 Remove evm default chain id #10490

ilija42 commented Sep 5, 2023 •

edited

Loading

github-actions bot commented Sep 5, 2023

github-actions bot commented Sep 5, 2023

github-actions bot commented Sep 7, 2023

github-actions bot commented Sep 7, 2023

github-actions bot commented Sep 8, 2023

samsondav left a comment

jmank88 commented Sep 14, 2023

ilija42 commented Sep 14, 2023 •

edited

Loading

RyanRHall left a comment

krehermann left a comment

cl-sonarqube-production bot commented Sep 14, 2023

BCF-2492 Remove evm default chain id #10490

BCF-2492 Remove evm default chain id #10490

Conversation

ilija42 commented Sep 5, 2023 • edited Loading

github-actions bot commented Sep 5, 2023

github-actions bot commented Sep 5, 2023

github-actions bot commented Sep 7, 2023

github-actions bot commented Sep 7, 2023

github-actions bot commented Sep 8, 2023

samsondav left a comment

Choose a reason for hiding this comment

jmank88 commented Sep 14, 2023

ilija42 commented Sep 14, 2023 • edited Loading

RyanRHall left a comment

Choose a reason for hiding this comment

krehermann left a comment

Choose a reason for hiding this comment

cl-sonarqube-production bot commented Sep 14, 2023

ilija42 commented Sep 5, 2023 •

edited

Loading

ilija42 commented Sep 14, 2023 •

edited

Loading