test: Add test for `sendmany` rpc that uses `subtractfeefrom` parameter #26733

yusufsahinhamza commented at 9:24 PM on December 20, 2022: contributor

This PR adds test that uses sendmany rpc to send BTC to multiple addresses using subtractfeefrom parameter, then checks receiver addresses balances to make sure fees are subtracted correctly.

DrahtBot commented at 9:24 PM on December 20, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Tests on Dec 20, 2022

in test/functional/wallet_basic.py:273 in b395999979 outdated

 266 | @@ -267,6 +267,20 @@ def run_test(self):
 267 |          assert_equal(self.nodes[2].getbalance(), node_2_bal)
 268 |          node_0_bal = self.check_fee_amount(self.nodes[0].getbalance(), node_0_bal + Decimal('10'), fee_per_byte, self.get_vsize(self.nodes[2].gettransaction(txid)['hex']))
 269 |  
 270 | +        # Sendmany 5 BTC to two address with subtracting fee from both
 271 | +        a0 = self.nodes[0].getnewaddress()
 272 | +        a1 = self.nodes[0].getnewaddress()
 273 | +        txid = self.nodes[2].sendmany('', {a0: 5, a1: 5}, 0, "", [a0, a1])

brunoerg commented at 10:33 PM on December 20, 2022:

Feel free to ignore, just a simple suggestion to make it friendly to read:

        txid = self.nodes[2].sendmany(dummy='', amounts={a0: 5, a1: 5}, minconf=0, comment="", subtractfeefrom=[a0, a1])

yusufsahinhamza commented at 10:47 PM on December 20, 2022:

@brunoerg I think that's better, thanks for the suggestion.

yusufsahinhamza force-pushed on Dec 20, 2022

in test/functional/wallet_basic.py:270 in f3fab28d8f outdated

 266 | @@ -267,6 +267,20 @@ def run_test(self):
 267 |          assert_equal(self.nodes[2].getbalance(), node_2_bal)
 268 |          node_0_bal = self.check_fee_amount(self.nodes[0].getbalance(), node_0_bal + Decimal('10'), fee_per_byte, self.get_vsize(self.nodes[2].gettransaction(txid)['hex']))
 269 |  
 270 | +        # Sendmany 5 BTC to two address with subtracting fee from both

kiminuo commented at 10:29 PM on December 30, 2022:

        # Sendmany 5 BTC to two addresses with subtracting fee from both

yusufsahinhamza commented at 7:43 PM on December 31, 2022:

@kiminuo Nice suggestion, i'm also open for better explanatory comment if you would like to tell any.

kiminuo commented at 10:56 PM on December 31, 2022:

It looks good enough to me. I mean the tests (in general) are commented less than I would love but your comment seems to fit in the file nicely.

yusufsahinhamza commented at 11:22 PM on December 31, 2022:

        # Sendmany 5 BTC to two addresses with subtracting fee from both addresses

A little addition, this seems better, thanks for the suggestion.

Add test for `sendmany` rpc that uses `subtractfeefrom` parameter 057057a2d7

in test/functional/wallet_basic.py:273 in f3fab28d8f outdated

 266 | @@ -267,6 +267,20 @@ def run_test(self):
 267 |          assert_equal(self.nodes[2].getbalance(), node_2_bal)
 268 |          node_0_bal = self.check_fee_amount(self.nodes[0].getbalance(), node_0_bal + Decimal('10'), fee_per_byte, self.get_vsize(self.nodes[2].gettransaction(txid)['hex']))
 269 |  
 270 | +        # Sendmany 5 BTC to two address with subtracting fee from both
 271 | +        a0 = self.nodes[0].getnewaddress()
 272 | +        a1 = self.nodes[0].getnewaddress()
 273 | +        txid = self.nodes[2].sendmany(dummy='', amounts={a0: 5, a1: 5}, minconf=0, comment='', subtractfeefrom=[a0, a1])

kiminuo commented at 10:33 PM on December 30, 2022:

Why is there the empty comment? It looks like the only occurrence of comment= in this test file. Is it needed?

yusufsahinhamza commented at 7:50 PM on December 31, 2022:

@kiminuo Other tests above and below also uses "comment" parameter but not with keyword/named arguments, i've also tried it with only necessary parameters and had no issue, but i wanted to stick to them anyway.

I'm not entirely sure if they're needed, and i was actually thinking about to remove them.

kiminuo commented at 8:17 PM on December 31, 2022:

I can't see any comment other than yours in https://github.com/bitcoin/bitcoin/blob/f3fab28d8f52f161db4aa431842bf7cb8573c7b0/test/functional/wallet_basic.py. That's why I asked. I'm not sure if you and I mean the same thing.

yusufsahinhamza commented at 10:38 PM on December 31, 2022:

@kiminuo Please consider reading my previous answer again. I would be more appreciated for you to tell me whether you think it's necessary or not, as i told you that i'm not sure.

yusufsahinhamza commented at 10:48 PM on December 31, 2022:

@kiminuo It seems like not necessary, i'll remove it in the next commit, thanks for the discussion.

kiminuo commented at 10:53 PM on December 31, 2022:

Ah, sorry, I get what you mean now.

FTR, the comment still looks unnecessary to me but I might be wrong. But I don't want to start bikeshedding. So I guess no need to change anything.

yusufsahinhamza commented at 10:58 PM on December 31, 2022:

@kiminuo Checking rpc docs of sendmany, "comment" parameter is optional and doesn't seem necessary in this test case, so i think i'll just remove it, thanks again.

In addition; "minconf" parameter is also optional and doesn't seem necessary in this test case, and i'll remove it too.

yusufsahinhamza force-pushed on Dec 31, 2022

achow101 requested review from achow101 on Apr 25, 2023

achow101 commented at 1:20 PM on May 1, 2023: member

ACK 057057a2d7e23c2e29cbfd29a5124b3947a33757

DrahtBot removed review request from achow101 on May 1, 2023

achow101 merged this on May 1, 2023

achow101 closed this on May 1, 2023

sidhujag referenced this in commit 7a8522f8f8 on May 1, 2023

bitcoin locked this on Apr 30, 2024