Skip to content

Added blockhash argument to getrawtransaction api#39

Open
PaarthAgarwal wants to merge 2 commits into
cryptoadvance:masterfrom
PaarthAgarwal:whitepaper-fix
Open

Added blockhash argument to getrawtransaction api#39
PaarthAgarwal wants to merge 2 commits into
cryptoadvance:masterfrom
PaarthAgarwal:whitepaper-fix

Conversation

@PaarthAgarwal

Copy link
Copy Markdown
Contributor

Fixes #32.

@moneymanolis

Copy link
Copy Markdown
Collaborator

Have you tested to retrieve the whitepaper with a Spectrum connection?

@PaarthAgarwal

Copy link
Copy Markdown
Contributor Author

Have you tested to retrieve the whitepaper with a Spectrum connection?

Yeah, I tested. It's failing on mine maybe because I haven't added any device, but I feel it's close. Have a look

@moneymanolis

Copy link
Copy Markdown
Collaborator

You don't need any device for this feature.

@PaarthAgarwal

Copy link
Copy Markdown
Contributor Author

You don't need any device for this feature.

I don't understand why it does not recognise the fourth argument block hash despite adding it.

Comment on lines -596 to +605
if verbose:
return self.sock.call("blockchain.transaction.get", [txid, True])
if blockhash:
if verbose:
return self.sock.call("blockchain.transaction.get", [txid, True, blockhash])
else:
return self.sock.call("blockchain.transaction.get", [txid, False, blockhash])
else:
return self.sock.call("blockchain.transaction.get", [txid, False])
if verbose:
return self.sock.call("blockchain.transaction.get", [txid, True])
else:
return self.sock.call("blockchain.transaction.get", [txid, False])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This can be simplified a lot. If you're unsure how to do that, please ask chatGPT to do it.

@k9ert

k9ert commented Feb 3, 2023

Copy link
Copy Markdown
Contributor

I don't understand why it does not recognise the fourth argument block hash despite adding it.

What do you mean by "recognise"? It would be cool to have a test here as well. Especially because you seem to do some kind of test and then claim that your test failed ("recognise it"). For some stuff we could do integration-tests. E.g. this one is imho a great candidate.
Have a look here getting an idea how this could look like to do tests on the mainchain:

It might be that this only works locally and not in the CI but i think it's still valuable to do that.

@k9ert

k9ert commented Feb 9, 2023

Copy link
Copy Markdown
Contributor

I would say that the ability of downloading the whitepaper when it comes to code in this repo might be the best acceptance criteria for this PR to merge. So is it possible?

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.

Feature: Enable whitepaper option in Spectrum

3 participants