-
Notifications
You must be signed in to change notification settings - Fork 50
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
feat(swap): Slippage screen #2639
Conversation
} | ||
|
||
const normalizeInputValue = (value: string) => { | ||
return value.length === 0 ? '0' : value.replace(',', '.').replace(/^0+(?=\d)/, '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- When the input is an empty string, convert to '0'
- Convert ',' to '.'
- Remove leading zeros for inputs like '03'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 @MichalSzorad can you check to use intl
number formatter? It might affect these rules. I think @nistadev updated extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @stackchain I'll have a look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stackchain Would we need a library like this one d2l-intl
to be able to parse the number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added parsing using BigNumber, but I don't think we can format the output using BigNumber, can we? Eg when the user types 3.
we do not want to convert the number to 3 and this seems what BigNumber is doing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still WIP, as I'm trying to integrate the util made by Jordi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is now integrated @stackchain
@@ -103,12 +117,11 @@ const invalidSwapContext = () => { | |||
|
|||
// * === SETTINGS === | |||
// * NOTE maybe it should be moved as part of wallet settings package | |||
export const useSwapSlippage = () => { | |||
const {slippage} = useSwap() | |||
export const useSwapSlippage = (swapManager: Readonly<Swap.Manager>) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what this change for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to reuse the hook in SwapProvider
to get the default values from the storage and the swapManager
is responsible for storing the values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⬆️
@@ -61,7 +61,7 @@ export const EditSlippageScreen = () => { | |||
} | |||
|
|||
const onSubmit = () => { | |||
const parsedNumber = selectedChoice ? BigNumber(inputValue, 10) : BigNumber(selectedChoice.value, 10) | |||
const parsedNumber = isSelectedChoiceManual ? BigNumber(inputValue, 10) : BigNumber(selectedChoice.value, 10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
according to confluence docs, it should .format
to 1 decimal only, can you check it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stackchain there is a validateSlippage
which checks if only 1 decimal is used. The button is disabled if there are more than 1 decimals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the decimal check from the validateSlippage
, because this is now validated in Quantities.parseFromText
After #2646 is merged, I'll rebase and use same approach to parse slippage |
Resolves YOMO-610