Fix RandomX hashrate units#3253
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for the RandomX mining algorithm by adding it to the GpuMiningAlgorithm enum and updating the CPU mining tile to use it. It also modifies the formatHashrate utility to support different units ('G' for C29 and 'H' for RandomX) and includes corresponding unit tests. Feedback suggests replacing the ternary logic in formatHashrate with an explicit mapping to improve extensibility and handle other algorithms that might share the same units.
| export function formatHashrate(hashrate: number, joinUnit = true, _algo = GpuMiningAlgorithm.C29): Hashrate { | ||
| const unit = 'G'; | ||
| export function formatHashrate(hashrate: number, joinUnit = true, algo = GpuMiningAlgorithm.C29): Hashrate { | ||
| const unit = algo === GpuMiningAlgorithm.C29 ? 'G' : 'H'; |
There was a problem hiding this comment.
The current ternary logic defaults to 'H' for any algorithm other than C29. This is fragile because other algorithms (like C31) also use Graph-rate units (G/s). Using an explicit mapping makes the code more robust and easier to extend as new algorithms are added, while also making the intent clearer.
const unit = {
[GpuMiningAlgorithm.C29]: 'G',
[GpuMiningAlgorithm.RandomX]: 'H',
}[algo];There was a problem hiding this comment.
Addressed in 0f098d81: formatHashrate now uses an explicit HASHRATE_BASE_UNITS mapping keyed by GpuMiningAlgorithm, with Record<GpuMiningAlgorithm, 'G' | 'H'> so future enum additions require an explicit unit choice at type-check time.
Re-ran npm test -- --run src/utils/formatters.test.ts, npm exec tsc -- --noEmit, and git diff --check.
2c18c77 to
0f098d8
Compare
|
Follow-up after another PR appeared for the same issue: I strengthened this PR so the algorithm type is no longer GPU-specific and the RandomX coverage is broader. Added in
Re-verified locally:
|
Verification on Windows: - npm test -- --run src/utils/formatters.test.ts (65 passed) - npm exec tsc -- --noEmit - git diff --check origin/main...HEAD
Summary
Fixes #3210 by making hashrate display units depend on the mining algorithm instead of always using graph-rate units.
This change:
G/s,kG/s,MG/s, etc.)H/s,kH/s,MH/s, etc.)RandomXexplicitly from the CPU mining tile so macOS CPU mining no longer displaysG/sVerification
npm test -- --run src/utils/formatters.test.ts— 62 tests passednpm exec tsc -- --noEmitgit diff --checkNote: Vitest printed its existing "close timed out" warning after the tests passed, but exited successfully.