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.

Sign up for GitHub

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

fix(swap): Fix limit price precision / denomination #2771

Merged
merged 11 commits into from
Oct 15, 2023
Merged

fix(swap): Fix limit price precision / denomination #2771

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b84a84a
Update EditLimitPrice.tsx
michaeljscript Oct 13, 2023
cb9efb0
Update parseFromText
michaeljscript Oct 13, 2023
db063e8
Revert previous changes
michaeljscript Oct 13, 2023
37123ab
Merge branch 'develop' into fix/fix-limit-price-calc
michaeljscript Oct 13, 2023
07ac81b
copy parseFromText
michaeljscript Oct 13, 2023
8cd6f0b
refactor decimal check
michaeljscript Oct 13, 2023
fec3d8e
Add comment
michaeljscript Oct 13, 2023
d46bc91
CR
michaeljscript Oct 13, 2023
b03200a
Add tests
michaeljscript Oct 13, 2023
de3565d
Add more tests
michaeljscript Oct 13, 2023
2860d16
Refactor parseFromText
michaeljscript Oct 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions apps/wallet-mobile/src/features/Swap/useCases/StartSwapScreen/CreateOrder/EditLimitPrice.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,9 @@ export const EditLimitPrice = () => {
}, [orderData.type, orderData.limitPrice, orderData.amounts.sell, denomination, orderData.selectedPoolCalculation])

const onChange = (text: string) => {
const [formattedPrice, price] = Quantities.parseFromText(text, PRECISION, numberLocale)
const value = Quantities.denominated(price, PRECISION)

const [formattedPrice, price] = Quantities.parseFromText(text, denomination, numberLocale, PRECISION)
setText(formattedPrice)
limitPriceChanged(value)
limitPriceChanged(price)
}

return (
Expand Down
8 changes: 4 additions & 4 deletions apps/wallet-mobile/src/yoroi-wallets/utils/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ export const Quantities = {

return absoluteQuantity.isEqualTo(minimalFractionalPart)
},
parseFromText: (text: string, denomination: number, format: NumberLocale) => {
parseFromText: (text: string, denomination: number, format: NumberLocale, precision = denomination) => {
const {decimalSeparator} = format
const invalid = new RegExp(`[^0-9${decimalSeparator}]`, 'g')
const sanitized = text === '' ? '' : text.replaceAll(invalid, '')
Expand All @@ -142,13 +142,13 @@ export const Quantities = {
const parts = sanitized.split(decimalSeparator)
const isDec = parts.length >= 2
jorbuedo marked this conversation as resolved.
Show resolved Hide resolved

jorbuedo marked this conversation as resolved.
Show resolved Hide resolved
const fullDecValue = isDec ? `${parts[0]}${decimalSeparator}${parts[1].slice(0, denomination)}1` : sanitized
const fullDecValue = isDec ? `${parts[0]}${decimalSeparator}${parts[1].slice(0, precision)}1` : sanitized
const fullDecFormat = new BigNumber(fullDecValue.replace(decimalSeparator, '.')).toFormat()
const input = isDec ? fullDecFormat.slice(0, -1) : fullDecFormat
Copy link
Member

@stackchain stackchain Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

move inside the if, add a comment that the trailing is removed inside the hasDecimals logic

Copy link
Contributor

Choose a reason for hiding this comment

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

You'll have to declare input as let before and move both lines inside the if

Which is why I preferred everything as const and the ternaries 😅

Copy link
Member

@stackchain stackchain Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

BigNumber is object so const/let won't make that difference cuz you still can mutate it, but I also prefer and agree on not changing after declaring with initialization. To look even better I'd move everything dec / not decimal and return based on that, we will have some duplication, but readability is more important than duplication.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's because I wrote it, but it looked more clear to me before. Now with the lets, an if dividing everything, the early return and a couple lines duplicated because of it, looks harder to follow to me.


const value = isDec ? `${parts[0]}${decimalSeparator}${parts[1].slice(0, denomination)}` : sanitized
const value = isDec ? `${parts[0]}${decimalSeparator}${parts[1].slice(0, precision)}` : sanitized
const quantity = new BigNumber(value.replace(decimalSeparator, '.'))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const quantity = new BigNumber(value.replace(decimalSeparator, '.'))
const quantity = asQuantity(new BigNumber(value.replace(decimalSeparator, '.'))

.decimalPlaces(denomination)
.decimalPlaces(precision)
.shiftedBy(denomination)
.toString(10)

Expand Down
7 changes: 4 additions & 3 deletions packages/swap/src/utils/quantities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ export const Quantities = {
text: string,
denomination: number,
format: Numbers.Locale,
precision = denomination,
) => {
const {decimalSeparator} = format
const invalid = new RegExp(`[^0-9${decimalSeparator}]`, 'g')
Expand All @@ -85,18 +86,18 @@ export const Quantities = {
const isDec = parts.length >= 2

const fullDecValue = isDec
? `${parts[0]}${decimalSeparator}${parts[1]?.slice(0, denomination)}1`
? `${parts[0]}${decimalSeparator}${parts[1]?.slice(0, precision)}1`
: sanitized
const fullDecFormat = new BigNumber(
fullDecValue.replace(decimalSeparator, '.'),
).toFormat()
const input = isDec ? fullDecFormat.slice(0, -1) : fullDecFormat

const value = isDec
? `${parts[0]}${decimalSeparator}${parts[1]?.slice(0, denomination)}`
? `${parts[0]}${decimalSeparator}${parts[1]?.slice(0, precision)}`
: sanitized
const quantity = new BigNumber(value.replace(decimalSeparator, '.'))
.decimalPlaces(denomination)
.decimalPlaces(precision)
.shiftedBy(denomination)
.toString(10)

Expand Down