Unit test for Transaction's to/from_json

chainside / btcpy

A Python3 SegWit-compliant library which provides tools to handle Bitcoin data structures in a simple fashion.

https://www.chainside.net

GNU Lesser General Public License v3.0

271 stars 74 forks source link

Unit test for Transaction's to/from_json #42

Closed belovachap closed 6 years ago

belovachap commented 6 years ago

Hello Chainside and @peerchemist :wave:

I was working on a downstream branch ( https://github.com/PeerAssets/btcpy ) and wrote a test for the Transaction's to_json and from_json methods and noticed that the last test transaction (a SegWit transaction) fails :(

python -m unittest tests.unit.TestTransaction is the quickest way I could find to run it if you're curious.

I'll try to figure out how to get the test to pass eventually but I think from_json doesn't notice that the incoming transaction json is for a segwit transaction and drops that information.

SimoneBronzini commented 6 years ago

Thanks a lot for your contribution! I already had a plan on adding better testing of from/to json methods. In fact #39 was not failing tests but was actually incorrect in the json conversions. I fixed it before merging but scheduled to add more json tests in the future. This is another case that proves we need more of those, I'll look into this as soon as possible!

SimoneBronzini commented 6 years ago

Just a heads-up: in fact there are some inconsistencies in the interface. While Transaction.unhexlify instantiates either a Transaction or a SegWitTransaction depending on the hex provided, on the other hand, Transaction.from_json and SegWitTransaction.from_json always instantiate an object of the class they were called upon. I think a solution to this would be implementing the following logics:

Having unhexlify and from_json always fail when called on a hex/json that do not match the type they were called upon
Creating a TransactionFactory class that provides generic unhexlify and from_json methods which instantiate either a Transaction or a SegWitTransaction depending on the data provided

Already working on this fix.

belovachap commented 6 years ago

I like the sound of both of those ideas. I haven't paid much attention to SegWit so I don't know if there are any drawbacks to what you suggested... but after playing with the btcpy code a little more I think you might be pointing in the right direction :)

SimoneBronzini commented 6 years ago

d9196b9d0fd08c263766269fc7880439f4190a14 fixes this