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

[yvesfracari]

Summary

Add CoW AMM Deposit Hook Dapp

To Test

1- Access /#/11155111/swap/hooks/WXDAI
2- Connect your wallet and switch to any cow amm deployed chain
3- Fill some swap order
4- In Hooks > Add Post-Hook Action, the CoW AMM Deposit should appear in the "All hooks" section
5- Fill the form data and add the post-hook
6- Check with the simulation feature if it working as expected

Background

Optional: Give background information for changes you've made, that might be difficult to explain via comments

[vercel]

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

A member of the Team first needs to authorize it.

[vercel]

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

Name	Status	Preview	Updated (UTC)
cowswap	✅ Ready (Inspect)	Visit Preview	Dec 3, 2024 1:49pm
sdk-tools	❌ Failed (Inspect)		Dec 3, 2024 1:49pm
swap-dev	✅ Ready (Inspect)	Visit Preview	Dec 3, 2024 1:49pm

[elena-zh]

Hey @yvesfracari , great job, thank you!
Some questions/issues from my side:

1. (improvement) it would be nice to have the same text everywhere for the 'Insufficient balance' case
   
   instead of
   

2. Same with the message when swap order has not been specified: this message sounds a bit better, and is already used in other hooks:
   

3. 'Approximate' sign and a warning is missing for all options (same as we do for 'Create llama vesting' hook)
   
   IMO, it would be nice to have it as a user will never know the exact of received tokens for Sell orders (only for sell, not for buy ones)

4. Adding a hook on Ethereum drastically increases the fees . Is it an expected behavior for this?
   
   

5. When I've added a hook with option 2, and press on the Edit hook, I can see that the 1st amount is reset, and the option 1 is picked. It would be nice to save user's selection when press on Edit hook
   

6. When pick the 2nd option (send all swap amount), I don't see a warning that the whole amount will be sent to a recipient. We discussed it in Add Create LlamaPay Vesting iframe hook #5045 (comment), but, at least, for one of options (entire swap) it was working correctly.

7. It would be great to have the same balance format like we have everywhere in the app
   
   

8. In this PR my local storage goes crazy :(
   
   Maybe because of this I sign permits every time when I update a hook even if I've already signed it

9. It would be nice to reset filled in amounts if a user picks another pool for adding liquidity in the selector. Currently, the numbers from a previous pool are saved.

10. In several cases I've got failed simulation/TX results:

• Gnosis chain: https://gnosisscan.io/tx/0x433a68fb3272c2752b415219a207d75d7ff6082de233a04fb023ee0f0456fa54

• https://dashboard.tenderly.co/public/cow-protocol/hooks-store/simulator/7389ff3e-8b5d-4215-9bdf-ca4781691219

• as a result, liquidity was not added to the pool. and I don't see tokens to rescue in my proxy
  

• arbitrum (failed simulation):
  

• or I can see on console an error with failed to estimate a hook gas, but the UI says that everything is good (I picked the 3rd option to add all token's balance after swap)
  

But this is maybe there is not enough test instructions, or not enough balance to cover increased fees. Not sure what caused all these, just speculating on observations.

[yvesfracari]

Thanks for the review @elena-zh

1- Done
2- Done
3- Actually, we're not using recipient override since we can't calculate all values on-chain because of security reasons. Instead, we're calculating all the values before hand. In that sense, all values are exact and not approx.
4- This hook is only executable after the swap, so we have to fill the data considering scenario that uses the most gas as possible on the hook. However, I think that we overshoot too much this value, now it should be more normal. On the PR #5039 we're using the simulations to update this value, so it should be updated.
5- Makes sense, will work on this in the next days.
6- Same as 3, we're not overriding the recipient here.
7- Done
8- Still taking a look, will work on this in the next days
9- Done, now the form is reset every time you select a new pool.
10- I think that this is related to that gas limit issue that I explained before. Maybe we could test again after PR #5039 is merged. Thanks for reporting it. About your funds, we're not overriding the recipient for the proxy, so the tokens are on your wallet. 😅

[anxolin]

I shared this already in our shard channel, but I'm getting this error:

Also, when you load the app we should clearly state we are loading something. For a few moments, it looks like a blank app and you don't know where to click.

cowamm.mov

[elena-zh]

Hey @yvesfracari , thank you. I've verified Done fixes, they are mostly look good.
Will wait for other fixes.

In addition, there are a few new issues to report:

1. It would be great to improve the scrollbar: In Win11+Chrome it looks this way:
   
2. It would be great to improve pools info displaying in a mobile view
   
3. Balances displaying: I think there is no need to show excessive zeros in balances:
   
   
4. I've been trying to add a hook, but then I rejected approval TX in the wallet and went back: I saw this message
   
   I don't have particular steps to reproduce. But at least, if this happens, this message should look nicer a bit.
5. Fees on Ethereum became lower (about 100 USD). Not sure, though, whether this still might be scaring for users or not. But I have another question to discuss:

• I specify a swap of 50 tokens
• Add Deposit Hook that uses the max available balance/balance after swap
• As soon as I add it, fee increases and my swap becomes no longer valid
• When I change an amount to sell to cover the fee, the hooks becomes invalid --> looks very weird.
  WDYT about including estimated fee amount into 'available balance' recalculation? Currently, you deduct a swap amount only.

6. Add post-hook button is enabled when 0 tokens are specified in the fields
   

7. I noticed that in some cases the app requires gasless approvals, in some - onchain. This depends on a token type to add (permittable or not). Right?
   

Thanks

[yvesfracari]

Thanks for the testing!

Both unsolved old issues should be solved now.

About the latest issues:

• [1, 2,3,4,6]: should be solved now.
• [5]: With the new tenderly simulations should be fixed. However, the dev is broken. Maybe we should test this again after fix(combinedBalances): Optimize balance diff calculations #5082 is merged.
• [8]: Yes, we use a permit when the token implements it. We use the same package as cowswap frontend to check if the token is permittable or not and encode it.

[elena-zh]

Hey @yvesfracari , the app in the preview link is barely working in terms of performance. I've reporded a video for you, however, not sure what causes this as there are no 'special' logs on console for this.
Could you please check why this is happening? At this time, I will try to test things out.. However, not sure if I manage to..

[elena-zh]

Hey @yvesfracari , I've retested these changes inside the #5082 PR.

Some issues that I'm still facing:

1. It would be nice to investigate why 'something went wrong' error may appear. This is the video for you with the STR
   

2. I'm still not sure whether this is OK that after adding this hook fees become 4x greater :(
   
   

3. This case is not fixed:
   When I've added a hook with option 2, and press on the Edit hook, I can see that the 1st amount is reset, and the option 1 is picked. It would be nice to save user's selection when press on Edit hook
   

4. I've tested it inside fix(combinedBalances): Optimize balance diff calculations #5082, the issue is still there. But I might be wrong and still need to wait until fix(combinedBalances): Optimize balance diff calculations #5082 is merged

[5]: With the new tenderly simulations should be fixed. However, the dev is broken. Maybe we should test this again after #5082 is merged.

5. Why should I re-sign permits each time when I update a hook? E.g. there are 2 tokens like COW and wstETH, they are permittable, and when I add the hook with these tokens for the 1st time, I sign unlimited permit for them. So it is not clear to me why 'sign permit' step is retriggered once again if I've changed a hook's amount to add into a deposit:
   

6. When I add a hook, I can see some failing simulation console logs, however, the UI shows that it is OK. Is it an expected behavior?
   

7. UI issue: could you please improve a bit column values alignment here?
   

Thanks!

[yvesfracari]

Thanks @elena-zh !

• Issues 1,3,4,5 and 7 should be fixed.
• For issue 2, we're now using Tenderly gas estimation, so it should be right as well. @anxolin mentioned that tenderly overestimates a little bit the gas to be used by the hook because of the kind of simulation we're making but if we want to change this we should change it for all tokens on the next PR.
• Issue 6 is expected. We're trying to estimate the hook gas but we can't because we depend of the swap execution. So, it is expected the gas simulation will fail in some cases on the hook side but then it should succeed on the tenderly side.

[elena-zh]

Hey @yvesfracari , thanks for addressing issues.

Still, I have something to report you back :)

1. (related not only to this hook): It has just came into my mind that we color 'Try again' button with red. And this looks weird as the is a 'positive' button allowing a user to restart the process. And usually red color indicates a failure. Might it be possible to change its color to a blue one?
   

2. Could you please clarify why the hook's name is unknown in the Confirm modal?
   

3. Still, there is an issue with balances displaying: I have 1400 in my current balance, but the hook shows me that I have only 1 token available. But when I press ont he Max button, it prefills the correct token's balance into the field:
   

4. This issue is not fixed (case 5 from this comment)

• 1. I specify a swap of 50 tokens
• 2. Add Deposit Hook that uses the max available balance/balance after swap
• 3. As soon as I add it, fee increases and my swap becomes no longer valid
• 4. When I change an amount to sell to cover the fee, the hooks becomes invalid --> looks very weird.
• WDYT about including estimated fee amount into 'available balance' recalculation? Currently, you deduct a swap amount only.
  

5. The issue with pools info alignment is not fully fixed:
   

6. When I add a hook, then I change a trading token pair, the hook remains to be displayed on the form, and simulation shows a success report. But when I open it using 'edit' button, I cannot see the hooks' details. Moreover, I can see that there are no relevant pools for this token pair:
   
   
   

Not sure about a possible solution here:

• still show details of the hook added (pool/amounts/previous token pair)
• remove the hook on a token pair change?

[yvesfracari]

Thanks for the review @elena-zh

Issues 1,3: were fixed with your suggestion

Issue 2: Not sure, but I think that is not related with this PR cc @shoom3301 maybe you can take a look

Issue 4: This issue is harder to resolve because it is hard to know how many fees will be charged. We know that this issue has this issue but I think that we can just solve it on a second version of the store where a more dynamic kind of hook would be allowed (like encoding of the hook on the order review). To work around this, I disabled the MAX button in the case of sell orders. FYI, I already talked with Sasha and Leandro to create a second part of this grant where this more dynamic form of encoding hooks will be created and then we can solve this problem.

Issue 5: I tried to implement this way but thought that the mobile view on small screens was kind of bad. Instead, I am aligning all this information on the right side. IMO this is the same way we did on the withdraw hook and we can use the same pattern here.

Issue 6: Fixed, now you will be able to see the pool. The pool selector will always show the pools that have the token buy and the current selected pool.

[elena-zh]

Hey @yvesfracari , great, thank you for your fixes!

Just some comments:

1. I've started to see a hook details on the Confirm modal (it might have been a glitch when I tested it last time)

2. Sorry if I misleaded you in terms of case 3, but I did not mean to remove MAX button for a manual option.
   
   But I very like newly added tooltips to the balances fields :)

3. Clicking on the tooltip calls the sign TX modal in the wallet (oops)
   
   

4. And clicking on the toolltip in a mobile view does not open its test to read

5. Related to the 6th case: Now I'm able to open a hooks detail when I change a token pair in the Swap field. However, when I pick a different option (e.g. from 'Input deposit amount manually' to 'Use all tokens from swap'), it crashes
   
   
   
   WDYT about disabling these buttons along with showing a message to pick another pool instead? E.g. "trade details changed. Please pick another pool to proceed'

Thanks!

[yvesfracari]

@elena-zh

1-4 - Solved
5 - I was solving this issue by disabling the button but them there was still the case when the user selected the "all from swap" or "all after swap" options, then changed the token and try to edit the order. The issue on this case was that the "all from swap" option might not be available anymore since the token might not be on the pool.

I rethink this case and in my perspective the "user input", "all from swap" and "all after swap" options are just helpers to allow the user to set the amounts and actually the usage of the "all after the swap" was the same of the "MAX" button. In that sense, I just add a "Buy amount" button as well. With that I think that the experience is better and more clear for the user.

[elena-zh]

Hey @yvesfracari , thank you!
Again, some issues.

1. I will start with the last issue: adding a hook, then changing a token pair: the data is displayed, I can change amount in this hook, but when I press on the 'Update hook' button, it shows nothing on the UI and the error on console:
   
   Again, I think that the most appropriate option here is to show a warning message to a user to change a pool in the hook once trade data has been changed

2. Tooltip button is still not clickable in a mobile view
   

3. Nitpicking on the tooltip desig: usually ,we show these colors for tooltips
   

• dark mode
  

• light mode
  

4. When I select a max balance for a hook (including a trade amount in the Buy field for a Sell order), then I change a slippage amount (increase it), the simulation is not re-run. But increasing a slippage leads to the receive amount change --> hook will not be executed. Might it be possible to re-run simulation when a slippage value is changed?
   

5. I've also noticed that quote can be changed too fast, right after adding a hook. So when I pick 'MAX' option (including a trade outcome), the hooks becomes an invalid one and requires an update. Might it be possible to implement a 'flexible' max amount option that will be changes based on these quotes fluctuations?

Thank you!

[yvesfracari]

@elena-zh

1,3- Fixed

2- Now while the tooltip icon is being pressed the message should be displayed. I think that this is the default behavior right?

4- I think that this should be done on a separate PR since it would impact multiple hooks. Also, I think that this is something that if 5 is fixed it will not be necessary anymore.

5- It makes sense and this is exactly my view on how to solve it. However, this dynamic approach is outside our grant scope and so it is outside the scope of this PR. However, I agree that it is important and a must-have for the next grant that we want to propose.

[elena-zh]

Hey @yvesfracari , thanks!

4- I think that this should be done on a separate PR since it would impact multiple hooks. Also, I think that this is something that if 5 is fixed it will not be necessary anymore.

Opened #5129 task for this

5- It makes sense and this is exactly my view on how to solve it. However, this dynamic approach is outside our grant scope and so it is outside the scope of this PR. However, I agree that it is important and a must-have for the next grant that we want to propose.

Opened #5132 task for this (for the future development)

2- Now while the tooltip icon is being pressed the message should be displayed. I think that this is the default behavior right?

Not exactly. It works a bit differently in a mobile device: it is opened on click and remains to be displayed until a user taps outside/scroll up/down the screen.
With the current implementation it is not possible to read a tooltip's value (unless a user keeps pressing on it). See the video how it is works rn, and there is a comparison how all the rest tooltips are working in the app:

IMG_7572.MP4

And I have not been able to add liquidity hook on Ethereum:

• error
  
• hook details to add
  
  Pool details: https://balancer.fi/pools/ethereum/cow/0xbf8868b754a77e90ea68ffc0b5b10a7c729457e1

Could you please check it?

[yvesfracari]

@elena-zh Both two new reported issue should be fixed now

[elena-zh]

Hey @yvesfracari , great job, thank you!

I've been able to place order with this hook on every chain, and it works!

Still, I have some nitpicks and concerns:

1. (nit) 'buy amount' button is glued to the USD estimation in a mobile view
   
2. (nit) when USD estimation < 0.01 USD, we usually show it with 1 in the end
   
3. (question) In the recently fixed changes (case 1 from this feat(hook-store): Add cow amm deposit hook dapp #5069 (comment)) we allowing to place an order with adding liquidity to a pool that does not have the same tokens on the swap form. So WDYT about showing all available cow amm pools once a trade is specified?
   
4. fees on Ethereum. I brought up this question before several times, However, today I finally placed an order on Ethereum and noticed that fees are tooooo overpriced based on the trade outcome.
   This is an example of my order where I should have received 1 safe token in the end due to high fees, but in the end I've received 15 safe tokens (e.g.+15 USD) on a trade with 92% Surplus.

One more example:


So I still think that fees are overestimated, and 15 USD is a solid value for this tiny trade given the fact that gas price is about 10 gwei atm.

[yvesfracari]

Thanks @elena-zh

1,2 were solved.

3 - I implemented this as well. Think that is a good option.

4 - I am not sure if there is much to be done on our side. This hook has a lot of call data and it can be kind of expansive (I double-checked ways to reduce it but it would create edge cases of worst UX like dust on the cow shed). All the gas quotes (used to generate fees) came from Tenderly and it is passed to the backend. There is some buffer but it shouldn't be that big. Maybe the gas cost changed during the quoting and execution decreased. However, the way of quoting is the same for all hooks, so if there is an overestimation issue all of the hooks are being affected and we should open a new PR/issue to solve it.

[elena-zh]

Hey @yvesfracari , thank you for your fixes!

One more nitpick I've noticed: when adding a dust into a pool, the app allows me to add this hook, but simulation immediately fails. See the images:



Simulation report: https://dashboard.tenderly.co/public/cow-protocol/hooks-store/simulator/08a937f5-6700-4351-be99-7fff9029227d

Might it be possible to specify a min value to add (e.g. 0.01 USD)?

As for fees estimation. The issue is relevant for this hook only.
Look:

• pre-hook (withdraw CoW AMM)
  
• post: create LLama Pay
  
• add liquidity (see how it is bigger)
  
• no hooks added
  

As for the assumption of a gas price change before/after order execution, it is not the case as everything took about 1 minute (estimation/order placement/execution) while gas price was about 9-10 gwei.

[yvesfracari]

@elena-zh

1 - About min USD prices, I think this can cause issues of blocking users when the price API is with some problem (we get the price from the balancer). To be honest, I think that receiving a failing simulation is a good handling for this case.
2 - About the fee, I double-checked and it is expected the fee to be larger than the other hooks. Comparing it with the withdrawal (approx values):

• Withdraw hook gas limit: 234513
• Deposit hook gas limit: 363987
  P.S.: To see these values open the hook details on the review dialog or check on the simulation link.

So, a difference of this magnitude is expected. However, as you reported the difference is bigger. I double-checked and we're passing the right values to the backend. If you want to check you can see the quote request on your network logger. I can see two options for the issue:

1 - Proxy initialization: The first time you execute a hook it should be more expansive since the proxy will be initialized. However, this cost should happen for any first hook being executed on the same account. However, since you already executed some hooks I think that is not the case.
2 - Backend issue: I double-checked and the right gas limit is being passed from the CoW Swap app to the backend. However, I am not sure how their logic works to estimate the fee based on this parameter. Help from their side would be good.

[elena-zh]

Hey @yvesfracari , I've got response from the BE team: no issues from their side.

• The swap itself is estimated to cost around 1.8M gas units
• The hooks specifies 3.6M gas units which is added on top

So in total with hook we use 3x the gas that we use without the hook and this is why the fee is 3x higher.

As for:

1 - About min USD prices, I think this can cause issues of blocking users when the price API is with some problem (we get the price from the balancer). To be honest, I think that receiving a failing simulation is a good handling for this case.

I partially agree with it. But my point here: we are adding a hook that shows 0 in one of fields.

And this is weird. If we allow adding dust, then we should show amounts to add (at least 0.00000001COW instead of 0). WDYT?

[yvesfracari]

@elena-zh

Thanks for the effort in checking this with the backend team. It seems then that there is no issue with the fee price, this hook is just more expansive.

About the dust, makes sense, now I am disabling the button when 1 of the two amounts is zero.

[elena-zh]

Hey @yvesfracari , thank you for the fix!
I think I've found one more edge case:

1. I withdraw liquidity from COW AMM pool in the pre-hook
2. I add liquidity into the same pool in the post-hook --> simulation fails
   https://dashboard.tenderly.co/public/cow-protocol/hooks-store/simulator/3567ca12-565e-455e-9ba7-28f40cb7f4c6
   Is this something that is possible to fix?

[elena-zh]

Also, as a recap of edge cases in this implementation:

1. Simulation may fail in 1-2 seconds after adding a hook when select 'max' amount to add into a pool including a trade amount:
   
   I believe it will be fixed in Hooks: Use dynamic approach for hook amounts calculation #5132
2. Hook is overestimated a bit that will cause huge surpluses after a trade execution:
   

[yvesfracari]

@elena-zh

About the simulation that you shared. It is the same issue of max amounts that don't work well because of the fee update after the hook creation. This should be fixed when we allow hooks that can be updated by the cow swap app #5132 .

The strategy to estimate the gas of each hook is the same for all of them, which is about 40k gas you can check on the simulation that you reported of a failed hook simulation on the "Initial gas".

On the order that you shared the gas consumed by the hook was 306341 and the limit was 347069. In that sense, we can confirm that we're passing the right gas for the hook. If we want to remove the buffer (or decrease it), it should be discussed, and since it would affect all hooks, it should be done on a separate PR.

[elena-zh]

Approved then