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.

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

refactor(swap): Move calculations to swap package #2697

Merged
merged 21 commits into from
Sep 23, 2023

Conversation

jorbuedo
Copy link
Contributor

@jorbuedo jorbuedo commented Sep 20, 2023

  • Removes calculations from app

  • Adds calculations in swap reducer

  • Limit calculations need discussing

  • We use in some places the pool.price, which is always tokenA/tokenB. We would have to calculate it instead, to use tokenB/tokenA when appropriate.

  • Precision, divisions, bigint, binumber, etc. may need a second pass

@jorbuedo jorbuedo added help wanted Extra attention is needed wip Shows that a PR shouldn't be merge dont-merge labels Sep 20, 2023
@jorbuedo jorbuedo self-assigned this Sep 20, 2023
@jorbuedo jorbuedo closed this Sep 21, 2023
@jorbuedo jorbuedo force-pushed the calculate-quantities-in-package branch from 91ffb4f to 81f8582 Compare September 21, 2023 11:45
@jorbuedo jorbuedo reopened this Sep 21, 2023
@jorbuedo jorbuedo marked this pull request as ready for review September 21, 2023 12:25
@jorbuedo jorbuedo requested a review from stackchain September 21, 2023 12:25
@jorbuedo jorbuedo added refactor and removed help wanted Extra attention is needed wip Shows that a PR shouldn't be merge dont-merge labels Sep 21, 2023
@jorbuedo jorbuedo added wip Shows that a PR shouldn't be merge dont-merge labels Sep 21, 2023
@jorbuedo jorbuedo marked this pull request as draft September 21, 2023 12:47
@stackchain stackchain added this to the 5.0.0 milestone Sep 21, 2023
@stackchain stackchain requested a review from lisicky September 22, 2023 07:56
@jorbuedo jorbuedo requested a review from stackchain September 22, 2023 08:37
@jorbuedo jorbuedo added help wanted Extra attention is needed and removed wip Shows that a PR shouldn't be merge labels Sep 22, 2023
Copy link
Contributor

@SorinC6 SorinC6 left a comment

Choose a reason for hiding this comment

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

@stackchain related to the price calculation when there is a limit order , I will propose to use the old formula for now - until we got more clarification specific to why is not taking the pool fee in consideration just to get the PR unblocked and will update it after more tests

  • regarding pool.price let's use tokenB/tokenA when appropriate 🤔

@jorbuedo jorbuedo requested a review from SorinC6 September 22, 2023 14:43
@jorbuedo jorbuedo removed help wanted Extra attention is needed dont-merge labels Sep 22, 2023
@jorbuedo jorbuedo marked this pull request as ready for review September 22, 2023 14:45
@@ -46,13 +46,20 @@ export const CreateOrder = () => {
tokenB: createOrder.amounts.buy.tokenId ?? '',
})

const bestPool = useMemo(() => {
if (poolList !== undefined && poolList.length > 0) {
return poolList.map((a) => a).sort((a, b) => a.price - b.price)[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the .map needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MichalSzorad sort modifies the array, which could affect other things outside this function. toSorted would be the newer function, but doesn't seem to work in our setup. The map is just to clone it. Could have used any of the ways to do it, come to think of it [...poolList].sort(...) may have been shorter.

Signed-off-by: Juliano Lazzarotto <30806844+stackchain@users.noreply.github.com>
Signed-off-by: Juliano Lazzarotto <30806844+stackchain@users.noreply.github.com>
Signed-off-by: Juliano Lazzarotto <30806844+stackchain@users.noreply.github.com>
…CreateOrder/CreateOrder.tsx

Signed-off-by: Juliano Lazzarotto <30806844+stackchain@users.noreply.github.com>
@stackchain stackchain merged commit 6068f12 into develop Sep 23, 2023
@stackchain stackchain deleted the calculate-quantities-in-package branch September 23, 2023 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants