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

[TT-1847] keystone smoke use chainlink deployment #16123

Merged
merged 75 commits into from
Jan 30, 2025

Conversation

Tofel
Copy link
Contributor

@Tofel Tofel commented Jan 29, 2025

Requires

Supports

@Tofel Tofel requested a review from b-gopalswami January 29, 2025 17:13
Copy link
Contributor

github-actions bot commented Jan 29, 2025

AER Report: CI Core ran successfully ✅

aer_workflow , commit

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

@Tofel Tofel changed the base branch from develop to tt-1969-debug-capabilities-test January 29, 2025 18:43
@Tofel Tofel requested a review from a team as a code owner January 29, 2025 18:43
// set admin address for non-bootstrap nodes
node.adminAddr = info.AdminAddr

// TODO remove me, hack so that code can progress
Copy link
Contributor

Choose a reason for hiding this comment

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

better comment: ~ "capability registry requires non-null admin address; use arbitrary default value if node is not configured"

JDId string // job distributor id returned by node after Job distributor is created in node
Name string // name of the node
AccountAddr map[uint64]string // chain id to node's account address mapping for supported chains
PeerID string // peer id of the node
Copy link
Contributor

Choose a reason for hiding this comment

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

iirc there is already code to extract the peer id from the labels. what's the reason to promote it up? i'm concerned that it may not be available in all cases b/c the jd itself doesn't require it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed. But what about ocr key bundle id? I still need it, at least until we make JD work. Any chances to either keep it here or add to labels as well?

@@ -226,6 +237,7 @@ func (n *Node) CreateCCIPOCRSupportedChains(ctx context.Context, chains []JDChai
if peerID == nil {
return fmt.Errorf("no peer id found for node %s", n.Name)
}
n.PeerID = *peerID
Copy link
Contributor

Choose a reason for hiding this comment

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

iiuc, we are already erroring if no p2p, so always ok to promote the field to the top level? are there other use cases of the Node struct that don't set the PeerID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, didn't pay attention to labels. Removed my change.

@@ -116,6 +116,9 @@ type WorkflowConfig struct {
ChainlinkCLI *ChainlinkCLIConfig `toml:"chainlink_cli"`
UseExising bool `toml:"use_existing"`
Existing *ExistingWorkflowConfig `toml:"existing"`
DonID uint32 `toml:"don_id" validate:"required"`
Copy link
Contributor

Choose a reason for hiding this comment

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

add comment about the intended value. is it supposed to be the same thing that comes from the cap registry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, after these heavy code modifications it remains unclear to me, how we link DONs to capabilities. Because that id is used to instruct the bootstrap node how to filter out workflows from the workflow registry. So okay, it now knows which workflow it needs to execute. It also knows the members of its workflow DON.

Ah, I think I see it. So when we call capabilities registry, we use hashed capability id and node's p2p id and that's the connection.

require.NotEmpty(t, os.Getenv(e2eJobDistributorImageEnvVarName), "missing env var: "+e2eJobDistributorImageEnvVarName)
require.NotEmpty(t, os.Getenv(e2eJobDistributorVersionEnvVarName), "missing env var: "+e2eJobDistributorVersionEnvVarName)

// disabled until we can figure out how to generate a gist read:write token in CI
Copy link
Contributor

Choose a reason for hiding this comment

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

expand this comment pls. in practice what does it mean for smth to be 'existing'? where does it need to exist and can you point to readme (in the comment) about how to create one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

maybe the naming isn't the most obvious, I am happy to change it, so that it's clearer.

NodeInfo: nodeInfo,
}

require.Len(t, bs.Nodes, 1, "expected only one node in the blockchain output")
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, changed it to >= 1, I originally added it just in case to avoid accessing an index that might not exist

capRegAddr, tx, capabilitiesRegistryInstance, err := cr_wrapper.DeployCapabilitiesRegistry(sc.NewTXOpts(), sc.Client)
_, decodeErr := sc.Decode(tx, err)
require.NoError(t, decodeErr, "failed to deploy capabilities registry contract")
type keystoneContracts struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

there is a contract set abstraction. can we use it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

nodesToAdd := make([]cr_wrapper.CapabilitiesRegistryNodeParams, len(nodesInfo)-1)
donNodes := make([][32]byte, len(nodesInfo)-1)
func configureWorkflowDON(t *testing.T, ctfEnv *deployment.Environment, don *devenv.DON, chainSelector uint64) {
kcrAllCaps := []keystone_changeset.DONCapabilityWithConfig{
Copy link
Contributor

Choose a reason for hiding this comment

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

pass cfg as input so other tests can reuse?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok to way for followup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you expand on that? Which config do you mean? Not this one I guess cfg keystone_changeset.InitialContractsCfg?

Because the whole point of having this method is to wrap the creation of this config struct.

Copy link
Contributor

Choose a reason for hiding this comment

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

i mean your are hardcoding the DONCapabilityWithConfig... iiuc and other tests will probably want to set this in a different way

Base automatically changed from tt-1969-debug-capabilities-test to develop January 30, 2025 08:13
krehermann
krehermann previously approved these changes Jan 30, 2025
nodesToAdd,
))
require.NoError(t, decodeErr, "failed to add nodes to capabilities registry")
oracleConfig := keystone_changeset.OracleConfig{
Copy link
Contributor

Choose a reason for hiding this comment

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

any specific rationale for these values? if so pls comment, if not, pls comment where the inspiration came from

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

@Tofel Tofel added this pull request to the merge queue Jan 30, 2025
Merged via the queue into develop with commit 457927c Jan 30, 2025
208 checks passed
@Tofel Tofel deleted the tt-1847-keystone-smoke-use-chainlink-deployment branch January 30, 2025 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants