Switch to bignumber.js in v7 #191
Conversation
tatomir-streamflow
requested review from
RolginRoman,
Yolley and
LukaStreamflow
and removed request for
RolginRoman
| . "$(dirname -- "$0")/_/husky.sh" | ||
|
|
||
| npm run lint | ||
| TARGET_FOLDER="packages/distributor/solana/generated" |
There was a problem hiding this comment.
tbh I don't think it should be a pre commit hook specifically. It is a one time operation after a folder update. Pre commit hooks must not change code logic; how is dev supposed to check validity if commit+push is too common an operation?
Another thing: how to scale this const to multiple sources?
more security: plans on banning BN.js imports? https://eslint.org/docs/latest/rules/no-restricted-imports
There was a problem hiding this comment.
The pre-commit hook is there so dev doesn't manually have to convert BN to BigNumber once generating code - I guess I could just make it a script that can be manually run.
I think a wildcard like "/generated/" or iterating through all folders named "generated" would work
We can go with banning BN.js imports
| @@ -1,5 +1,4 @@ | |||
| import { TransactionInstruction, PublicKey, AccountMeta } from "@solana/web3.js"; // eslint-disable-line @typescript-eslint/no-unused-vars | |||
| import BN from "bn.js"; // eslint-disable-line @typescript-eslint/no-unused-vars | |||
There was a problem hiding this comment.
Yes, is there a reason why it's there?
There was a problem hiding this comment.
the generating template puts it everywhere
| partnerFeeWithdrawn: new BN(raw.partner_fee_withdrawn, LE), | ||
| partnerFeePercent: new BN(raw.partner_fee_percent, LE), | ||
| streamflowFeeTotal: BigNumber(toUInt64String(raw.streamflow_fee_total)), | ||
| streamflowFeeWithdrawn: BigNumber(toUInt64String(raw.streamflow_fee_withdrawn)), |
There was a problem hiding this comment.
why similar constructions were converted to different calls?
from ⬇️
streamflowFeeWithdrawn: new BN(raw.streamflow_fee_withdrawn, LE),
streamflowFeePercent: new BN(raw.streamflow_fee_percent, LE),
to ⬇️
streamflowFeeWithdrawn: BigNumber(toUInt64String(raw.streamflow_fee_withdrawn)),
streamflowFeePercent: BigNumber(raw.streamflow_fee_percent),
There was a problem hiding this comment.
very interesting question - since our fees are <1 this was always 0 (since BN can't do decimals), fortunately this doesn't seem to be used anywhere except in topup fee calculation in SUI client, I didn't have the time to test whether it can actually cause a bug - someone topping up a SUI stream without paying fees.
|
|
||
| import { streamLayout } from "./layout.js"; | ||
| import { DecodedStream, BatchItem, BatchItemResult } from "./types.js"; | ||
| import { SOLANA_ERROR_MAP, SOLANA_ERROR_MATCH_REGEX } from "./constants.js"; | ||
|
|
||
| const decoder = new TextDecoder("utf-8"); | ||
| const LE = "le"; //little endian | ||
|
|
||
| const toUInt64String = (uintArr: Uint8Array): string => |
There was a problem hiding this comment.
please describe in comments or another way when and why it is necessary to use these functions
There was a problem hiding this comment.
BigNumber doesn't have native support for little endian parsing of a byteArray so this is essentially that - will add a comment like this
* switch to bignubmer.js initial + rebase * fixes * fixes * public API and README update * bump to 7.0.0-alpa.2 * fix pre-commit * lerna version bump and import fixes * remove comments * remove more leftover comments
Overview
We want to switch to using bignumber.js library due to more precision when doing operations like division and multiplication and to have consistency across the codebases (to only use 1 big number library).
Addressing comments by @LukaStreamflow on original PR
#186 (comment)
I don't see this function being called anywhere in js-sdk or our app? - no reason to change it
#186 (comment)
We are dropping support for EVM AFAIK - I created a to-do to remove this once safe
Other comments are resolved