mirror of
https://github.com/XRPLF/rippled.git
synced 2025-12-06 17:27:55 +00:00
APIv2(DeliverMax): add alias for Amount in Payment transactions (#4733)
Using the "Amount" field in Payment transactions can cause incorrect interpretation. There continue to be problems from the use of this field. "Amount" is rarely the correct field to use; instead, "delivered_amount" (or "DeliveredAmount") should be used. Rename the "Amount" field to "DeliverMax", a less misleading name. With api_version: 2, remove the "Amount" field from Payment transactions. - Input: "DeliverMax" in `tx_json` is an alias for "Amount" - sign - submit (in sign-and-submit mode) - submit_multisigned - sign_for - Output: Add "DeliverMax" where transactions are provided by the API - ledger - tx - tx_history - account_tx - transaction_entry - subscribe (transactions stream) - Output: Remove "Amount" from API version 2 Fix #3484 Fix #3902
This commit is contained in:
Bronek Kozicki
@@ -119,14 +119,23 @@ class AccountTx_test : public beast::unit_test::suite
|
||||
// Ledger 3 has the two txs associated with funding the account
|
||||
// All other ledgers have no txs
|
||||
|
||||
auto hasTxs = [](Json::Value const& j) {
|
||||
auto hasTxs = [apiVersion](Json::Value const& j) {
|
||||
return j.isMember(jss::result) &&
|
||||
(j[jss::result][jss::status] == "success") &&
|
||||
(j[jss::result][jss::transactions].size() == 2) &&
|
||||
(j[jss::result][jss::transactions][0u][jss::tx]
|
||||
[jss::TransactionType] == jss::AccountSet) &&
|
||||
(j[jss::result][jss::transactions][1u][jss::tx]
|
||||
[jss::TransactionType] == jss::Payment);
|
||||
[jss::TransactionType] == jss::Payment) &&
|
||||
(j[jss::result][jss::transactions][1u][jss::tx]
|
||||
[jss::DeliverMax] == "10000000010") &&
|
||||
((apiVersion > 1 &&
|
||||
!j[jss::result][jss::transactions][1u][jss::tx].isMember(
|
||||
jss::Amount)) ||
|
||||
(apiVersion <= 1 &&
|
||||
j[jss::result][jss::transactions][1u][jss::tx][jss::Amount] ==
|
||||
j[jss::result][jss::transactions][1u][jss::tx]
|
||||
[jss::DeliverMax]));
|
||||
};
|
||||
|
||||
auto noTxs = [](Json::Value const& j) {
|
||||
|
||||
@@ -1035,6 +1035,26 @@ static constexpr TxnTestData txnTestArray[] = {
|
||||
"Missing field 'tx_json.Destination'.",
|
||||
"Missing field 'tx_json.Destination'."}}},
|
||||
|
||||
{"Missing 'Destination' in sign_for, use DeliverMax",
|
||||
__LINE__,
|
||||
R"({
|
||||
"command": "doesnt_matter",
|
||||
"account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
|
||||
"secret": "masterpassphrase",
|
||||
"tx_json": {
|
||||
"Account": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA",
|
||||
"DeliverMax": "1000000000",
|
||||
"Fee": 50,
|
||||
"Sequence": 0,
|
||||
"SigningPubKey": "",
|
||||
"TransactionType": "Payment"
|
||||
}
|
||||
})",
|
||||
{{"Missing field 'tx_json.Destination'.",
|
||||
"Missing field 'tx_json.Destination'.",
|
||||
"Missing field 'tx_json.Destination'.",
|
||||
"Missing field 'tx_json.Destination'."}}},
|
||||
|
||||
{"Missing 'Fee' in sign_for.",
|
||||
__LINE__,
|
||||
R"({
|
||||
@@ -1692,6 +1712,34 @@ static constexpr TxnTestData txnTestArray[] = {
|
||||
"Missing field 'account'.",
|
||||
"Invalid field 'tx_json.Amount'."}}},
|
||||
|
||||
{"Invalid DeliverMax in submit_multisigned Payment.",
|
||||
__LINE__,
|
||||
R"({
|
||||
"command": "submit_multisigned",
|
||||
"tx_json": {
|
||||
"Account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
|
||||
"DeliverMax": "NotANumber",
|
||||
"Destination": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA",
|
||||
"Fee": 50,
|
||||
"Sequence": 0,
|
||||
"Signers": [
|
||||
{
|
||||
"Signer": {
|
||||
"Account": "rPcNzota6B8YBokhYtcTNqQVCngtbnWfux",
|
||||
"TxnSignature": "3045022100F9ED357606932697A4FAB2BE7F222C21DD93CA4CFDD90357AADD07465E8457D6022038173193E3DFFFB5D78DD738CC0905395F885DA65B98FDB9793901FE3FD26ECE",
|
||||
"SigningPubKey": "02FE36A690D6973D55F88553F5D2C4202DE75F2CF8A6D0E17C70AC223F044501F8"
|
||||
}
|
||||
}
|
||||
],
|
||||
"SigningPubKey": "",
|
||||
"TransactionType": "Payment"
|
||||
}
|
||||
})",
|
||||
{{"Missing field 'secret'.",
|
||||
"Missing field 'secret'.",
|
||||
"Missing field 'account'.",
|
||||
"Invalid field 'tx_json.Amount'."}}},
|
||||
|
||||
{"No build_path in submit_multisigned.",
|
||||
__LINE__,
|
||||
R"({
|
||||
@@ -1905,6 +1953,72 @@ static constexpr TxnTestData txnTestArray[] = {
|
||||
"A Signer may not be the transaction's Account "
|
||||
"(rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh)."}}},
|
||||
|
||||
{"Empty Signers array in submit_multisigned, use DeliverMax",
|
||||
__LINE__,
|
||||
R"({
|
||||
"command": "submit_multisigned",
|
||||
"tx_json": {
|
||||
"Account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
|
||||
"DeliverMax": "10000000",
|
||||
"Destination": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA",
|
||||
"Fee": 50,
|
||||
"Sequence": 0,
|
||||
"Signers": [
|
||||
],
|
||||
"SigningPubKey": "",
|
||||
"TransactionType": "Payment"
|
||||
}
|
||||
})",
|
||||
{{"Missing field 'secret'.",
|
||||
"Missing field 'secret'.",
|
||||
"Missing field 'account'.",
|
||||
"tx_json.Signers array may not be empty."}}},
|
||||
|
||||
{"Empty Signers array in submit_multisigned, use DeliverMax and Amount",
|
||||
__LINE__,
|
||||
R"({
|
||||
"command": "submit_multisigned",
|
||||
"tx_json": {
|
||||
"Account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
|
||||
"Amount": "10000000",
|
||||
"DeliverMax": "10000000",
|
||||
"Destination": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA",
|
||||
"Fee": 50,
|
||||
"Sequence": 0,
|
||||
"Signers": [
|
||||
],
|
||||
"SigningPubKey": "",
|
||||
"TransactionType": "Payment"
|
||||
}
|
||||
})",
|
||||
{{"Missing field 'secret'.",
|
||||
"Missing field 'secret'.",
|
||||
"Missing field 'account'.",
|
||||
"tx_json.Signers array may not be empty."}}},
|
||||
|
||||
{"Payment cannot specify different DeliverMax and Amount.",
|
||||
__LINE__,
|
||||
R"({
|
||||
"command": "doesnt_matter",
|
||||
"account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
|
||||
"secret": "masterpassphrase",
|
||||
"debug_signing": 0,
|
||||
"tx_json": {
|
||||
"Account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
|
||||
"Amount": "1000000000",
|
||||
"DeliverMax": "1000000020",
|
||||
"Destination": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA",
|
||||
"Fee": 50,
|
||||
"Sequence": 0,
|
||||
"SigningPubKey": "",
|
||||
"TransactionType": "Payment"
|
||||
}
|
||||
})",
|
||||
{{"Cannot specify differing 'Amount' and 'DeliverMax'",
|
||||
"Cannot specify differing 'Amount' and 'DeliverMax'",
|
||||
"Cannot specify differing 'Amount' and 'DeliverMax'",
|
||||
"Cannot specify differing 'Amount' and 'DeliverMax'"}}},
|
||||
|
||||
};
|
||||
|
||||
class JSONRPC_test : public beast::unit_test::suite
|
||||
|
||||
@@ -194,8 +194,16 @@ public:
|
||||
// Check stream update for payment transaction
|
||||
BEAST_EXPECT(wsc->findMsg(5s, [&](auto const& jv) {
|
||||
return jv[jss::meta]["AffectedNodes"][1u]["CreatedNode"]
|
||||
["NewFields"][jss::Account] ==
|
||||
Account("alice").human();
|
||||
["NewFields"][jss::Account] //
|
||||
== Account("alice").human() &&
|
||||
jv[jss::transaction][jss::TransactionType] //
|
||||
== jss::Payment &&
|
||||
jv[jss::transaction][jss::DeliverMax] //
|
||||
== "10000000010" &&
|
||||
jv[jss::transaction][jss::Fee] //
|
||||
== "10" &&
|
||||
jv[jss::transaction][jss::Sequence] //
|
||||
== 1;
|
||||
}));
|
||||
|
||||
// Check stream update for accountset transaction
|
||||
@@ -211,7 +219,16 @@ public:
|
||||
// Check stream update for payment transaction
|
||||
BEAST_EXPECT(wsc->findMsg(5s, [&](auto const& jv) {
|
||||
return jv[jss::meta]["AffectedNodes"][1u]["CreatedNode"]
|
||||
["NewFields"][jss::Account] == Account("bob").human();
|
||||
["NewFields"][jss::Account] //
|
||||
== Account("bob").human() &&
|
||||
jv[jss::transaction][jss::TransactionType] //
|
||||
== jss::Payment &&
|
||||
jv[jss::transaction][jss::DeliverMax] //
|
||||
== "10000000010" &&
|
||||
jv[jss::transaction][jss::Fee] //
|
||||
== "10" &&
|
||||
jv[jss::transaction][jss::Sequence] //
|
||||
== 2;
|
||||
}));
|
||||
|
||||
// Check stream update for accountset transaction
|
||||
|
||||
@@ -17,6 +17,8 @@
|
||||
*/
|
||||
//==============================================================================
|
||||
|
||||
#include <ripple/json/json_reader.h>
|
||||
#include <ripple/json/json_value.h>
|
||||
#include <ripple/protocol/jss.h>
|
||||
#include <test/jtx.h>
|
||||
#include <test/jtx/Env.h>
|
||||
@@ -150,7 +152,7 @@ class TransactionEntry_test : public beast::unit_test::suite
|
||||
auto check_tx = [this, &env](
|
||||
int index,
|
||||
std::string const txhash,
|
||||
std::string const type = "") {
|
||||
std::string const expected_json = "") {
|
||||
// first request using ledger_index to lookup
|
||||
Json::Value const resIndex{[&env, index, &txhash]() {
|
||||
Json::Value params{Json::objectValue};
|
||||
@@ -164,13 +166,30 @@ class TransactionEntry_test : public beast::unit_test::suite
|
||||
return;
|
||||
|
||||
BEAST_EXPECT(resIndex[jss::tx_json][jss::hash] == txhash);
|
||||
if (!type.empty())
|
||||
if (!expected_json.empty())
|
||||
{
|
||||
BEAST_EXPECTS(
|
||||
resIndex[jss::tx_json][jss::TransactionType] == type,
|
||||
txhash + " is " +
|
||||
resIndex[jss::tx_json][jss::TransactionType]
|
||||
.asString());
|
||||
Json::Value expected;
|
||||
Json::Reader().parse(expected_json, expected);
|
||||
if (RPC::contains_error(expected))
|
||||
Throw<std::runtime_error>(
|
||||
"Internal JSONRPC_test error. Bad test JSON.");
|
||||
|
||||
for (auto memberIt = expected.begin();
|
||||
memberIt != expected.end();
|
||||
memberIt++)
|
||||
{
|
||||
auto const name = memberIt.memberName();
|
||||
if (BEAST_EXPECT(resIndex[jss::tx_json].isMember(name)))
|
||||
{
|
||||
auto const received = resIndex[jss::tx_json][name];
|
||||
BEAST_EXPECTS(
|
||||
received == *memberIt,
|
||||
txhash + " contains \n\"" + name + "\": " //
|
||||
+ to_string(received) //
|
||||
+ " but expected " //
|
||||
+ to_string(expected));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// second request using ledger_hash to lookup and verify
|
||||
@@ -216,8 +235,30 @@ class TransactionEntry_test : public beast::unit_test::suite
|
||||
|
||||
// these are actually AccountSet txs because fund does two txs and
|
||||
// env.tx only reports the last one
|
||||
check_tx(env.closed()->seq(), fund_1_tx);
|
||||
check_tx(env.closed()->seq(), fund_2_tx);
|
||||
check_tx(env.closed()->seq(), fund_1_tx, R"(
|
||||
{
|
||||
"Account" : "r4nmQNH4Fhjfh6cHDbvVSsBv7KySbj4cBf",
|
||||
"Fee" : "10",
|
||||
"Sequence" : 3,
|
||||
"SetFlag" : 8,
|
||||
"SigningPubKey" : "0324CAAFA2212D2AEAB9D42D481535614AED486293E1FB1380FF070C3DD7FB4264",
|
||||
"TransactionType" : "AccountSet",
|
||||
"TxnSignature" : "3044022007B35E3B99460534FF6BC3A66FBBA03591C355CC38E38588968E87CCD01BE229022071A443026DE45041B55ABB1CC76812A87EA701E475BBB7E165513B4B242D3474",
|
||||
"hash" : "F4E9DF90D829A9E8B423FF68C34413E240D8D8BB0EFD080DF08114ED398E2506"
|
||||
}
|
||||
)");
|
||||
check_tx(env.closed()->seq(), fund_2_tx, R"(
|
||||
{
|
||||
"Account" : "rGpeQzUWFu4fMhJHZ1Via5aqFC3A5twZUD",
|
||||
"Fee" : "10",
|
||||
"Sequence" : 3,
|
||||
"SetFlag" : 8,
|
||||
"SigningPubKey" : "03CFF28E067A2CCE6CC5A598C0B845CBD3F30A7863BE9C0DD55F4960EFABCCF4D0",
|
||||
"TransactionType" : "AccountSet",
|
||||
"TxnSignature" : "3045022100C8857FC0759A2AC0D2F320684691A66EAD252EAED9EF88C79791BC58BFCC9D860220421722286487DD0ED6BBA626CE6FCBDD14289F7F4726870C3465A4054C2702D7",
|
||||
"hash" : "6853CD8226A05068C951CB1F54889FF4E40C5B440DC1C5BA38F114C4E0B1E705"
|
||||
}
|
||||
)");
|
||||
|
||||
env.trust(A2["USD"](1000), A1);
|
||||
// the trust tx is actually a payment since the trust method
|
||||
@@ -231,16 +272,70 @@ class TransactionEntry_test : public beast::unit_test::suite
|
||||
boost::lexical_cast<std::string>(env.tx()->getTransactionID());
|
||||
env.close();
|
||||
|
||||
check_tx(env.closed()->seq(), trust_tx);
|
||||
check_tx(env.closed()->seq(), pay_tx, jss::Payment.c_str());
|
||||
check_tx(env.closed()->seq(), trust_tx, R"(
|
||||
{
|
||||
"Account" : "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
|
||||
"DeliverMax" : "10",
|
||||
"Destination" : "r4nmQNH4Fhjfh6cHDbvVSsBv7KySbj4cBf",
|
||||
"Fee" : "10",
|
||||
"Flags" : 2147483648,
|
||||
"Sequence" : 3,
|
||||
"SigningPubKey" : "0330E7FC9D56BB25D6893BA3F317AE5BCF33B3291BD63DB32654A313222F7FD020",
|
||||
"TransactionType" : "Payment",
|
||||
"TxnSignature" : "3044022033D9EBF7F02950AF2F6B13C07AEE641C8FEBDD540A338FCB9027A965A4AED35B02206E4E227DCC226A3456C0FEF953449D21645A24EB63CA0BB7C5B62470147FD1D1",
|
||||
"hash" : "C992D97D88FF444A1AB0C06B27557EC54B7F7DA28254778E60238BEA88E0C101"
|
||||
}
|
||||
)");
|
||||
|
||||
check_tx(
|
||||
env.closed()->seq(),
|
||||
pay_tx,
|
||||
R"(
|
||||
{
|
||||
"Account" : "rGpeQzUWFu4fMhJHZ1Via5aqFC3A5twZUD",
|
||||
"DeliverMax" :
|
||||
{
|
||||
"currency" : "USD",
|
||||
"issuer" : "rGpeQzUWFu4fMhJHZ1Via5aqFC3A5twZUD",
|
||||
"value" : "5"
|
||||
},
|
||||
"Destination" : "r4nmQNH4Fhjfh6cHDbvVSsBv7KySbj4cBf",
|
||||
"Fee" : "10",
|
||||
"Flags" : 2147483648,
|
||||
"Sequence" : 4,
|
||||
"SigningPubKey" : "03CFF28E067A2CCE6CC5A598C0B845CBD3F30A7863BE9C0DD55F4960EFABCCF4D0",
|
||||
"TransactionType" : "Payment",
|
||||
"TxnSignature" : "30450221008A722B7F16EDB2348886E88ED4EC682AE9973CC1EE0FF37C93BB2CEC821D3EDF022059E464472031BA5E0D88A93E944B6A8B8DB3E1D5E5D1399A805F615789DB0BED",
|
||||
"hash" : "988046D484ACE9F5F6A8C792D89C6EA2DB307B5DDA9864AEBA88E6782ABD0865"
|
||||
}
|
||||
)");
|
||||
|
||||
env(offer(A2, XRP(100), A2["USD"](1)));
|
||||
auto offer_tx =
|
||||
boost::lexical_cast<std::string>(env.tx()->getTransactionID());
|
||||
|
||||
env.close();
|
||||
|
||||
check_tx(env.closed()->seq(), offer_tx, jss::OfferCreate.c_str());
|
||||
check_tx(
|
||||
env.closed()->seq(),
|
||||
offer_tx,
|
||||
R"(
|
||||
{
|
||||
"Account" : "rGpeQzUWFu4fMhJHZ1Via5aqFC3A5twZUD",
|
||||
"Fee" : "10",
|
||||
"Sequence" : 5,
|
||||
"SigningPubKey" : "03CFF28E067A2CCE6CC5A598C0B845CBD3F30A7863BE9C0DD55F4960EFABCCF4D0",
|
||||
"TakerGets" :
|
||||
{
|
||||
"currency" : "USD",
|
||||
"issuer" : "rGpeQzUWFu4fMhJHZ1Via5aqFC3A5twZUD",
|
||||
"value" : "1"
|
||||
},
|
||||
"TakerPays" : "100000000",
|
||||
"TransactionType" : "OfferCreate",
|
||||
"TxnSignature" : "304502210093FC93ACB77B4E3DE3315441BD010096734859080C1797AB735EB47EBD541BD102205020BB1A7C3B4141279EE4C287C13671E2450EA78914EFD0C6DB2A18344CD4F2",
|
||||
"hash" : "5FCC1A27A7664F82A0CC4BE5766FBBB7C560D52B93AA7B550CD33B27AEC7EFFB"
|
||||
}
|
||||
)");
|
||||
}
|
||||
|
||||
public:
|
||||
|
||||
@@ -19,6 +19,7 @@
|
||||
|
||||
#include <ripple/app/rdb/backend/SQLiteDatabase.h>
|
||||
#include <ripple/protocol/ErrorCodes.h>
|
||||
#include <ripple/protocol/STBase.h>
|
||||
#include <ripple/protocol/jss.h>
|
||||
#include <ripple/rpc/CTID.h>
|
||||
#include <optional>
|
||||
@@ -692,6 +693,60 @@ class Transaction_test : public beast::unit_test::suite
|
||||
}
|
||||
}
|
||||
|
||||
void
|
||||
testRequest(FeatureBitset features)
|
||||
{
|
||||
testcase("Test Request");
|
||||
|
||||
using namespace test::jtx;
|
||||
using std::to_string;
|
||||
|
||||
const char* COMMAND = jss::tx.c_str();
|
||||
|
||||
Env env{*this};
|
||||
Account const alice{"alice"};
|
||||
Account const alie{"alie"};
|
||||
Account const gw{"gw"};
|
||||
auto const USD{gw["USD"]};
|
||||
|
||||
env.fund(XRP(1000000), alice, gw);
|
||||
env.close();
|
||||
|
||||
// AccountSet
|
||||
env(noop(alice));
|
||||
|
||||
// Payment
|
||||
env(pay(alice, gw, XRP(100)));
|
||||
|
||||
std::shared_ptr<STTx const> txn = env.tx();
|
||||
env.close();
|
||||
std::shared_ptr<STObject const> meta =
|
||||
env.closed()->txRead(env.tx()->getTransactionID()).second;
|
||||
|
||||
Json::Value expected = txn->getJson(JsonOptions::none);
|
||||
expected[jss::DeliverMax] = expected[jss::Amount];
|
||||
|
||||
auto const result =
|
||||
env.rpc(COMMAND, to_string(txn->getTransactionID()));
|
||||
BEAST_EXPECT(result[jss::result][jss::status] == jss::success);
|
||||
|
||||
for (auto memberIt = expected.begin(); memberIt != expected.end();
|
||||
memberIt++)
|
||||
{
|
||||
std::string const name = memberIt.memberName();
|
||||
if (BEAST_EXPECT(result[jss::result].isMember(name)))
|
||||
{
|
||||
auto const received = result[jss::result][name];
|
||||
BEAST_EXPECTS(
|
||||
received == *memberIt,
|
||||
"Transaction contains \n\"" + name + "\": " //
|
||||
+ to_string(received) //
|
||||
+ " but expected " //
|
||||
+ to_string(expected));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
public:
|
||||
void
|
||||
run() override
|
||||
@@ -708,6 +763,7 @@ public:
|
||||
testRangeCTIDRequest(features);
|
||||
testCTIDValidation(features);
|
||||
testCTIDRPC(features);
|
||||
testRequest(features);
|
||||
}
|
||||
};
|
||||
|
||||
|
||||
Reference in New Issue
Block a user