-
Notifications
You must be signed in to change notification settings - Fork 84
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
Add Validator for remote L1 without importing blockchain #2607
Conversation
Signed-off-by: sukantoraymond <[email protected]>
Signed-off-by: sukantoraymond <[email protected]>
rpcURL string, | ||
managerAddress common.Address, | ||
) bool { | ||
out, err := contract.CallToMethod( |
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.
can you check before this, by calling a PoA method, that this is a validator manager and that there is a contract on that direction?
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.
not needed imo, it will fail anyways down the line if contract doesnt exist
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.
it is needed. you need to check for error, to clasify between:
- PoA
- PoS
- nothing is there, or any other smart contract
If not, you are assuming something is PoS and can be nothing
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.
getSubnet returns if subnet is permissioned or not permissioned and the validator manage raddress. If not permissioned and contains validator manager address, it has to mean that the l1 either contains poa or pos. Either way i can't find a good way to determine if poa validator manager exists. Feel free to implement in separate pr. #2635
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.
for sure. function should be general, instead of assuming particular origin of the input contract (the general
idea for this function is not always to call it after a getSubnet).
it should really return an enum including: PoS, PoA, UnknowContract, NoContract
cmd/blockchaincmd/add_validator.go
Outdated
return fmt.Errorf("use avalanche addValidator <subnetName> command to use local machine as new validator") | ||
} | ||
var subnetID ids.ID | ||
if subnetIDstr == "" { |
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.
you can use the rpc endpoint to get the blockchain id (using warp) and the get the subnet if from the
blockchain id
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 should go into that direction if you are to easy the users way
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.
if warp not available, ask for the subnet id (but you can also ask for the blockchain id and get the subnet id from it)
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 ignore which is best if subnet id or blockchain id
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.
can u make pr to get blockchain id from rpc endpoint using warp
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 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.
then you can call contract.GetSubnetID to obtain the subnet id from the chain id
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.
ofc this mail fail if warp if disabled, so, on error, you default to ask for subnet id (or blockchain id). prompt order to be first rpc then subnet id if warp query fails
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.
will impleent this once #2633 is merged into main.
@@ -119,31 +122,50 @@ Testnet or Mainnet.`, | |||
cmd.Flags().StringVar(&outputTxPath, "output-tx-path", "", "(for Subnets, not L1s) file path of the add validator tx") | |||
cmd.Flags().BoolVar(&waitForTxAcceptance, "wait-for-tx-acceptance", true, "(for Subnets, not L1s) just issue the add validator tx, without waiting for its acceptance") | |||
cmd.Flags().Uint16Var(&delegationFee, "delegation-fee", 100, "(PoS only) delegation fee (in bips)") | |||
cmd.Flags().StringVar(&subnetIDstr, "subnet-id", "", "subnet ID (only if blockchain name is not provided)") |
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.
(Not about this PR) But the strings here, was it a deliberate choice not to move the strings here into constants or more so something that just grew this way?
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.
we can technically move all the flags that we have into a separate file like flags.go and have var for all the flags we use. This is a tech debt
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 are some parts of code where the flags names are constants. But overall, it grew this way. We need
to improve (or have) code standards here.
@@ -119,31 +122,50 @@ Testnet or Mainnet.`, | |||
cmd.Flags().StringVar(&outputTxPath, "output-tx-path", "", "(for Subnets, not L1s) file path of the add validator tx") | |||
cmd.Flags().BoolVar(&waitForTxAcceptance, "wait-for-tx-acceptance", true, "(for Subnets, not L1s) just issue the add validator tx, without waiting for its acceptance") | |||
cmd.Flags().Uint16Var(&delegationFee, "delegation-fee", 100, "(PoS only) delegation fee (in bips)") | |||
cmd.Flags().StringVar(&subnetIDstr, "subnet-id", "", "subnet ID (only if blockchain name is not provided)") | |||
cmd.Flags().StringVar(&validatorManagerOwnerAddress, "validator-manager-owner", "", "validator manager owner address (only if blockchain name is not provided)") | |||
cmd.Flags().Uint64Var(&weight, validatorWeightFlag, uint64(constants.DefaultStakeWeight), "set the weight of the validator") |
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.
Have we considered refactoring the input validation to be on the flags rather than in the commands?
Similarly, have we considered reusing these flag definitions across all commands rather than defining them in each cmd file?
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.
addressed above, ya this is a tech debt.
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.
For values that have many possible input flags, we have made partial work.
We have for example contract.ChainSpec struct that has methods to validate
itself and to automatically add flags to a command. Same with contract.PrivateKeyFlags.
We also have a function for the network model, on networkoptions.AddNetworkFlagsToCmd.
We may want to unify and expand on this pattern, and also apply to some remaining multiple
flags value, like p-chain input key.
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.
We also have made partial work on avoiding using global variables for command flags.
Best of the worlds IMO is to have each command have its unique flags struct.
Example eg cmd/interchain/messengercmd/deploy.go
Why is that? To avoid interference on default flag value initialization between different
commands (each command make its own default value initialization and we had cases
where one command overwrote the default flag value of another one). And to avoid depending
on global variables among different methods which also introduces secondary effects from time
to time. Finally, this simplifies and cleans calling command from another commands.
We need some code standard here.
pkg/contract/contract.go
Outdated
@@ -630,3 +630,24 @@ func UnpackLog( | |||
contract := bind.NewBoundContract(common.Address{}, *abi, nil, nil, nil) | |||
return contract.UnpackLog(event, eventName, log) | |||
} | |||
|
|||
// GetContractOwner gets owner for https://docs.openzeppelin.com/contracts/2.x/api/ownership#Ownable-owner contracts | |||
func GetContractOwner( |
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.
What is this used for? And what determines which contract addresses we call this on?
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 to see who can make changes to a PoA L1 since only the holder of the private key to validator manager controller address can make changes like add and remove validator. Since validator manager contracts are openzeppelin contracts we can use https://docs.openzeppelin.com/contracts/2.x/api/ownership#Ownable-owner to see which address is controlling the validator manager contract.
sc, err := app.LoadSidecar(blockchainName) | ||
if err != nil { | ||
return fmt.Errorf("failed to load sidecar: %w", err) | ||
func addValidator(cmd *cobra.Command, args []string) 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.
Do we have any unite tests + e2e tests defined on this?
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.
no unit test as of now for remote validators. To test this, we will have to simulate local network, create an L1 on top of it (using ANR to use local machine for bootstrap validators) and create another validator to add (using local machine to create another instance of validator). This means that we have to create two separate processes of local machine validators, which we can't do currently.
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.
It's on backlog to do once TMPnet is implement for local machine clusters
Enables add validator for remote L1s using
avalanche blockchain addValidator command
.This PR introduces
importL1()
command, which enables commands like addValidator to interact with remote L1s without having to have L1 sidecar.json in local machine. User will need to supply subnet id and rpc url andimportL1()
will get blockchainID, if PoA/PoS, validatorManagerAddress, validatorManagerOwner on-chain.This PR also enables users to set the weight of the validator to be added, either through prompt or flag
--weight
.importL1()
function can be extended to other parts of the codebase:blockchain removeValidator
,contract initValidatorManager