Skip to content
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

Improved error message on mistyped command [RIPD-1527] #2283

Closed
wants to merge 2 commits into from

Conversation

scottschurr
Copy link
Collaborator

First things first; only the (currently) top-most commit needs to be reviewed. This pull request sits atop an earlier pull request from @HowardHinnant who is working in the same area.

Previously if you mistyped the "submit_multisigned" command as "submit_multisign", the returned message was "Internal error". Not very helpful. It turns out this was caused by a small amount of code in RPCCall.cpp. Removing that code improves two situations:

  1. It improves the situation with a mistyped command. Now the command returns "Unknown method" and provides the string of the mistyped command.

  2. The "transaction_entry" RPC command, if properly entered in its command line form, would fire an assert. That assert is now removed.

In the process, it was discovered that the command line form of the "transaction_entry" command has not worked correctly for at least a year. Therefore support for the command line form of "transaction_entry" is added along with appropriate unit tests.

Reviewers: @HowardHinnant for familiarity with RPC, @mellery451 for familiarity with TransactionEntry_test.cpp.

@codecov-io
Copy link

codecov-io commented Nov 27, 2017

Codecov Report

Merging #2283 into develop will increase coverage by 0.58%.
The diff coverage is 84.84%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2283      +/-   ##
===========================================
+ Coverage    70.22%   70.81%   +0.58%     
===========================================
  Files          692      692              
  Lines        50961    51050      +89     
===========================================
+ Hits         35787    36149     +362     
+ Misses       15174    14901     -273
Impacted Files Coverage Δ
src/ripple/rpc/impl/ServerHandlerImp.cpp 89.98% <81.25%> (-3.93%) ⬇️
src/ripple/net/impl/RPCCall.cpp 58.73% <94.44%> (+19.34%) ⬆️
src/ripple/rpc/handlers/AccountObjects.cpp 95.45% <0%> (-2.28%) ⬇️
src/ripple/rpc/handlers/AccountOffers.cpp 96.82% <0%> (-1.59%) ⬇️
src/ripple/server/impl/BaseWSPeer.h 70.51% <0%> (-0.65%) ⬇️
src/ripple/protocol/impl/Serializer.cpp 69.96% <0%> (-0.35%) ⬇️
src/ripple/server/impl/BaseHTTPPeer.h 87.57% <0%> (+0.62%) ⬆️
src/ripple/beast/rfc2616.h 79.45% <0%> (+1.36%) ⬆️
src/ripple/json/impl/json_reader.cpp 60.79% <0%> (+1.4%) ⬆️
src/ripple/json/impl/json_writer.cpp 80.31% <0%> (+4.46%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6dc79c2...0681702. Read the comment docs.

Copy link
Contributor

@HowardHinnant HowardHinnant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Good log message.

Copy link
Contributor

@mellery451 mellery451 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@scottschurr scottschurr added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Nov 27, 2017
HowardHinnant and others added 2 commits November 30, 2017 18:16
* Can be exercised from the command line with json2

* Rewrite Env::do_rpc to call the same code as
  rpc from the command line.  This puts rpc
  handling logic in one place.
Previously if you mistyped the "submit_multisigned" command as
"submit_multisign", the returned message was "Internal error".  Not
very helpful.  It turns out this was caused by a small amount of
code in RPCCall.cpp.  Removing that code improves two situations:

1. It improves the situation with a mistyped command.  Now the
   command returns "Unknown method" and provides the string of
   the mistyped command.

2. The "transaction_entry", if properly entered in its command
   line form, would fire an assert.  That assert is now removed.

In the process, it was discovered that the command line form of
the "transaction_entry" command has not worked correctly for at
least a year.  Therefore support for that the command line form
of "transaction_entry" is added along with appropriate unit
tests.
@scottschurr
Copy link
Collaborator Author

Rebased to 0.80.1.

@seelabs
Copy link
Collaborator

seelabs commented Dec 1, 2017

In 0.90.0-b1

@seelabs seelabs closed this Dec 1, 2017
@scottschurr scottschurr deleted the howard-json2 branch January 12, 2018 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants