-
Notifications
You must be signed in to change notification settings - Fork 86
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
Document & test the semantics of GetRawTransaction
, and fix platform-dependent parsing.
#495
Document & test the semantics of GetRawTransaction
, and fix platform-dependent parsing.
#495
Conversation
getrawtransaction
results.GetRawTransaction
, and fix platform-dependent parsing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed d17baf7.
frontend/service.go
Outdated
// the meanings of the `Height` field of the `RawTransaction` type are as | ||
// follows: | ||
// | ||
// * height 0: the transaction is in the mempool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This aliases with the genesis block height, but is safe solely because the mainnet Zcash genesis block only contained a single transaction (the coinbase) which had no outputs (due to slow start), so no wallet will ever be interested in it even if they incorrectly observe it as "in the mempool".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK b5f3f63 with note about how GetMempool*
will not uniformly return 0. It would be nice to apply the moves from the last commit to the earlier commits in which the moved content was added.
…RawTransaction` This also modifies the internal `common.ZcashdRpcReplyGetrawtransaction` type to ensure that the reinterpretation of the `-1` height value from a `getrawtransaction` response is not platform-dependent.
b5f3f63
to
56fe52a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-utACK 56fe52a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 90116a7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doc changes are clear and good. I don't know Go enough to offer a decent review of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
post-merge ACK
Please ensure this checklist is followed for any pull requests for this repo. This checklist must be checked by both the PR creator and by anyone who reviews the PR.
As a note, all CI tests need to be passing and all appropriate code reviews need to be done before this PR can be merged
Fixes #493