Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

get_transaction will now return a tx from backlog, even if there are some in invalid blocks #793

Merged
merged 3 commits into from Nov 8, 2016

Conversation

ttmc
Copy link
Contributor

@ttmc ttmc commented Nov 5, 2016

I made a slight change to how Bigchain.get_transaction() works.

Before, if there was a transaction with the specified txid in the backlog and in some invalid blocks (but nowhere else), Bigchain.get_transaction() would return None. I changed it so that in that case, it returns the transaction from the backlog (and if the user asked for the transaction's status, it will be 'backlog').

I updated and expanded the docstring for Bigchain.get_transaction().

I also changed the test which checked to see the response to Bigchain.get_transaction() when there's a copy of the transaction in an invalid block. Before, it didn't care about the backog: the response was always expected to be None. Now it does care about the backlog, so the original test was split into two tests:

  1. one with a copy of the transaction in an invalid block, and nowhere else
  2. one with a copy of the transaction in the backlog and in an invalid block

@ttmc ttmc changed the title get_transaction will return a tx from backlog, even if there are some in invalid blocks get_transaction will now return a tx from backlog, even if there are some in invalid blocks Nov 5, 2016
@ttmc
Copy link
Contributor Author

ttmc commented Nov 6, 2016

Note: I originally didn't add the second test and code coverage went down slightly. Once I added the second test, code coverage went up by 0.06%.

Copy link
Contributor

@vrde vrde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

input_tx = b.get_transaction(input_tx.txid)
inputs = input_tx.to_inputs()
tx = Transaction.transfer(inputs, [user_vk], input_tx.asset)
tx = tx.sign([user_sk])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note, not a blocker to merge this PR.

I wonder whether it is really needed to write a "real" tx for this test, as it seems all we would need is a "dummy" tx, just to make sure the block in-validation can take place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but I'm not sure how to go about that. I based the two "new" tests on the one original test, so at least this is no worse than before.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, ok, no problem!

@sbellem
Copy link
Contributor

sbellem commented Nov 8, 2016

👍

@ttmc ttmc merged commit 04824df into master Nov 8, 2016
@ttmc ttmc deleted the modify-get-transaction branch December 10, 2016 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants