-
Notifications
You must be signed in to change notification settings - Fork 89
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
[SWAP REFACTORING #2] refactor swap business logic #1039
[SWAP REFACTORING #2] refactor swap business logic #1039
Conversation
… refactor/wrap-native-token � Conflicts: � src/custom/hooks/useSwapCallback.ts � src/custom/pages/Swap/SwapMod.tsx � src/custom/pages/Swap/index.tsx
Important: example how to customize components - DestCurrencyInputPanel
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.
Nice one
|
||
export async function swapFlow(input: SwapFlowContext) { | ||
logSwapFlow('STEP 1: confirm price impact') | ||
if (input.context.priceImpact && !confirmPriceImpactWithoutFee(input.context.priceImpact)) return |
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.
Of the top of my head, this could be broken down a bit into smaller functions
also, theres a bit of repetition why not destructuring input to have some consts.
I will want to get to the last PR to review the code and see the result of the full refactor. Observing every commit makes it a bit hard to review it correctly. Lets revisit in the tip of the waterfall
import { getProviderErrorMessage, isRejectRequestProviderError } from 'utils/misc' | ||
import { logSwapFlow } from 'pages/Swap/swapFlow/logger' | ||
|
||
export async function swapFlow(input: SwapFlowContext) { |
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.
One thing I find strange in this PR is that we are adding a lot of logics in the pages. Is this intended? do we want it there or in the components?
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.
It must be discussed.
I did it intentionally, but not with the goal of placing it in the page directory, but in order to highlight it in the domain.
return Math.min(validTo, MAX_VALID_TO_EPOCH) | ||
} | ||
|
||
export function useSwapFlowContext(): SwapFlowContext | null { |
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 kind of a big hook, although cool that we get all the swap context.
Probably we will find ways later on to keep simplifying and cleaning up. Maybe when we refactor further for limit orders
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.
For sure, this hooks is an intermediate solution
[SWAP REFACTORING #5] code review fixes
[SWAP REFACTORING #4.5] new swap page, cosmos fixtures and documentation
[SWAP REFACTORING #4.4] new swap page, merge develop, code cleanup
[SWAP REFACTORING #4.3] new swap page, recipient controls, trade rates, high slippage warning
[SWAP REFACTORING #4.2] new swap page, bind data rendering and logic
[SWAP REFACTORING #4.1] new swap page, setup layout
[SWAP REFACTORING #3] refactor wrap/unwrap flow
Summary
https://docs.google.com/document/d/1QCuT2zT-G7fKB5azLmssj8O3yd2dwBpDuoRJuz0cw0U/edit#
How to review
Sorry for the big PR, for easier code review, please review per commit, starting with the oldest
Refactoring PR's list:
4.1. Setup layout
4.2. Bind data rendering and logic
4.3. Recipient controls, trade rates, high slippage warning
4.4. Merge develop, code cleanup
4.5. Cosmos fixtures and documentation
Testing
Don't test this PR, test the final PR - New swap page