fix(combinedBalances): Optimize balance diff calculations #5082

yvesfracari · 2024-11-11T19:20:36Z

Summary

On the current implementation of the combinedBalances module the swap widget on the Hook tab is slow / stuck. This is happening because the applyBalanceDiffs to add the token amount changes from pre-hooks is slow and is being triggered more times than it should be.

This PR resolves these two issues by:

Refactoring the combinedBalances from memory to a state;
Using an Updater strategy to only update the state once per parameter change;
Optimize the applyBalanceDiffs function knowing that the currentBalances object is larger than the balanceDiff object.

To Test

Open the Hook tab
Add a pre-hook that would add tokens to you (I usually use the Claim Airdrop on Sepolia or Withdraw CoW AMM on any chain)
Test the swap widget if it the UX is smooth or not.

Also, this modification shouldn't impact the other widgets or pages of the app.

…fs function

vercel · 2024-11-11T19:20:40Z

@yvesfracari is attempting to deploy a commit to the cow Team on Vercel.

A member of the Team first needs to authorize it.

anxolin

Thanks for the fix

anxolin · 2024-11-11T19:36:22Z

apps/cowswap-frontend/src/modules/combinedBalances/updater/BalancesCombinedUpdater.tsx

+      return
+    }
+    const accountBalancesDiff = preHooksBalancesDiff[account.toLowerCase()] || {}
+    setBalancesCombined(applyBalanceDiffs(tokenBalances, accountBalancesDiff))


Do you know why this is being re-computed still every few seconds?

Maybe this can help to figure it out https://github.com/simbathesailor/use-what-changed

IIUC is because the tokenBalances state is updated every few seconds.

Then our fault, we will need to look at this tomorrow, cause my balance were not changing, so probably is not correctly memorized and its likely there's a lot more parts of the app that gets redrawn because of this

vercel · 2024-11-11T20:28:55Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
explorer-dev	✅ Ready (Inspect)	Visit Preview	Nov 13, 2024 3:54pm
swap-dev	✅ Ready (Inspect)	Visit Preview	Nov 13, 2024 3:54pm
widget-configurator	✅ Ready (Inspect)	Visit Preview	Nov 13, 2024 3:54pm

elena-zh · 2024-11-12T09:42:24Z

Hey @yvesfracari , when I imported a LP on Sepolia (0x9d5899bd9220515a6d9300a3f4071b1daf0fd337), the app got crashed:

Steps that I did:

Opened Hooks tab
Connected to a wallet on Sepolia
Added a hook to withdraw liquidity with the token above
Imported this token

This does not happen on Develop. in this PR only

elena-zh · 2024-11-12T10:04:06Z

It might be a complex scenario that was not in the scope of work, so I'd better ask:
I see that balance incrementation from a pre-hook is not reflected in a post-hook. Is this an expected behavior?
See: here I add a pre-hook that changes my sdai balance

But a post hook does not 'see' it:

elena-zh · 2024-11-12T10:16:13Z

Some more questions:

when I press on the Swap and go to the Confirm modal, I can see that tenderly simulation is running there but we don't show simulation report there yet. Should we?
And when I go back, the simulation is also running. Is this expected?
In my case the simulation failed , but I had not changed anything. I didn't see the balance incrementation any more. I updated the hook, and noticed that the reason was in the balance change (e.g. 1% is not equal to the previous token's amount to withdraw).. Is this an expected behavior?

elena-zh · 2024-11-12T10:34:06Z

I've placed an order with a pre-hook, and set the max amount to sell including incrementation. All simulations showed successful reports, but in the end my order never got executed :(

yvesfracari · 2024-11-12T17:37:45Z

@elena-zh Thanks for the testing.

UI Crasching: It was an issue with negative balances, it should be fixed now.
Create Vesting App token balance: Makes sense, I will fix it. To do so I will need the CoW Team to release a new version of the hook-dapp-lib. Thanks for catching this but from my understanding this shouldn't block this PR since it is more related to an individual and external hook issue while this PR is related to the hook store widget. Once the new version of the package is released I will solve this. By the way, the use all tokens after the swap issue should be only visual.
Simulation link on confirm modal: I am not sure actually, we didn't define this. If the CoW Team thinks that we should add a simulation link there I can add it.
Simulation running on going back from the confirm modal: It should be fixed now.
Withdraw simulation failing after not changing any parameter: The withdraw hook is a little time-sensitive. We encode the "minimum" amount of tokens that the user wants to receive in order to protect him from sandwich attacks. To do that, we're using 0% of slippage like the Balancer UI. However, if the cow amm does any trade after you encode the hook, the hook will start to fail, since it changes the amounts that the user can receive. We have already seen that issue and want to fix it in the future with a new grant project. The idea is to update the hook on order/blockchain changes.
Order falling: I had the same issue on my side. I think that we will need the help of the backend team to understand more about the problem.

elena-zh · 2024-11-13T12:20:24Z

Hey @yvesfracari , there is an error in the code that prevents from the PR deployemtn. Could you please fix this?

As for this issue:

Order falling: I had the same issue on my side. I think that we will need the help of the backend team to understand more about the problem.

I've verified this with the BE team. The order is not executed due to 'insufficient balance' error.
In particular, here is the simulation link with success validation that I had before the order placement: https://dashboard.tenderly.co/public/cow-protocol/hooks-store/simulator/8fd5d7e7-d599-4296-be83-48dd92112b5c

But when the order was placed, at the auction creation, the pool info changes, and the output amount becomes insufficient (here is the simulation report in the same block https://www.tdly.co/shared/simulation/1b85ee97-beab-40ca-873c-c696e0705f96)

The most possible way how to reproduce this:

Add Withdraw liquidity pre-hook
Pick a Sell token with an incremented balance
Fill in the MAX amount
Wait for 20-30 seconds before the order placement. Just in case, re-run the simulation --> it will be in green
Place the order.

yvesfracari · 2024-11-13T12:54:53Z

@elena-zh

The deployment is fixed now.

Looking at the simulation it seems to be the problem with the slippage. I increased the slippage to 1% (it was 0% before). This would make the withdrawal execution more robust. However, it can generate dust (since the MAX amount of the sold token can be underestimated). I did the test and worked: https://dev.explorer.cow.fi/arb1/orders/0x2b6b6d8f3d433c994e0ae7346e826cb923d1c29511f6839011ce75d0b8ba154e76b0340e50bd9883d8b2ca5fd9f52439a9e7cf586734a662?tab=overview

elena-zh · 2024-11-13T14:47:56Z

Hey @yvesfracari , for the the order creation still fails (insufficient balance) with the slippage changes that you've added.
Still investigating this with the team internally.

As for this

Simulation link on confirm modal: I am not sure actually, we didn't define this. If the CoW Team thinks that we should add a simulation link there I can add it.

I've clarified this internally. For consistency, if a simulation runs on the order confirm modal, it would be nice to show a link to the simulation report there as well :)

All the rest changes LGTM

anxolin · 2024-11-13T15:45:51Z

Hey @yvesfracari , for the the order creation still fails (insufficient balance) with the slippage changes that you've added.
Still investigating this with the team internally.

We are making hook swaps partially fillable for now, just to account for small changes that reduce the received tokens

anxolin · 2024-11-13T15:50:59Z

@elena-zh @yvesfracari could we merge if this one is stable and follow up in a new PR any enhancement. As this PR primary fixes the diff calculations, if that part works I would suggest to not mix UI issues with this PR

elena-zh · 2024-11-13T15:52:50Z

@elena-zh @yvesfracari could we merge if this one is stable and follow up in a new PR any enhancement. As this PR primary fixes the diff calculations, if that part works I would suggest to not mix UI issues with this PR

sure

yvesfracari · 2024-11-13T16:03:04Z

@anxolin Sorry, I didn't see your last message and pushed one commit adding the simulation link that Elena suggested. If you don't want to use it just ignore the commit 824b94c

anxolin · 2024-11-14T04:53:58Z

@anxolin Sorry, I didn't see your last message and pushed one commit adding the simulation link that Elena suggested. If you don't want to use it just ignore the commit 824b94c

Its fine, I just want to consolidate things asap and get this thing as stable as possible. Tomorrow is the hackathon

$yvesfracari$

$@yvesfracari$

chore: create state for balance combined and optimize applyBalanceDif…

ffe08ff

…fs function

anxolin reviewed Nov 11, 2024

View reviewed changes

vercel bot deployed to Preview – swap-dev November 11, 2024 20:31 View deployment

yvesfracari added 2 commits November 12, 2024 14:20

$@yvesfracari$

fix: avoid negative user balance

c227ae6

$@yvesfracari$

fix: remove refetch tendernly simulation on mount

75a381d

$@yvesfracari$ yvesfracari mentioned this pull request Nov 12, 2024

feat(hook-store): Add cow amm deposit hook dapp #5069

Merged

vercel bot had a problem deploying to Preview – swap-dev November 13, 2024 11:56 Failure

$@yvesfracari$

fix: build error

37bf526

vercel bot deployed to Preview – swap-dev November 13, 2024 12:43 View deployment

elena-zh approved these changes Nov 13, 2024

View reviewed changes

vercel bot deployed to Preview – widget-configurator November 13, 2024 15:53 View deployment

vercel bot deployed to Preview – explorer-dev November 13, 2024 15:54 View deployment

$@yvesfracari$

feat: add simulation link on order review

824b94c

$@yvesfracari$ yvesfracari force-pushed the pedro/fix-hook-widget-optimization branch from 3a5bdba to 824b94c Compare November 13, 2024 16:37

anxolin merged commit 38aae71 into cowprotocol:develop Nov 14, 2024
5 of 12 checks passed

github-actions bot locked and limited conversation to collaborators Nov 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(combinedBalances): Optimize balance diff calculations #5082

fix(combinedBalances): Optimize balance diff calculations #5082

$@yvesfracari$ yvesfracari commented Nov 11, 2024

vercel bot commented Nov 11, 2024

anxolin left a comment

anxolin Nov 11, 2024

$@yvesfracari$ yvesfracari Nov 11, 2024

anxolin Nov 11, 2024

vercel bot commented Nov 11, 2024 •

edited

Loading

elena-zh commented Nov 12, 2024

elena-zh commented Nov 12, 2024

elena-zh commented Nov 12, 2024 •

edited

Loading

elena-zh commented Nov 12, 2024

yvesfracari commented Nov 12, 2024 •

edited

Loading

elena-zh commented Nov 13, 2024

yvesfracari commented Nov 13, 2024

elena-zh commented Nov 13, 2024 •

edited

Loading

anxolin commented Nov 13, 2024

anxolin commented Nov 13, 2024

elena-zh commented Nov 13, 2024

yvesfracari commented Nov 13, 2024 •

edited

Loading

anxolin commented Nov 14, 2024

fix(combinedBalances): Optimize balance diff calculations #5082

fix(combinedBalances): Optimize balance diff calculations #5082

Conversation

yvesfracari commented Nov 11, 2024

Summary

To Test

vercel bot commented Nov 11, 2024

anxolin left a comment

Choose a reason for hiding this comment

anxolin Nov 11, 2024

Choose a reason for hiding this comment

yvesfracari Nov 11, 2024

Choose a reason for hiding this comment

anxolin Nov 11, 2024

Choose a reason for hiding this comment

vercel bot commented Nov 11, 2024 • edited Loading

elena-zh commented Nov 12, 2024

elena-zh commented Nov 12, 2024

elena-zh commented Nov 12, 2024 • edited Loading

elena-zh commented Nov 12, 2024

yvesfracari commented Nov 12, 2024 • edited Loading

elena-zh commented Nov 13, 2024

yvesfracari commented Nov 13, 2024

elena-zh commented Nov 13, 2024 • edited Loading

anxolin commented Nov 13, 2024

anxolin commented Nov 13, 2024

elena-zh commented Nov 13, 2024

yvesfracari commented Nov 13, 2024 • edited Loading

anxolin commented Nov 14, 2024

$@yvesfracari$ yvesfracari commented Nov 11, 2024

$@yvesfracari$ yvesfracari Nov 11, 2024

vercel bot commented Nov 11, 2024 •

edited

Loading

elena-zh commented Nov 12, 2024 •

edited

Loading

yvesfracari commented Nov 12, 2024 •

edited

Loading

elena-zh commented Nov 13, 2024 •

edited

Loading

yvesfracari commented Nov 13, 2024 •

edited

Loading