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

Use network name instead of cluster name on sidecar #2303

Merged
merged 15 commits into from
Nov 13, 2024

Conversation

sukantoraymond
Copy link
Collaborator

@sukantoraymond sukantoraymond commented Oct 31, 2024

Why this should be merged

Currently if a blockchain is created with cluster flag and user uses additional commands like addValidator

avalanche-cli/bin/avalanche blockchain addValidator <blockchainName>
✔ Devnet
✔ Custom
✔ Devnet Endpoint: https://etna.avax-dev.network/█
✔ Use stored key
✔ mytestkey
What is the NodeID of the node you want to add as a blockchain validator?: NodeID-8XXvVGYhuQTgDpU5xQrAAYHA96o5tNSiR
Next, we need the public key and proof of possession of the node's BLS
Check https://docs.avax.network/api-reference/info-api#infogetnodeid for instructions on calling info.getNodeID API
✔ What is the node's BLS public key?: 0x8e1de3a59cc61657a120ed330d77bb48439b5b9084ed51ae2571a8c34c8da35e21a07df5bb0a1ffd3c18b41af63d8e09█
✔ What is the node's BLS proof of possession?: 0xb54be09f8c8d2e5e5effe99e40ca141d186baa01e7a1b7c81376e2590b2c264084b9e80cecc61c80a59d43133533cd38146e2b533912b0ac33d3c44d79e7057ce5a161a0605b267fa477a7740bbd17a41e7db9bea82096587c5a2397502cf570█
PoA manager owner 0x6b603F6B045D89beE25dbA0963B1D40687602F00 pays for the initialization of the validator's registration (Blockchain gas token)
Error: blockchain has not been deployed to Devnet https://etna.avax-dev.network/

This is because we are storing where the blockchain is deployed in in cluster name instead of which network it was deployed in.

How this works

Sidecar should show network name instead of cluster name to enable interchangeability of using --cluster vs --network (e.g. --fuji / --etna-devnet). This PR enable interchangeability of --cluster and --network flags.

How this was tested

Should pass CI

How is this documented

NA

@sukantoraymond sukantoraymond requested a review from a team as a code owner October 31, 2024 21:22
@sukantoraymond sukantoraymond changed the base branch from main to acp-77 October 31, 2024 21:22
@sukantoraymond sukantoraymond marked this pull request as draft October 31, 2024 21:22
@sukantoraymond sukantoraymond marked this pull request as ready for review November 4, 2024 18:04
@sukantoraymond
Copy link
Collaborator Author

sukantoraymond commented Nov 5, 2024

Network (for sidecar.json) is now stored in the form of etna / fuji / mainnet instead of cluster name even if clustername flag is used. ConvertClusterToNetwork is implemented in this PR to convert from cluster name to network name

@sukantoraymond
Copy link
Collaborator Author

Could have implemented this change in network function level, but this will impact node wiz and other node functionalities (separate from acp-77), which is why this change was not considered.

@arturrez
Copy link
Collaborator

arturrez commented Nov 6, 2024

I feel like it adds a confusion if we do it, if I understand this PR correctly.
if we feel like --cluster flags is confusing users for blockchain deploy cmd, we should remove support for this flag and only support --network flag

Copy link
Collaborator

@felipemadero felipemadero left a comment

Choose a reason for hiding this comment

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

I believe it is ok to add cluster name to sidecar network info.
But probably best would had been to directly change network.Name
instead of removing cluster name info from the network model.
Also, there are functions that offer or try to offer cluster options for
a given blockchain, and that was based on network name on
sidecar, so now it should be based on cluster name info of
sidecar network info.

cmd/blockchaincmd/add_validator.go Outdated Show resolved Hide resolved
cmd/blockchaincmd/deploy.go Outdated Show resolved Hide resolved
pkg/models/network.go Outdated Show resolved Hide resolved
return NewMainnetNetwork(), nil
case clusterNetwork.ID == constants.EtnaDevnetNetworkID:
return NewEtnaDevnetNetwork(), nil
default:
Copy link
Collaborator

Choose a reason for hiding this comment

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

it seems a devnet case is missing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can't know what the devnet network id will be

Copy link
Collaborator

Choose a reason for hiding this comment

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

you can query the network id from the endpoint, such a networkoptions does

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

addressed

pkg/models/network.go Show resolved Hide resolved
pkg/node/node.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@felipemadero felipemadero left a comment

Choose a reason for hiding this comment

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

preaproving. will appreciate if you tackle the open conversion.

@sukantoraymond sukantoraymond merged commit ea37fed into acp-77 Nov 13, 2024
14 checks passed
@sukantoraymond sukantoraymond deleted the use-network-not-cluster branch November 13, 2024 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done ✅
Development

Successfully merging this pull request may close these issues.

3 participants