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

gRPC support #3159

Merged
merged 4 commits into from Jan 14, 2020
Merged

gRPC support #3159

merged 4 commits into from Jan 14, 2020

Conversation

@cjcobb23
Copy link
Contributor

cjcobb23 commented Nov 15, 2019

This PR is to add gRPC support to rippled. In this initial iteration, 4 API's are implemented: fee, account_info, tx and submit. tx only supports Payment transactions at this time, but will be expanded on in a future iteration.

Design document can be found here: https://github.com/cjcobb23/grpcRippledDesign/blob/master/design/design.md
Requirements document can be found here: https://github.com/cjcobb23/grpcRippledDesign/blob/master/requirements/requirements.md
Full RPC documentation will be provided in a later PR.

Please note, there are issues with Travis CI currently. We are working on them and will get them resolved before merging. This will most likely result in significant changes to the cmake files. However, we would like to get eyes on the code in the meantime. For now, to build the code: clone the repo, checkout the branch, and run:
git submodule update --init --recursive.

Reach out to me if you have any issues.

Copy link
Contributor

MarkusTeufelberger left a comment

Didn't look at the C++ code, only mainly at the proto files.

I hope I don't come across as too negative, I'm actually happy that rippled will get a gRPC API. I'm just a bit concerned about how it'll play out and some details. In general I like the concept.

.gitmodules Outdated
@@ -0,0 +1,3 @@
[submodule "grpc"]

This comment has been minimized.

Copy link
@MarkusTeufelberger

MarkusTeufelberger Nov 16, 2019

Contributor

Why a submodule, not a subtree as every other vendored dependency in this repository?

This comment has been minimized.

Copy link
@mellery451

mellery451 Nov 18, 2019

Contributor

good point - we'll be fixing that shortly (to make grpc consistent with our other third party deps).

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

changed

@@ -134,6 +134,8 @@ target_include_directories (xrpl_core
PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/src>
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/src/ripple>
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/grpc>
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/proto>

This comment has been minimized.

Copy link
@MarkusTeufelberger

MarkusTeufelberger Nov 16, 2019

Contributor

Why a top level subfolder, not a folder in /src?

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Nov 18, 2019

Author Contributor

you are right, I think the proto files should be moved to /src/ripple/proto/rpc/v1. Also, grpc will be removed as a submodule most likely

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

moved to src/ripple/proto/rpc/v1

exclude_if_included (rippled)
# define a macro for tests that might need to
# be exluded or run differently in CI environment
if (is_ci)
target_compile_definitions(rippled PRIVATE RIPPLED_RUNNING_IN_CI)
endif ()

if(NOT WIN32)
set_source_files_properties(src/ripple/unity/app_main1.cpp PROPERTIES COMPILE_FLAGS -Wno-suggest-override)

This comment has been minimized.

Copy link
@MarkusTeufelberger

MarkusTeufelberger Nov 16, 2019

Contributor

Would be nice to know why -Wno-suggest-override is enabled everywhere here. Maybe in a comment?

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Nov 18, 2019

Author Contributor

This is because when grpc was included as a submodule, building grpc generated a lot of warnings (all suggest-override warnings). However, since grpc will most likely be removed as a submodule (and included in a different way), suppressing these warnings will not be necessary

// A request to get info about an account.
message GetAccountInfoRequest {
// The address to get info about.
string address = 1;

This comment has been minimized.

Copy link
@MarkusTeufelberger

MarkusTeufelberger Nov 16, 2019

Contributor

This is called "account" in https://xrpl.org/account_info.html and described as "most commonly", not "always" being the address.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Nov 18, 2019

Author Contributor

Will change to "account"

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

changed

bool strict = 2;

oneof ledger_index {
string ledger_index_shortcut_string = 3;

This comment has been minimized.

Copy link
@MarkusTeufelberger

MarkusTeufelberger Nov 16, 2019

Contributor

This can only be one of "validated", "closed" or "current", might be worth a separate type instead of a full string.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Nov 18, 2019

Author Contributor

Good idea. I could just use an enum

This comment has been minimized.

Copy link
@seelabs

seelabs Nov 21, 2019

Contributor

It's not obvious that a "shortcut_string" is "validated" or "closed". I know that this is going to be changed to an enum, but I have two pieces of feedback:

  1. We should document every fields in these proto buf files, including strings encode data
  2. I would not add a type suffix to field names. I.e. no _string here.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

changed to an enum

}

message Payment {
// The amount of currency to pay, in either fiat or XRP.

This comment has been minimized.

Copy link
@MarkusTeufelberger

MarkusTeufelberger Nov 16, 2019

Contributor
Suggested change
// The amount of currency to pay, in either fiat or XRP.
// The amount of currency to pay, in either issued currency or XRP.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

changed

string issuer = 3;
}

message Payment {

This comment has been minimized.

Copy link
@MarkusTeufelberger

MarkusTeufelberger Nov 16, 2019

Contributor

Tons more fields on this transaction type, it's not the easiest one for a start. ;-)

This comment has been minimized.

Copy link
@keefertaylor

keefertaylor Nov 18, 2019

I believe (and CJ can correct me) that this is at least the basics we would need to start building XRP transactions here. We just need to make sure what we have is extensible and doesn't cause breaking changes in the future.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Nov 19, 2019

Author Contributor

I'm just going to add the missing fields. SendMax and Paths will be moved from the Transaction type to here. I see that DestinationTag, InvoiceID and DeliverMin are missing. Any others?

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

added fields, lmk if i missed any (non-deprecated) fields

This comment has been minimized.

Copy link
@dangell7

dangell7 Jan 4, 2020

Are these fields getting added?

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Jan 5, 2020

Author Contributor

The fields to be added were added. If there are fields missing, please point them out. Note though, we are only supporting Payment transactions at this time, so some fields necessary for other transaction types were not added.

This comment has been minimized.

Copy link
@dangell7

dangell7 Jan 6, 2020

Fields missing; TransactionType, AccountTxnID. But everything else looks good. Transaction, TX and meta. You're returning all the necessary information via messages.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Jan 6, 2020

Author Contributor

TransactionType is omitted, since the type is defined via protobuf; each transaction type has it's own unique protobuf message type.
You are right that AccountTxnID is missing from the Transaction message. I added it to the AccountRoot ledger object message, but missed it in the Transaction message. I'll add that now; thanks for pointing it out.

This comment has been minimized.

Copy link
@dangell7

dangell7 Jan 6, 2020

Oh cool. So I'm looking to add Escrow next. Seems I would follow the message format of the Payment protobuf? I can help out as my use case is wrapping up and Escrow is next.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Jan 6, 2020

Author Contributor

@dangell7 I added AccountTxnID (naming is a bit different though).

As to adding Escrow: I assume you mean adding protobuf messages for EscrowCreate, EscrowFinish and EscrowCancel. You are free to do so and create a PR, and yes, follow the same format as the Payment message, but you are going to have to also implement that functionality on the C++ side as well, in addition to adding the protobuf messages. Also, you would need to also add support (in protobuf and C++) for all ledger objects that those Escrow transaction types affect, in order to return complete meta for those Escrow transactions.

bytes hash = 1;

// if true, return data in binary format
// TODO: does this make sense for protobuf?

This comment has been minimized.

Copy link
@MarkusTeufelberger

MarkusTeufelberger Nov 16, 2019

Contributor

...maaaybe? A client could always just encode the result themselves, but it might be nice to know what the server is actually working with.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 2, 2019

Author Contributor

@keefertaylor what do you think about this? Is there any benefit to returning this data in binary format? protobuf is already a binary format over the wire and is heavily optimized, so I don't think it makes sense to provide a binary option for efficiency reasons. But maybe there are other reasons a client would want the data in binary form.

This comment has been minimized.

Copy link
@keefertaylor

keefertaylor Dec 2, 2019

As far as I can tell I don't think there is.

It would be easiest to leave this off for now, and if we find a use case we really must have later we can always add it back in (conversely, it's harder to remove stuff).

This comment has been minimized.

Copy link
@MarkusTeufelberger

MarkusTeufelberger Dec 2, 2019

Contributor

A simple use case would be that this is the bulk of the data one needs to verify hashes. If data only arrives in some deserialized format, one first needs tom implement a serializer to verify incoming data.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

I kept this and removed the comment

bytes tx_bytes = 2;
};
// Sequence number of ledger that contains this transaction
uint64 ledger_index = 3;

This comment has been minimized.

Copy link
@MarkusTeufelberger

MarkusTeufelberger Nov 16, 2019

Contributor

Usually you used uint32 for this one elsewhere so far.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Nov 19, 2019

Author Contributor

Good catch. I am wondering whether it makes sense to define a separate type for this, so that way one does not have to remember that ledger sequence numbers are uint32

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

changed to uint32



// RPCs available to interact with the XRP Ledger.
service XRPLedgerAPIService {

This comment has been minimized.

Copy link
@MarkusTeufelberger

MarkusTeufelberger Nov 16, 2019

Contributor

No error handling so far, I guess that'll come later?

This comment has been minimized.

Copy link
@keefertaylor

keefertaylor Nov 18, 2019

Could you elaborate on what you'd expect to see here?

Obviously, gRPC supports plain error codes and descriptions. There's the advanced error model, which isn't really standard (yet): https://cloud.google.com/apis/design/errors#error_model.

If we really needed to encode error details, we could always roll them into the response metadata?

Or are you asking for their to be a generic ResponseMessage, that looks like:

message Response {
   oneof  {
      google.protobuf.Any response = 1
      google.rpc.Status error = 2;
   }
}

message RippledError {
  // ...
}

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Nov 19, 2019

Author Contributor

Error handling is built into grpc in a way. I'm using these status codes, along with string messages: https://github.com/grpc/grpc/blob/master/doc/statuscodes.md
If you look at the c++ handler code, you can see how I'm handling errors. For instance, if one passes a non-existent account to the GetAccountInfo RPC, I return status code NOT_FOUND, with message "account not found". gRPC will return this status code and message instead of the defined response type.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

@MarkusTeufelberger lmk what you think here or just resolve this

This comment has been minimized.

Copy link
@MarkusTeufelberger

MarkusTeufelberger Dec 10, 2019

Contributor

This is a comment, it can't be "resolved" on GitHub. Also I'm not in the Ripple team, so I also don't have some moderation tools available that project owners might have.

If the response to errors looks like in JSONRPC I guess it's fine.

Copy link

keefertaylor left a comment

Only really looked at the .proto files.

import "rpc/v1/ledger_objects.proto";

// A request to get info about an account.
message GetAccountInfoRequest {

This comment has been minimized.

Copy link
@keefertaylor

keefertaylor Nov 18, 2019

Just FWIW, Google's naming conventions also recommend the Get prefix (https://cloud.google.com/apis/design/naming_convention#method_names). Uber's prototool also seems to implicitly support this as well: https://github.com/uber/prototool/tree/dev/style#servicesrpcs.

Just because other folks are using get doesn't mean we have to. I do think that (AFAIK) these all are pretty widely used style guides which makes it somewhat idiomatic to folks who work with gRPC.


package rpc.v1;

message CurrencyAmount {

This comment has been minimized.

Copy link
@keefertaylor

keefertaylor Nov 18, 2019

This is a bad abstraction which is my fault. Supportive of moving it to be a single field called IssuedCurrencyAmount.

// A representation of an amount of XRP.
message XRPAmount {
// A numeric string representing the number of drops of XRP.
uint64 drops = 1;

This comment has been minimized.

Copy link
@keefertaylor

keefertaylor Nov 18, 2019

I think the important thing is that this is a typed number (rather than an untyped uint64 or untyped string). That way developers have to consciously spell out the word drops rather than guessing at the unit. I suppose we could remove this message and have each field be postfixed with drops?

Regardless, XRPDrops sounds reasonable to me. WDYT?

string value = 2;

// Unique account address of the entity issuing the currency.
string issuer = 3;

This comment has been minimized.

Copy link
@keefertaylor

keefertaylor Nov 18, 2019

I think this matches up with the pattern we are using with XRPAmount, and does make it safer for developers to understand what's going on. Happy to hear thoughts from others.


}

//Response to a GetFee RPC

This comment has been minimized.

Copy link
@keefertaylor

keefertaylor Nov 18, 2019

nit: add a space after //

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

added space


uint32 last_ledger_sequence = 8;

repeated Path paths = 9;

This comment has been minimized.

Copy link
@keefertaylor

keefertaylor Nov 18, 2019

+1, I think this lives in payments.

string issuer = 3;
}

message Payment {

This comment has been minimized.

Copy link
@keefertaylor

keefertaylor Nov 18, 2019

I believe (and CJ can correct me) that this is at least the basics we would need to start building XRP transactions here. We just need to make sure what we have is extensible and doesn't cause breaking changes in the future.

message TxResponse {

// The actual transaction
oneof sttx {

This comment has been minimized.

Copy link
@keefertaylor

keefertaylor Nov 18, 2019

Not sure what sttx means. Would recommend dropping the abbreviation.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Nov 19, 2019

Author Contributor

It's a naming convention I copied from rippled (think it means Serialized Type Tx), though I agree that it is very cryptic. What do you think is a good name for the one of? I will probably expand tx and tx_bytes to transaction and transaction_bytes btw.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Nov 19, 2019

Author Contributor

I could call it serialized_transaction

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

changed to serialized_transaction

bool validated = 5;

// metadata about the transaction
oneof stmeta {

This comment has been minimized.

Copy link
@keefertaylor

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Nov 19, 2019

Author Contributor

What do you think is a good name for this? I could call it serialized_meta.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

changed to serialized_meta



// RPCs available to interact with the XRP Ledger.
service XRPLedgerAPIService {

This comment has been minimized.

Copy link
@keefertaylor

keefertaylor Nov 18, 2019

Could you elaborate on what you'd expect to see here?

Obviously, gRPC supports plain error codes and descriptions. There's the advanced error model, which isn't really standard (yet): https://cloud.google.com/apis/design/errors#error_model.

If we really needed to encode error details, we could always roll them into the response metadata?

Or are you asking for their to be a generic ResponseMessage, that looks like:

message Response {
   oneof  {
      google.protobuf.Any response = 1
      google.rpc.Status error = 2;
   }
}

message RippledError {
  // ...
}
@seelabs

This comment has been minimized.

Copy link
Contributor

seelabs commented Nov 19, 2019

Great job on this @cjcobb23! You got up to speed very quickly and submitted an ambitions addition to rippled. I'll have detailed feedback later, but I wanted to give some initial feedback high-level feedback:

  1. As others have pointed out, fields like send_max and paths are payment specific, and not part of all transactions. There are also common fields like SourceTag that are important to support. The documentation on the common transaction fields is here: https://xrpl.org/transaction-common-fields.html. If we're going to initially support the payment transaction, I'd add all the fields that a payment transaction can support. The documentation for that is here: https://xrpl.org/payment.html. I don't think it makes sense to release this API without supporting all the options available for payments, but we decide not to, we at least need to support DestinationTag.

  2. I have to think about the "string" based interface proposed here. As written, many binary values are encoded strings (addresses, amounts, keys, for example). Since protocol buffers are a binary format I would have assumed these would be binary encoded and making these binary would simplify the handlers in rippled. I do understand this would require clients to implement encoders and decoders for base58 and hex at least. Is that the trade-off or is there another reason for the string-based interface?

  3. This patch made compile time shoot up 50% on my system (5m 40s vs 3m 38s on a debug build). We'll have to address that.

  4. We don't usually merge partial functionality into rippled. I'd have to think about merging an API that just supported the payment call only. I'd vote to wait until a much larger set of API calls were supported before this was merged into rippled.

  5. I'd recommend organizing the commits before you submit a pull-request. Before submitting, I usually either squash it down to one commit, or prepare a set of commits that can be logically reviewed as a unit. As it is, the 61 commits on this PR don't really help reviewers.

@cjcobb23

This comment has been minimized.

Copy link
Contributor Author

cjcobb23 commented Nov 19, 2019

@seelabs About the strings interface: there is no difference between strings and bytes over the wire; both are encoded the same way and use the same amount of space. When using strings, protobuf enforces the invariant that each character is UTF-8. Since there is no difference in encoding, I decided that strings were a better type for data that is meant to be human readable. The string type signals that the data is meant to be readable, and the UTF-8 invariant enforces that the data actually is readable. Btw, with the C++ api, bytes and strings are both represented as std::string. In the API for other languages however, strings are represented differently and it is (supposedly) more convenient to work with strings than bytes when the data is meant to be readable (see here: https://groups.google.com/forum/#!topic/protobuf/au6eQBkRT5s ) @keefertaylor what do you think about this?

I'll move send_max and paths to the Payment type, and also add support for all the options available for Payments. It shouldn't be too difficult.

I agree that merging partial functionality is less than ideal; however, providing full support for the Tx command would require expanding on the proto files quite a bit, as well as writing a lot more handler code. @keefertaylor is going to use this to develop Xpring SDK's, and doesn't need the full Tx functionality (correct me if I'm wrong here keefer), so in the interest of time, I just implemented what is necessary for Payments.

About the commits, I do see the benefits of organizing the commits better, though I'm not sure that squashing something this big into one commit makes sense. I could squash this into several commits (say 10 commits as opposed to 61), though doing that now would require me to force-push to this branch. Should I reorganize the commits and force-push, or just leave as is, and remember to better organize the commits for future PRs?

I appreciate the praise. @pwang200 and @p2peer deserve some credit as well, as they helped write the unit tests and work out build issues.

Edit: I actually am going to take back what I said about squashing into several commits as opposed to 1. I think it will be difficult to create a handful of sensible commits out of these 61 commits, and its much simpler and less error prone just to squash everything into one commit. Plus, I have some trailing white space, which I could get rid of during the squashing.

@seelabs

This comment has been minimized.

Copy link
Contributor

seelabs commented Nov 19, 2019

@cjcobb23 @keefertaylor

Re: Binary Interface

When I say "string" based I mean this API encodes binary data as ASCII strings, instead of binary data directly. I.e with a hex encoding the 8-bit number 255 is not encoded as one byte, but by the two ASCII characters 'f' and 'f'. Protocol buffers support binary data, why are we encoding it?

This is an API to interface between the rippled server and other programming languages. The client may take encoded data from a human and decode it to binary before talking to rippled, and it may encode binary data from rippled before presenting it to a human, but why should the two programming languages talk to each other with encoded data?

I do understand this requires encoders and decoders in the client (base58 and hex at least), but it also simplifies the handlers in rippled. I'd much rather have a binary interface to rippled and then build encoded APIs on top of that than the other way around. (As an aside, I understand the rippled json interface requires these codecs, but I'm hoping we can eventually move that outside of rippled and use this new binary interface instead).

Unless I'm missing a reason why the interface is using strings, I'd prefer a binary interface.

Re: Implementing a subset of functionality

I understand that a fuller API will require more time. I'm OK supporting the small subset of functionality needed by xspring, but with a couple conditions:

  1. This has to be the start of a new API for rippled that supports everything we do with the JSON API now. It's hard to justify the cost of integrating gRPC into rippled to support the small set of functionality in this PR.

  2. The API has to be marked as unstable/experimental, and the API must be able to change until we mark it as stable. I expect this will ship in 1.5 as "unstable/experimental" and then 1.6 will have a fuller API with possible breakage. I want to remove the unstable API after a very short transition (maybe one version). xspring will have to update their back end when we break the API. If we're committing to a backward compatible API now or supporting an "experimental" API for several releases, then we want to do much, much more upfront design of the API. If a backward compatible API is needed, we should think about having xspring use the JSON API until the new one is ready, and I doubt it would be ready for 1.5.

@keefertaylor

This comment has been minimized.

Copy link

keefertaylor commented Nov 20, 2019

Re, Strings vs Bytes:
Certainly agree that we're doubling the API response size for these fields across the wire. In general though, the data that we're talking about is small (20 bytes vs 40 bytes of ASCII for an account).

In general, I would optimize a public API for useability rather than raw efficiency. For instance, when you return the bytes for an account, are you going to return the raw (unprefixed, unchecksummed) bytes? Or will you add the prefix and checksum and return that as bytes? If the former, now the client has to understand Base58Check encoding and the specific inputs ripple uses, if the latter, then the client still probably has to go read that documentation anyway, then run the base58check encoding. Then, you have to have a Base58Check encoding library in your language and to wire it up. In general, I'd favor centralizing this business logic wherever possible (it's easier for rippled to implement this once than for every developer to implement it).

Put succinctly, is saving 20 bytes over the wire a valuable tradeoff for every developer who touches this API needing to go read the documentation to go understand what they have to do to get to an address string, which is what they wanted anyway.

As additional data: We're already returning these fields as ASCII in the JSON API - are folks actively complaining about the data or RPC size?

@seelabs

This comment has been minimized.

Copy link
Contributor

seelabs commented Nov 20, 2019

@keefertaylor I'm not worried about the size of the data on the wire at all. I'm also not arguing that a "string" based API that encodes and decodes from our various formats isn't needed - it absolutely is. Here's where I'm coming from: I've thought about adding a low-level API to rippled, and moving the high-level APIs to a separate server outside of rippled. I was trying to steer this gRPC API into the low-level API that I want.

However, I agree: without a high-level gRPC API in place, it's not realistic to force clients to do base58 encoding and decoding themselves. Address at least should be base58 encoded and not raw binary.

I'm not sure I agree about hex encoded data though. Why are we encoding hex data instead of sending raw bytes?

I also want to give an example why I want to eventually move the encoding out of rippled. Take a look at the code to decode secrets seeds: First it checks if it's been encoded as a ripple lib seed, because that used a non-standard encoding. Next, it checks if it's encoded as a base58 account, node public key, node private key, or account secret, in case someone accidentally sends those instead of a secret. Next it checks if it's encoded as hex. Next it checks if it's encoded as a base 58 seed. Moving this complexity out of rippled is a win. I understand it needs to live somewhere, but I'd like to make rippled itself as stable and secure as possible.


// A representation of an amount of XRP.
message XRPAmount {
// A numeric string representing the number of drops of XRP.

This comment has been minimized.

Copy link
@keefertaylor

keefertaylor Nov 20, 2019

Comment out of date

@keefertaylor

This comment has been minimized.

Copy link

keefertaylor commented Nov 20, 2019

Cool. I think we've reached agreement on string making sense for addresses.

As far as other data, I think I generally agree with you too! I was narrowed in on addresses. I audited the code just now and it appears to (to me) that the only places strings appear in proto files are:

  • As addresses / accountIDs across a slew of messages - it sounds like we think this is valid
  • As a currency identifier
  • As an engine_result and engine_result_message (there's an open discussion to move the former to an enum
  • ledger_entry_type in AffectedNode
  • transaction_result in Meta (also has an open discussion about moving to an enum)
  • As a value in an IssuedCurrency
  • As a ledger_index_shortcut_string in GetAccountInfoRequest (also has an open discussion about moving to an enum)

I think all of those (modulo the enum changes are valid use cases for strings). Let me know if there is a place I am missing.

Apologies if you were looking at some of the early prototype files I was using in Xpring. I was building an MVP super quickly and I think CJ has really done a great job at refactoring away my hastily made and incorrect abstractions.

Edit: tagging @seelabs to make sure this comment gets visibility.

@seelabs

This comment has been minimized.

Copy link
Contributor

seelabs commented Nov 20, 2019

@keefertaylor I was looking at the bytes signing_public_key_hex field: https://github.com/cjcobb23/rippled/blob/grpcSupport/proto/rpc/v1/transaction.proto#L24-L25. Is there a reason that's hex encoded?

@seelabs seelabs self-requested a review Nov 20, 2019
@seelabs seelabs requested a review from mDuo13 Nov 20, 2019
@seelabs seelabs requested a review from mellery451 Nov 20, 2019
@keefertaylor

This comment has been minimized.

Copy link

keefertaylor commented Nov 20, 2019

@seelabs That field is actually of type bytes, though it's certainly incorrectly named. I flagged the naming comment in the review.

}

// The public key of the account which signed the transaction in hexadecimal.
bytes signing_public_key_hex = 5;

This comment has been minimized.

Copy link
@keefertaylor

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

dropped

@cjcobb23

This comment has been minimized.

Copy link
Contributor Author

cjcobb23 commented Nov 20, 2019

@seelabs signing_public_key_hex is just named wrong. It is of the bytes type, and in the c++ code, I send the raw bytes and don't encode to hex: https://github.com/cjcobb23/rippled/blob/grpcSupport/src/ripple/rpc/impl/RPCHelpers.cpp#L1252
I did not intend to encode any data as a hex string in this API, since converting to and from hex is rather simple; if I am sending or receiving hex strings, it is a mistake on my part and should be fixed.

Copy link
Contributor

seelabs left a comment

I did a pass mostly focusing on the GRPCServer files. I'll do some more review later, but I think I have enough in this batch that I'd like post them.

There are a couple of formatting nits we need to fix (I didn't flag these individually):

  1. There are several lines that end with extra whitespace, we need to remove the whitespace
  2. The public: and private: keywords aren't indented like the rest of rippled's code
  3. We put a space after commas in function arguments

Instead of auditing the code and fixing those, I'd just set up clang-format and use the .clang-format file in the repo to reformat the new code.

There are also a couple of style nits I'd like to fix:

  1. There are places where the code uses snake_case for variables and functions. We use pascalCase.
  2. I wouldn't add comments that don't add value, they get in the way of code. It's not a big deal, and there are not that many in this patchset, but I usually flag comments I don't think add value so too many of them don't build up. Examples:
    // gRPC server
    std::unique_ptr<grpc::Server> server_;

    // referernce to ripple::Application
    Application& app_;
    
    // hash
    bytes hash = 4;
oneof ledger_index {
string ledger_index_shortcut_string = 3;
uint32 ledger_index_seq = 4;
bytes ledger_hash = 5;

This comment has been minimized.

Copy link
@seelabs

seelabs Nov 21, 2019

Contributor

The field "ledger index" contains things that are not ledger indexes (the shortcut string isn't an index, and this hash isn't an index). I'd rename parent group.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Nov 22, 2019

Author Contributor

Can we rename the parent group to just "ledger"?

This comment has been minimized.

Copy link
@seelabs

seelabs Nov 22, 2019

Contributor

@cjcobb23 What do you think of ledger_id?

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

@seelabs I changed this, lmk what you think

bool strict = 2;

oneof ledger_index {
string ledger_index_shortcut_string = 3;

This comment has been minimized.

Copy link
@seelabs

seelabs Nov 21, 2019

Contributor

It's not obvious that a "shortcut_string" is "validated" or "closed". I know that this is going to be changed to an enum, but I have two pieces of feedback:

  1. We should document every fields in these proto buf files, including strings encode data
  2. I would not add a type suffix to field names. I.e. no _string here.
{
auto ptr = std::make_shared<CallData<Request,Response>>(
service_,*cq_,app_,bl,handler, condition, load_type);
return std::static_pointer_cast<Processor>(ptr);

This comment has been minimized.

Copy link
@seelabs

seelabs Nov 21, 2019

Contributor

There's no need for this cast, we can just return the result of make_shared.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Nov 22, 2019

Author Contributor

Will remove this whole function

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

removed

{
auto ptr = makeCallData(bl,handler,condition,load_type);
requests_.push_front(ptr);
ptr->set_iter(requests_.begin());

This comment has been minimized.

Copy link
@seelabs

seelabs Nov 21, 2019

Contributor

We shouldn't be storing the iterator in the object.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

removed


//converts data to a string of bytes
template <class T>
std::string toBytes(T const & data)

This comment has been minimized.

Copy link
@seelabs

seelabs Nov 21, 2019

Contributor

I would remove this function. Even if we needed this for protobufs (I don't think we do), this function has too wide of a scope. I'd like to use the void set_foo(const void* value, size_t size); overloads instead of the string overloads..

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Nov 22, 2019

Author Contributor

We can definitely just use those overloads; that will be more efficient anyways since we don't have to create a std::string temporary. Will remove this toBytes() function

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

removed this function and used the other overloads

BindListener<Request,Response> bl,
Handler<Request,Response> handler,
RPC::Condition condition,
Resource::Charge load_type)

This comment has been minimized.

Copy link
@seelabs

seelabs Nov 21, 2019

Contributor

If we're getting rid of storing the iterator with the object, I'd remove this function.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Nov 22, 2019

Author Contributor

Will remove this function as well

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Nov 26, 2019

Author Contributor

@seelabs I actually don't think this function should be removed. While we don't need to set the iterator, we still need to add each shared_ptr<CallData> object to the requests_ collection. Without this function, I would have to write requests_.push_front() (or whatever insertion method is used) for each RPC, as opposed to just calling this function

This comment has been minimized.

Copy link
@seelabs

seelabs Nov 26, 2019

Contributor

We have to call makeAndPush for each RPC now. The difference is we replace code like

makeAndPush<rpc::v1::GetFeeRequest, rpc::v1::GetFeeResponse>(
            &rpc::v1::XRPLedgerAPIService::AsyncService::RequestGetFee,
            doFeeGrpc,
            RPC::NEEDS_CURRENT_LEDGER,
            Resource::feeReferenceRPC
            );

with

{
        using cd = CallData<rpc::v1::GetFeeRequest, rpc::v1::GetFeeResponse>;
        requests_.push_front(std::make_shared<cd>(
            service_, *cq_, app_,
            &rpc::v1::XRPLedgerAPIService::AsyncService::RequestGetFee,
            doFeeGrpc, RPC::NEEDS_CURRENT_LEDGER, Resource::feeReferenceRPC));
}

You do save repeating the service_, *cq_, app_, parameters with a helper function, but I'm not sure it's worth defining a helper function just for that. I don't really object if you want to keep a helper function like this, there's not great harm, but I also don't see much benefit either.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

removed

BindListener<Request,Response> bl,
Handler<Request,Response> handler,
RPC::Condition condition,
Resource::Charge load_type)

This comment has been minimized.

Copy link
@seelabs

seelabs Nov 21, 2019

Contributor

I'd remove this function and just call make_shared directly where needed.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

removed

src/ripple/app/main/GRPCServer.h Outdated Show resolved Hide resolved
// Possible states of the RPC
enum CallStatus { PROCESSING, FINISH };
// The current serving state.
CallStatus status_;

This comment has been minimized.

Copy link
@seelabs

seelabs Nov 21, 2019

Contributor

I think a bool finished_ or somesuch is clearer. I understand this started with more states, and we pared it down to two, but now that it's two states I'd just use a bool. Tho I guess I don't object too strongly if other reviews are OK with enum.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Nov 22, 2019

Author Contributor

I think a bool would be clearer as well, will change

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

changed to bool


// iterator pointing to this object in the requests list
// for lifetime management
boost::optional<std::list<std::shared_ptr<Processor>>::iterator> iter_;

This comment has been minimized.

Copy link
@seelabs

seelabs Nov 21, 2019

Contributor

I'd remove iter_ as a member.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

removed

// metadata about the transaction
oneof stmeta {
Meta meta = 6;
bytes meta_bytes = 7;

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Nov 22, 2019

Author Contributor

@seelabs would you also omit the type suffix here? I could change it to meta_binary , but that's still adding the type as a suffix, even if its not the exact name of the protobuf type.

This comment has been minimized.

Copy link
@seelabs

seelabs Nov 22, 2019

Contributor

@cjcobb23 Maybe raw_meta? I'm OK with _binary too. Names are hard, maybe someone else has a good suggestion.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

changed to meta_binary and transaction_binary

bind_listener_(bind_listener),
handler_(handler),
required_condition_(required_condition),
load_type_(load_type)

This comment has been minimized.

Copy link
@mellery451

mellery451 Nov 25, 2019

Contributor

pass by value types can be moved

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

changed to move

Section section = app_.config().section("port_grpc");

//get the default values of ip and port
std::size_t colon_pos = server_address_.find(':');

This comment has been minimized.

Copy link
@mellery451

mellery451 Nov 25, 2019

Contributor

I don't think we should have default grpc server addr/port. If it wasn't specified in the config, it's either an error or you just don't start grpc (probably the latter).

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Nov 26, 2019

Author Contributor

@mellery451 I think we should not start grpc if the addr/port is not in the config. However, the unit tests need the grpc server to start, so if someone runs the unit tests without addr/port for grpc in the config, the unit tests will fail.

Edit: I guess we could just detect in the unit test whether the grpc server should be running (probably just test if addr/port for grpc is present in the config), and if its not, just do nothing.

This comment has been minimized.

Copy link
@mellery451

mellery451 Nov 26, 2019

Contributor

we have a default config that is used for unit tests:

setupConfigForUnitTests (Config& cfg)
{
std::string const port_peer = to_string(port_base);
std::string port_rpc = to_string(port_base + 1);
std::string port_ws = to_string(port_base + 2);
cfg.overwrite (ConfigSection::nodeDatabase (), "type", "memory");
cfg.overwrite (ConfigSection::nodeDatabase (), "path", "main");
cfg.deprecatedClearSection (ConfigSection::importNodeDatabase ());
cfg.legacy("database_path", "");
cfg.setupControl(true, true, true);
cfg["server"].append("port_peer");
cfg["port_peer"].set("ip", getEnvLocalhostAddr());
cfg["port_peer"].set("port", port_peer);
cfg["port_peer"].set("protocol", "peer");
cfg["server"].append("port_rpc");
cfg["port_rpc"].set("ip", getEnvLocalhostAddr());
cfg["port_rpc"].set("admin", getEnvLocalhostAddr());
cfg["port_rpc"].set("port", port_rpc);
cfg["port_rpc"].set("protocol", "http,ws2");
cfg["server"].append("port_ws");
cfg["port_ws"].set("ip", getEnvLocalhostAddr());
cfg["port_ws"].set("admin", getEnvLocalhostAddr());
cfg["port_ws"].set("port", port_ws);
cfg["port_ws"].set("protocol", "ws");
cfg.SSL_VERIFY = false;
}

I would suggest adding a grpc_server helper function in that same file that sets-up the grpc config needed and then add that to the Env initialization for any tests that need it (e.g. Env env {*this, envconfig(grpc_server)}; )

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

Added the helper and initialization code

port_str = port_pair.first;
}

server_address_ = ip_str + ":" + port_str;

This comment has been minimized.

Copy link
@mellery451

mellery451 Nov 25, 2019

Contributor

this is not ipv6 friendly. I would suggest using beast::IP::Endpoint to create a string from previously parsed address and port values (see setup_ServerHandler above.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

changed

namespace {

//helper function. strips port from endpoint string
std::string getEndpoint(std::string const& peer)

This comment has been minimized.

Copy link
@mellery451

mellery451 Nov 25, 2019

Contributor

this method is not ipv6 friendly. I'm not sure what format these peer strings have, but it seems like mainly they are endpoint strings with an optional scheme (dns://) ? See if parseURL will work for you (I think it will only parse if a scheme is present..not sure) or maybe just strip the scheme yourself by searching for // and then use Endpoint::from_string to do the rest.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Nov 27, 2019

Author Contributor

In my testing, the peer strings look like ipv4:127.0.0.1:49808. parseUrl is not working, and Endpoint::from_string does not work either. If I strip ipv4 off of the front, I can then pass the string to Endpoint::from_string, which is actually what I'm doing already (if you look at where this function is used). I'm not sure the best way to do this; the documentation for the peer() method says a string is returned in uri format.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

I think what I have here will work, though correct me if I'm wrong. Apparently, the string is always of the form protocol:ip:port. See this PR for reference (you sent this to me actually): grpc/grpc#14667

@seelabs

This comment has been minimized.

Copy link
Contributor

seelabs commented Nov 27, 2019

The non-unity build doesn't compile.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 28, 2019

Codecov Report

Merging #3159 into develop will decrease coverage by 0.08%.
The diff coverage is 65.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #3159      +/-   ##
==========================================
- Coverage    70.78%   70.7%   -0.09%     
==========================================
  Files          689     691       +2     
  Lines        54296   55089     +793     
==========================================
+ Hits         38432   38949     +517     
- Misses       15864   16140     +276
Impacted Files Coverage Δ
src/ripple/rpc/handlers/LedgerHandler.h 68.75% <ø> (ø) ⬆️
src/ripple/rpc/impl/RPCHelpers.h 100% <ø> (ø) ⬆️
src/ripple/rpc/handlers/DownloadShard.cpp 0% <0%> (ø) ⬆️
src/ripple/rpc/handlers/Reservations.cpp 0% <0%> (ø) ⬆️
src/ripple/rpc/handlers/Random.cpp 0% <0%> (ø) ⬆️
src/ripple/rpc/handlers/LogRotate.cpp 0% <0%> (ø) ⬆️
src/ripple/rpc/handlers/BlackList.cpp 0% <0%> (ø) ⬆️
src/ripple/rpc/handlers/AccountTxOld.cpp 0% <0%> (ø) ⬆️
src/ripple/rpc/handlers/LedgerCleanerHandler.cpp 0% <0%> (ø) ⬆️
src/ripple/rpc/handlers/LogLevel.cpp 0% <0%> (ø) ⬆️
... and 70 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 c624fc9...7d867b8. Read the comment docs.

@@ -27,8 +27,7 @@ namespace test {

extern std::atomic<bool> envUseIPv4;

inline

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 3, 2019

Author Contributor

I setup my editor to use the .clang-format file in the repo, but inadvertently reformatted code that I didn't touch. I'm going to revert those formatting changes to this file (and the others).

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 4, 2019

Author Contributor

I'll revert these changes when I squash the commits

@cjcobb23

This comment has been minimized.

Copy link
Contributor Author

cjcobb23 commented Dec 4, 2019

@seelabs @keefertaylor @MarkusTeufelberger I pushed all requested changes. Take a look and let me know if I missed anything or if I should change anything. Otherwise, resolve your comments.

return sPtr.get() == ptr;
});
BOOST_ASSERT(it != requests.end());
*it = requests.back();

This comment has been minimized.

Copy link
@seelabs

seelabs Dec 6, 2019

Contributor

Don't assign, swap instead. Assigning needs to change the atomic reference counts. Swapping should be able to just move pointers around.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 6, 2019

Author Contributor

changed to swap

GRPCServerImpl::start()
{
// if config does not specify a grpc server address, don't start
if (serverAddress_ == "")

This comment has been minimized.

Copy link
@seelabs

seelabs Dec 6, 2019

Contributor

I'd call empty() instead of comparing to ""

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 6, 2019

Author Contributor

changed to calling empty()

public:
virtual ~Processor()
{
}

This comment has been minimized.

Copy link
@seelabs

seelabs Dec 6, 2019

Contributor

I'd default this instead. virtual ~Processor() = default

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 6, 2019

Author Contributor

defaulted

~GRPCServerImpl()
{
}
Comment on lines 107 to 109

This comment has been minimized.

Copy link
@seelabs

seelabs Dec 6, 2019

Contributor

I don't think we need to define this at all. I'd leave it out.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 6, 2019

Author Contributor

removed

template <class T>
struct ContextGeneric
{
beast::Journal j;
T params;
Application& app;
Resource::Charge& loadType;
NetworkOPs& netOps;
LedgerMaster& ledgerMaster;
Resource::Consumer& consumer;
Role role;
std::shared_ptr<JobQueue::Coro> coro;
InfoSub::pointer infoSub;
};

Comment on lines 63 to 74

This comment has been minimized.

Copy link
@seelabs

seelabs Dec 6, 2019

Contributor

This duplicates most of the Context struct. Maybe have a base class with the common fields? Also ContextGeneric isn't a great name. Maybe GRPCContext or somesuch.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 6, 2019

Author Contributor

Created a base class and changed the names of the derived classes.

rpc::v1::CurrencyAmount& proto,
RPC::ContextGeneric<T>& context,
std::shared_ptr<Transaction> transaction,
TxMeta const& transactionMeta)

This comment has been minimized.

Copy link
@seelabs

seelabs Dec 6, 2019

Contributor

We should look at trying to get rid of this code duplication if we can. Maybe we can do it in another PR - but we should at least make a TODO to look at this (you don't need to do this now).

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 6, 2019

Author Contributor

Added a TODO

rpc::v1::CurrencyAmount&,
ContextGeneric<rpc::v1::GetTxRequest>&,
std::shared_ptr<Transaction>,
TxMeta const&);

This comment has been minimized.

Copy link
@seelabs

seelabs Dec 6, 2019

Contributor

We're going to have to insert one of these for every request type that is added. I don't love those kind of manual changes (tho we will get a linker error to remind us to do it, so it's not killer).

It looks like we only need the app and the ledger master. What do you think about passing those as parameters directly? That way this function doesn't need to be a template. If that's a problem, we should look at moving this implementation to the header.

If Context and ContextGeneric were refactor into a common base class, we could also look at passing the base class in. That might help with the code duplication as well.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 6, 2019

Author Contributor

Passed in base class to remove template


struct GRPCTestClientBase
{
GRPCTestClientBase(std::string const& port)

This comment has been minimized.

Copy link
@seelabs

seelabs Dec 6, 2019

Contributor

Be careful with one parameter non-explicit constructors - they define an automatic conversion. Some classes want that, but most of the time we should declare these constructors explicit. I'd declare this one explicit.

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 6, 2019

Author Contributor

changed to explicit

rpc::v1::SubmitTransactionRequest request;
rpc::v1::SubmitTransactionResponse reply;

SubmitClient(std::string const& port) : GRPCTestClientBase(port)

This comment has been minimized.

Copy link
@seelabs

seelabs Dec 6, 2019

Contributor

explicit

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 6, 2019

Author Contributor

changed

rpc::v1::GetTxRequest request;
rpc::v1::GetTxResponse reply;

GrpcTxClient(std::string const& port) : GRPCTestClientBase(port)

This comment has been minimized.

Copy link
@seelabs

seelabs Dec 6, 2019

Contributor

explicit

This comment has been minimized.

Copy link
@cjcobb23

cjcobb23 Dec 6, 2019

Author Contributor

changed

@cjcobb23

This comment has been minimized.

Copy link
Contributor Author

cjcobb23 commented Dec 6, 2019

@seelabs pushed the additional requested changes

@seelabs
seelabs approved these changes Dec 6, 2019
Copy link
Contributor

seelabs left a comment

👍 Assuming tests pass.

We still need to make it clear that this is experimental/prototype code that may be removed in the future or changed in backward incompatible ways. Let's make sure that's documented somewhere front and center or we're going to make life hard for ourselves.

The two biggest things we need to work on to are:

  1. A spec for what this gRPC API should be. We have limited chances to introduce new APIs. Let's make the most of our chance here. Elliot and Rome can help.

  2. We need to reduce the code duplication of two nearly identical APIs.

This was referenced Jan 23, 2020