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

updates to book_changes RPC command #5096

Merged
merged 9 commits into from
Sep 19, 2024
100 changes: 100 additions & 0 deletions src/test/rpc/BookChanges_test.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
//------------------------------------------------------------------------------
/*
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 <test/jtx.h>

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");
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));
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
12 changes: 12 additions & 0 deletions src/xrpld/rpc/BookChanges.h
Bronek marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,15 @@
#ifndef RIPPLE_RPC_BOOKCHANGES_H_INCLUDED
#define RIPPLE_RPC_BOOKCHANGES_H_INCLUDED

#include <xrpl/json/json_value.h>
#include <xrpl/protocol/LedgerFormats.h>
#include <xrpl/protocol/STAmount.h>
#include <xrpl/protocol/STObject.h>
#include <xrpl/protocol/TxFormats.h>
#include <xrpl/protocol/jss.h>

#include <memory>

namespace Json {
class Value;
}
Expand Down Expand Up @@ -171,6 +180,9 @@ computeBookChanges(std::shared_ptr<L const> 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(
Expand Down
10 changes: 5 additions & 5 deletions src/xrpld/rpc/handlers/BookOffers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -204,13 +204,13 @@ doBookOffers(RPC::JsonContext& context)
Json::Value
doBookChanges(RPC::JsonContext& context)
{
auto res = RPC::getLedgerByContext(context);
std::shared_ptr<ReadView const> ledger;

if (std::holds_alternative<Json::Value>(res))
return std::get<Json::Value>(res);
Json::Value result = RPC::lookupLedger(ledger, context);
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Does this change still yield the same error from checking ledger_index etc.?
  2. Can we do the same in LedgerRequest.cpp and eliminate getLedgerByContext() entirely?

Copy link
Collaborator

@Bronek Bronek Sep 16, 2024

Choose a reason for hiding this comment

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

  1. @godexsoft the ledger_index handling (and errors) will be the same as in almost every other command, since the vast majority of commands use lookupLedger. However, since with this change, the ledger won't be requested from other nodes, if a ledger which is not in the local cache is requested, this method will fail with ledgerNotFound error (just like other methods do in such situation)
  2. No; the ADMIN method ledger_request which, with this PR, will be the only method using getLedgerByContext(), and it needs to rely on this function in order to download the ledger from other nodes.

if (ledger == nullptr)
return result;

return RPC::computeBookChanges(
std::get<std::shared_ptr<Ledger const>>(res));
return RPC::computeBookChanges(ledger);
}

} // namespace ripple
Loading