From af53da0225a7f14e6412ed2bb25c139fb44141ed Mon Sep 17 00:00:00 2001 From: Simon Date: Wed, 30 Nov 2016 14:18:29 -0800 Subject: [PATCH] Closes #1903. Add fee parameter to z_sendmany. --- doc/payment-api.md | 2 +- qa/rpc-tests/wallet_protectcoinbase.py | 31 +++++++++++++++++++++-- src/rpcclient.cpp | 1 + src/test/rpc_wallet_tests.cpp | 23 ++++++++++++++++- src/wallet/asyncrpcoperation_sendmany.cpp | 11 +++++--- src/wallet/asyncrpcoperation_sendmany.h | 5 ++-- src/wallet/rpcwallet.cpp | 21 ++++++++++++--- 7 files changed, 81 insertions(+), 13 deletions(-) diff --git a/doc/payment-api.md b/doc/payment-api.md index 7435acf81..ccbb5725c 100644 --- a/doc/payment-api.md +++ b/doc/payment-api.md @@ -71,7 +71,7 @@ z_importwallet | filename | _Requires an unlocked wallet or an unencrypted walle Command | Parameters | Description --- | --- | --- z_listreceivedbyaddress
| zaddr [minconf=1] | Return a list of amounts received by a zaddr belonging to the node’s wallet.

Optionally set the minimum number of confirmations which a received amount must have in order to be included in the result. Use 0 to count unconfirmed transactions.

Output:
[{
“txid”: “4a0f…”,
“amount”: 0.54,
“memo”:”F0FF…”,}, {...}, {...}
] -z_sendmany
| fromaddress amounts [minconf=1] | _This is an Asynchronous RPC call_

Send funds from an address to multiple outputs. The address can be either a taddr or a zaddr.

Amounts is a list containing key/value pairs corresponding to the addresses and amount to pay. Each output address can be in taddr or zaddr format.

When sending to a zaddr, you also have the option of attaching a memo in hexadecimal format.

**NOTE:**When sending coinbase funds to a zaddr, the node's wallet does not allow any change. Put another way, spending a partial amount of a coinbase utxo is not allowed. This is not a consensus rule but a local wallet rule due to the current implementation of z_sendmany. In future, this rule may be removed.

Example of Outputs parameter:
[{“address”:”t123…”, “amount”:0.005},
,{“address”:”z010…”,”amount”:0.03, “memo”:”f508af…”}]

Optionally set the minimum number of confirmations which a private or transparent transaction must have in order to be used as an input.

The transaction fee will be determined by the node’s wallet. Any transparent change will be sent to a new transparent address. Any private change will be sent back to the zaddr being used as the source of funds.

Returns an operationid. You use the operationid value with z_getoperationstatus and z_getoperationresult to obtain the result of sending funds, which if successful, will be a txid. +z_sendmany
| fromaddress amounts [minconf=1] [fee=0.0001] | _This is an Asynchronous RPC call_

Send funds from an address to multiple outputs. The address can be either a taddr or a zaddr.

Amounts is a list containing key/value pairs corresponding to the addresses and amount to pay. Each output address can be in taddr or zaddr format.

When sending to a zaddr, you also have the option of attaching a memo in hexadecimal format.

**NOTE:**When sending coinbase funds to a zaddr, the node's wallet does not allow any change. Put another way, spending a partial amount of a coinbase utxo is not allowed. This is not a consensus rule but a local wallet rule due to the current implementation of z_sendmany. In future, this rule may be removed.

Example of Outputs parameter:
[{“address”:”t123…”, “amount”:0.005},
,{“address”:”z010…”,”amount”:0.03, “memo”:”f508af…”}]

Optionally set the minimum number of confirmations which a private or transparent transaction must have in order to be used as an input.

Optionally set a transaction fee, which by default is 0.0001 ZEC.

Any transparent change will be sent to a new transparent address. Any private change will be sent back to the zaddr being used as the source of funds.

Returns an operationid. You use the operationid value with z_getoperationstatus and z_getoperationresult to obtain the result of sending funds, which if successful, will be a txid. ### Operations diff --git a/qa/rpc-tests/wallet_protectcoinbase.py b/qa/rpc-tests/wallet_protectcoinbase.py index d072c8ce6..4073a7181 100755 --- a/qa/rpc-tests/wallet_protectcoinbase.py +++ b/qa/rpc-tests/wallet_protectcoinbase.py @@ -190,6 +190,27 @@ class WalletProtectCoinbaseTest (BitcoinTestFramework): node2balance = amount_per_recipient * num_t_recipients assert_equal(self.nodes[2].getbalance(), node2balance) + # Send will fail because fee is negative + try: + self.nodes[0].z_sendmany(myzaddr, recipients, 1, -1) + except JSONRPCException,e: + errorString = e.error['message'] + assert_equal("Invalid amount" in errorString, True) + + # Send will fail because fee is larger than MAX_MONEY + try: + self.nodes[0].z_sendmany(myzaddr, recipients, 1, Decimal('21000000.00000001')) + except JSONRPCException,e: + errorString = e.error['message'] + assert_equal("Invalid amount" in errorString, True) + + # Send will fail because fee is larger than sum of outputs + try: + self.nodes[0].z_sendmany(myzaddr, recipients, 1, (amount_per_recipient * num_t_recipients) + Decimal('0.00000001')) + except JSONRPCException,e: + errorString = e.error['message'] + assert_equal("is greater than the sum of outputs" in errorString, True) + # Send will succeed because the balance of non-coinbase utxos is 10.0 try: self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 9) @@ -208,10 +229,14 @@ class WalletProtectCoinbaseTest (BitcoinTestFramework): recipients = [] num_recipients = 3 amount_per_recipient = Decimal('0.002') + minconf = 1 + send_amount = num_recipients * amount_per_recipient + custom_fee = Decimal('0.00012345') + zbalance = self.nodes[0].z_getbalance(myzaddr) for i in xrange(0,num_recipients): newzaddr = self.nodes[2].z_getnewaddress() recipients.append({"address":newzaddr, "amount":amount_per_recipient}) - myopid = self.nodes[0].z_sendmany(myzaddr, recipients) + myopid = self.nodes[0].z_sendmany(myzaddr, recipients, minconf, custom_fee) self.wait_and_assert_operationid_status(myopid) self.sync_all() self.nodes[1].generate(1) @@ -219,7 +244,9 @@ class WalletProtectCoinbaseTest (BitcoinTestFramework): # check balances resp = self.nodes[2].z_gettotalbalance() - assert_equal(Decimal(resp["private"]), num_recipients * amount_per_recipient) + assert_equal(Decimal(resp["private"]), send_amount) + resp = self.nodes[0].z_getbalance(myzaddr) + assert_equal(Decimal(resp), zbalance - custom_fee - send_amount) if __name__ == '__main__': WalletProtectCoinbaseTest().main() diff --git a/src/rpcclient.cpp b/src/rpcclient.cpp index 2841a3793..b482a8eb9 100644 --- a/src/rpcclient.cpp +++ b/src/rpcclient.cpp @@ -103,6 +103,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "z_gettotalbalance", 0}, { "z_sendmany", 1}, { "z_sendmany", 2}, + { "z_sendmany", 3}, { "z_getoperationstatus", 0}, { "z_getoperationresult", 0}, { "z_importkey", 1 } diff --git a/src/test/rpc_wallet_tests.cpp b/src/test/rpc_wallet_tests.cpp index 2eaeb7e61..8471ebebc 100644 --- a/src/test/rpc_wallet_tests.cpp +++ b/src/test/rpc_wallet_tests.cpp @@ -820,7 +820,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_parameters) BOOST_CHECK_THROW(CallRPC("z_sendmany"), runtime_error); BOOST_CHECK_THROW(CallRPC("z_sendmany toofewargs"), runtime_error); - BOOST_CHECK_THROW(CallRPC("z_sendmany too many args here"), runtime_error); + BOOST_CHECK_THROW(CallRPC("z_sendmany just too many args here"), runtime_error); // bad from address BOOST_CHECK_THROW(CallRPC("z_sendmany " @@ -841,6 +841,27 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_parameters) " {\"address\":\"tmQP9L3s31cLsghVYf2Jb5MhKj1jRBPoeQn\", \"amount\":12.0} ]" ), runtime_error); + // invalid fee amount, cannot be negative + BOOST_CHECK_THROW(CallRPC("z_sendmany " + "tmRr6yJonqGK23UVhrKuyvTpF8qxQQjKigJ " + "[{\"address\":\"tmQP9L3s31cLsghVYf2Jb5MhKj1jRBPoeQn\", \"amount\":50.0}] " + "1 -0.0001" + ), runtime_error); + + // invalid fee amount, bigger than MAX_MONEY + BOOST_CHECK_THROW(CallRPC("z_sendmany " + "tmRr6yJonqGK23UVhrKuyvTpF8qxQQjKigJ " + "[{\"address\":\"tmQP9L3s31cLsghVYf2Jb5MhKj1jRBPoeQn\", \"amount\":50.0}] " + "1 21000001" + ), runtime_error); + + // fee amount is bigger than sum of outputs + BOOST_CHECK_THROW(CallRPC("z_sendmany " + "tmRr6yJonqGK23UVhrKuyvTpF8qxQQjKigJ " + "[{\"address\":\"tmQP9L3s31cLsghVYf2Jb5MhKj1jRBPoeQn\", \"amount\":50.0}] " + "1 50.00000001" + ), runtime_error); + // memo bigger than allowed length of ZC_MEMO_SIZE std::vector v (2 * (ZC_MEMO_SIZE+1)); // x2 for hexadecimal string format std::fill(v.begin(),v.end(), 'A'); diff --git a/src/wallet/asyncrpcoperation_sendmany.cpp b/src/wallet/asyncrpcoperation_sendmany.cpp index 9e27b971a..c416a146c 100644 --- a/src/wallet/asyncrpcoperation_sendmany.cpp +++ b/src/wallet/asyncrpcoperation_sendmany.cpp @@ -50,9 +50,12 @@ AsyncRPCOperation_sendmany::AsyncRPCOperation_sendmany( std::string fromAddress, std::vector tOutputs, std::vector zOutputs, - int minDepth) : - fromaddress_(fromAddress), t_outputs_(tOutputs), z_outputs_(zOutputs), mindepth_(minDepth) + int minDepth, + CAmount fee) : + fromaddress_(fromAddress), t_outputs_(tOutputs), z_outputs_(zOutputs), mindepth_(minDepth), fee_(fee) { + assert(fee_ > 0); + if (minDepth < 0) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Minconf cannot be negative"); } @@ -147,8 +150,8 @@ bool AsyncRPCOperation_sendmany::main_impl() { bool isSingleZaddrOutput = (t_outputs_.size()==0 && z_outputs_.size()==1); bool isMultipleZaddrOutput = (t_outputs_.size()==0 && z_outputs_.size()>=1); bool isPureTaddrOnlyTx = (isfromtaddr_ && z_outputs_.size() == 0); - CAmount minersFee = ASYNC_RPC_OPERATION_DEFAULT_MINERS_FEE; - + CAmount minersFee = fee_; + // When spending coinbase utxos, you can only specify a single zaddr as the change must go somewhere // and if there are multiple zaddrs, we don't know where to send it. if (isfromtaddr_) { diff --git a/src/wallet/asyncrpcoperation_sendmany.h b/src/wallet/asyncrpcoperation_sendmany.h index df9c3d8d2..1f8659658 100644 --- a/src/wallet/asyncrpcoperation_sendmany.h +++ b/src/wallet/asyncrpcoperation_sendmany.h @@ -16,7 +16,7 @@ #include -// TODO: Compute fee based on a heuristic, e.g. (num tx output * dust threshold) + joinsplit bytes * ? +// Default transaction fee if caller does not specify one. #define ASYNC_RPC_OPERATION_DEFAULT_MINERS_FEE 10000 using namespace libzcash; @@ -43,7 +43,7 @@ struct AsyncJoinSplitInfo class AsyncRPCOperation_sendmany : public AsyncRPCOperation { public: - AsyncRPCOperation_sendmany(std::string fromAddress, std::vector tOutputs, std::vector zOutputs, int minDepth); + AsyncRPCOperation_sendmany(std::string fromAddress, std::vector tOutputs, std::vector zOutputs, int minDepth, CAmount fee = ASYNC_RPC_OPERATION_DEFAULT_MINERS_FEE); virtual ~AsyncRPCOperation_sendmany(); // We don't want to be copied or moved around @@ -59,6 +59,7 @@ public: private: friend class TEST_FRIEND_AsyncRPCOperation_sendmany; // class for unit testing + CAmount fee_; int mindepth_; std::string fromaddress_; bool isfromtaddr_; diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index b10a08efe..3cc75daa5 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3182,9 +3182,9 @@ Value z_sendmany(const Array& params, bool fHelp) if (!EnsureWalletIsAvailable(fHelp)) return Value::null; - if (fHelp || params.size() < 2 || params.size() > 3) + if (fHelp || params.size() < 2 || params.size() > 4) throw runtime_error( - "z_sendmany \"fromaddress\" [{\"address\":... ,\"amount\":...},...] ( minconf )\n" + "z_sendmany \"fromaddress\" [{\"address\":... ,\"amount\":...},...] ( minconf ) ( fee )\n" "\nSend multiple times. Amounts are double-precision floating point numbers." "\nChange from a taddr flows to a new taddr address, while change from zaddr returns to itself." "\nWhen sending coinbase UTXOs to a zaddr, change is not allowed. The entire value of the UTXO(s) must be consumed." @@ -3199,6 +3199,8 @@ Value z_sendmany(const Array& params, bool fHelp) " \"memo\":memo (string, optional) If the address is a zaddr, raw data represented in hexadecimal string format\n" " }, ... ]\n" "3. minconf (numeric, optional, default=1) Only use funds confirmed at least this many times.\n" + "4. fee (numeric, optional, default=" + + strprintf("%s", FormatMoney(ASYNC_RPC_OPERATION_DEFAULT_MINERS_FEE)) + ") The fee amount to attach to this transaction.\n" "\nResult:\n" "\"operationid\" (string) An operationid to pass to z_getoperationstatus to get the result of the operation.\n" ); @@ -3239,6 +3241,7 @@ Value z_sendmany(const Array& params, bool fHelp) // Recipients std::vector taddrRecipients; std::vector zaddrRecipients; + CAmount nTotalOut = 0; BOOST_FOREACH(Value& output, outputs) { @@ -3294,6 +3297,8 @@ Value z_sendmany(const Array& params, bool fHelp) } else { taddrRecipients.push_back( SendManyRecipient(address, nAmount, memo) ); } + + nTotalOut += nAmount; } // Check the number of zaddr outputs does not exceed the limit. @@ -3329,9 +3334,19 @@ Value z_sendmany(const Array& params, bool fHelp) throw JSONRPCError(RPC_INVALID_PARAMETER, "Minimum number of confirmations cannot be less than 0"); } + // Fee in Zatoshis, not currency format) + CAmount nFee = ASYNC_RPC_OPERATION_DEFAULT_MINERS_FEE; + if (params.size() > 3) { + nFee = AmountFromValue( params[3] ); + // Check that the user specified fee is sane. + if (nFee > nTotalOut) { + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Fee %s is greater than the sum of outputs %s", FormatMoney(nFee), FormatMoney(nTotalOut))); + } + } + // Create operation and add to global queue std::shared_ptr q = getAsyncRPCQueue(); - std::shared_ptr operation( new AsyncRPCOperation_sendmany(fromaddress, taddrRecipients, zaddrRecipients, nMinDepth) ); + std::shared_ptr operation( new AsyncRPCOperation_sendmany(fromaddress, taddrRecipients, zaddrRecipients, nMinDepth, nFee) ); q->addOperation(operation); AsyncRPCOperationId operationId = operation->getId(); return operationId;