Skip to content

refactor: replace shannonToCKB with lumos formatUnit - #3111

Closed
twhy wants to merge 2 commits into
nervosnetwork:developfrom
twhy:refactor/replace-shannonToCKB-with-formatUnit
Closed

refactor: replace shannonToCKB with lumos formatUnit#3111
twhy wants to merge 2 commits into
nervosnetwork:developfrom
twhy:refactor/replace-shannonToCKB-with-formatUnit

Conversation

@twhy

@twhy twhy commented Apr 8, 2024

Copy link
Copy Markdown
Contributor

How this PR was implemented

  1. Rewrite shannonToCKB with @ckb-lumos/bi/formatUnit as below.
import { formatUnit, ckbDecimals } from '@ckb-lumos/bi'

const shannonToCKB = (shannon: bigint) => {
  return new Intl.NumberFormat('en-US', {
    useGrouping: false,
    signDisplay: 'always',
    minimumFractionDigits: ckbDecimals,
    maximumFractionDigits: ckbDecimals,
  }).format(Number(formatUnit(shannon, 'ckb')))
}

export default shannonToCKB
  1. Run shannonToCKB.test.ts to make sure the new implementation pass all tests.
image
  1. Go to to-csv-row.ts(the only file using shannonToCKB), rewrite
amount = shannonToCKB(BigInt(tx.value ?? ''))

as

amount = new Intl.NumberFormat('en-US', {
  useGrouping: false,
  signDisplay: 'always',
  minimumFractionDigits: ckbDecimals,
  maximumFractionDigits: ckbDecimals,
}).format(Number(formatUnit(BigInt(tx.value ?? '0'), 'ckb'))) 

BigInt('') and BigInt('0') are both evaluated to 0n, but BigInt('0') is more explicit.

  1. Delete shannonToCKB.ts and shannonToCKB.test.ts

@homura

homura commented Apr 8, 2024

Copy link
Copy Markdown
Contributor

Looks good. Since version 0.22, Lumos has provided an entry package @ckb-lumos/lumos that includes almost all common functions. It's suggested to import only @ckb-lumos/lumos for more convenience

import { formatUnit } from '@ckb-lumos/lumos/utils'
formatUnit(...)

// or
import { utils } from '@ckb-lumos/lumos'
utils.formatUnit(...)

@twhy

twhy commented Apr 8, 2024

Copy link
Copy Markdown
Contributor Author

@homura That's very convinient!

But the neuron-wallet is still using 0.21.1
https://github.com/nervosnetwork/neuron/blob/develop/packages/neuron-wallet/package.json#L45

Also, the lumos package has not exported ckbDecimals
https://github.com/ckb-js/lumos/blob/develop/packages/lumos/src/utils.ts#L10

So to keep this PR simple, we probably should create a separate PR to upgrade the lumos dependencies and update the imports after this one get merged.

Comment thread packages/neuron-wallet/src/utils/to-csv-row.ts Outdated
Comment thread packages/neuron-wallet/tests/utils/to-csv-row/fixtures.json
@twhy twhy closed this Apr 10, 2024
@twhy
twhy deleted the refactor/replace-shannonToCKB-with-formatUnit branch April 10, 2024 02:19
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.

3 participants