-
Notifications
You must be signed in to change notification settings - Fork 43
fix(types): use f64 for GetMiningInfo networkhashps field #440
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
base: master
Are you sure you want to change the base?
Conversation
Bitcoin Core returns networkhashps as a double. The previous type (i64) causes deserialisation to fail.
jamillambert
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 f01c4a9
|
Took me a minute to work out why the original test did not catch this bug. And the reason seems to be that when networkhashps is zero, Core returns Adding |
| // v29 added fields (bits, target) that can fail to parse. | ||
| #[cfg(not(feature = "v28_and_below"))] | ||
| { | ||
| let model: Result<mtype::GetMiningInfo, GetMiningInfoError> = json.into_model(); |
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.
We want to keep this line with the explicit Result type. The reason we do this is to ensure that we exported all the error types correctly. In the past before we did this we forgot a bunch 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.
FTR most of the 'weird' stuff here is for a specific reason, not always obvious. Feel free to ask about anything that looks odd. Also if anything is non-uniform them we probably made a mistake - it happens.
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.
Noted, thanks for the info
|
|
||
| assert!(model.network_hash_ps > 0.0); | ||
| } | ||
|
|
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.
Do what you think is best but to me it looks like the only real changes needed to this test are adding the wallet funding and adding the assertion on network_hash_ps.
As per raised in #429, addressing type discrepancies between corepc-types and Bitcoin Core.
Bitcoin Core has always returned
networkhashpsas a double sincegetmininginfowas introduced. The previous type (i64) causes deserialisation to fail. Here's a minimal example:This change:
networkhashpsfield type fromi64tof64networkhashps> 0.0) and remove redundant comments