-
Notifications
You must be signed in to change notification settings - Fork 1k
Add instant seal to omni-node #10008
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
base: master
Are you sure you want to change the base?
Conversation
Open to add a new option as well if you think that make more sense |
/cmd prdoc --audience runtime_dev --bump patch |
…time_dev --bump patch'
/cmd fmt |
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.
LGTM! Just a few nits
Co-authored-by: Iulian Barbu <[email protected]>
Co-authored-by: Iulian Barbu <[email protected]>
/cmd fmt |
substrate/client/cli/src/config.rs
Outdated
/// By default this is retrieved from `SharedParams`. | ||
fn is_dev(&self) -> Result<bool> { | ||
Ok(self.shared_params().is_dev()) | ||
fn is_dev(&self) -> bool { |
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.
Please revert this unrequired change.
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 use is_dev() in this commit, this didnt make any sense, why does it have to return a result?
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 probably unwrap_or(false)
there instead to avoid doing changes here. I don't have a good example for why to return Result<bool>
- the only thing that comes to mind is that some nodes might work in a development setup where nodes query memory or IO to find whether they should run in development (however convoluted this might seem), and a third state like Err
could represent an internal error happened during the query, which is not equivalent with true
or false
(which represent the query result). Also, changing it could break users (in an unnecessary way IMO) and would be simpler (from a case perspective) to pack it with a bigger breaking change that's needed (if any).
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 am happy to rollback 7f73cfb if you can give me an explanation of why something that is always returning Ok(bool) should not instead just return bool
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 true that "always returning Ok(bool)" is equivalent to just returning bool
, but the method can be overridden to return also Err
- given it is a trait fn. However, I also find it very unlikely that node developers would need the Err
to be returned by is_dev
- whatever they need to do based on that error can still happen inside is_dev
override (mostly panicing). Returning the error though will give the flexibility to handle it outside is_dev
too, which might be needed in case some outer scope variables that can not be passed to is_dev
are needed (but the logic can also have this context on self
, and made available in is_dev
, but that enforces some constraints on implementations). I would personally not touch this for this PR, since it raises unrelated concerns, and maybe attack it via a different PR, but if we do it here, the prdoc should be updated accordingly.
This reverts commit 7f73cfb.
Co-authored-by: Iulian Barbu <[email protected]>
Co-authored-by: Iulian Barbu <[email protected]>
All GitHub workflows were cancelled due to failure one of the required jobs. |
Adds a new
--instant-seal
CLI flag to enable instant seal mode in omni-node.fixes #9996