From 52cd63c9a0f19b0bf3181b7d0abdfe8f9226dca5 Mon Sep 17 00:00:00 2001 From: Chenna Keshava B S Date: Thu, 8 Aug 2024 19:35:09 -0700 Subject: [PATCH 1/6] [FIX] #5034 -- book_changes rpc method accepts conventional ledger-strings --- src/test/rpc/BookChanges_test.cpp | 96 +++++++++++++++++++++++++++ src/xrpld/rpc/handlers/BookOffers.cpp | 10 +-- 2 files changed, 101 insertions(+), 5 deletions(-) create mode 100644 src/test/rpc/BookChanges_test.cpp diff --git a/src/test/rpc/BookChanges_test.cpp b/src/test/rpc/BookChanges_test.cpp new file mode 100644 index 00000000000..caf9dba95c2 --- /dev/null +++ b/src/test/rpc/BookChanges_test.cpp @@ -0,0 +1,96 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2024 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include +#include +#include +#include + +namespace ripple { +namespace test { + +class BookChanges_test : public beast::unit_test::suite +{ +public: + void + testConventionalLedgerInputStrings() + { + testcase("Specify well-known strings as ledger input"); + jtx::Env env(*this); + Json::Value params, resp; + + // As per convention in XRPL, ledgers can be specified with strings + // "closed", "validated" or "current" + params["ledger"] = "validated"; + resp = env.rpc("json", "book_changes", to_string(params)); + BEAST_EXPECT(!resp[jss::result].isMember(jss::error)); + BEAST_EXPECT(resp[jss::result][jss::status] == "success"); + + params["ledger"] = "current"; + resp = env.rpc("json", "book_changes", to_string(params)); + BEAST_EXPECT(!resp[jss::result].isMember(jss::error)); + BEAST_EXPECT(resp[jss::result][jss::status] == "success"); + + params["ledger"] = "closed"; + resp = env.rpc("json", "book_changes", to_string(params)); + BEAST_EXPECT(!resp[jss::result].isMember(jss::error)); + BEAST_EXPECT(resp[jss::result][jss::status] == "success"); + + // non-conventional ledger input should throw an error + params["ledger"] = "non_conventional_ledger_input"; + resp = env.rpc("json", "book_changes", to_string(params)); + BEAST_EXPECT(resp[jss::result].isMember(jss::error)); + BEAST_EXPECT(resp[jss::result][jss::status] != "success"); + } + + void + testLedgerInputDefaultBehavior() + { + testcase( + "If ledger_hash or ledger_index is not specified, the behavior " + "must default to the `current` ledger"); + jtx::Env env(*this); + + // As per convention in XRPL, ledgers can be specified with strings + // "closed", "validated" or "current" + Json::Value const resp = + env.rpc("json", "book_changes", to_string(Json::Value{})); + BEAST_EXPECT(!resp[jss::result].isMember(jss::error)); + BEAST_EXPECT(resp[jss::result][jss::status] == "success"); + + // I dislike asserting the below statement, because its dependent on the + // unit-test framework BEAST_EXPECT(resp[jss::result][jss::ledger_index] + // == 3); + } + + void + run() override + { + testConventionalLedgerInputStrings(); + testLedgerInputDefaultBehavior(); + + // Note: Other aspects of the book_changes rpc are fertile grounds for + // unit-testing purposes. It can be included in future work + } +}; + +BEAST_DEFINE_TESTSUITE(BookChanges, app, ripple); + +} // namespace test +} // namespace ripple diff --git a/src/xrpld/rpc/handlers/BookOffers.cpp b/src/xrpld/rpc/handlers/BookOffers.cpp index 6126913a5b6..dccc03de5be 100644 --- a/src/xrpld/rpc/handlers/BookOffers.cpp +++ b/src/xrpld/rpc/handlers/BookOffers.cpp @@ -204,13 +204,13 @@ doBookOffers(RPC::JsonContext& context) Json::Value doBookChanges(RPC::JsonContext& context) { - auto res = RPC::getLedgerByContext(context); + std::shared_ptr ledger; - if (std::holds_alternative(res)) - return std::get(res); + Json::Value result = RPC::lookupLedger(ledger, context); + if (ledger == nullptr) + return result; - return RPC::computeBookChanges( - std::get>(res)); + return RPC::computeBookChanges(ledger); } } // namespace ripple From 88ef02e8cf8bbc95752ddea70e68389ab25f7b1d Mon Sep 17 00:00:00 2001 From: Chenna Keshava B S Date: Fri, 9 Aug 2024 20:05:49 -0700 Subject: [PATCH 2/6] [FIX] #5035 - book_changes rpc method response includes a "validated" field --- src/test/rpc/BookChanges_test.cpp | 7 +++++++ src/xrpld/rpc/BookChanges.h | 3 +++ 2 files changed, 10 insertions(+) diff --git a/src/test/rpc/BookChanges_test.cpp b/src/test/rpc/BookChanges_test.cpp index caf9dba95c2..55ffae4d641 100644 --- a/src/test/rpc/BookChanges_test.cpp +++ b/src/test/rpc/BookChanges_test.cpp @@ -41,17 +41,24 @@ class BookChanges_test : public beast::unit_test::suite resp = env.rpc("json", "book_changes", to_string(params)); BEAST_EXPECT(!resp[jss::result].isMember(jss::error)); BEAST_EXPECT(resp[jss::result][jss::status] == "success"); + BEAST_EXPECT(resp[jss::result][jss::validated] == true); params["ledger"] = "current"; resp = env.rpc("json", "book_changes", to_string(params)); BEAST_EXPECT(!resp[jss::result].isMember(jss::error)); BEAST_EXPECT(resp[jss::result][jss::status] == "success"); + BEAST_EXPECT(resp[jss::result][jss::validated] == false); params["ledger"] = "closed"; resp = env.rpc("json", "book_changes", to_string(params)); BEAST_EXPECT(!resp[jss::result].isMember(jss::error)); BEAST_EXPECT(resp[jss::result][jss::status] == "success"); + // In the unit-test framework, requesting for "closed" ledgers appears + // to yield "validated" ledgers. This is not new behavior. It is also + // observed in the unit tests for the LedgerHeader class. + BEAST_EXPECT(resp[jss::result][jss::validated] == true); + // non-conventional ledger input should throw an error params["ledger"] = "non_conventional_ledger_input"; resp = env.rpc("json", "book_changes", to_string(params)); diff --git a/src/xrpld/rpc/BookChanges.h b/src/xrpld/rpc/BookChanges.h index 7d7978d3fe2..0401917a89c 100644 --- a/src/xrpld/rpc/BookChanges.h +++ b/src/xrpld/rpc/BookChanges.h @@ -171,6 +171,9 @@ computeBookChanges(std::shared_ptr const& lpAccepted) Json::Value jvObj(Json::objectValue); jvObj[jss::type] = "bookChanges"; + + // retrieve validated information from LedgerHeader class + jvObj[jss::validated] = lpAccepted->info().validated; jvObj[jss::ledger_index] = lpAccepted->info().seq; jvObj[jss::ledger_hash] = to_string(lpAccepted->info().hash); jvObj[jss::ledger_time] = Json::Value::UInt( From e20f4a62962604c6764009a251a7c4b372ad5b81 Mon Sep 17 00:00:00 2001 From: Chenna Keshava B S Date: Tue, 10 Sep 2024 14:25:16 -0700 Subject: [PATCH 3/6] specify include header directives in BookChanges header file --- src/xrpld/rpc/BookChanges.h | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/xrpld/rpc/BookChanges.h b/src/xrpld/rpc/BookChanges.h index 0401917a89c..c87fa0ccf4e 100644 --- a/src/xrpld/rpc/BookChanges.h +++ b/src/xrpld/rpc/BookChanges.h @@ -20,6 +20,15 @@ #ifndef RIPPLE_RPC_BOOKCHANGES_H_INCLUDED #define RIPPLE_RPC_BOOKCHANGES_H_INCLUDED +#include +#include +#include +#include +#include +#include + +#include + namespace Json { class Value; } From b2603ed4420be1e6ab6f900fd934b099190b5ec9 Mon Sep 17 00:00:00 2001 From: Chenna Keshava B S Date: Wed, 11 Sep 2024 10:14:56 -0700 Subject: [PATCH 4/6] remove non-test-framework #includes from BookChanges_test file. This commit removes the includes for protocol, json_reader and json_writer files --- src/test/rpc/BookChanges_test.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/test/rpc/BookChanges_test.cpp b/src/test/rpc/BookChanges_test.cpp index 55ffae4d641..95997538d79 100644 --- a/src/test/rpc/BookChanges_test.cpp +++ b/src/test/rpc/BookChanges_test.cpp @@ -18,9 +18,6 @@ //============================================================================== #include -#include -#include -#include namespace ripple { namespace test { From f69b235792530fd2de644243c970e5c8b53a5de7 Mon Sep 17 00:00:00 2001 From: Chenna Keshava B S Date: Mon, 16 Sep 2024 14:45:43 -0700 Subject: [PATCH 5/6] update API-changelog --- API-CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/API-CHANGELOG.md b/API-CHANGELOG.md index 58ceed0cde1..4a00f41f579 100644 --- a/API-CHANGELOG.md +++ b/API-CHANGELOG.md @@ -28,6 +28,11 @@ The `network_id` field was added in the `server_info` response in version 1.5.0 ## XRP Ledger server version 2.0.0 +### Updates in 2.3.0 + +(Breaking Changes) +- Changes to the `book_changes` RPC: This RPC now returns a `validated` field in its response, which was missing in prior versions. Additionally, `book_changes` RPC exhibits similar behavior as the other non-Admin RPC commands, insofar as ledger-retrieval is concerned. Specifically, if the requested ledger version is old, then a `ledgerNotFound` error is returned. This is a breaking change, because prior versions would attempt to acquire such an old ledger. + ### Additions in 2.2 Additions are intended to be non-breaking (because they are purely additive). From 75cb23359c18bade0ad5e39ef1db1119cc9a3ac4 Mon Sep 17 00:00:00 2001 From: Chenna Keshava B S <21219765+ckeshava@users.noreply.github.com> Date: Tue, 17 Sep 2024 13:36:47 -0700 Subject: [PATCH 6/6] Update API-CHANGELOG.md Co-authored-by: Elliot Lee --- API-CHANGELOG.md | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/API-CHANGELOG.md b/API-CHANGELOG.md index 4a00f41f579..967e1ce6052 100644 --- a/API-CHANGELOG.md +++ b/API-CHANGELOG.md @@ -26,12 +26,15 @@ In `api_version: 2`, the `signer_lists` field [will be moved](#modifications-to- The `network_id` field was added in the `server_info` response in version 1.5.0 (2019), but it is not returned in [reporting mode](https://xrpl.org/rippled-server-modes.html#reporting-mode). -## XRP Ledger server version 2.0.0 +### Breaking change in 2.3 -### Updates in 2.3.0 +- `book_changes`: If the requested ledger version is not available on this node, a `ledgerNotFound` error is returned and the node does not attempt to acquire the ledger from the p2p network (as with other non-admin RPCs). -(Breaking Changes) -- Changes to the `book_changes` RPC: This RPC now returns a `validated` field in its response, which was missing in prior versions. Additionally, `book_changes` RPC exhibits similar behavior as the other non-Admin RPC commands, insofar as ledger-retrieval is concerned. Specifically, if the requested ledger version is old, then a `ledgerNotFound` error is returned. This is a breaking change, because prior versions would attempt to acquire such an old ledger. +Admins can still attempt to retrieve old ledgers with the `ledger_request` RPC. + +### Addition in 2.3 + +- `book_changes`: Returns a `validated` field in its response, which was missing in prior versions. ### Additions in 2.2