remove bn.js #1720
remove bn.js #1720
Conversation
dynst
force-pushed
the
decimal
branch
2 times, most recently
from
ebdcdc4 to
ce6aaa0
Compare
packages/math/src/decimal.ts
Outdated
| public static compare(a: Decimal, b: Decimal): number { | ||
| if (a.fractionalDigits !== b.fractionalDigits) throw new Error("Fractional digits do not match"); | ||
| return a.data.atomics.cmp(new BN(b.atomics)); | ||
| return Math.sign(Number(a.data.atomics - b.data.atomics)); |
There was a problem hiding this comment.
Can we implement this manually instead of using Math.sign, please? This function can cause all sorts of issues with -0 or NaN as return values. We don't need those cases as we have 2 ints as inputs.
There was a problem hiding this comment.
Since we have two bigint as inputs, how would any issues even happen?
There was a problem hiding this comment.
I'm not aware of any inputs that can cause any issues. But the number type is ugly as a result of bigint math. E.g. we can have a diff value that cannot be converted to number without loss.
With sucht a simple 3liner we don't need to know how number and Math.sign behave together with bigint
if (a.data.atomics === b.data.atomics) return 0;
else if (a.data.atomics < b.data.atomics) return -1;
else return 1;There was a problem hiding this comment.
Okay, used if statements for the sake of future code archaeologists who don't realize Number(2n ** 970n - 2n ** 1024n) clamps the number to -Infinity.
There was a problem hiding this comment.
For the time when number will be removed from the language ;)
| atomics: BigInt(atomics), | ||
| fractionalDigits: fractionalDigits, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Slightly out of scope, but we can now also change the constuctor argument to atomics: string | bigint for more efficientcy in a few places.
Number(bigint) will clamp large numbers to positive or negative Infinity so the code works fine but this is simply clearer-looking.
|
Looks good to me, thank you! We just need an entry in the CHANGELOG.md. I will read through it again tomorrow and then merge |
2 participants
Closes #1711
bigintdoesn't seem to actually be missing any math?The new byte array convesion helper function from #1698
bytesToUnsignedBigInt()really helps though.