feature(twap-orders): refactor NumberOfParts and Slippage (9) #2479
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
src/cow-react/modules/advancedOrders/state/advancedOrdersSettingsAtom.ts
Outdated
Show resolved
Hide resolved
| const setSlippage = useSetSlippage() | ||
|
|
||
| return useCallback( | ||
| (value: string) => { | ||
| // populate what the user typed and clear the error | ||
| setSlippageInput(value) | ||
| setSlippageError(false) | ||
| setSlippageError(null) | ||
| setSlippageWarning(null) | ||
|
|
||
| if (value.length === 0) { |
There was a problem hiding this comment.
I think the logic with parsing value must be inside TradeNumberInput.
const onSlippageChange = (value: null | number) => {
setSlippage(value === null ? 'auto' : value)
}
return < TradeNumberInput ...props, decimals={2} onChange={onSlippageChange}>
There was a problem hiding this comment.
Not sure if you meant inside the AdvancedOrdersWidget because slippage and number of parts use different parsing, so it can't be in TradeNumberInput. But this hook useParseSlippage has a lot of code and I don't want to put all that code inside the AdvancedOrdersWidget and the same for number of parts parsing. That is why I prefered to have 2 different components for slippage and number of parts
| updateAdvancedOrdersSettingsAtom, | ||
| NumberOfPartsError, | ||
| } from '@cow/modules/advancedOrders/state/advancedOrdersSettingsAtom' | ||
| import { updateAdvancedOrdersSettingsAtom } from '@cow/modules/advancedOrders/state/advancedOrdersSettingsAtom' | ||
| import { MIN_PARTS_NUMBER, MAX_PARTS_NUMBER } from '@src/constants' | ||
|
|
||
| export function useParseNumberOfParts() { |
There was a problem hiding this comment.
I think we should continue to follow the pattern like in updateLimitOrdersAtom.
And I guess we won't need this hook if we move parsing logic to TradeNumberInput, see: https://github.com/cowprotocol/cowswap/pull/2479/files#r1198591948
There was a problem hiding this comment.
Similar to my comment above #2479 (comment)
src/cow-react/modules/advancedOrders/containers/AdvancedOrdersWidget/index.tsx
Outdated
Show resolved
Hide resolved
| const parseNumberOfParts = useParseNumberOfParts() | ||
|
|
||
| // Slippage | ||
| const [slippageInput, setSlippageInput] = useState('') |
There was a problem hiding this comment.
In general, the logic should fit this diagram.
Basic principles:
TradeNumberInputreturns onlynumberornullTradeNumberInputhasprecisiona number of digits after comma (like here)- Validation of slippage and numOfParts should be presented as a pure function because it's deterministic and depends only on
number | nullvalue - Since a validation error is deterministic, we don't need to store it. It always can be calculated on flight
There was a problem hiding this comment.
Since a validation error is deterministic
I'm not sure about this, because we will probably use those errors for Buttons state (disabled/enabled) and I think its easier to just access it from the state instead of passing it down
There was a problem hiding this comment.
Why it's not deterministic? It supposed to be a pure function
There was a problem hiding this comment.
I would store it because it will be used in different components
| setSlippageError, | ||
| setSlippageWarning, | ||
| }) | ||
| const displaySlippageValue = useDisplaySlippageValue(slippageInput) |
There was a problem hiding this comment.
I think the hook is redundant. The value of slippage in atom should be ready for displaying as is
There was a problem hiding this comment.
We set slippage to auto in atom but we don't want to show auto in UI 🤔
|
@nenadV91 please get approvals before merging. |
|
@alfetopito I should have mentioned that we are merging all of these PR's into this one #2426, my mistake, sorry about that. |
Summary
Refactors NumberOfParts and Slippage components
As suggested by @shoom3301 these two components will not use the shared
TradeNumberInputcomponentTo test