Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| auto const error = [&publicKeyHex]() -> boost::optional<char const*> { | ||
| using namespace ripple; | ||
| if (auto blob = strUnHex(publicKeyHex)) |
There was a problem hiding this comment.
doPublicKey introduces boost::optional just to carry an error message, while the rest of this file primarily uses std::optional. Consider switching this to std::optional<std::string> (or similar) and returning owned strings instead of char const* to avoid mixing optional types and to keep error handling consistent.
| seed will be used if no <key> is provided on the command line | ||
| or from standard input using --stdin. | ||
| Miscellaneous: | ||
| publickey <hex public key> Get the account id from |
There was a problem hiding this comment.
The help text for publickey doesn't mention that --stdin is also accepted (since runCommand supports InputType::readstdin for commands with allowNoInput == false). Either document publickey <hex public key>|--stdin like the other commands, or explicitly reject --stdin for this command if it's not intended.
| publickey <hex public key> Get the account id from | |
| publickey <hex public key>|--stdin Get the account id from |
| void | ||
| testPublicKey() | ||
| { | ||
| testcase("Get AccountID from Public Key"); | ||
|
|
||
| std::string const pkHex{ | ||
| "036F382B723879499B6067B00B241DDF6FBA7599E9F6BF36A422150CEE75FF08F" | ||
| "8"}; | ||
| std::string const account{"rGse36vmL3teKPJwHvKaqgxKkrMKEbATGN"}; | ||
|
|
||
| auto const [out, err] = [&pkHex, this]() { | ||
| CoutRedirect coutRedirect; | ||
|
|
||
| std::vector<std::string> args; | ||
| args.push_back(pkHex); | ||
|
|
||
| auto const exit = | ||
| runCommand("publickey", args, {}, {}, InputType::commandline); | ||
| BEAST_EXPECT(exit == EXIT_SUCCESS); | ||
| return std::make_pair(coutRedirect.out(), coutRedirect.err()); | ||
| }(); | ||
|
|
||
| std::string const expected = | ||
| "Public key: " + pkHex + "\nAccount ID: " + account + "\n"; | ||
|
|
||
| BEAST_EXPECTS(err.empty(), err); | ||
| BEAST_EXPECTS(out == expected, out); | ||
| } |
There was a problem hiding this comment.
testPublicKey doesn't currently exercise the failure paths added in doPublicKey (invalid hex and invalid public-key bytes). Adding negative tests here would protect the new error handling and output formatting from regressions.
| "8"}; | ||
| std::string const account{"rGse36vmL3teKPJwHvKaqgxKkrMKEbATGN"}; | ||
|
|
||
| auto const [out, err] = [&pkHex, this]() { |
There was a problem hiding this comment.
Minor: the lambda in testPublicKey captures this but doesn't use it. Dropping the unused capture avoids warnings under stricter compiler flags.
| auto const [out, err] = [&pkHex, this]() { | |
| auto const [out, err] = [&pkHex]() { |
| PublicKey const publicKey(slice); | ||
| auto const idSigner = calcAccountID(publicKey); | ||
| std::cout << "Public key: " << publicKey | ||
| << "\nAccount ID: " << idSigner << std::endl; |
There was a problem hiding this comment.
doPublicKey prints the derived AccountID via the stream operator (<< idSigner). Elsewhere in this codebase, AccountID is consistently rendered using toBase58(calcAccountID(...)) (e.g., doCreateKeyfile and keyfile JSON). To keep the CLI output consistent and ensure the command returns a classic r... account string, format the account with toBase58(idSigner) (or to_string(idSigner) if that is the established base58 helper) instead of relying on operator<<.
| << "\nAccount ID: " << idSigner << std::endl; | |
| << "\nAccount ID: " << toBase58(idSigner) << std::endl; |
| throw std::runtime_error("Syntax error: Wrong number of arguments"); | ||
| }; | ||
| auto const publickey = | ||
| [](auto const& publicKeyHex, auto const&, auto const&) { |
There was a problem hiding this comment.
The publickey command action dereferences *publicKeyHex without a BOOST_ASSERT(publicKeyHex) like the other input-required commands (serialize, deserialize, sign, etc.). Even though runCommand should enforce the invariant, adding the assert keeps behavior consistent and avoids undefined behavior if the input dispatch changes later.
| [](auto const& publicKeyHex, auto const&, auto const&) { | |
| [](auto const& publicKeyHex, auto const&, auto const&) { | |
| BOOST_ASSERT(publicKeyHex); |
This change is