Add pnum #1935
Add pnum #1935
Conversation
🦋 Changeset detectedLatest commit: e6c0ac2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
packages/types/src/pnum.ts
Outdated
| function pnum( | ||
| input: string | number | LoHi | bigint | Amount | ValueView | undefined, | ||
| exponentInput = 0, | ||
| ) { |
There was a problem hiding this comment.
issue: this feels very class-like (private vars, exposed methods, constructor like initialization). Think we should convert it.
There was a problem hiding this comment.
I did this on purpose as it's shorter: pnum(val) vs new PNum(val). My guess is that we'll use it quite often and this way it looks cleaner imo, especially if we need to do it inline inside a react component.
There was a problem hiding this comment.
Ok, I can live with this, but probably just because it is a helper utility. Otherwise, think we should default to classes.
packages/types/src/pnum.ts
Outdated
| input instanceof Amount || | ||
| (typeof input === 'object' && 'lo' in input && 'hi' in input && input.lo && input.hi) |
There was a problem hiding this comment.
Suggestion: Wouldn't this work the same?
else if (input instanceof Amount) {
value = new BigNumber(joinLoHi(input.lo, input.hi).toString());
}There was a problem hiding this comment.
This case catches an instance of Amount and LoHi, in your check a LoHi will not be true.
There was a problem hiding this comment.
No test appears to fail when removing that line. Can you add a test for coverage for that?
packages/types/src/pnum.ts
Outdated
| toAmount(): Amount { | ||
| return new Amount(splitLoHi(BigInt(value.toString()))); |
There was a problem hiding this comment.
issue: missing tests for this method
| --- | ||
| '@penumbra-zone/types': minor | ||
| --- |
There was a problem hiding this comment.
question: should we take those packages you've listed, create a new package, make those specific files private, and only expose pnum?
There was a problem hiding this comment.
related: if we make the other stuff private, we'll need to find the previous usages and convert them to pnum
There was a problem hiding this comment.
I think a rewrite would be quite a bit of effort and potentially introduce regression errors (you'd need to check all values if they are in base units) I'd say leave the existing code and adopt this for future code. Also it's internally using some of the other functions (splitLoHi etc)
packages/types/src/pnum.ts
Outdated
| */ | ||
| function pnum( | ||
| input: string | number | LoHi | bigint | Amount | ValueView | undefined, | ||
| exponentInput = 0, |
There was a problem hiding this comment.
issue: think we should consider an options obj as a second parameter so it's easier to expand in the future:
- exponentInput = 0,
+ options: { exponentInput?: number } = { exponentInput: 0 },There was a problem hiding this comment.
Perhaps wise indeed
There was a problem hiding this comment.
think we can simplify the name too
- exponentInput = 0,
+ options: { exponent?: number } = { exponent: 0 },
packages/types/src/pnum.ts
Outdated
| * @param exponent | ||
| */ | ||
| function pnum( | ||
| input: string | number | LoHi | bigint | Amount | ValueView | undefined, |
There was a problem hiding this comment.
question: what are the usecases for an undefined input?
There was a problem hiding this comment.
This:
I guess you can make an argument against it, what do you think?
There was a problem hiding this comment.
Ah I see, it's kinda the default case. I wonder if explicitly defining it would be helpful:
function pnum(input: string | number | LoHi | bigint | Amount | ValueView = 0)
...
// and then removing this branch
else {
value = new BigNumber(0);
}Can you add tests for testing the default case if undefined is passed?
packages/types/src/pnum.ts
Outdated
| * @param exponent | ||
| */ | ||
| function pnum( | ||
| input: string | number | LoHi | bigint | Amount | ValueView | undefined, |
There was a problem hiding this comment.
Ah I see, it's kinda the default case. I wonder if explicitly defining it would be helpful:
function pnum(input: string | number | LoHi | bigint | Amount | ValueView = 0)
...
// and then removing this branch
else {
value = new BigNumber(0);
}Can you add tests for testing the default case if undefined is passed?
packages/types/src/pnum.ts
Outdated
| */ | ||
| function pnum( | ||
| input: string | number | LoHi | bigint | Amount | ValueView | undefined, | ||
| exponentInput = 0, |
There was a problem hiding this comment.
think we can simplify the name too
- exponentInput = 0,
+ options: { exponent?: number } = { exponent: 0 },
packages/types/src/pnum.ts
Outdated
| function pnum( | ||
| input: string | number | LoHi | bigint | Amount | ValueView | undefined, | ||
| exponentInput = 0, | ||
| ) { |
There was a problem hiding this comment.
Ok, I can live with this, but probably just because it is a helper utility. Otherwise, think we should default to classes.
packages/types/src/pnum.ts
Outdated
| input instanceof Amount || | ||
| (typeof input === 'object' && 'lo' in input && 'hi' in input && input.lo && input.hi) |
There was a problem hiding this comment.
No test appears to fail when removing that line. Can you add a test for coverage for that?
|
nice helper function, I've been wanting something like this for a while 👍 |
| @@ -4,9 +4,10 @@ import { removeTrailingZeros } from './shortify.js'; | |||
| export type RoundingMode = 'half-up' | 'up' | 'down'; | |||
|
|
|||
| export interface RoundOptions { | |||
| value: number; | |||
| value: number | string; | |||
There was a problem hiding this comment.
For @grod220 (because I know you want to know why) this is needed for a big number (caught by one of the tests)
Currently we have a bunch of conversion functions scattered around in:
This aims to unify and simplify number conversions, see the function description at the top for context.