Unexpected behaviour from P2pkhScript.to_address

[l0rb]

The to_address() method on the P2pkhScript returns a p2sh address, while the address() method returns the expected p2pkh address. This happens because the class itself does not implement the to_address() and only inherits it from ScriptPubKey where it is hardcoded to always give a p2sh if it's not segwit. I can see two ways to fix this, either in the parent or the child class: In the parent class change the address() method to default to p2sh and change the else part in to_address to return self.address(). This preserves the old behaviour for child classes that do not implement address() but fixes it for those that do. Or change the child class(es) to implement to_address().

Edit: created a PR with a proposed solution (only touches parent class)

Below a minimal test case that shows the unexpected/surprising behaviour:

from btcpy.structs.script import P2pkhScript
from btcpy.structs.crypto import PublicKey

btcpy.setup.setup('mainnet')
script = P2pkhScript(PublicKey.unhexlify('02f1a8ff45acaf2c4405436e44302fcf5e56c1c366e5ad2aebb148e8de4aa92341'))
print(script.to_address()) # 39GzTi9aMuVUwPX9DxxokygtLNvfBLzPhR
print(script.address()) # 1HXWXZovrY7rT7TFQq2dmd9knPFYAXa6VG
assert script.to_address() == script.address()

[SimoneBronzini]

Hi, thank you for pointing out this ambiguity. address() is meant to return an address for scriptPubkeys that support it (in our case, we consider only P2PKH, P2SH, P2WPKH and P2WSH to properly support an address). to_address(segwit_version=None) is instead implemented for any script type and is meant to produce a P2(W)SH address for the script.

I can see the semantic ambiguity, so instead of fixing this as proposed in #28, I'd suggest renaming the two methods to something more appropriate. Any suggestion?

[l0rb]

I think it would be best to put the intention in the name to avoid the confusion and rename to_address to to_p2sh_address or to_scripthash_address ?