-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
deployment: Changes for keystone #14837
base: develop
Are you sure you want to change the base?
Conversation
465fd3c
to
d34c3c4
Compare
AER Report: CI Core ran successfully ✅AER Report: Operator UI CI ran successfully ✅ |
9d56a91
to
898fd46
Compare
require.NoError(t, err) | ||
// sepoliaArbitrumChainSel, err := chainsel.SelectorFromChainId(sepoliaArbitrumChainId) | ||
// require.NoError(t, err) | ||
// aptosChainSel := uint64(999) // TODO: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently disabled because non-EVMs aren't listed in chain-selectors yet
switch family { | ||
case chainsel.FamilyEVM: | ||
ocrtype = chaintype.EVM | ||
case chainsel.FamilySolana: | ||
ocrtype = chaintype.Solana | ||
case chainsel.FamilyStarknet: | ||
ocrtype = chaintype.StarkNet | ||
case chainsel.FamilyCosmos: | ||
ocrtype = chaintype.Cosmos | ||
case chainsel.FamilyAptos: | ||
ocrtype = chaintype.Aptos | ||
default: | ||
panic(fmt.Sprintf("Unsupported chain family %v", family)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's a bunch of these type conversions but I'm not sure where to put the helper functions
361ed0f
to
b60faa0
Compare
@@ -45,6 +45,20 @@ query FetchAccounts { | |||
} | |||
} | |||
|
|||
query FetchKeys { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this in this change? all of the data should be coming from the JD right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is used by node-operations create-ccip-chain
(which can be used for keystone too) when it's generating ChainConfig for the nodes (which is then exposed by JD) -- take a look at CreateCCIPOCRSupportedChains
@@ -164,41 +163,40 @@ func makeNodeKeysSlice(nodes []*ocr2Node) []NodeKeys { | |||
// in is in a convenient form to handle the CLO representation of the nop data | |||
type DonCapabilities struct { | |||
Name string | |||
Nops []*models.NodeOperator // each nop is a node operator and may have multiple nodes | |||
Nodes []string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs to be p2pkey. it may be more awkward wrt to the JD api, but it makes the definition independent of jd instance b/c it doesn't leak jd-instance-specific state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we able to query by p2pkey on the ListNodes
API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't know, we could request it a feature.
at worst we list all the nodes in jd and filter them locally until they give us a better api
f65cf81
to
786390a
Compare
786390a
to
0fc498b
Compare
Quality Gate passedIssues Measures |
Flaky Test Detector for
|
Flaky Test Detector for
|
I see you updated files related to
|
Flaky Test Detector for
|
@@ -29,6 +29,18 @@ type MemoryEnvironmentConfig struct { | |||
RegistryConfig deployment.CapabilityRegistryConfig | |||
} | |||
|
|||
// For placeholders like aptos | |||
func NewMemoryChain(t *testing.T, selector uint64) deployment.Chain { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note TODO here regarding the thinking for non-evms
chainlink/deployment/environment.go
Line 63 in c6c8fbd
// TODO: Add SolChains, AptosChain etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up not even using this in the tests since chain-selectors doesn't support non-EVMs, but most other chains don't have a simulated backend. We'd need to start a local node and test against that. For Keystone tests at the moment we don't actually need a fully configured node, just an instance of the node with the Aptos keys available so we can configure the contracts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chain-selectors fully was intended to support non-evms, should be fixed soon.
but most other chains don't have a simulated backend. We'd need to start a local node and test against that.
The onchain interface isn't tied to a simulated backend. The intention there is to use the docker based environment for families where that doesn't exist. The AptosOnchainClient or w/e interface would just have an implementation only in the devenv not memory
Capabilities []kcr.CapabilitiesRegistryCapability // every capability is hosted on each node | ||
} | ||
|
||
type Node struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks product agnostic - should be able to combine/merge with
chainlink/deployment/environment.go
Line 217 in c6c8fbd
type Node struct { |
ChainConfigs []*nodev1.ChainConfig | ||
} | ||
|
||
func NodesFromJD(name string, nodeIDs []string, jd deployment.OffchainClient) ([]Node, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto here should be a way to either combine with or have another helper alongside
chainlink/deployment/environment.go
Line 247 in c6c8fbd
func NodeInfo(nodeIDs []string, oc NodeChainConfigsLister) (Nodes, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code in there was very CCIP specific, only supporting EVM which needs a larger cleanup. Keystone also uses peer IDs for node IDs so our lookup differs. I'd prefer to leave a TODO here and fix this in a follow up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see it only support EVM at the moment, but what part is CCIP specific?
No description provided.