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

refactor: replace shannonToCKB with lumos formatUnit #3111

Closed
wants to merge 2 commits into from
Closed

refactor: replace shannonToCKB with lumos formatUnit #3111

wants to merge 2 commits into from

Conversation

twhy
Copy link
Contributor

@twhy twhy commented Apr 8, 2024

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
Copy link
Collaborator

homura commented Apr 8, 2024

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
Copy link
Contributor Author

twhy commented Apr 8, 2024

@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.

@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