ElementsProject / libwally-core

Useful primitives for wallets
Other
281 stars 135 forks source link

scriptpubkey_to_address() does not give clear error messages #354

Open Randy808 opened 1 year ago

Randy808 commented 1 year ago

After attempting to use the library function scriptpubkey_to_address from the Python distribution, I got the error message ValueError: Invalid argument

This was the line I used to invoke the function: wallycore.scriptpubkey_to_address(bytes([0x6a]), wallycore.WALLY_NETWORK_LIQUID)

I tried looking through the repo to see if there's any examples of scriptpubkey_to_address and didn't find much in the code aside from this (which linked the python call to 'wally_scriptpubkey_to_address'): https://github.com/ElementsProject/libwally-core/blob/38b85bf06194ebe0d1d841e11cd8e1b58aa2a9ca/include/wally.hpp#L1360

I did find an example in one of the commits matching the search result: https://github.com/ElementsProject/libwally-core/commit/399906aba358a16f05b233f619fa08aec6bfe116#diff-a1dde0a251f92d169d953a7e3b12d4f97d98a5985abf5b1a0e48075c062e708c

The standalone example file in that commit can be found here (although it's not in master anymore): https://github.com/ElementsProject/libwally-core/blob/399906aba358a16f05b233f619fa08aec6bfe116/src/swig_python/contrib/address.py

When I tried running this line using data from the test case it worked: wallycore.scriptpubkey_to_address(wallycore.hex_to_bytes("76a914bef5a2f9a56a94aab12459f72ad9cf8cf19c7bbe88ac"), wallycore.WALLY_NETWORK_LIQUID)

So then I decided to look into the implementation for 'wally_scriptpubkey_to_address': https://github.com/ElementsProject/libwally-core/blob/e1fc3cd4a3e0947d017ad323126b5413538b4710/src/address.c#L154

There's a line that tries to determine the script 'type': https://github.com/ElementsProject/libwally-core/blob/e1fc3cd4a3e0947d017ad323126b5413538b4710/src/address.c#L160

And then only processes the types WALLY_SCRIPT_TYPE_P2PKH and WALLY_SCRIPT_TYPE_P2SH

The function that gets the types ('wally_scriptpubkey_get_type') can be found here: https://github.com/ElementsProject/libwally-core/blob/38b85bf06194ebe0d1d841e11cd8e1b58aa2a9ca/src/script.c#L369

Although a type is returned for OP_RETURN, it doesn't accept it in 'wally_scriptpubkey_to_address'

The error I get from the call could tell me it wasn't supported instead of the generic 'WALLY_EINVAL' that's returned as the default catch-all: https://github.com/ElementsProject/libwally-core/blob/e1fc3cd4a3e0947d017ad323126b5413538b4710/src/address.c#L209

Randy808 commented 1 year ago

For more context, I originally tried the function call in nodejs: image

And then tried in Python:

Screenshot 2022-12-02 at 1 54 21 PM
jgriffiths commented 1 year ago

Hi @Randy808

There is no address format for OP_RETURN outputs AFAIU, so passing one to this function is always going to be invalid. I think the documentation should state that only p2pkh and p2sh addresses are valid inputs.

The address API was added while I was on sabbatical and would not have passed review had I reviewed the calls tbh. There should be no need in a better API for the caller to treat different address types manually, but for now that's the state of things.

Wally has very limited error reporting capabilities at present, since initially it was not going to have higher level APIs like PSBT. I intend to extend the error reporting to give optional human readable error codes in due course.

Given that this call will only ever support p2pkh/p2sh, the caller is always going to have to check the script type before calling, so I'm not sure that returning e.g. WALLY_ERROR for an unsupported script type helps much.