Skip to content
This repository has been archived by the owner on Jan 15, 2021. It is now read-only.

[Fixes] Warning rounding calculation error + remove smart format from receive amount #1583

Merged
merged 2 commits into from
Nov 4, 2020

Conversation

W3stside
Copy link
Contributor

@W3stside W3stside commented Nov 4, 2020

Fixes calculation issues found in confirmation modal price impact and fixes smart formatting for small amounts in Receive at least input when using really low price and/or 0 sell amount

@ghost
Copy link

ghost commented Nov 4, 2020

Travis automatic deployment:

@W3stside
Copy link
Contributor Author

W3stside commented Nov 4, 2020

merging without approval as it's late - checked that it works.

change was minor, and introduces no new features

@W3stside W3stside merged commit dd27f47 into release/v1.6.0 Nov 4, 2020
@W3stside W3stside mentioned this pull request Nov 9, 2020
Comment on lines +31 to 35
export function formatPriceToPrecision(price: BigNumber, useThreshold = false): string {
return price.gt(LOW_PRICE_FLOOR) || !useThreshold
? price.toFixed(PRICE_ESTIMATION_PRECISION)
: '< ' + LOW_PRICE_FLOOR.toString()
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

U know I'm not fan of this expressions. They are too complicated. I spent a bit watching until I understood, what means that if I someone don't want to dedicate that time, will overlook if there's an error in the logic

I know that this one is not that bad, but I still find it important to keep it simple. Also it's not optimal (I don't care that much about this part), since you calculate price.gt(LOW_PRICE_FLOOR) when is not needed (in !useThreshold case)

Suggested change
export function formatPriceToPrecision(price: BigNumber, useThreshold = false): string {
return price.gt(LOW_PRICE_FLOOR) || !useThreshold
? price.toFixed(PRICE_ESTIMATION_PRECISION)
: '< ' + LOW_PRICE_FLOOR.toString()
}
export function formatPriceToPrecision(price: BigNumber, useThreshold = false): string {
if (useThreshold) {
return price.gt(LOW_PRICE_FLOOR) ? price.toFixed(PRICE_ESTIMATION_PRECISION) : '< ' + LOW_PRICE_FLOOR.toString()
} else {
return price.toFixed(PRICE_ESTIMATION_PRECISION)
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternative, if u don't mind to generate the formatted string even if u don't use it sometimes, its slightly more readable:

export function formatPriceToPrecision(price: BigNumber, useThreshold = false): string {
  const priceFormatted = price.toFixed(PRICE_ESTIMATION_PRECISION)
  if (useThreshold) {
    return price.gt(LOW_PRICE_FLOOR) ? priceFormatted : '< ' + LOW_PRICE_FLOOR.toString()
  } else {
    return priceFormatted
  }
}

Furthermore, u can create a constant:

const LOWER_PRICE_VALUE =  '< ' + LOW_PRICE_FLOOR.toString()

then it can be even simpler:

export function formatPriceToPrecision(price: BigNumber, useThreshold = false): string {
  const priceFormatted = price.toFixed(PRICE_ESTIMATION_PRECISION)
  if (useThreshold) {
    return price.gt(LOW_PRICE_FLOOR) ? priceFormatted : PRICE_ESTIMATION_PRECISION
  } else {
    return priceFormatted
  }
}

I don't want to over discuss 3 lines of code, I would like just to agree that || expressions with !negations and ternary expressions, all at once are evil, and damage a bit the brain of the reader

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants