feat: asset-value #1206
feat: asset-value #1206
Conversation
pastaghost
commented
Feb 22, 2023
•
pastaghost
commented
- adds new asset-value package
| }, | ||
| "repository": { | ||
| "type": "git", | ||
| "url": "https://github.com/pastaghost/asset-value" |
There was a problem hiding this comment.
update with shapeshift repo
| state: AssetValueState | ||
|
|
||
| public constructor(params: AssetValueParams | SerializedAssetValue) { | ||
| if (isAssetValueParams(params)) { |
There was a problem hiding this comment.
wouldn't an if (typeof params === 'string') { doSerializedStuff } be more straight forward?
| return `${serializedState}|${hash}` as SerializedAssetValue | ||
| } | ||
|
|
||
| public mult = this.multipliedBy.bind(this) |
There was a problem hiding this comment.
to follow bignumberjs shorthand
| public mult = this.multipliedBy.bind(this) | |
| public times = this.multipliedBy.bind(this) |
|
|
||
| public mult = this.multipliedBy.bind(this) | ||
| public div = this.dividedBy.bind(this) | ||
| public gte = this.isGreaterThanOrEqualTo.bind(this) |
|
|
||
| //TODO(pastaghost): Add JSDoc documentation | ||
| export class AssetValue { | ||
| state: AssetValueState |
There was a problem hiding this comment.
reason for object over standard class vars? also should probably make private to avoid mutation outside of class
| break | ||
| } | ||
| this.state = { | ||
| assetId: params.asset ? params.asset.assetId : params.assetId, |
There was a problem hiding this comment.
shorthand option available for setting these values
| assetId: params.asset ? params.asset.assetId : params.assetId, | |
| assetId: params.assetId ?? params.asset.assetId, |
| PRECISION = 2, | ||
| } | ||
|
|
||
| export type AssetValueParams = { |
There was a problem hiding this comment.
personally would just add these types along side asset value class for easier readability in such a small package (could just reduce to single index.ts really imo)
| @@ -0,0 +1,2 @@ | |||
| export const CHECKSUM_LENGTH = 8 | |||
| export const DEFAULT_FORMAT_DECIMALS = 6 | |||
There was a problem hiding this comment.
could be useful to support configurable DEFAULT_FORMAT_DECIMALS possibly?
| if (!(serializedState && hash)) { | ||
| throw new Error('Cannot initialize AssetValue from improperly-formatted SerializedAssetValue') | ||
| } | ||
| const parsedState = JSON.parse(serializedState) |
There was a problem hiding this comment.
I don't recall the thrown error from JSON.parse, but it can throw if you want to throw a custom error
| } | ||
| const parsedState = JSON.parse(serializedState) | ||
| if (!(parsedState && parsedState.a && parsedState.p && parsedState.v)) { | ||
| throw new Error('Cannot initialize AssetValue from underspecified SerializedAssetValue') |
There was a problem hiding this comment.
could be extra nice and keep track of missing keys and provide as detail for the error
| v: this.state.value, | ||
| }) | ||
| const hash = Md5.hashStr(serializedState).slice(0, CHECKSUM_LENGTH) | ||
| return `${serializedState}|${hash}` as SerializedAssetValue |
There was a problem hiding this comment.
don't think as cast should be necessary
| } | ||
|
|
||
| //TODO(pastaghost): Add JSDoc documentation | ||
| export class AssetValue { |
There was a problem hiding this comment.
my only other thought is around the use of bnOrZero throughout in relation to being a big number wrapped asset. kind of makes it unpure in a sense, though it is for our use case and we use that for all of our bignumber math (I still have mixed feeling about this in general but it is what it is haha)
| } | ||
|
|
||
| public dividedBy(factor: string | number): AssetValue { | ||
| const value = bnOrZero(this.state.value).dividedBy(bnOrZero(factor)).toFixed() |
There was a problem hiding this comment.
I have mentioned staying away from bnOrZero specifically for dividedBy. Result is Infinity vs NaN with toFixed. Honestly not sure how we handle either value in the client throughout though...
