Page MenuHomePhabricator

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

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



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

rABC Bitcoin ABC
Lint Not Applicable
Tests Not Applicable

Event Timeline

PiRK requested review of this revision.Mon, Nov 13, 15:47
Fabien requested changes to this revision.Tue, Nov 14, 08:03
Fabien added a subscriber: Fabien.
Fabien added inline comments.
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 ...
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.

211 ↗(On Diff #43063)

This is more robust

This revision now requires changes to proceed.Tue, Nov 14, 08:03
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).

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.

Address.from_cashaddr_string nit

This revision is now accepted and ready to land.Tue, Nov 14, 08:53