Skip to content
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

Format miner hash rate using metric system #921

Merged
merged 2 commits into from
Jan 20, 2022
Merged

Conversation

NullSoldier
Copy link
Contributor

@NullSoldier NullSoldier commented Jan 20, 2022

Summary

This PR wil now format the hash rate in metric increments similar to
file sizes while still printing the original value. It also adds tests
for the FileUtils, and fixes a few bugs I saw with rounding that
produced odd values.

// old
Mining block 76858 on request 32...  2276451 H/s
// new
Mining block 76858 on request 32...  2.27 MH/s (2276451)

Testing Plan

Added tests and tested miner locally

Breaking Change

Is this a breaking change? If yes, add notes below on why this is breaking and
what additional work is required, if any.

[ ] Yes
[x] No

@NullSoldier NullSoldier requested a review from a team as a code owner January 20, 2022 22:23
@NullSoldier NullSoldier marked this pull request as draft January 20, 2022 22:24
This PR wil now format the hash rate in metric increments similar to
file sizes while still printing the original value. It also adds tests
for the FileUtils, and fixes a few bugs I saw with rounding that
produced odd values.
@NullSoldier NullSoldier marked this pull request as ready for review January 20, 2022 23:08
Copy link
Contributor

@deekerno deekerno left a comment

Choose a reason for hiding this comment

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

Looks good to me; left one comment.


const fileSizeSuffix: SizeSuffix = { B: 'B', KB: 'KB', MB: 'MB', GB: 'GB' }
const memorySizeSuffix: SizeSuffix = { B: 'B', KB: 'KiB', MB: 'MiB', GB: 'GiB' }
/* This Source Code Form is subject to the terms of the Mozilla Public
Copy link
Contributor

Choose a reason for hiding this comment

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

This was probably unintentional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Member

@dguenther dguenther left a comment

Choose a reason for hiding this comment

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

Looks good! Seems like it doesn't touch anything except CLI display so should be safe

@NullSoldier NullSoldier merged commit 7c7eca0 into staging Jan 20, 2022
@NullSoldier NullSoldier deleted the hash-format branch January 20, 2022 23:25
NullSoldier pushed a commit that referenced this pull request Jan 20, 2022
* Add TCP Secure flag to allow secure config over TCP (#908)

* Dont set config overrides unless not default value (#910)

We found a bug where the base command was setting config overrides for
values that are the same as the flags default value. This causes the
config file to be ignored.

Now, we only set the config override if it's different from the default,
which allows the config not to be over-ridden with the flag default.

* Fix the Sapling related links (#907)

* Move mining logic from NodeJS to Rust (#916)

* Move mining logic from NodeJS to Rust

* Linter/license cleanup

* Fix one more lint check

* Add trait  to  since it already implements it

* Change ironfish-cli build dir to ./ from ./src (#915)

Need to make this change so we can include package.json in the
source

* Fix up ChainProcessor and use in Accounts store for processing blocks (#914)

* Remove unused name field from chainProcessor

* Tag the chainProcessor logger

* Throw error if chain processor head not found in chain

* Make chainProcessor add/remove private

These should only be called inside chainProcessor when updating the head.

* Set hash to previousBlockHash when removing blocks

* Use chainProcessor in Accounts

* Add hashChanged to chainProcessor update

* Remove unnecessary test

* Fix circular dependency import

* Clear the chain processor when resetting accounts

Co-authored-by: NullSoldier <nullprogrammer@gmail.com>

* Allow version to be driven by lib consumer (#917)

This makes a change so that previously, the version sent out everywhere
and the agent string was driven by the library. This doesn't tell the
full story, because the real version is the person who is consuming the
library and potentially writing custom code on top of the
library.

This lets users who consume the library override the package and produce
their own agent and version library using the new Package type that you
can produce from a Package.json file and pass down into the node.

* feat(cli,ironfish): Show heap total in status and rename RSS to Memory (#918)

* feat(cli,ironfish): Show heap total in status and rename RSS to Memory

* Fix test

* Format miner hash rate using metric system (#921)

* Format miner hash rate using metric system

This PR wil now format the hash rate in metric increments similar to
file sizes while still printing the original value. It also adds tests
for the FileUtils, and fixes a few bugs I saw with rounding that
produced odd values.

* Removed double license

* Add debug command with accounts head info (#920)

* Add debug command with accounts head info

* Add more system info, whether telemetry is enabled

* consistent casing and name fix

* Update Package usage to match new code

* Minor typos in README.md (#919)

## Summary

`More details on our documentation` -> `More details in our documentation`
And hypenating `command line interface` -> `command-line interface`

## Testing Plan

n/a

## Breaking Change

Is this a breaking change? If yes, add notes below on why this is breaking and
what additional work is required, if any.

`Documentation update, not breaking change`

* Bump version to 0.1.20

Co-authored-by: mat-if <97762857+mat-if@users.noreply.github.com>
Co-authored-by: Andrei Singular/Kitezh <77510705+andreysingular@users.noreply.github.com>
Co-authored-by: Derek Guenther <derek@ironfish.network>
Co-authored-by: Rohan Jadvani <5459049+rohanjadvani@users.noreply.github.com>
Co-authored-by: Ben Holden-Crowther <4144334+blrhc@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants