Update default slippage #718
Conversation
|
📦 build.zip [updated at Feb 20, 7:33:39 AM UTC] |
| ); | ||
|
|
||
| const { defaultSlippagePercent } = getSlippageOptions(chain); | ||
| const slippage = userSlippage ?? defaultSlippagePercent / 100; |
There was a problem hiding this comment.
I think, it can be more consistent to use the same units all over the code.
What do you think about just have defaultSlippage = 0.01?
There was a problem hiding this comment.
However, I can see that is was also this inconsistent before, so not a big problem then
There was a problem hiding this comment.
I agree, I am also not happy about this but decided to leave it as it was before. I mean I didn't want to refactor this as part of the PR and keep changes minimal, easier to review
| ? toPercents(userSlippage) | ||
| : defaultSlippagePercent; | ||
|
|
||
| const [percentValue, setPercentValue] = useState(() => String(slippage)); |
There was a problem hiding this comment.
Can we cast String() in the previous expression? And use here just useState(slippage)?
|
|
||
| const DEFAULT_SLIPPAGE_OPTIONS: SlippageOptions = [1, [0.5, 1.0]]; | ||
| const SLIPPAGE_BY_CHAIN: SlippageConfig = { | ||
| ethereum: [1, [0.2, 0.5]], |
There was a problem hiding this comment.
shouldn't it be ethereum: [0.5, [0.2, 0.5]],?
There was a problem hiding this comment.
Ah, I see. Well let's make this an array then? This first value is a bit confusing :)
There was a problem hiding this comment.
Yeah... maybe, this is why I added comment above the SlippageOptions types:
// [defaultOptionIndex, [option1, option2]]
The first field is the defaultOptionIndex
There was a problem hiding this comment.
Just updated this part, it is an object now
There was a problem hiding this comment.
I changed these types as we discussed ✅
| import type { Chain } from 'src/modules/networks/Chain'; | ||
|
|
||
| // [defaultOptionIndex, [option1, option2]] | ||
| type SlippageOptions = [number, [number, number]]; |
There was a problem hiding this comment.
What do you think about making this an object?
type SlippageOptions = {
default: number,
options: [number, number]
}
There was a problem hiding this comment.
I initially implemented the SlippageOptions type as an object type, exactly as shown in your snippet. However, I later reconsidered this approach as I dislike the repetition of field names, especially considering we might switch to firebase configuration in the future. In my opinion, a more elegant solution would be to use a direct "tuple type" like this: type SlippageOptions = [number, number, number]; with a descriptive comment above. But perhaps this is just my preference...
There was a problem hiding this comment.
Just discussed this with @everdimension , going to update this type to: { default: number; options: [number, number]; }
| </> | ||
| )} | ||
| renderWhenOpen={() => { | ||
| invariant(chain, 'Chain must be defined'); |
There was a problem hiding this comment.
Can't this lead to an error for the user? Shouldn't we just wait until chain is not null?
There was a problem hiding this comment.
Ok, I see that the line that should call for this view shows only when chain is not null. So may be there is no problem
There was a problem hiding this comment.
Yes, we have similar invariant below, shouldn't be a problem
|
We need to change validation logic here: |
|
Also now we show warning line when slippage is 1% |
|
|
||
| const { default: defaultSlippagePercent } = getSlippageOptions(chain); | ||
| const slippagePercent = String( | ||
| userSlippage ? toPercents(userSlippage) : defaultSlippagePercent |
There was a problem hiding this comment.
We shouldn't forget this PR top be merged https://github.com/zeriontech/transactions-ui/pull/15/files and package.json to be updated
There was a problem hiding this comment.
Also here we need to use ?? instead of ?
Slippage: 0 technically should be possible.
| ); | ||
|
|
||
| const { default: defaultSlippagePercent } = getSlippageOptions(chain); | ||
| const slippage = userSlippage ?? defaultSlippagePercent / 100; |
There was a problem hiding this comment.
Let's share this code together. Maybe inside a function. Cause right now the have different implementation even with the fact that they should be almost similar :))
As an idea, getSlippageOptions can take userSlippage as an optional param and take this into consideration
|
|
||
| const price = swapView.receiveAsset?.price?.value || 0; | ||
| const fiatValue = new BigNumber(receiveInput || 0) | ||
| .times(price) | ||
| .times(1 - slippage); | ||
|
|
||
| return slippage >= HIGH_SLIPPAGE_THRESHOLD ? ( | ||
| return slippage > HIGH_SLIPPAGE_THRESHOLD ? ( |
There was a problem hiding this comment.
There was a discussion about this logic not so long ago but I think, this is ok for now. I don't want to make HIGH_SLIPPAGE_THRESHOLD dependant on chain too in this PR
| getSlippageOptions(chain); | ||
|
|
||
| const slippage = String( | ||
| userSlippage ? toPercents(userSlippage) : defaultSlippagePercent |
There was a problem hiding this comment.
No description provided.