[DPA-1151]: fix: handle duplicate chain id across network better for ethkeys, ethTransactions & chains query. #14791

graham-chainlink · 2024-10-16T06:32:01Z

I recommend going through each commit one at a time for better reviewing experience

We currently have a loader loadByIDs but this does not take into consideration when same Id exists across different network. It has a bug in there where it will either return an error or worst returns an incorrect results from a different chain. Eg if solana and ethereum both have chain ID 1.

loadByRelayIDs rectifies this by performing lookup with ID and Network. It migrates ethKeys , ethTransactions and chain to use the new loader.

This PR also introduce lookup for chain query to include network argument.

Node and Nodes graphQL query are still using the loadByIDs loader. To fix them, we need to rewrite Node and Nodes to fetch relayid from the relayer and use that to perform lookup.

JIRA: https://smartcontract-it.atlassian.net/browse/DPA-1151

Chain query with network lookup

github-actions · 2024-10-17T03:28:18Z

AER Report: CI Core ran successfully ✅

aer_workflow , commit

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

core/web/schema/type/chain.graphql

core/web/schema/schema.graphql

core/services/relay/relay.go

core/web/schema/type/chain.graphql

core/web/loader/chain.go

jmank88 · 2024-10-29T11:20:31Z

Does this PR need a corresponding operator-ui change?

graham-chainlink · 2024-10-30T02:15:21Z

I thought the readme instructions suggested that a follow up PR is necessary since that repo depends on this one. Is that not always the case?

@jmank88 I dont believe so. Since the types are generated on the fly, the schema is always updated. However because the chain graphql query is not being used in the operator ui, the generated typescript typings were never referenced in the react code, so there are no code to change too.

We currently have a loader `loadByIDs` but this does not take into consideration when same Id exists across different network. Eg if solana and ethereum both have chain ID 1. `loadByIDs` will either return an error or worst returns an incorrect results. `loadByRelayIDs` rectifies this by performing lookup with ID and Network. JIRA: https://smartcontract-it.atlassian.net/browse/DPA-1151

Update ethKeys & ethTransaction query to use the new dataloader `GetChainByRelayID` as the existing loader does not handle duplicate Id across different network.

Allow chain lookup by network and chain Id so we can differentiate same chain id in different networks. JIRA: https://smartcontract-it.atlassian.net/browse/DPA-1151

cl-sonarqube-production · 2024-10-30T02:50:07Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
78.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

patrickhuie19 · 2024-10-31T13:43:46Z

core/web/loader/getters.go

+func GetChainByRelayID(ctx context.Context, id string) (*commonTypes.ChainStatusWithID, error) {
+	ldr := For(ctx)
+
+	thunk := ldr.ChainsByRelayIDLoader.Load(ctx, dataloader.StringKey(id))


what does thunk mean as a var?

Great question, in this context, thunk is just promise, but mainly i was following the rest of the data loaders, they seem to call the value returned by load thunk

patrickhuie19 · 2024-10-31T13:44:56Z

core/web/loader/loader.go

@@ -45,6 +46,7 @@ func New(app chainlink.Application) *Dataloader {
 		app: app,

 		ChainsByIDLoader:                          dataloader.NewBatchedLoader(chains.loadByIDs),


Is there a ticket to clean out the ChainsByIDLoader usage?

Not yet, there is still code using it for backwards compatibility as it is exposed via API, and to be honest i dont know enough about how this repo works and removing this is a breaking change for the API, like we would have to communicate to consumers to not use this or monitor the usage of it.

graham-chainlink force-pushed the ggoh/DPA-1151/handle-same-id-differentnetwork branch from 8915795 to f116714 Compare October 17, 2024 03:09

graham-chainlink force-pushed the ggoh/DPA-1151/handle-same-id-differentnetwork branch 4 times, most recently from b402d38 to d93a55f Compare October 17, 2024 06:39

graham-chainlink changed the title ~~testing~~ [DPA-1151]: fix: handle duplicate chain id across network better. Oct 17, 2024

graham-chainlink force-pushed the ggoh/DPA-1151/handle-same-id-differentnetwork branch 2 times, most recently from 0be8692 to 41a6b00 Compare October 18, 2024 04:10

graham-chainlink marked this pull request as ready for review October 18, 2024 04:10

graham-chainlink requested review from a team as code owners October 18, 2024 04:10

graham-chainlink requested a review from george-dorin October 18, 2024 04:10

graham-chainlink changed the title ~~[DPA-1151]: fix: handle duplicate chain id across network better.~~ [DPA-1151]: fix: handle duplicate chain id across network better for ethkeys, ethTransactions & chains query. Oct 18, 2024

graham-chainlink requested review from jkongie and archseer October 18, 2024 04:12

jkongie reviewed Oct 22, 2024

View reviewed changes

core/web/schema/type/chain.graphql Outdated Show resolved Hide resolved

graham-chainlink force-pushed the ggoh/DPA-1151/handle-same-id-differentnetwork branch 3 times, most recently from 5f85e57 to 7f834e2 Compare October 22, 2024 13:06

archseer reviewed Oct 23, 2024

View reviewed changes

core/web/schema/schema.graphql Outdated Show resolved Hide resolved

graham-chainlink force-pushed the ggoh/DPA-1151/handle-same-id-differentnetwork branch from 7f834e2 to 780e581 Compare October 23, 2024 14:24

graham-chainlink requested a review from archseer October 23, 2024 15:10

jmank88 reviewed Oct 24, 2024

View reviewed changes

core/services/relay/relay.go Outdated Show resolved Hide resolved

core/services/relay/relay.go Outdated Show resolved Hide resolved

core/web/schema/type/chain.graphql Outdated Show resolved Hide resolved

graham-chainlink force-pushed the ggoh/DPA-1151/handle-same-id-differentnetwork branch from 780e581 to e9a7615 Compare October 29, 2024 04:38

graham-chainlink requested a review from a team as a code owner October 29, 2024 04:38

graham-chainlink requested a review from jmank88 October 29, 2024 04:39

jmank88 reviewed Oct 29, 2024

View reviewed changes

core/web/loader/chain.go Outdated Show resolved Hide resolved

graham-chainlink force-pushed the ggoh/DPA-1151/handle-same-id-differentnetwork branch from a58e3e5 to 38a40ad Compare October 30, 2024 02:28

graham-chainlink requested a review from a team as a code owner October 30, 2024 02:28

graham-chainlink added 3 commits October 30, 2024 11:29

fix(eth): update ethKeys & ethTransaction query

78a7b30

Update ethKeys & ethTransaction query to use the new dataloader `GetChainByRelayID` as the existing loader does not handle duplicate Id across different network.

fix(chain): add network args to chain query

dc96cdc

Allow chain lookup by network and chain Id so we can differentiate same chain id in different networks. JIRA: https://smartcontract-it.atlassian.net/browse/DPA-1151

graham-chainlink force-pushed the ggoh/DPA-1151/handle-same-id-differentnetwork branch from 38a40ad to dc96cdc Compare October 30, 2024 02:30

graham-chainlink requested review from a team as code owners October 30, 2024 02:30

graham-chainlink requested a review from jmank88 October 31, 2024 01:46

jmank88 approved these changes Oct 31, 2024

View reviewed changes

patrickhuie19 reviewed Oct 31, 2024

View reviewed changes

patrickhuie19 approved these changes Oct 31, 2024

View reviewed changes

graham-chainlink added this pull request to the merge queue Oct 31, 2024

Merged via the queue into develop with commit 0900637 Oct 31, 2024
132 checks passed

graham-chainlink deleted the ggoh/DPA-1151/handle-same-id-differentnetwork branch October 31, 2024 14:43

github-actions bot mentioned this pull request Oct 31, 2024

[DO NOT MERGE] Changeset Release Preview - v2.19.0 #13148

Draft

chainchad mentioned this pull request Nov 1, 2024

release/2.18.0 -> develop #15076

Closed

github-actions bot mentioned this pull request Nov 2, 2024

[DO NOT MERGE] Changeset Release Preview - v2.19.0 BunsDev/chainlink#1

Draft

This was referenced Nov 20, 2024

[DO NOT MERGE] Changeset Release Preview - v2.19.0 Emirhan-Cavusoglu-sftw/chainlink#1

Draft

[DO NOT MERGE] Changeset Release Preview - v2.19.0 galactic-beyond/chainlink#1

Draft

github-actions bot mentioned this pull request Dec 18, 2024

[DO NOT MERGE] Changeset Release Preview - v2.19.0 petermetz/chainlink#1

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DPA-1151]: fix: handle duplicate chain id across network better for ethkeys, ethTransactions & chains query. #14791

[DPA-1151]: fix: handle duplicate chain id across network better for ethkeys, ethTransactions & chains query. #14791

graham-chainlink commented Oct 16, 2024 •

edited

Loading

github-actions bot commented Oct 17, 2024 •

edited

Loading

jmank88 commented Oct 29, 2024

graham-chainlink commented Oct 30, 2024

cl-sonarqube-production bot commented Oct 30, 2024

patrickhuie19 Oct 31, 2024

graham-chainlink Oct 31, 2024

patrickhuie19 Oct 31, 2024

graham-chainlink Oct 31, 2024

		@@ -45,6 +46,7 @@ func New(app chainlink.Application) *Dataloader {
		app: app,

		ChainsByIDLoader: dataloader.NewBatchedLoader(chains.loadByIDs),

[DPA-1151]: fix: handle duplicate chain id across network better for ethkeys, ethTransactions & chains query. #14791

[DPA-1151]: fix: handle duplicate chain id across network better for ethkeys, ethTransactions & chains query. #14791

Conversation

graham-chainlink commented Oct 16, 2024 • edited Loading

Chain query with network lookup

github-actions bot commented Oct 17, 2024 • edited Loading

AER Report: CI Core ran successfully ✅

AER Report: Operator UI CI ran successfully ✅

jmank88 commented Oct 29, 2024

graham-chainlink commented Oct 30, 2024

cl-sonarqube-production bot commented Oct 30, 2024

Quality Gate passed

patrickhuie19 Oct 31, 2024

Choose a reason for hiding this comment

graham-chainlink Oct 31, 2024

Choose a reason for hiding this comment

patrickhuie19 Oct 31, 2024

Choose a reason for hiding this comment

graham-chainlink Oct 31, 2024

Choose a reason for hiding this comment

graham-chainlink commented Oct 16, 2024 •

edited

Loading

github-actions bot commented Oct 17, 2024 •

edited

Loading