-
Notifications
You must be signed in to change notification settings - Fork 160
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
feat(shed): add forest-tool shed f3 check-activation-raw #5309
Conversation
2f5ec93
to
60f6a3c
Compare
src/tool/subcommands/shed_cmd/f3.rs
Outdated
CheckActivation { | ||
/// Contract address | ||
#[arg(long, required = true)] | ||
contract: String, |
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 a huge deal given it's a testing tool, but it'd be great to use concrete types here instead of String
if possible.
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.
Fixed.
pub enum F3Commands { | ||
CheckActivation { | ||
/// Contract address | ||
#[arg(long, required = true)] |
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.
Perhaps let's just make this a positional argument?
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 think it's fine to keep the usage the same as lotus shed f3
as this command is not expected to be used often. What do you think?
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 a test command so no strong opinion.
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 a test tool it's fine. For code actually running with the node, we'd need more safety mechanisms and documentation, especially given the amount of magic numbers.
Let's add it to the CLI docs reference? It's a new command so it won't be automagically picked up by the leshy bot. |
@LesnyRumcajs will do in a subsequent PR |
Summary of changes
Part of #5307
Changes introduced in this pull request:
Lotus code:
Output:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist