Remove gRPC code previously used for the xpring SDK & enable gRPC on localhost by default by cjcobb23 · Pull Request #4272 · XRPLF/rippled

cjcobb23 · 2022-08-11T22:06:09Z

There was a lot of gRPC code in rippled that has long been deprecated. It was used for the xpring sdk, which has been abandoned. Some of the gRPC code is used for reporting mode and clio however. This PR removes the code that is no longer needed, and only retains the gRPC code needed for clio and reporting mode.

nbougalis · 2022-08-12T01:24:42Z

HALLELUJAH!

natenichols

After a first pass I have a couple of questions

natenichols · 2022-08-15T01:25:39Z

cfg/rippled-example.cfg

-#port = 50051
-#ip = 0.0.0.0
-#secure_gateway = 127.0.0.1
+[port_grpc]


Why are we uncommenting this in the rippled-example.cfg. Do we want gRPC to be enabled by default?

We talked about this in the huddle. I asked if we could uncomment this by default, and the agreement was yes if the xpring sdk code was removed.

natenichols · 2022-08-15T01:27:24Z

src/ripple/proto/org/xrpl/rpc/v1/ledger.proto

 }

-// Next field: 3
-message LedgerRange


Do we use this in Clio or reporting mode, or only get the LedgerRange from the ledger subscription stream?

We don't use this in clio or reporting mode

natenichols · 2022-08-15T01:29:17Z

src/ripple/proto/org/xrpl/rpc/v1/xrp_ledger.proto

-
  // Get a specific ledger, optionally including transactions and any modified,
  // added or deleted ledger objects
  rpc GetLedger(GetLedgerRequest) returns (GetLedgerResponse);


Can we also remove GetLedgerDiff() and GetLedgerEntry? AFAIK, Clio only uses GetLedgerRequest and GetLedgerDataRequest

Those RPCs are not part of the xpring sdk, and I actually think they could be useful. If we want to remove them, we should discuss and do it as part of a separate PR.

natenichols · 2022-08-15T01:32:52Z

src/ripple/rpc/handlers/Tx.cpp

-}
-
 Json::Value
 populateJsonResponse(


I don't actually know what this looked like before we added these gRPC methods, so take what I say next with a grain of salt.

Should we leave doTxHelp and populateJsonResponse as their own functions, or move it into doTx(). It occurs to me that we may have split this out to enable code reuse between the gRPC and JSON-RPC endpoints.

Is it worth leaving doTxHelp as a helper function?

I thought about this. We could move it, but I think since we already went through the work of breaking up this function, we should keep the helper function as is. What if we want to add a different response interface in the future? The helper function would be useful. I don't see it causing a problem as is.

natenichols · 2022-08-15T01:36:34Z

src/test/rpc/ReportingETL_test.cpp

    {
        testcase("NeedCurrentOrClosed");
-        {
-            org::xrpl::rpc::v1::GetAccountInfoRequest request;


Why are we removing this test? Flagging this because it's in ReportingETL_test.cpp

GetAccountInfoRequest is removed. This won't compile if we leave it.

scottschurr

Thanks so much for taking care to this @cjcobb23! It's great to see so much of this code gone. I left a couple of comments that may be relevant. But other than those this looks to me like it is good to go. 👍

scottschurr · 2022-08-16T22:32:14Z

cfg/rippled-example.cfg

+[port_grpc]
+port = 50051
+ip = 0.0.0.0
+secure_gateway = 127.0.0.1


Surprised to see this stanza un-commented as part of this pull request. There should be very few circumstances where this is useful, no? I could imagine it left commented out or even removed?

This is just to make it easier for people to run clio, since clio uses this port to talk to rippled. Though now that I'm thinking about it, it might be best to change the IP to 127.0.0.1, so that way the port will only listen locally by default. If someone is running clio remotely, they'll need to modify their config anyways.

scottschurr · 2022-08-16T23:11:58Z

src/test/protocol/KnownFormatToGRPC_test.cpp

 #include <ripple/protocol/LedgerFormats.h>
 #include <ripple/protocol/TxFormats.h>

-#include "org/xrpl/rpc/v1/ledger_objects.pb.h"


Seems like this entire file should be removed? Hallelujah!

scottschurr

Thanks for the update @cjcobb23. I think there are a couple more things that need to be tuned.

The RippledCore.cmake file needs to have the KnownFormatToGRPC_test.cpp file removed.
The ordering.txt file needs to be tweaked so the levelization CI test passes.

If it's helpful, here's a commit that makes those changes: scottschurr@f5d9f05

Once those changes are made I think this pull request is good to go.

scottschurr

👍 Looks great! Thanks.

Remove gRPC code previously used for the xpring SDK

8efc094

nbougalis requested review from natenichols and scottschurr August 12, 2022 01:24

natenichols suggested changes Aug 15, 2022

View reviewed changes

scottschurr approved these changes Aug 17, 2022

View reviewed changes

natenichols approved these changes Aug 17, 2022

View reviewed changes

[FOLD] address review comments

5f85226

scottschurr suggested changes Aug 31, 2022

View reviewed changes

seelabs mentioned this pull request Sep 1, 2022

XLS-38d: XChainBridge (side chains) #4292

Merged

[FOLD] Fix build and CI issues

7c6dbd0

scottschurr approved these changes Sep 14, 2022

View reviewed changes

seelabs added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Oct 12, 2022

manojsdoshi mentioned this pull request Oct 13, 2022

Proposed 1.10.0-b2 #4321

Merged

legleux mentioned this pull request Oct 13, 2022

Fix build by removing the #include in rippled/src/ripple/rpc/DeliveredAmount.h #4322

Closed

1 task

manojsdoshi closed this in #4321 Oct 17, 2022

intelliot changed the title ~~Remove gRPC code previously used for the xpring SDK~~ Remove gRPC code previously used for the xpring SDK & enable gRPC on localhost by default Feb 22, 2023

intelliot referenced this pull request in manojsdoshi/rippled Mar 7, 2023

Remove gRPC code previously used for the xpring SDK

28f4cc7

Conversation

cjcobb23 commented Aug 11, 2022

Uh oh!

nbougalis commented Aug 12, 2022

Uh oh!

natenichols left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottschurr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottschurr left a comment

Choose a reason for hiding this comment

Uh oh!

scottschurr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants