-
Notifications
You must be signed in to change notification settings - Fork 50
Add missing transaction categories #89
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
Add missing transaction categories #89
Conversation
Contrary to Bitcoin Core's v17 docs, there are *5* transaction categories that might be returned as part of a response to `listtransactions`, `listsinceblock`, or `gettransaction`. While Core fixed this omission in the docs in bitcoin/bitcoin#14653, we didn't account for them in the respective model's `enum`, leading to calls to `get_transaction` randomly failing with the rather obscure error message: ``` Err(JsonRpc(Json(Error("unknown variant `generate`, expected `send` or `receive`", line: 1, column: 682)))) ``` Here, we fix this omission and add the missing categories.
|
This is broken in more than just the last release (I assume) and also since the docs build is broken I rekon we just fix both and do a new release - and make the policy for now that only the latest version is supported. Ok with you? EDIT: Oh woops, we need a point release to fix the docs so I'll include this patch too. |
| /// Transactions is a receive. | ||
| /// Non-coinbase transactions received. | ||
| Receive, | ||
| /// Coinbase transactions received with more than 100 confirmations. |
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.
I'm going to merge this but just flagging that now there are docs in v17 that came from a later version of Core. We have an outstanding issue #59 that describes how the re-exports include docs so version specific types have the 'wrong' docs on them.
tcharding
left a comment
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.
ACK f31fe46
Yeah this is always the kind of error message, its not great is it. That is the reason I want this crate really well tested (its not currently) so downstream never sees this - that is the dream anyway. |
|
Backported to 0.6.0 in #91 |
f31fe4687fdf0e6e5c798b58a93e6df2e7aad6db Add missing transaction categories (Elias Rohrer) Pull request description: Contrary to Bitcoin Core's v17 docs, there are *five* transaction categories that might be returned as part of a response to `listtransactions`, `listsinceblock`, or `gettransaction`. While Core fixed this omission in the docs in bitcoin/bitcoin#14653, we didn't account for them in the respective model's `enum`, leading to calls to `get_transaction` randomly failing with the rather obscure error message: ``` Err(JsonRpc(Json(Error("unknown variant `generate`, expected `send` or `receive`", line: 1, column: 682)))) ``` Here, we fix this omission and add the missing categories. Note that while the doc fix bitcoin/bitcoin#14653 was shipped as part of `v18`, the categories are pre-existing, which is why we simply fix the `v17` and `model` variants in-place here. @tcharding This might warrant a patch release, IMO? ACKs for top commit: tcharding: ACK f31fe4687fdf0e6e5c798b58a93e6df2e7aad6db Tree-SHA512: cfadb9862c25d7e871426c53b083d162b2a336ea4cc37b7d5c6be69f47147c13a0df86a02f7af3838a691f8fb692c12e1f3c2fb41e05b22ef7d68b4b75da48f8
Contrary to Bitcoin Core's v17 docs, there are five transaction categories that might be returned as part of a response to
listtransactions,listsinceblock, orgettransaction. While Core fixed this omission in the docs in bitcoin/bitcoin#14653, we didn't account for them in the respective model'senum, leading to calls toget_transactionrandomly failing with the rather obscure error message:Here, we fix this omission and add the missing categories. Note that while the doc fix bitcoin/bitcoin#14653 was shipped as part of
v18, the categories are pre-existing, which is why we simply fix thev17andmodelvariants in-place here.@tcharding This might warrant a patch release, IMO?