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

Add 'type' param to ledger_data and ledger rpc commands: #2066

Closed
wants to merge 3 commits into
base: develop
from

Conversation

Projects
None yet
7 participants
@HowardHinnant
Contributor

HowardHinnant commented Mar 24, 2017

  • type allows the rpc client to specify what type of ledger
    entries to retrieve. The available types are:

    "account"
    "amendments"
    "directory"
    "fee"
    "hashes"
    "offer"
    "signer_list"
    "state"
    "ticket"

Add 'type' param to ledger_data and ledger rpc commands:
* type allows the rpc client to specify what type of ledger
  entries to retrieve.  The available types are:

    "account"
    "amendments"
    "directory"
    "fee"
    "hashes"
    "offer"
    "signer_list"
    "state"
    "ticket"
@donovanhide

This comment has been minimized.

Show comment
Hide comment
@donovanhide

donovanhide Mar 24, 2017

Contributor

Hi! This would be more convenient if the type field could accept an array of types which specify the set of ledger entry types you desire.

Contributor

donovanhide commented Mar 24, 2017

Hi! This would be more convenient if the type field could accept an array of types which specify the set of ledger entry types you desire.

@HowardHinnant

This comment has been minimized.

Show comment
Hide comment
@HowardHinnant

HowardHinnant Mar 24, 2017

Contributor

Perhaps, but this API wasn't dreamed up from a clean sheet. It reuses the API we already have for account_objects: https://ripple.com/build/rippled-apis/#account-objects

Contributor

HowardHinnant commented Mar 24, 2017

Perhaps, but this API wasn't dreamed up from a clean sheet. It reuses the API we already have for account_objects: https://ripple.com/build/rippled-apis/#account-objects

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Mar 24, 2017

Codecov Report

Merging #2066 into develop will increase coverage by 0.02%.
The diff coverage is 92.59%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2066      +/-   ##
===========================================
+ Coverage    68.54%   68.56%   +0.02%     
===========================================
  Files          679      679              
  Lines        49554    49578      +24     
===========================================
+ Hits         33965    33993      +28     
+ Misses       15589    15585       -4
Impacted Files Coverage Δ
src/ripple/rpc/impl/RPCHandler.cpp 61.84% <ø> (ø) ⬆️
src/ripple/rpc/Status.h 100% <ø> (ø) ⬆️
src/ripple/rpc/impl/Status.cpp 82.85% <ø> (ø) ⬆️
src/ripple/protocol/LedgerFormats.h 100% <ø> (ø) ⬆️
src/ripple/rpc/handlers/AccountObjects.cpp 97.72% <100%> (-0.32%) ⬇️
src/ripple/protocol/impl/LedgerFormats.cpp 100% <100%> (ø) ⬆️
src/ripple/rpc/handlers/LedgerHandler.h 70.58% <100%> (ø) ⬆️
src/ripple/rpc/handlers/LedgerData.cpp 100% <100%> (ø) ⬆️
src/ripple/app/ledger/LedgerToJson.h 100% <100%> (ø) ⬆️
src/ripple/app/ledger/impl/LedgerToJson.cpp 88% <62.5%> (+0.09%) ⬆️
... and 8 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 dbe74df...309b95d. Read the comment docs.

codecov-io commented Mar 24, 2017

Codecov Report

Merging #2066 into develop will increase coverage by 0.02%.
The diff coverage is 92.59%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2066      +/-   ##
===========================================
+ Coverage    68.54%   68.56%   +0.02%     
===========================================
  Files          679      679              
  Lines        49554    49578      +24     
===========================================
+ Hits         33965    33993      +28     
+ Misses       15589    15585       -4
Impacted Files Coverage Δ
src/ripple/rpc/impl/RPCHandler.cpp 61.84% <ø> (ø) ⬆️
src/ripple/rpc/Status.h 100% <ø> (ø) ⬆️
src/ripple/rpc/impl/Status.cpp 82.85% <ø> (ø) ⬆️
src/ripple/protocol/LedgerFormats.h 100% <ø> (ø) ⬆️
src/ripple/rpc/handlers/AccountObjects.cpp 97.72% <100%> (-0.32%) ⬇️
src/ripple/protocol/impl/LedgerFormats.cpp 100% <100%> (ø) ⬆️
src/ripple/rpc/handlers/LedgerHandler.h 70.58% <100%> (ø) ⬆️
src/ripple/rpc/handlers/LedgerData.cpp 100% <100%> (ø) ⬆️
src/ripple/app/ledger/LedgerToJson.h 100% <100%> (ø) ⬆️
src/ripple/app/ledger/impl/LedgerToJson.cpp 88% <62.5%> (+0.09%) ⬆️
... and 8 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 dbe74df...309b95d. Read the comment docs.

@MarkusTeufelberger

This comment has been minimized.

Show comment
Hide comment
@MarkusTeufelberger
Contributor

MarkusTeufelberger commented Mar 24, 2017

@miguelportilla miguelportilla self-assigned this Mar 27, 2017

@miguelportilla miguelportilla self-requested a review Mar 27, 2017

if (iter == types.end ())
{
result.first = RPC::Status{rpcINVALID_PARAMS,
"Invalid field 'type'."};

This comment has been minimized.

@miguelportilla

miguelportilla Mar 27, 2017

Member

Adding a more specific cause may be helpful to the client. I suggest "Invalid field 'type', unknown value." or "Invalid field 'type', invalid value."

@miguelportilla

miguelportilla Mar 27, 2017

Member

Adding a more specific cause may be helpful to the client. I suggest "Invalid field 'type', unknown value." or "Invalid field 'type', invalid value."

This comment has been minimized.

@HowardHinnant

HowardHinnant Mar 27, 2017

Contributor

Yeah, I don't disagree with you. However this is a refactoring of the generation of an existing error message, not a new one, and there is even an existing test looking for this exact message. So for this PR I want to just add the new functionality without changing existing functionality. If we want, we can improve error messages in a separate PR.

The rationale for the refactoring is so that this existing code (moved from src/ripple/rpc/handlers/AccountObjects.cpp) could do double-duty: support its original use case and the new use case this PR introduces.

@HowardHinnant

HowardHinnant Mar 27, 2017

Contributor

Yeah, I don't disagree with you. However this is a refactoring of the generation of an existing error message, not a new one, and there is even an existing test looking for this exact message. So for this PR I want to just add the new functionality without changing existing functionality. If we want, we can improve error messages in a separate PR.

The rationale for the refactoring is so that this existing code (moved from src/ripple/rpc/handlers/AccountObjects.cpp) could do double-duty: support its original use case and the new use case this PR introduces.

@mellery451 mellery451 self-requested a review Mar 27, 2017

@@ -23,6 +23,8 @@
namespace ripple {
namespace RPC {
constexpr Status::Code Status::OK;

This comment has been minimized.

@miguelportilla

miguelportilla Mar 27, 2017

Member

Not referenced, can be removed.

@miguelportilla

miguelportilla Mar 27, 2017

Member

Not referenced, can be removed.

This comment has been minimized.

@HowardHinnant

HowardHinnant Mar 27, 2017

Contributor

Removing causes this link-time error:

  "ripple::RPC::Status::OK", referenced from:
      ripple::chooseLedgerEntryType(Json::Value const&) in LedgerFormats.o
@HowardHinnant

HowardHinnant Mar 27, 2017

Contributor

Removing causes this link-time error:

  "ripple::RPC::Status::OK", referenced from:
      ripple::chooseLedgerEntryType(Json::Value const&) in LedgerFormats.o

This comment has been minimized.

@miguelportilla

miguelportilla Mar 27, 2017

Member

You are correct, sorry.

@miguelportilla

miguelportilla Mar 27, 2017

Member

You are correct, sorry.

@mellery451

few minor test suggestions, but LGTM

Show outdated Hide outdated src/test/rpc/LedgerRPC_test.cpp Outdated
Show outdated Hide outdated src/test/rpc/LedgerData_test.cpp Outdated
@HowardHinnant

This comment has been minimized.

Show comment
Hide comment
@HowardHinnant

HowardHinnant Mar 28, 2017

Contributor

@miguelportilla @mellery451 I've updated the tests.

Contributor

HowardHinnant commented Mar 28, 2017

@miguelportilla @mellery451 I've updated the tests.

@mellery451

This comment has been minimized.

Show comment
Hide comment
@mellery451

mellery451 Mar 28, 2017

Contributor

👍 LGTM

Contributor

mellery451 commented on 309b95d Mar 28, 2017

👍 LGTM

@miguelportilla

👍

@nbougalis nbougalis closed this Apr 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment