-
Notifications
You must be signed in to change notification settings - Fork 44
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,19 +39,21 @@ fn mining__get_block_template__modelled() { | |
| #[test] | ||
| fn mining__get_mining_info() { | ||
| let node = Node::with_wallet(Wallet::Default, &[]); | ||
| node.fund_wallet(); | ||
|
|
||
| let json: GetMiningInfo = node.client.get_mining_info().expect("rpc"); | ||
|
|
||
| // Up to v28 (i.e., not 29_0) there is no error converting into model. | ||
| #[cfg(feature = "v28_and_below")] | ||
| let _: mtype::GetMiningInfo = json.into_model(); | ||
| let model: mtype::GetMiningInfo = json.into_model(); | ||
|
|
||
| // v29 onwards | ||
| #[cfg(not(feature = "v28_and_below"))] | ||
| { | ||
| let model: Result<mtype::GetMiningInfo, GetMiningInfoError> = json.into_model(); | ||
| model.unwrap(); | ||
| } | ||
| let model: mtype::GetMiningInfo = { | ||
| let result: Result<mtype::GetMiningInfo, GetMiningInfoError> = json.into_model(); | ||
| result.unwrap() | ||
| }; | ||
|
|
||
| assert!(model.network_hash_ps > 0.0); | ||
| } | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed and actioned, thanks @tcharding! |
||
| #[test] | ||
|
|
||
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
Resulttype. 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