From 7728f69100bb275bf9b8b13ab3369df6333c6beb Mon Sep 17 00:00:00 2001 From: Edward Hennis Date: Fri, 4 Dec 2015 19:14:24 -0500 Subject: [PATCH] Allow fractional fee multipliers (RIPD-626): * Auto-fill fee maximum is `base * fee_mult_max / fee_div_max`. * `fee_div_max` defaults to 1 to preserve backward compatibility. --- src/ripple/protocol/JsonFields.h | 1 + src/ripple/rpc/impl/TransactionSign.cpp | 21 +++- src/ripple/rpc/impl/TransactionSign.h | 8 +- src/ripple/rpc/impl/Tuning.h | 1 + src/ripple/rpc/tests/JSONRPC.test.cpp | 156 ++++++++++++++++++++++++ 5 files changed, 184 insertions(+), 3 deletions(-) diff --git a/src/ripple/protocol/JsonFields.h b/src/ripple/protocol/JsonFields.h index a28fc335f..7e0f8284a 100644 --- a/src/ripple/protocol/JsonFields.h +++ b/src/ripple/protocol/JsonFields.h @@ -158,6 +158,7 @@ JSS ( feature ); // in: Feature JSS ( features ); // out: Feature JSS ( fee ); // out: NetworkOPs, Peers JSS ( fee_base ); // out: NetworkOPs +JSS ( fee_div_max ); // in: TransactionSign JSS ( fee_mult_max ); // in: TransactionSign JSS ( fee_ref ); // out: NetworkOPs JSS ( fetch_pack ); // out: NetworkOPs diff --git a/src/ripple/rpc/impl/TransactionSign.cpp b/src/ripple/rpc/impl/TransactionSign.cpp index 18317392d..009725f3a 100644 --- a/src/ripple/rpc/impl/TransactionSign.cpp +++ b/src/ripple/rpc/impl/TransactionSign.cpp @@ -631,6 +631,7 @@ Json::Value checkFee ( return RPC::missing_field_error ("tx_json.Fee"); int mult = Tuning::defaultAutoFillFeeMultiplier; + int div = Tuning::defaultAutoFillFeeDivisor; if (request.isMember (jss::fee_mult_max)) { if (request[jss::fee_mult_max].isNumeric ()) @@ -643,6 +644,21 @@ Json::Value checkFee ( RPC::expected_field_message (jss::fee_mult_max, "a number")); } } + if (request.isMember(jss::fee_div_max)) + { + if (request[jss::fee_div_max].isNumeric()) + { + div = request[jss::fee_div_max].asInt(); + if (div == 0) + return RPC::make_error(rpcINVALID_PARAMS, + RPC::expected_field_message(jss::fee_div_max, "non-zero")); + } + else + { + return RPC::make_error(rpcHIGH_FEE, + RPC::expected_field_message(jss::fee_div_max, "a number")); + } + } // Default fee in fee units. std::uint64_t const feeDefault = config.TRANSACTION_FEE_BASE; @@ -652,8 +668,9 @@ Json::Value checkFee ( feeTrack.scaleFeeLoad (feeDefault, ledger->fees().base, ledger->fees().units, isUnlimited (role)); - std::uint64_t const limit = mult * feeTrack.scaleFeeBase ( - feeDefault, ledger->fees().base, ledger->fees().units); + auto const limit = mulDivThrow(feeTrack.scaleFeeBase ( + feeDefault, ledger->fees().base, ledger->fees().units), + mult, div); if (fee > limit) { diff --git a/src/ripple/rpc/impl/TransactionSign.h b/src/ripple/rpc/impl/TransactionSign.h index 5615dd10c..12806781d 100644 --- a/src/ripple/rpc/impl/TransactionSign.h +++ b/src/ripple/rpc/impl/TransactionSign.h @@ -44,9 +44,15 @@ namespace RPC { wants the fee filled in. "fee_mult_max" A multiplier applied to the current ledger's transaction - fee that caps the maximum the fee server should auto fill. + fee that caps the maximum fee the server should auto fill. If this optional field is not specified, then a default multiplier is used. + "fee_div_max" A divider applied to the current ledger's transaction + fee that caps the maximum fee the server should auto fill. + If this optional field is not specified, then a default + divider (1) is used. "fee_mult_max" and "fee_div_max" + are both used such that the maximum fee will be + `base * fee_mult_max / fee_div_max` as an integer. @param tx The JSON corresponding to the transaction to fill in. @param ledger A ledger for retrieving the current fee schedule. diff --git a/src/ripple/rpc/impl/Tuning.h b/src/ripple/rpc/impl/Tuning.h index 145c8de8f..e38f41d77 100644 --- a/src/ripple/rpc/impl/Tuning.h +++ b/src/ripple/rpc/impl/Tuning.h @@ -48,6 +48,7 @@ static LimitRange const bookOffers = {0, 0, 400}; static LimitRange const noRippleCheck = {10, 300, 400}; static int const defaultAutoFillFeeMultiplier = 10; +static int const defaultAutoFillFeeDivisor = 1; static int const maxPathfindsInProgress = 2; static int const maxPathfindJobCount = 50; static int const maxJobQueueClients = 500; diff --git a/src/ripple/rpc/tests/JSONRPC.test.cpp b/src/ripple/rpc/tests/JSONRPC.test.cpp index 2fa2c359d..f81b6519b 100644 --- a/src/ripple/rpc/tests/JSONRPC.test.cpp +++ b/src/ripple/rpc/tests/JSONRPC.test.cpp @@ -143,6 +143,27 @@ R"({ "Missing field 'tx_json.Fee'.", "Missing field 'tx_json.SigningPubKey'."}}, +{ "Add 'fee_mult_max' and 'fee_div_max' field.", +R"({ + "command": "doesnt_matter", + "account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "secret": "masterpassphrase", + "fee_mult_max": 7, + "fee_div_max": 4, + "tx_json": { + "Sequence": 0, + "Account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "Amount": "1000000000", + "Destination": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", + "TransactionType": "Payment" + } +})", +{ +"", +"", +"Missing field 'tx_json.Fee'.", +"Missing field 'tx_json.SigningPubKey'."}}, + { "fee_mult_max is ignored if 'Fee' is present.", R"({ "command": "doesnt_matter", @@ -164,6 +185,28 @@ R"({ "A Signer may not be the transaction's Account (rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh).", "Missing field 'tx_json.SigningPubKey'."}}, +{ "fee_div_max is ignored if 'Fee' is present.", +R"({ + "command": "doesnt_matter", + "account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "secret": "masterpassphrase", + "fee_mult_max": 100, + "fee_div_max": 1000, + "tx_json": { + "Sequence": 0, + "Fee": 10, + "Account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "Amount": "1000000000", + "Destination": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", + "TransactionType": "Payment" + } +})", +{ +"", +"", +"A Signer may not be the transaction's Account (rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh).", +"Missing field 'tx_json.SigningPubKey'."}}, + { "Invalid 'fee_mult_max' field.", R"({ "command": "doesnt_matter", @@ -184,6 +227,27 @@ R"({ "Missing field 'tx_json.Fee'.", "Missing field 'tx_json.SigningPubKey'."}}, +{ "Invalid 'fee_div_max' field.", +R"({ + "command": "doesnt_matter", + "account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "secret": "masterpassphrase", + "fee_mult_max": 5, + "fee_div_max": "NotAFeeMultiplier", + "tx_json": { + "Sequence": 0, + "Account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "Amount": "1000000000", + "Destination": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", + "TransactionType": "Payment" + } +})", +{ +"Invalid field 'fee_div_max', not a number.", +"Invalid field 'fee_div_max', not a number.", +"Missing field 'tx_json.Fee'.", +"Missing field 'tx_json.SigningPubKey'."}}, + { "Invalid value for 'fee_mult_max' field.", R"({ "command": "doesnt_matter", @@ -204,6 +268,48 @@ R"({ "Missing field 'tx_json.Fee'.", "Missing field 'tx_json.SigningPubKey'."}}, +{ "Invalid value for 'fee_div_max' field.", +R"({ + "command": "doesnt_matter", + "account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "secret": "masterpassphrase", + "fee_mult_max": 4, + "fee_div_max": 7, + "tx_json": { + "Sequence": 0, + "Account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "Amount": "1000000000", + "Destination": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", + "TransactionType": "Payment" + } +})", +{ +"Fee of 10 exceeds the requested tx limit of 5", +"Fee of 10 exceeds the requested tx limit of 5", +"Missing field 'tx_json.Fee'.", +"Missing field 'tx_json.SigningPubKey'."}}, + +{ "Invalid zero value for 'fee_div_max' field.", +R"({ + "command": "doesnt_matter", + "account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "secret": "masterpassphrase", + "fee_mult_max": 4, + "fee_div_max": 0, + "tx_json": { + "Sequence": 0, + "Account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "Amount": "1000000000", + "Destination": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", + "TransactionType": "Payment" + } +})", +{ +"Invalid field 'fee_div_max', not non-zero.", +"Invalid field 'fee_div_max', not non-zero.", +"Missing field 'tx_json.Fee'.", +"Missing field 'tx_json.SigningPubKey'."}}, + { "Missing 'Amount'.", R"({ "command": "doesnt_matter", @@ -1619,6 +1725,18 @@ public: expect (! RPC::contains_error (result), "Legal checkFee"); } + { + Json::Value req; + Json::Reader().parse( + "{ \"fee_mult_max\" : 3, \"fee_div_max\" : 2, " + "\"tx_json\" : { } } ", req); + Json::Value result = + checkFee(req, Role::ADMIN, true, + env.app().config(), feeTrack, ledger); + + expect(!RPC::contains_error(result), "Legal checkFee"); + } + { Json::Value req; Json::Reader ().parse ( @@ -1629,6 +1747,44 @@ public: expect (RPC::contains_error (result), "Invalid checkFee"); } + + { + // 3/6 = 1/2, but use the bigger number make sure + // we're dividing. + Json::Value req; + Json::Reader().parse( + "{ \"fee_mult_max\" : 3, \"fee_div_max\" : 6, " + "\"tx_json\" : { } } ", req); + Json::Value result = + checkFee(req, Role::ADMIN, true, + env.app().config(), feeTrack, ledger); + + expect(RPC::contains_error(result), "Invalid checkFee"); + } + + { + Json::Value req; + Json::Reader().parse( + "{ \"fee_mult_max\" : 0, \"fee_div_max\" : 2, " + "\"tx_json\" : { } } ", req); + Json::Value result = + checkFee(req, Role::ADMIN, true, + env.app().config(), feeTrack, ledger); + + expect(RPC::contains_error(result), "Invalid checkFee"); + } + + { + Json::Value req; + Json::Reader().parse( + "{ \"fee_mult_max\" : 10, \"fee_div_max\" : 0, " + "\"tx_json\" : { } } ", req); + Json::Value result = + checkFee(req, Role::ADMIN, true, + env.app().config(), feeTrack, ledger); + + expect(RPC::contains_error(result), "Divide by 0"); + } } // A function that can be called as though it would process a transaction.