Skip to content

Commit

Permalink
fix: issues in simulate RPC (#5265)
Browse files Browse the repository at this point in the history 
Make `simulate` RPC easier to use:
* Prevent the use of `seed`, `secret`, `seed_hex`, and `passphrase` fields (to avoid confusing with the signing methods).
* Add autofilling of the `NetworkID` field.
  • Loading branch information
@mvadari
mvadari authored Feb 7, 2025
1 parent 02387fd commit d9e4009
Show file tree
Hide file tree
Showing 7 changed files with 203 additions and 18 deletions.
2 changes: 1 addition & 1 deletion API-CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ As of 2025-01-28, version 2.4.0 is in development. You can use a pre-release ver
- `ledger_entry`: `state` is added an alias for `ripple_state`.
- `validators`: Added new field `validator_list_threshold` in response.
- `simulate`: A new RPC that executes a [dry run of a transaction submission](https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-0069d-simulate#2-rpc-simulate)
- Signing methods autofill fees better and properly handle transactions that don't have a base fee.
- Signing methods autofill fees better and properly handle transactions that don't have a base fee, and will also autofill the `NetworkID` field.

## XRP Ledger server version 2.3.0

Expand Down
1 change: 1 addition & 0 deletions src/test/jtx/JTx.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ struct JTx
bool fill_fee = true;
bool fill_seq = true;
bool fill_sig = true;
bool fill_netid = true;
std::shared_ptr<STTx const> stx;
std::function<void(Env&, JTx&)> signer;

Expand Down
9 changes: 6 additions & 3 deletions src/test/jtx/impl/Env.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -493,9 +493,12 @@ Env::autofill(JTx& jt)
if (jt.fill_seq)
jtx::fill_seq(jv, *current());

uint32_t networkID = app().config().NETWORK_ID;
if (!jv.isMember(jss::NetworkID) && networkID > 1024)
jv[jss::NetworkID] = std::to_string(networkID);
if (jt.fill_netid)
{
uint32_t networkID = app().config().NETWORK_ID;
if (!jv.isMember(jss::NetworkID) && networkID > 1024)
jv[jss::NetworkID] = std::to_string(networkID);
}

// Must come last
try
Expand Down
31 changes: 31 additions & 0 deletions src/test/rpc/JSONRPC_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2049,6 +2049,7 @@ class JSONRPC_test : public beast::unit_test::suite
void
testBadRpcCommand()
{
testcase("bad RPC command");
test::jtx::Env env(*this);
Json::Value const result{
env.rpc("bad_command", R"({"MakingThisUp": 0})")};
Expand All @@ -2061,6 +2062,7 @@ class JSONRPC_test : public beast::unit_test::suite
void
testAutoFillFees()
{
testcase("autofill fees");
test::jtx::Env env(*this);
auto ledger = env.current();
auto const& feeTrack = env.app().getFeeTrack();
Expand Down Expand Up @@ -2207,6 +2209,7 @@ class JSONRPC_test : public beast::unit_test::suite
void
testAutoFillEscalatedFees()
{
testcase("autofill escalated fees");
using namespace test::jtx;
Env env{*this, envconfig([](std::unique_ptr<Config> cfg) {
cfg->loadFromString("[" SECTION_SIGNING_SUPPORT "]\ntrue");
Expand Down Expand Up @@ -2538,6 +2541,32 @@ class JSONRPC_test : public beast::unit_test::suite
}
}

void
testAutoFillNetworkID()
{
testcase("autofill NetworkID");
using namespace test::jtx;
Env env{*this, envconfig([&](std::unique_ptr<Config> cfg) {
cfg->NETWORK_ID = 1025;
return cfg;
})};

{
Json::Value toSign;
toSign[jss::tx_json] = noop(env.master);

BEAST_EXPECT(!toSign[jss::tx_json].isMember(jss::NetworkID));
toSign[jss::secret] = "masterpassphrase";
auto rpcResult = env.rpc("json", "sign", to_string(toSign));
auto result = rpcResult[jss::result];

BEAST_EXPECT(!RPC::contains_error(result));
BEAST_EXPECT(
result[jss::tx_json].isMember(jss::NetworkID) &&
result[jss::tx_json][jss::NetworkID] == 1025);
}
}

// A function that can be called as though it would process a transaction.
static void
fakeProcessTransaction(
Expand All @@ -2552,6 +2581,7 @@ class JSONRPC_test : public beast::unit_test::suite
void
testTransactionRPC()
{
testcase("sign/submit RPCs");
using namespace std::chrono_literals;
// Use jtx to set up a ledger so the tests will do the right thing.
test::jtx::Account const a{"a"}; // rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA
Expand Down Expand Up @@ -2678,6 +2708,7 @@ class JSONRPC_test : public beast::unit_test::suite
testBadRpcCommand();
testAutoFillFees();
testAutoFillEscalatedFees();
testAutoFillNetworkID();
testTransactionRPC();
}
};
Expand Down
155 changes: 141 additions & 14 deletions src/test/rpc/Simulate_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,13 +93,17 @@ class Simulate_test : public beast::unit_test::suite
validate,
bool testSerialized = true)
{
env.close();

Json::Value params;
params[jss::tx_json] = tx;
validate(env.rpc("json", "simulate", to_string(params)), tx);

params[jss::binary] = true;
validate(env.rpc("json", "simulate", to_string(params)), tx);
validate(env.rpc("simulate", to_string(tx)), tx);
validate(env.rpc("simulate", to_string(tx), "binary"), tx);

if (testSerialized)
{
// This cannot be tested in the multisign autofill scenario
Expand All @@ -119,6 +123,10 @@ class Simulate_test : public beast::unit_test::suite
validate(env.rpc("simulate", tx_blob), tx);
validate(env.rpc("simulate", tx_blob, "binary"), tx);
}

BEAST_EXPECTS(
env.current()->txCount() == 0,
std::to_string(env.current()->txCount()));
}

Json::Value
Expand Down Expand Up @@ -235,6 +243,58 @@ class Simulate_test : public beast::unit_test::suite
resp[jss::result][jss::error_message] ==
"Invalid field 'tx_json', not object.");
}
{
// `seed` field included
Json::Value params = Json::objectValue;
params[jss::seed] = "doesnt_matter";
Json::Value tx_json = Json::objectValue;
tx_json[jss::TransactionType] = jss::AccountSet;
tx_json[jss::Account] = env.master.human();
params[jss::tx_json] = tx_json;
auto const resp = env.rpc("json", "simulate", to_string(params));
BEAST_EXPECT(
resp[jss::result][jss::error_message] ==
"Invalid field 'seed'.");
}
{
// `secret` field included
Json::Value params = Json::objectValue;
params[jss::secret] = "doesnt_matter";
Json::Value tx_json = Json::objectValue;
tx_json[jss::TransactionType] = jss::AccountSet;
tx_json[jss::Account] = env.master.human();
params[jss::tx_json] = tx_json;
auto const resp = env.rpc("json", "simulate", to_string(params));
BEAST_EXPECT(
resp[jss::result][jss::error_message] ==
"Invalid field 'secret'.");
}
{
// `seed_hex` field included
Json::Value params = Json::objectValue;
params[jss::seed_hex] = "doesnt_matter";
Json::Value tx_json = Json::objectValue;
tx_json[jss::TransactionType] = jss::AccountSet;
tx_json[jss::Account] = env.master.human();
params[jss::tx_json] = tx_json;
auto const resp = env.rpc("json", "simulate", to_string(params));
BEAST_EXPECT(
resp[jss::result][jss::error_message] ==
"Invalid field 'seed_hex'.");
}
{
// `passphrase` field included
Json::Value params = Json::objectValue;
params[jss::passphrase] = "doesnt_matter";
Json::Value tx_json = Json::objectValue;
tx_json[jss::TransactionType] = jss::AccountSet;
tx_json[jss::Account] = env.master.human();
params[jss::tx_json] = tx_json;
auto const resp = env.rpc("json", "simulate", to_string(params));
BEAST_EXPECT(
resp[jss::result][jss::error_message] ==
"Invalid field 'passphrase'.");
}
{
// Invalid transaction
Json::Value params = Json::objectValue;
Expand Down Expand Up @@ -412,7 +472,10 @@ class Simulate_test : public beast::unit_test::suite
testcase("Successful transaction");

using namespace jtx;
Env env(*this);
Env env{*this, envconfig([&](std::unique_ptr<Config> cfg) {
cfg->NETWORK_ID = 0;
return cfg;
})};
static auto const newDomain = "123ABC";

{
Expand Down Expand Up @@ -473,8 +536,6 @@ class Simulate_test : public beast::unit_test::suite

// test without autofill
testTx(env, tx, validateOutput);

// TODO: check that the ledger wasn't affected
}
}

Expand Down Expand Up @@ -523,8 +584,6 @@ class Simulate_test : public beast::unit_test::suite

// test without autofill
testTx(env, tx, testSimulation);

// TODO: check that the ledger wasn't affected
}
}

Expand Down Expand Up @@ -604,8 +663,6 @@ class Simulate_test : public beast::unit_test::suite

// test without autofill
testTx(env, tx, testSimulation);

// TODO: check that the ledger wasn't affected
}
}

Expand All @@ -625,6 +682,7 @@ class Simulate_test : public beast::unit_test::suite

// set up valid multisign
env(signers(alice, 1, {{becky, 1}, {carol, 1}}));
env.close();

{
auto validateOutput = [&](Json::Value const& resp,
Expand Down Expand Up @@ -662,7 +720,7 @@ class Simulate_test : public beast::unit_test::suite
BEAST_EXPECT(finalFields[sfDomain] == newDomain);
}
}
BEAST_EXPECT(metadata[sfTransactionIndex.jsonName] == 1);
BEAST_EXPECT(metadata[sfTransactionIndex.jsonName] == 0);
BEAST_EXPECT(
metadata[sfTransactionResult.jsonName] == "tesSUCCESS");
}
Expand Down Expand Up @@ -697,8 +755,6 @@ class Simulate_test : public beast::unit_test::suite

// test without autofill
testTx(env, tx, validateOutput);

// TODO: check that the ledger wasn't affected
}
}

Expand Down Expand Up @@ -754,8 +810,6 @@ class Simulate_test : public beast::unit_test::suite

// test without autofill
testTx(env, tx, testSimulation);

// TODO: check that the ledger wasn't affected
}
}

Expand Down Expand Up @@ -825,8 +879,6 @@ class Simulate_test : public beast::unit_test::suite

// test without autofill
testTx(env, tx, validateOutput);

// TODO: check that the ledger wasn't affected
}
}

Expand Down Expand Up @@ -948,6 +1000,80 @@ class Simulate_test : public beast::unit_test::suite
BEAST_EXPECT(ownerCount(env, subject) == 0);
}

void
testSuccessfulTransactionNetworkID()
{
testcase("Successful transaction with a custom network ID");

using namespace jtx;
Env env{*this, envconfig([&](std::unique_ptr<Config> cfg) {
cfg->NETWORK_ID = 1025;
return cfg;
})};
static auto const newDomain = "123ABC";

{
auto validateOutput = [&](Json::Value const& resp,
Json::Value const& tx) {
auto result = resp[jss::result];
checkBasicReturnValidity(
result, tx, 1, env.current()->fees().base);

BEAST_EXPECT(result[jss::engine_result] == "tesSUCCESS");
BEAST_EXPECT(result[jss::engine_result_code] == 0);
BEAST_EXPECT(
result[jss::engine_result_message] ==
"The simulated transaction would have been applied.");

if (BEAST_EXPECT(
result.isMember(jss::meta) ||
result.isMember(jss::meta_blob)))
{
Json::Value const metadata = getJsonMetadata(result);

if (BEAST_EXPECT(
metadata.isMember(sfAffectedNodes.jsonName)))
{
BEAST_EXPECT(
metadata[sfAffectedNodes.jsonName].size() == 1);
auto node = metadata[sfAffectedNodes.jsonName][0u];
if (BEAST_EXPECT(
node.isMember(sfModifiedNode.jsonName)))
{
auto modifiedNode = node[sfModifiedNode];
BEAST_EXPECT(
modifiedNode[sfLedgerEntryType] ==
"AccountRoot");
auto finalFields = modifiedNode[sfFinalFields];
BEAST_EXPECT(finalFields[sfDomain] == newDomain);
}
}
BEAST_EXPECT(metadata[sfTransactionIndex.jsonName] == 0);
BEAST_EXPECT(
metadata[sfTransactionResult.jsonName] == "tesSUCCESS");
}
};

Json::Value tx;

tx[jss::Account] = env.master.human();
tx[jss::TransactionType] = jss::AccountSet;
tx[sfDomain] = newDomain;

// test with autofill
testTx(env, tx, validateOutput);

tx[sfSigningPubKey] = "";
tx[sfTxnSignature] = "";
tx[sfSequence] = 1;
tx[sfFee] = env.current()->fees().base.jsonClipped().asString();
tx[sfNetworkID] = 1025;

// test without autofill
testTx(env, tx, validateOutput);
}
}

public:
void
run() override
Expand All @@ -961,6 +1087,7 @@ class Simulate_test : public beast::unit_test::suite
testTransactionSigningFailure();
testMultisignedBadPubKey();
testDeleteExpiredCredentials();
testSuccessfulTransactionNetworkID();
}
};

Expand Down
7 changes: 7 additions & 0 deletions src/xrpld/rpc/detail/TransactionSign.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,13 @@ transactionPreProcessImpl(

if (!tx_json.isMember(jss::Flags))
tx_json[jss::Flags] = tfFullyCanonicalSig;

if (!tx_json.isMember(jss::NetworkID))
{
auto const networkId = app.config().NETWORK_ID;
if (networkId > 1024)
tx_json[jss::NetworkID] = to_string(networkId);
}
}

{
Expand Down
Loading

0 comments on commit d9e4009

Please sign in to comment.