Skip to content

Merge bitcoin/bitcoin#26349: rpc: make address field optional list{transactions, sinceblock} response #119

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
DashCoreAutoGuix wants to merge 1 commit into from
+26 −3
Closed

Merge bitcoin/bitcoin#26349: rpc: make address field optional list{transactions, sinceblock} response #119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/wallet/rpc/transactions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,8 +456,7 @@ RPCHelpMan listtransactions()
{RPCResult::Type::OBJ, "", "", Cat(Cat<std::vector<RPCResult>>(
{
{RPCResult::Type::BOOL, "involvesWatchonly", /* optional */ true, "Only returns true if imported addresses were involved in transaction"},
{RPCResult::Type::STR, "address", "The Dash address of the transaction. Not present for\n"
"move transactions (category = move)."},
{RPCResult::Type::STR, "address", /* optional */ true, "The Dash address of the transaction (not returned if the output does not have an address, e.g. OP_RETURN null data)."},
{RPCResult::Type::STR, "category", "The transaction category.\n"
"\"send\" Transactions sent.\n"
"\"coinjoin\" Transactions sent using CoinJoin funds.\n"
Expand Down Expand Up @@ -572,7 +571,7 @@ RPCHelpMan listsinceblock()
{RPCResult::Type::OBJ, "", "", Cat(Cat<std::vector<RPCResult>>(
{
{RPCResult::Type::BOOL, "involvesWatchonly", /* optional */ true, "Only returns true if imported addresses were involved in transaction"},
{RPCResult::Type::STR, "address", "The Dash address of the transaction."},
{RPCResult::Type::STR, "address", /* optional */ true, "The Dash address of the transaction (not returned if the output does not have an address, e.g. OP_RETURN null data)."},
{RPCResult::Type::STR, "category", "The transaction category.\n"
"\"send\" Transactions sent.\n"
"\"coinjoin\" Transactions sent using CoinJoin funds.\n"
Expand Down
12 changes: 12 additions & 0 deletions test/functional/wallet_listsinceblock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ def run_test(self):
self.test_double_send()
self.double_spends_filtered()
self.test_targetconfirmations()
self.test_op_return()

@coderabbitai coderabbitai Bot Jul 25, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

LGTM! Comprehensive test for OP_RETURN in listsinceblock.

The test correctly verifies that OP_RETURN transactions don't include an "address" field in listsinceblock results, complementing the listtransactions test. The approach of capturing the block hash before the transaction is sent is appropriate for testing listsinceblock functionality.

Consider addressing the static analysis hints:

-    def test_op_return(self):
+    def test_op_return(self) -> None:
         """Test if OP_RETURN outputs will be displayed correctly."""
         block_hash = self.nodes[2].getbestblockhash()
         raw_tx = self.nodes[2].createrawtransaction([], [{'data': 'aa'}])
         signed_raw_tx = self.nodes[2].signrawtransactionwithwallet(raw_tx)
         tx_id = self.nodes[2].sendrawtransaction(signed_raw_tx['hex'])

-        op_ret_tx = [tx for tx in self.nodes[2].listsinceblock(blockhash=block_hash)["transactions"] if tx['txid'] == tx_id][0]
+        op_ret_tx = next(tx for tx in self.nodes[2].listsinceblock(blockhash=block_hash)["transactions"] if tx['txid'] == tx_id)

         assert 'address' not in op_ret_tx

Also applies to: 392-401

🤖 Prompt for AI Agents 
In test/functional/wallet_listsinceblock.py around lines 44 and 392 to 401,
address the static analysis hints by reviewing the code for potential issues
such as unused variables, improper exception handling, or code style
inconsistencies. Make necessary adjustments to improve code quality and adhere
to best practices without changing the test logic.


def test_no_blockhash(self):
self.log.info("Test no blockhash")
Expand Down Expand Up @@ -388,5 +389,16 @@ def double_spends_filtered(self):
assert_equal(original_found, False)
assert_equal(double_found, False)

def test_op_return(self):
"""Test if OP_RETURN outputs will be displayed correctly."""
block_hash = self.nodes[2].getbestblockhash()
raw_tx = self.nodes[2].createrawtransaction([], [{'data': 'aa'}])
signed_raw_tx = self.nodes[2].signrawtransactionwithwallet(raw_tx)
tx_id = self.nodes[2].sendrawtransaction(signed_raw_tx['hex'])

op_ret_tx = [tx for tx in self.nodes[2].listsinceblock(blockhash=block_hash)["transactions"] if tx['txid'] == tx_id][0]

assert 'address' not in op_ret_tx

if __name__ == '__main__':
ListSinceBlockTest().main()
12 changes: 12 additions & 0 deletions test/functional/wallet_listtransactions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,5 +97,17 @@ def run_test(self):
{"category": "receive", "amount": Decimal("0.1")},
{"txid": txid, "label": "watchonly"})

self.test_op_return()

def test_op_return(self):
"""Test if OP_RETURN outputs will be displayed correctly."""
raw_tx = self.nodes[0].createrawtransaction([], [{'data': 'aa'}])
signed_raw_tx = self.nodes[0].signrawtransactionwithwallet(raw_tx)
tx_id = self.nodes[0].sendrawtransaction(signed_raw_tx['hex'])

op_ret_tx = [tx for tx in self.nodes[0].listtransactions() if tx['txid'] == tx_id][0]

assert 'address' not in op_ret_tx
Comment on lines +100 to +110

@coderabbitai coderabbitai Bot Jul 25, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

LGTM! Focused test for OP_RETURN address field behavior.

The test correctly verifies that transactions with OP_RETURN outputs do not include an "address" field in listtransactions results, which aligns perfectly with the Bitcoin backport objectives.

Consider addressing the static analysis hints:

-    def test_op_return(self):
+    def test_op_return(self) -> None:
         """Test if OP_RETURN outputs will be displayed correctly."""
         raw_tx = self.nodes[0].createrawtransaction([], [{'data': 'aa'}])
         signed_raw_tx = self.nodes[0].signrawtransactionwithwallet(raw_tx)
         tx_id = self.nodes[0].sendrawtransaction(signed_raw_tx['hex'])

-        op_ret_tx = [tx for tx in self.nodes[0].listtransactions() if tx['txid'] == tx_id][0]
+        op_ret_tx = next(tx for tx in self.nodes[0].listtransactions() if tx['txid'] == tx_id)

         assert 'address' not in op_ret_tx
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
self.test_op_return()
def test_op_return(self):
"""Test if OP_RETURN outputs will be displayed correctly."""
raw_tx = self.nodes[0].createrawtransaction([], [{'data': 'aa'}])
signed_raw_tx = self.nodes[0].signrawtransactionwithwallet(raw_tx)
tx_id = self.nodes[0].sendrawtransaction(signed_raw_tx['hex'])
op_ret_tx = [tx for tx in self.nodes[0].listtransactions() if tx['txid'] == tx_id][0]
assert 'address' not in op_ret_tx
def test_op_return(self) -> None:
"""Test if OP_RETURN outputs will be displayed correctly."""
raw_tx = self.nodes[0].createrawtransaction([], [{'data': 'aa'}])
signed_raw_tx = self.nodes[0].signrawtransactionwithwallet(raw_tx)
tx_id = self.nodes[0].sendrawtransaction(signed_raw_tx['hex'])
op_ret_tx = next(tx for tx in self.nodes[0].listtransactions() if tx['txid'] == tx_id)
assert 'address' not in op_ret_tx
🧰 Tools
🪛 Ruff (0.12.2)

102-102: Missing return type annotation for public function test_op_return

Add return type annotation: None

(ANN201)

108-108: Prefer next(...) over single element slice

Replace with next(...)

(RUF015)

🤖 Prompt for AI Agents 
In test/functional/wallet_listtransactions.py around lines 100 to 110, the test
correctly checks that OP_RETURN outputs do not have an "address" field in
listtransactions results. To address static analysis hints, ensure all variables
are used appropriately and consider adding explicit type annotations or comments
if needed to clarify intent. Also, verify that no unused imports or variables
exist in this test function to keep the code clean.


if __name__ == '__main__':
ListTransactionsTest().main()