evmc tool CLI improvements #574
Conversation
| return evmc::from_hex(content); | ||
| } | ||
|
|
||
| struct HexValidator : public CLI::Validator |
tools/evmc/main.cpp
Outdated
|
|
||
| CLI::App app{"EVMC tool"}; | ||
| const auto& version_flag = *app.add_flag("--version", "Print version information and exit"); | ||
| const auto& vm_option = | ||
| *app.add_option("--vm", vm_config, "EVMC VM module")->envname("EVMC_VM"); | ||
|
|
||
| auto& run_cmd = *app.add_subcommand("run", "Execute EVM bytecode"); | ||
| run_cmd.add_option("code", code_hex, "Hex-encoded bytecode")->required(); | ||
| run_cmd.add_option("code", code_hex, "Bytecode")->required()->check(Hex | CLI::ExistingFile); |
tools/evmc/main.cpp
Outdated
|
|
||
| namespace | ||
| { | ||
| /// Returns the input str if already valid hex string. Otherwise, interprets the str as a file | ||
| /// name and loads the hex string from this file. | ||
| /// @todo The file content is not validated. |
There was a problem hiding this comment.
So does the file needs to be a hex string, or it is a binary file?
There was a problem hiding this comment.
Can it be validated? Even if just validating it here with abort, I know it is not nice, but reduces confusion should someone misuse it.
There was a problem hiding this comment.
It still throws "invalid hex digit" exception or similar. This is caught and reported although it is not clear where the error comes from. I added test for this case.
There was a problem hiding this comment.
Isn't it coming from somewhere inside cmd::run?
There was a problem hiding this comment.
It is. But I mean user will see the error about invalid hex without information about the source. He has to guess it is code and/or input what is invalid.
There was a problem hiding this comment.
I think it comes from there as it uses load_hex which throws the error and main catches that. Can you leave this as a comment here (that it is caught later in evmc::run), for now?
chfast
force-pushed
the
tools_cli
branch
2 times, most recently
from
966f86d to
501e9a5
Compare
|
|
||
| add_evmc_tool_test( | ||
| invalid_hex_input | ||
| "--vm $<TARGET_FILE:evmc::example-vm> run 0x --input aa0y" |
There was a problem hiding this comment.
Hmm, we accept this weird Ethereum concept of 0x for empty input? Does it accept run '' or run "" ?
There was a problem hiding this comment.
Could you add one more test with that? I hope we can remove the run 0x case at some point in the future :)
There was a problem hiding this comment.
There was a problem hiding this comment.
I meant adding a test for run "".
And yes, I know it currently accepts 0x as empty and not asking to remove it right now :)
There was a problem hiding this comment.
I tried 30 mins, but I don't know how to do this in CMake.
There was a problem hiding this comment.
Hmm, I thought "--vm $<TARGET_FILE:evmc::example-vm> run '' --input aa0y" would have been enough?
| add_evmc_tool_test( | ||
| invalid_hex_code | ||
| "--vm $<TARGET_FILE:evmc::example-vm> run 0x600" | ||
| "code: \\(incomplete hex byte pair\\) OR \\(File does not exist: 0x600\\)" |
There was a problem hiding this comment.
Error messge is a bit weird... Both statements are true, it's incomplete hex pair AND file does not exist.
Is there a way to customize the message somehow maybe?
There was a problem hiding this comment.
This is CLI11 default but probably can be changed if you re-implement many things. I'm not going to do it now.
Mark run subcommand as "fallthrough" - unknown options are passed to the main app. This allows using it as `evmc run --vm vm.so 0x00` instead of having only fixed order `evmc --vm vm.so run 0x00`.
For arguments
codeand--inputwe try to guess if the value is a file path or hex string. If it is a file then we load hex string from the file.There are some gaps here still: we assume the file contains valid hex string. We could additionally inspect the file content but that requires custom
ExistingFilevalidator.Closes #569.