Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Validator for remote L1 without importing blockchain #2607
Changes from 20 commits
fd813de
84f8196
62ca8d1
90f1ae1
a57ad9e
929f620
7de4e3f
afed4c0
044b877
2f511be
4639e58
ee7df72
e502b7d
6770c15
b737623
1c86b21
18af22b
670a51f
e95c6a7
66263c1
5c7cf59
ee40c92
d6a2674
6ecef06
86093cb
52b3e02
97c4b96
e6dfab7
149d8f4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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
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.
#2633
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.