[electrum] fix address resolving and building transaction from hex in RPC commands
ClosedPublic
Actions

Authored by PiRK on Nov 13 2023, 15:47.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABCec6259228561: [electrum] fix address resolving and building transaction from hex in RPC…

Summary

This diff fixes RPC commands that were broken in two ways.

The alias resolving code was moved from the Contacts class to the alias module in D14547. This broke the RPC commands that use wallet.contacts.resolve. Fix it by using the correct alias.resolve function.
Note that nothing in the moved code was specific to a particular wallet. It is just code to validate an address or convert an alias to an address.

The broadcast, deserialize and signtransaction commands that take a hex transaction as main parameter were broken because the Transaction class now expect bytes in __init__. It was tested using the command line in D14530 which works because the conversion was done via argparse, but not with a RPC call. Call rawtx_from_str directly in the 3 commands instead of making it a type conversion in argparse, so that it works both as a command line command and an RPC command.

Test Plan

pytest electrumabc/tests/regtest

./electrum-abc deserialize 0100000001f184f3128dc56f6575cf876c77657af21888fab6e602d1e3df7aa24d1d36c632010000006a47304402207f0066e8c48bc73fbd58045b9ce7272d5cd71fdd86ab1debdd0628f5cc14a77d02207b2c92f785dd89387eadc37cfaa825392c2dfa8e24b7673546b5ed02050639a8412103562731a08eb23e6260b516c4564f746033e9080bc9f61ad2158a63927500b8b1ffffffff02a6ca360c000000001976a9142560d750b51eebd180cef831b4a5c7aafc487bbd88acc2b64d03000000001976a914231f7087937684790d1049294f3aef9cfb7b05dd88ac00000000

Diff Detail

Repository

rABC Bitcoin ABC

Branch

regtest_history

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 25611
Build 50803: Build Diff	electrum-tests
Build 50802: arc lint + arc unit

Event Timeline

PiRK created this revision.Nov 13 2023, 15:47

Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 13 2023, 15:47

PiRK requested review of this revision.Nov 13 2023, 15:47

Harbormaster completed remote builds in B25606: Diff 43063.Nov 13 2023, 15:49

PiRK edited the summary of this revision. (Show Details)Nov 13 2023, 16:48

Fabien requested changes to this revision.Nov 14 2023, 08:03

Fabien added a subscriber: Fabien.

Fabien added inline comments.

electrum/electrumabc/address.py
422 ↗	(On Diff #43063)	You'd better add a list type constant as well, and use a conditional assignment: whitelisted_prefixes = ... if ... else .. if ... else ...
electrum/electrumabc/commands.py
457 ↗	(On Diff #43063)	Interesting, it doesn't return a Transaction but the transaction bytes, and not from a string but a union string/json. Not confusing at all.
electrum/electrumabc/tests/regtest/util.py
211 ↗	(On Diff #43063)	This is more robust

This revision now requires changes to proceed.Nov 14 2023, 08:03

PiRK added inline comments.Nov 14 2023, 08:40

electrum/electrumabc/commands.py
457 ↗	(On Diff #43063)	D14530 was an attempt at making the return type a bit more clear in the name (it was `tx_from_str` before).
electrum/electrumabc/tests/regtest/util.py
211 ↗	(On Diff #43063)	The way it is used, I expect an exact number of items in the list (a specific transaction history). I wan to use it in a future test to see the history get shorter as a double spend occurs.