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 all 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