bcoin-org / bcoin

Javascript bitcoin library for node.js and browsers
https://bcoin.io
Other
3.01k stars 811 forks source link

fix: neither uint64 or int64 safe integer #1164

Closed ChrisCho-H closed 1 year ago

ChrisCho-H commented 1 year ago

Number.isSafeInteger is only true when int54. Because here also checks whether positive, it's actually uint53 which can pass these assertions. both int64 and uint64 wrong and misleading

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage has no change and project coverage change: -0.02 :warning:

Comparison is base (b005869) 69.55% compared to head (5228a00) 69.54%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1164 +/- ## ========================================== - Coverage 69.55% 69.54% -0.02% ========================================== Files 158 159 +1 Lines 26603 26607 +4 ========================================== - Hits 18505 18503 -2 - Misses 8098 8104 +6 ``` | [Impacted Files](https://app.codecov.io/gh/bcoin-org/bcoin/pull/1164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcoin-org) | Coverage Δ | | |---|---|---| | [lib/btc/amount.js](https://app.codecov.io/gh/bcoin-org/bcoin/pull/1164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcoin-org#diff-bGliL2J0Yy9hbW91bnQuanM=) | `20.63% <ø> (ø)` | | | [lib/btc/uri.js](https://app.codecov.io/gh/bcoin-org/bcoin/pull/1164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcoin-org#diff-bGliL2J0Yy91cmkuanM=) | `0.00% <ø> (ø)` | | | [lib/primitives/coin.js](https://app.codecov.io/gh/bcoin-org/bcoin/pull/1164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcoin-org#diff-bGliL3ByaW1pdGl2ZXMvY29pbi5qcw==) | `89.85% <ø> (ø)` | | | [lib/primitives/output.js](https://app.codecov.io/gh/bcoin-org/bcoin/pull/1164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcoin-org#diff-bGliL3ByaW1pdGl2ZXMvb3V0cHV0Lmpz) | `88.63% <ø> (ø)` | | ... and [7 files with indirect coverage changes](https://app.codecov.io/gh/bcoin-org/bcoin/pull/1164/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcoin-org)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

pinheadmz commented 1 year ago

I understand where you're coming from but I don't think int53 is a real type? If we do change these messages maybe it should just just say "safe integer" specifically?

ChrisCho-H commented 1 year ago

It's normally not in use but can be used in some languages where u can set any bit size for int or uint. But for javascript, as u suggested, I think safe integer would sound appropriate enough for error message(number must be below 2^53). I've changed it.