Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Fixpending #1074

Merged
merged 17 commits into from
May 24, 2016
Merged

Fixpending #1074

merged 17 commits into from
May 24, 2016

Conversation

debris
Copy link
Collaborator

@debris debris commented May 11, 2016

fixes querying pending in most cases except:

  • eth_getTransactionReceipt, cause the docs state Note That the receipt is not available for pending transactions..

@debris debris added the A0-pleasereview 🤓 Pull request needs code review. label May 11, 2016
let queue = self.transaction_queue.lock().unwrap();
queue.find(hash)
}
}
}

fn pending_transactions(&self) -> Vec<SignedTransaction> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pending transactions is used also when propagating transactions - does it mean that we don't want to propagate everything from queue?

Copy link
Collaborator

Choose a reason for hiding this comment

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

All valid transactions should be propogated

@tomusdrw tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels May 11, 2016
@gavofyork
Copy link
Contributor

doesn't actually work according to spec - watched pending events are notified many times over.

@gavofyork gavofyork added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels May 14, 2016
@debris debris added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels May 17, 2016
@arkpar arkpar added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels May 18, 2016
@gavofyork
Copy link
Contributor

gavofyork commented May 18, 2016

eth_getTransactionReceipt, cause the docs state Note That the receipt is not available for pending transactions..

so that was never ratified by me. fabian added it unilaterally on July 7, 2015 in https://github.com/ethereum/wiki/wiki/JSON-RPC/_compare/f5d8c9d4c99cd3a4502eba1384e19244e1bf0e0d...88b78a4cb70ccf401c088b928365c87de45064f5 almost certainly just proscribing based on geth's behaviour. cpp-ethereum did provide pending receipts (since they're rather useful).

@debris
Copy link
Collaborator Author

debris commented May 18, 2016

yes :/ And I don't know if we should change this, cause there might be a code relying this behaviour

@gavofyork
Copy link
Contributor

we'll do our own pending behaviour and issue our own JSONRPC spec until i get word from vitalik that there will be an inclusive process for managing alterations to the spec.

@gavofyork
Copy link
Contributor

still broken. gets called multiple times as the client syncs new blocks.

@gavofyork
Copy link
Contributor

gavofyork commented May 19, 2016

it should be called once and only once when:

  • when transaction goes from being unknown into 'pending'/'mined' (e.g. local user submitted the transaction/new block imported that contained a previously unknown transaction)

rarer events that should also be supported:

  • when transaction goes from being 'mined'/'pending' to unknown (e.g. a double-spend)

also, transactionHash should be supplied.

@debris debris added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels May 24, 2016
@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels May 24, 2016
@gavofyork gavofyork merged commit ebd0cdb into master May 24, 2016
@gavofyork gavofyork deleted the fixpending branch May 24, 2016 19:56
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants