Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

корректное отображение минимальноый суммы #1567

Merged
merged 2 commits into from Feb 14, 2019

Conversation

Pasha8914
Copy link
Collaborator

@Pasha8914 Pasha8914 commented Feb 13, 2019

Pull request template

Checklist

  • I have read the CONTRIBUTING guide
  • branch rebased on develop
  • styleguide check npm run validate
  • good naming
  • keep simple
  • video or screenshot attached
  • testing instruction attached
  • check your PR once again

Description

Video or screenshot

https://www.useloom.com/share/25dca9c5a5d64faa87baabd80858a190

What and how to test

const linked = Link.all(this, 'exchangeRate', 'buyAmount', 'sellAmount')
const minAmountSell = coinsWithDynamicFee.includes(sellCurrency) ? minimalestAmountForSell : minAmountOffer[sellCurrency]
const minAmountBuy = coinsWithDynamicFee.includes(buyCurrency) ? minimalestAmountForBuy : minAmountOffer[buyCurrency]

const minimalAmountSell = !isToken ? Math.floor(minAmountSell * 1e6) / 1e6 : 0
const minimalAmountBuy = !isToken ? Math.floor(minAmountBuy * 1e6) / 1e6 : 0
const minimalAmountSell = !isTokenSell ? Math.floor(minAmountSell * 1e6) / 1e6 : 0
Copy link

@ghost ghost Feb 13, 2019

Choose a reason for hiding this comment

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

То есть, если токен, то минималка будет 0? Я могу свопнуться на 1е-20 или 1е-22 токена, хотя его decimals максимум 18, и следовательно минималка реальная 1е-18

Вынести этот метод в heplpers.ethToken.estimateMinAmount({ name } = {})

screenshot_20190213_181847

Добавь в него const decimals = constants.tokenDecimals[name.toLowerCase()] вместо currentDecimals
А лучше переименуй constants.tokenDecimals -> constants.decimals, так как там не только токены. Этот конфиг используется только в двух модалках, включая эту.

И присваивай minAmount токенам по их имени.

Math.floor(minAmountSell * 1e6) / 1e6 лучше заменить на BigNumber(x).dp(decimals, 6)

Suggested change
const minimalAmountSell = !isTokenSell ? Math.floor(minAmountSell * 1e6) / 1e6 : 0
const minimalAmountSell = !isTokenSell
? BigNumber(minAmountSell).dp(constants.tokenDecimals[sellCurrency.toLowerCase()], BigNumber.ROUND_DOWN).toSting()
: helpers.ethToken.estimateMinAmount({ name: sellCurrency })

Такая ситуация с токеном все еще актуальна:

screenshot_20190213_181015

Copy link
Collaborator

Choose a reason for hiding this comment

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

Math.floor(minAmountSell * 1e6) / 1e6 лучше заменить на Number(x).dp(decimals, 6)

BigNumber?

Copy link

Choose a reason for hiding this comment

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

Math.floor(minAmountSell * 1e6) / 1e6 лучше заменить на Number(x).dp(decimals, 6)

BigNumber?

Конечно! Опечатка просто


const isDisabled = !exchangeRate
|| !buyAmount && !sellAmount
|| sellAmount > balance
|| !isToken && sellAmount < minimalAmountSell
Copy link
Collaborator

Choose a reason for hiding this comment

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

Мы же тут уже исправляли, тут нужен BN


if (linked.sellAmount.value !== '') {
if (linked.sellAmount.value !== '' && minimalAmountSell > 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

и тут!

Copy link
Collaborator

@caffeinum caffeinum left a comment

Choose a reason for hiding this comment

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

тебе нужно сделать мерж developa, только очень аккуратно, и ни в коем случае не делай force push

const minimalAmountBuy = !isTokenBuy ? Math.floor(minAmountBuy * 1e6) / 1e6 : 0
const minimalAmountSell = !isTokenSell
? coinsWithDynamicFee.includes(sellCurrency) ? minimalestAmountForSell : minAmountOffer[buyCurrency]
: 0.001
Copy link
Collaborator

Choose a reason for hiding this comment

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

Не понял, что тут проверяется. 0.001 это для такого случая? Для всех не-токенов?

Copy link

Choose a reason for hiding this comment

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

Для всех токенов

await this.switching()
} else {

this.switching()
Copy link

Choose a reason for hiding this comment

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

Зачем убрал await у асинхронного метода?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

смысла нет

Copy link

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

спасибо

@@ -142,8 +151,7 @@ export default class AddOffer extends Component {

if (sellCurrency === value) {
await this.switching()
} else {
Copy link

Choose a reason for hiding this comment

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

Почему убрaл else?

Copy link

Choose a reason for hiding this comment

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

В switching() полностью дублирует код ниже для случая, если нужно поменять местами валюты.

Copy link

Choose a reason for hiding this comment

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

Лучше подумать, как сделать из них один метод.

screenshot_20190215_002807


this.setState({
buyCurrency: value,
sellAmount: Number.isNaN(sellAmount) ? '' : sellAmount,
Copy link

Choose a reason for hiding this comment

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

Зачем снова вернул sellAmount, buyAmount в этот стейт, если это делается в handleBuyAmountChange, handleSellAmountChange ?

@@ -364,10 +390,8 @@ export default class AddOffer extends Component {
buyCurrency: sellCurrency,
}))

if (sellAmount > 0 || buyAmount > 0) {
this.handleBuyAmountChange(sellAmount)
Copy link

@ghost ghost Feb 14, 2019

Choose a reason for hiding this comment

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

А это зачем убрал? Теперь при смене направлений оба инпута обнуляются

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

это логично

@Pasha8914 Pasha8914 reopened this Feb 14, 2019
@Pasha8914 Pasha8914 merged commit 9f4ef32 into develop Feb 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants