Skip to content

Commit

Permalink
wallet: fix bug in RPC send options
Browse files Browse the repository at this point in the history
when empty, options were not being populated by arguments of the same name

found while adding test coverage in 603c005
  • Loading branch information
jonatack committed Nov 11, 2020
1 parent 155bf91 commit 3f72791
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 15 deletions.
2 changes: 1 addition & 1 deletion src/wallet/rpcwallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4087,7 +4087,7 @@ static RPCHelpMan send()
if (!wallet) return NullUniValue;
CWallet* const pwallet = wallet.get();

UniValue options = request.params[3];
UniValue options{request.params[3].isNull() ? UniValue::VOBJ : request.params[3]};
if (options.exists("feeRate") || options.exists("fee_rate") || options.exists("estimate_mode") || options.exists("conf_target")) {
if (!request.params[1].isNull() || !request.params[2].isNull()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Use either conf_target and estimate_mode or the options dictionary to control fee rate");
Expand Down
29 changes: 15 additions & 14 deletions test/functional/wallet_send.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,15 @@
"""Test the send RPC command."""

from decimal import Decimal, getcontext
from itertools import product

from test_framework.authproxy import JSONRPCException
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
assert_equal,
assert_fee_amount,
assert_greater_than,
assert_raises_rpc_error
assert_raises_rpc_error,
)

class WalletSendTest(BitcoinTestFramework):
Expand Down Expand Up @@ -271,8 +273,9 @@ def run_test(self):
fee = self.nodes[1].decodepsbt(res["psbt"])["fee"]
assert_fee_amount(fee, Decimal(len(res["hex"]) / 2), Decimal("0.00003"))

# TODO: This test should pass with all modes, e.g. with the next line uncommented, for consistency with the other explicit feerate RPCs.
# for mode in ["unset", "economical", "conservative", "btc/kb", "sat/b"]:
for target, mode in product([-1, 0, 1009], ["economical", "conservative"]):
self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=target, estimate_mode=mode,
expect_error=(-8, "Invalid conf_target, must be between 1 and 1008"))
for mode in ["btc/kb", "sat/b"]:
self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=-1, estimate_mode=mode,
expect_error=(-3, "Amount out of range"))
Expand All @@ -282,17 +285,15 @@ def run_test(self):
for mode in ["foo", Decimal("3.141592")]:
self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=0.1, estimate_mode=mode,
expect_error=(-8, "Invalid estimate_mode parameter"))
# TODO: these 2 equivalent sends with an invalid estimate_mode arg should both fail, but they do not...why?
# self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=0.1, arg_estimate_mode=mode,
# expect_error=(-8, "Invalid estimate_mode parameter"))
# assert_raises_rpc_error(-8, "Invalid estimate_mode parameter", lambda: w0.send({w1.getnewaddress(): 1}, 0.1, mode))

# TODO: These tests should pass for consistency with the other explicit feerate RPCs, but they do not.
# for mode in ["unset", "economical", "conservative", "btc/kb", "sat/b"]:
# self.log.debug("{}".format(mode))
# for k, v in {"string": "", "object": {"foo": "bar"}}.items():
# self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=v, estimate_mode=mode,
# expect_error=(-3, "Expected type number for conf_target, got {}".format(k)))
self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=0.1, arg_estimate_mode=mode,
expect_error=(-8, "Invalid estimate_mode parameter"))
assert_raises_rpc_error(-8, "Invalid estimate_mode parameter", lambda: w0.send({w1.getnewaddress(): 1}, 0.1, mode))

for mode in ["economical", "conservative", "btc/kb", "sat/b"]:
self.log.debug("{}".format(mode))
for k, v in {"string": "true", "object": {"foo": "bar"}}.items():
self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=v, estimate_mode=mode,
expect_error=(-3, "Expected type number for conf_target, got {}".format(k)))

# TODO: error should use sat/B instead of BTC/kB if sat/B is selected.
# Test setting explicit fee rate just below the minimum.
Expand Down

0 comments on commit 3f72791

Please sign in to comment.