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.

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

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/honest-bugs-grin.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"chainlink": minor
---

#updated
update ethkeys, ethtransactions to handle duplicate chain id in different network
introduce network arg input to Chain graphql query to allow better lookup based on network and chain id
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,11 @@ func (f *FakeRelayerChainInteroperators) List(filter chainlink.FilterFn) chainli
}

func (f *FakeRelayerChainInteroperators) Get(id types.RelayID) (loop.Relayer, error) {
panic("unimplemented")
r, ok := f.Relayers[id]
if !ok {
return nil, chainlink.ErrNoSuchRelayer
}
return r, nil
}

func (f *FakeRelayerChainInteroperators) GetIDToRelayerMap() (map[types.RelayID]loop.Relayer, error) {
Expand Down
34 changes: 34 additions & 0 deletions core/web/loader/chain.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import (

"github.com/graph-gophers/dataloader"

commonTypes "github.com/smartcontractkit/chainlink-common/pkg/types"

"github.com/smartcontractkit/chainlink/v2/common/types"
"github.com/smartcontractkit/chainlink/v2/core/chains"
"github.com/smartcontractkit/chainlink/v2/core/services/chainlink"
Expand All @@ -15,6 +17,7 @@ type chainBatcher struct {
app chainlink.Application
}

// Deprecated: use loadByChainIDs is deprecated and we should be using loadByRelayIDs.
func (b *chainBatcher) loadByIDs(ctx context.Context, keys dataloader.Keys) []*dataloader.Result {
// Create a map for remembering the order of keys passed in
keyOrder := make(map[string]int, len(keys))
Expand Down Expand Up @@ -68,3 +71,34 @@ func (b *chainBatcher) loadByIDs(ctx context.Context, keys dataloader.Keys) []*d

return results
}

func (b *chainBatcher) loadByRelayIDs(ctx context.Context, keys dataloader.Keys) []*dataloader.Result {
results := make([]*dataloader.Result, 0, len(keys))
for _, key := range keys {
var relay commonTypes.RelayID
err := relay.UnmarshalString(key.String())
if err != nil {
results = append(results, &dataloader.Result{Data: nil, Error: err})
continue
}

relayer, err := b.app.GetRelayers().Get(relay)
if err != nil {
results = append(results, &dataloader.Result{Data: nil, Error: chains.ErrNotFound})
continue
}

status, err := relayer.GetChainStatus(ctx)
if err != nil {
results = append(results, &dataloader.Result{Data: nil, Error: err})
continue
}

results = append(results, &dataloader.Result{Data: types.ChainStatusWithID{
ChainStatus: status,
RelayID: relay,
}, Error: err})
}

return results
}
19 changes: 19 additions & 0 deletions core/web/loader/getters.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
var ErrInvalidType = errors.New("invalid type")

// GetChainByID fetches the chain by it's id.
// Deprecated: use GetChainByRelayID.
func GetChainByID(ctx context.Context, id string) (*commonTypes.ChainStatusWithID, error) {
ldr := For(ctx)

Expand All @@ -37,6 +38,24 @@ func GetChainByID(ctx context.Context, id string) (*commonTypes.ChainStatusWithI
return &chain, nil
}

// GetChainByRelayID fetches the chain by it's relayId.
func GetChainByRelayID(ctx context.Context, id string) (*commonTypes.ChainStatusWithID, error) {
ldr := For(ctx)

thunk := ldr.ChainsByRelayIDLoader.Load(ctx, dataloader.StringKey(id))
Copy link
Contributor

Choose a reason for hiding this comment

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

what does thunk mean as a var?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

result, err := thunk()
if err != nil {
return nil, err
}

chain, ok := result.(commonTypes.ChainStatusWithID)
if !ok {
return nil, ErrInvalidType
}

return &chain, nil
}

// GetNodesByChainID fetches the nodes for a chain.
func GetNodesByChainID(ctx context.Context, id string) ([]types.Node, error) {
ldr := For(ctx)
Expand Down
2 changes: 2 additions & 0 deletions core/web/loader/loader.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ type Dataloader struct {
app chainlink.Application

ChainsByIDLoader *dataloader.Loader
ChainsByRelayIDLoader *dataloader.Loader
EthTxAttemptsByEthTxIDLoader *dataloader.Loader
FeedsManagersByIDLoader *dataloader.Loader
FeedsManagerChainConfigsByManagerIDLoader *dataloader.Loader
Expand Down Expand Up @@ -45,6 +46,7 @@ func New(app chainlink.Application) *Dataloader {
app: app,

ChainsByIDLoader: dataloader.NewBatchedLoader(chains.loadByIDs),
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 ticket to clean out the ChainsByIDLoader usage?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

ChainsByRelayIDLoader: dataloader.NewBatchedLoader(chains.loadByRelayIDs),
EthTxAttemptsByEthTxIDLoader: dataloader.NewBatchedLoader(attmpts.loadByEthTransactionIDs),
FeedsManagersByIDLoader: dataloader.NewBatchedLoader(mgrs.loadByIDs),
FeedsManagerChainConfigsByManagerIDLoader: dataloader.NewBatchedLoader(ccfgs.loadByManagerIDs),
Expand Down
73 changes: 73 additions & 0 deletions core/web/loader/loader_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,79 @@ func TestLoader_Chains(t *testing.T) {
assert.ErrorIs(t, results[2].Error, chains.ErrNotFound)
}

func TestLoader_ChainsRelayID_HandleDuplicateIDAcrossNetworks(t *testing.T) {
t.Parallel()

app := coremocks.NewApplication(t)
ctx := InjectDataloader(testutils.Context(t), app)

one := ubig.NewI(1)
chain := toml.EVMConfig{ChainID: one, Chain: toml.Defaults(one)}
two := ubig.NewI(2)
chain2 := toml.EVMConfig{ChainID: two, Chain: toml.Defaults(two)}
config1, err := chain.TOMLString()
require.NoError(t, err)
config2, err := chain2.TOMLString()
require.NoError(t, err)

evm1 := commontypes.RelayID{
Network: relay.NetworkEVM,
ChainID: "1",
}
evm2 := commontypes.RelayID{
Network: relay.NetworkEVM,
ChainID: "2",
}
// check if can handle same chain ID but different network
solana1 := commontypes.RelayID{
Network: relay.NetworkSolana,
ChainID: "1",
}
app.On("GetRelayers").Return(&chainlinkmocks.FakeRelayerChainInteroperators{Relayers: map[commontypes.RelayID]loop.Relayer{
evm1: testutils2.MockRelayer{ChainStatus: commontypes.ChainStatus{
ID: "1",
Enabled: true,
Config: config1,
}},
evm2: testutils2.MockRelayer{ChainStatus: commontypes.ChainStatus{
ID: "2",
Enabled: true,
Config: config2,
}},
solana1: testutils2.MockRelayer{ChainStatus: commontypes.ChainStatus{
ID: "1",
Enabled: true,
Config: "config",
}},
}})

evm3 := commontypes.RelayID{
Network: relay.NetworkEVM,
ChainID: "3",
}

batcher := chainBatcher{app}
keys := dataloader.NewKeysFromStrings([]string{evm2.String(), evm1.String(), evm3.String()})
results := batcher.loadByRelayIDs(ctx, keys)

assert.Len(t, results, 3)

require.NoError(t, err)

assert.Equal(t, types.ChainStatusWithID{
ChainStatus: commontypes.ChainStatus{ID: "2", Enabled: true, Config: config2},
RelayID: evm2,
}, results[0].Data.(types.ChainStatusWithID))

assert.Equal(t, types.ChainStatusWithID{
ChainStatus: commontypes.ChainStatus{ID: "1", Enabled: true, Config: config1},
RelayID: evm1,
}, results[1].Data.(types.ChainStatusWithID))
assert.Nil(t, results[2].Data)
require.Error(t, results[2].Error)
require.ErrorIs(t, results[2].Error, chains.ErrNotFound)
}

func TestLoader_Nodes(t *testing.T) {
t.Parallel()

Expand Down
61 changes: 61 additions & 0 deletions core/web/resolver/chain_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,22 @@ func TestResolver_Chain(t *testing.T) {
}
}
`
queryWithNetwork = `
query GetChain($network: String) {
chain(id: "1", network: $network) {
... on Chain {
id
enabled
config
network
}
... on NotFoundError {
code
message
}
}
}
`
configTOML = `ChainID = '1'
AutoCreateKey = false
BlockBackfillDepth = 100
Expand Down Expand Up @@ -291,6 +307,51 @@ ResendAfterThreshold = '1h0m0s'
},
},
},
{
name: "should return aptos chain if network is aptos",
authenticated: true,
variables: map[string]interface{}{
"network": relay.NetworkAptos,
},
before: func(ctx context.Context, f *gqlTestFramework) {
chainConf := evmtoml.EVMConfig{
Chain: chain,
ChainID: &chainID,
}

chainConfToml, err2 := chainConf.TOMLString()
require.NoError(t, err2)

f.App.On("GetRelayers").Return(&chainlinkmocks.FakeRelayerChainInteroperators{Relayers: map[commontypes.RelayID]loop.Relayer{
commontypes.RelayID{
Network: relay.NetworkEVM,
ChainID: chainID.String(),
}: testutils.MockRelayer{ChainStatus: commontypes.ChainStatus{
ID: chainID.String(),
Enabled: chainConf.IsEnabled(),
Config: chainConfToml,
}},
commontypes.RelayID{
Network: relay.NetworkAptos,
ChainID: chainID.String(),
}: testutils.MockRelayer{ChainStatus: commontypes.ChainStatus{
ID: chainID.String(),
Enabled: chainConf.IsEnabled(),
Config: chainConfToml,
}},
}})
},
query: queryWithNetwork,
result: fmt.Sprintf(`
{
"chain": {
"id": "1",
"enabled": true,
"config": %s,
"network": "aptos"
}
}`, configTOMLEscaped),
},
}

RunGQLTests(t, testCases)
Expand Down
6 changes: 5 additions & 1 deletion core/web/resolver/eth_key.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,12 @@ import (
"github.com/ethereum/go-ethereum/common"
"github.com/graph-gophers/graphql-go"

commonTypes "github.com/smartcontractkit/chainlink-common/pkg/types"

"github.com/smartcontractkit/chainlink/v2/core/chains/evm/types"
"github.com/smartcontractkit/chainlink/v2/core/chains/legacyevm"
"github.com/smartcontractkit/chainlink/v2/core/services/keystore/keys/ethkey"
"github.com/smartcontractkit/chainlink/v2/core/services/relay"
"github.com/smartcontractkit/chainlink/v2/core/web/loader"
)

Expand Down Expand Up @@ -37,7 +40,8 @@ func NewETHKeys(keys []ETHKey) []*ETHKeyResolver {
}

func (r *ETHKeyResolver) Chain(ctx context.Context) (*ChainResolver, error) {
chain, err := loader.GetChainByID(ctx, r.key.state.EVMChainID.String())
relayID := commonTypes.NewRelayID(relay.NetworkEVM, r.key.state.EVMChainID.String())
chain, err := loader.GetChainByRelayID(ctx, relayID.Name())
if err != nil {
return nil, err
}
Expand Down
6 changes: 5 additions & 1 deletion core/web/resolver/eth_transaction.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,11 @@ import (
"github.com/ethereum/go-ethereum/common/hexutil"
"github.com/graph-gophers/graphql-go"

commonTypes "github.com/smartcontractkit/chainlink-common/pkg/types"

"github.com/smartcontractkit/chainlink/v2/core/chains/evm/assets"
"github.com/smartcontractkit/chainlink/v2/core/chains/evm/txmgr"
"github.com/smartcontractkit/chainlink/v2/core/services/relay"
"github.com/smartcontractkit/chainlink/v2/core/utils/stringutils"
"github.com/smartcontractkit/chainlink/v2/core/web/loader"
)
Expand Down Expand Up @@ -98,7 +101,8 @@ func (r *EthTransactionResolver) Hex(ctx context.Context) string {

// Chain resolves the node's chain object field.
func (r *EthTransactionResolver) Chain(ctx context.Context) (*ChainResolver, error) {
chain, err := loader.GetChainByID(ctx, string(r.EVMChainID()))
relayID := commonTypes.NewRelayID(relay.NetworkEVM, string(r.EVMChainID()))
chain, err := loader.GetChainByRelayID(ctx, relayID.Name())
if err != nil {
return nil, err
}
Expand Down
23 changes: 21 additions & 2 deletions core/web/resolver/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import (
"github.com/graph-gophers/graphql-go"
"github.com/pkg/errors"

"github.com/smartcontractkit/chainlink-common/pkg/types"
jmank88 marked this conversation as resolved.
Show resolved Hide resolved

commonTypes "github.com/smartcontractkit/chainlink/v2/common/types"
"github.com/smartcontractkit/chainlink/v2/core/bridges"
"github.com/smartcontractkit/chainlink/v2/core/chains"
Expand Down Expand Up @@ -64,12 +66,29 @@ func (r *Resolver) Bridges(ctx context.Context, args struct {
}

// Chain retrieves a chain by id.
func (r *Resolver) Chain(ctx context.Context, args struct{ ID graphql.ID }) (*ChainPayloadResolver, error) {
func (r *Resolver) Chain(ctx context.Context,
args struct {
ID graphql.ID
Network *string
}) (*ChainPayloadResolver, error) {
if err := authenticateUser(ctx); err != nil {
return nil, err
}

id, err := loader.GetChainByID(ctx, string(args.ID))
// fall back to original behaviour if network is not provided
if args.Network == nil {
id, err := loader.GetChainByID(ctx, string(args.ID))
if err != nil {
if errors.Is(err, chains.ErrNotFound) {
return NewChainPayload(commonTypes.ChainStatusWithID{}, chains.ErrNotFound), nil
}
return nil, err
}
return NewChainPayload(*id, nil), nil
}

relayID := types.NewRelayID(*args.Network, string(args.ID))
id, err := loader.GetChainByRelayID(ctx, relayID.Name())
if err != nil {
if errors.Is(err, chains.ErrNotFound) {
return NewChainPayload(commonTypes.ChainStatusWithID{}, chains.ErrNotFound), nil
Expand Down
2 changes: 1 addition & 1 deletion core/web/schema/schema.graphql
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ schema {
type Query {
bridge(id: ID!): BridgePayload!
bridges(offset: Int, limit: Int): BridgesPayload!
chain(id: ID!): ChainPayload!
chain(id: ID!, network: String): ChainPayload!
chains(offset: Int, limit: Int): ChainsPayload!
configv2: ConfigV2Payload!
csaKeys: CSAKeysPayload!
Expand Down
Loading
Loading