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

feat(hook-store): Add Uniswap v2 withdraw hook dapp #5155

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

yvesfracari
Copy link
Contributor

Summary

Add Uniswap V2 Withdraw Hook Dapp

To Test

1- Connect your wallet and switch to any uni v2 deployed chain (Arbitrum and Mainnet)
2- In Hooks > Add Pre-Hook Action, the Uni V2 Deposit should appear in the "All hooks" section
3- Fill the form data and add the pre-hook
4- Check with the simulation feature if it is working as expected.

Notes:

  • The token list should be the same as the CoW Swap Yield list.
  • If the pool that you want to withdraw is not on the token list you can import it by pasting the address on the dropdown menu.
  • This feature should show an error if the pool is not a valid contract or you don't have funds.
  • All the UI was already reviewed.

Copy link

vercel bot commented Dec 4, 2024

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

A member of the Team first needs to authorize it.

Copy link

vercel bot commented Dec 4, 2024

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

Name Status Preview Updated (UTC)
cosmos ✅ Ready (Inspect) Visit Preview Dec 17, 2024 6:20pm
cowfi ✅ Ready (Inspect) Visit Preview Dec 17, 2024 6:20pm
cowswap ✅ Ready (Inspect) Visit Preview Dec 17, 2024 6:20pm
explorer-dev ✅ Ready (Inspect) Visit Preview Dec 17, 2024 6:20pm
sdk-tools ❌ Failed (Inspect) Dec 17, 2024 6:20pm
swap-dev ✅ Ready (Inspect) Visit Preview Dec 17, 2024 6:20pm
widget-configurator ✅ Ready (Inspect) Visit Preview Dec 17, 2024 6:20pm

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

Hey @yvesfracari , thank you

But I have to say that nothing is working here.

  1. I have a liquidity token on Ethereum, and it is in the Yield list. but it is not displayed in the pools list in the hook
    no pool
    token
  2. When I search for this pool, it shows me an error
    error loading
  3. Same happens on Arbitrum
  4. It would be nice to add this functionality to Sepolia
  5. It would be nice to truncate trailing and preceding spaces when paste an address into the search field
    spaces
  6. Overall, it would be great to add a simple validation on an address format into the field
    image

Overall, could you please check the functionality using the preview link on your side before sending it for my review?

Thank you!

@yvesfracari
Copy link
Contributor Author

@elena-zh

All the reported errors should be fixed now. Thanks and let me know of more errors.

@yvesfracari yvesfracari requested a review from elena-zh December 5, 2024 17:16
@alfetopito
Copy link
Collaborator

Hey Pedro, I have a question about this screen:

spaces

Here, it's searching by an address.
It says no results found, and it also says to search for it in the search bar.
The second phrase is a bit confusing to me. If you haven't found, would it make any difference if you search for it again?
Unless I misunderstood, I think this sentence should be removed altogether.

@anxolin
Copy link
Contributor

anxolin commented Dec 6, 2024

I tried to use it, but TBH not sure how 😅

I get this message when I try to use the drop-down:
image

Also, after a few seconds, it re-renders everything again.

I'm also not sure what the message is saying.
I guess is trying to see if I have balance on any of the pools. Imo, and this would apply to the other hooks where I also think suffer from the same issue, we should show all the pools even if there's no balance for them. The logic is very similar to our token selector, we show the balances of all tokens, but we show first in the list the ones with balance greater than zero. This way, the user understand they need to select a LP, they can see the options, and they see they cannot add the pre-hook cause their balance is zero for all of them.

image

In the instructions says, I should use Mainnet/Arbitrum, but I think we have UniV2 available also in Base and Sepolia.
image

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

Hey @yvesfracari ,

thanks, but still:

  1. Could you please clarify why this happens on console when I open the hook on Ethereum?
    mad console

  2. On Ethereum, I don't see my UNI pool details (however, I have an LP token there)
    eth

  3. I don't see this hook on Sepolia :(
    ho hook sepolia

  4. As Leandro pointed out in the comment above, I'm also confused with the message that is displayed by default on the list opening: I have not done anything yet, and it says about incorrect format...
    i have not done

  5. As for address validation, might it be possible to show this error when a user starts typing it? Something like it is already implemented in one of vesting hooks:
    example

  6. I've noticed that the pool's search is reset in several seconds: the loader appears, and the search field area is closed. Could you please take a look why this happens?
    Thanks!

@yvesfracari
Copy link
Contributor Author

@anxolin @alfetopito @elena-zh Thank you all for the feedbacks.

Sorry for the errors. I will try to group the feedbacks and reply them together.

1 - Supported chains: I added the support for sepolia on the manifest.json but I think that the deployment wasn't updated with my latest commit (I think that someone have to approve it). I didn't added base because I thought it wasn't fully integrated yet, if you guys want I can add it.

2 - Importing and searching pools: The text field is used for both importing and search for pools. That is why I didn't implemented the suggested invalid address format error message and instead there is a "No pools found" message. If the user paste and address that is not a pool it should see an "Error loading new pool.".

3- Pool list: As it works on the "CoW AMM Withdraw" hook dapp, the app was loading only pools that the user has. However, now the app loads all pools and sort them (as @anxolin suggested). Since this is a long list, we're no longer displaying price information of the pools. Instead, we're showing the user balance in LP tokens. This also fix the rate limit errors that @elena-zh reported on mainnet (sorry I was testing on arb) and the re-renders reported by @anxolin .

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

Hey @yvesfracari , thank you!
But still there is a bunch of issues I faced:

  1. Pool tokens balances are not displayed on Arb and Ethereum
    not 0 balance
    Screenshot_1
    usd
  2. Link to a pool details navigate to Balancer
    uni pool navigates to balancer
  3. When I search for a token, and a token is a LP token, it says 'not found'
    illogical
  4. Besides, for me it is illogical to show 'Try placing your LP token address on the search bar.' if I've already done it. I'd remove this message at all...
    remove
  5. Sometimes, when I search for a LP token, I face a red error message, but IDK what is wrong here:
    why error
    Might it be possible to add explanation about the error?
  6. The hook has not been added to Base (as @anxolin suggested)
  7. There are a lot of v2 pools on sepolia, but nothing is displayed in the list. I assume that this is due to they are absent in the LP list. Might it be possible to fetch them?
  8. USD estimation for WETH token is missing on Sepolia
    sep usd estimation
  9. I tried to add a hook on sepolia, but it ended up with error:
    sepolia hook gas
    Details:
Calldata
: 
"0xa8481abe00000000000000000000000000000000000000000000000000000000000000a031373333383435323630353136000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000067586f0f0000000000000000000000009fa3c00a92ec5f96b1ad2527ab41b3932efeda580000000000000000000000000000000000000000000000000000000000000720000000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000800000000000000000000000000000000000000000000000000000000000000240000000000000000000000000000000000000000000000000000000000000038000000000000000000000000000000000000000000000000000000000000004a0000000000000000000000000164ffe42adcf8a147090a5981ad0881cc11ecfe6000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000a00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000e4d505accf0000000000000000000000009fa3c00a92ec5f96b1ad2527ab41b3932efeda5800000000000000000000000079932c178ad3d0a4864b0322ce7a8405d74de290ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0000000000000000000000000000000000000000000000000000000070c00868000000000000000000000000000000000000000000000000000000000000001c8852758c0f6da983164020656e5a3516b71f4cfe59085cb8a45476cbc7d9e7511189263771d79566a4c4d91ef9c669b67e41baea3dea8c980ff84f9772eba23700000000000000000000000000000000000000000000000000000000000000000000000000000000164ffe42adcf8a147090a5981ad0881cc11ecfe6000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000a000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000006423b872dd0000000000000000000000009fa3c00a92ec5f96b1ad2527ab41b3932efeda5800000000000000000000000079932c178ad3d0a4864b0322ce7a8405d74de29000000000000000000000000000000000000000000000000003ecdbf2cc5b8c7700000000000000000000000000000000000000000000000000000000000000000000000000000000164ffe42adcf8a147090a5981ad0881cc11ecfe6000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000a0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000044095ea7b3000000000000000000000000ee567fe1712faf6149d80da1e6934e354124cfe300000000000000000000000000000000000000000000000003ecdbf2cc5b8c7700000000000000000000000000000000000000000000000000000000000000000000000000000000ee567fe1712faf6149d80da1e6934e354124cfe3000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000a00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000e4baa2abde0000000000000000000000007b79995e5f793a07bc00c21412e50ecae098e7f9000000000000000000000000b4f1737af37711e9a5890d9510c9bb60e170cb0d00000000000000000000000000000000000000000000000003ecdbf2cc5b8c77000000000000000000000000000000000000000000000000004657febe8d7fff00000000000000000000000000000000000000000000000036f4bf04de8bff680000000000000000000000009fa3c00a92ec5f96b1ad2527ab41b3932efeda580000000000000000000000000000000000000000000000000000000067586eea0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000419cdcedb3e1dbcd31c91d1c8b9cb3360acd78c1a3b765e3ff0480450f225db4623a2d78b9e7b759f93b21b9c736f0e82e4d85a79c417eaa0eec13e1e036fe92901c000000000000000000000000000000000000000000000000000000000000000001"
chainId
: 
11155111
currentBlock
: 
7251147n
currentTimestamp
: 
1733845271
target
: 
"0x00E989b87700514118Fa55326CD1cCE82faebEF6"

Thanks

@yvesfracari
Copy link
Contributor Author

@elena-zh Thanks for the feedbacks!

1- It was a rounding issue. Now should be ok.
2- Fixed.
3- I try to fix some bugs related to inconsistency of this search. Now should be fixed. Here is a video of my tests: https://www.loom.com/share/7ae2d355d5574c3da77fe0d0e61e8b26
4- Removed the second line of the message.
5- Now we're displaying more details of the error that happened.
6- Now it should be added on Base.
7- That is correct. On my understanding, I think that we agreed on using the same list of CoW Swap was using. If the pools were added there it should show up here too.
8- We're using CoW API price information (what I think that is ok on this case). For sepolia it is common some token missing price information, if it happens on a non test net we can change our approach.
9- As I mentioned in our private chat, the pool that was used to test was not created by the official factory. I added a validation about this on the pool importing. However, now the pool is already on your local storage, so to test I recommend you to clear and try to import it again.

By the way, all these replies are related with my local tests. Can you authorize the new deployment so I can test?

@shoom3301
Copy link
Collaborator

@yvesfracari could you pull the latest changes from develop into this PR please?

@elena-zh
Copy link

Hi @yvesfracari , thank you for your fixes!

Still, some issues:

  1. Hook on Base:
  • it does not change balance when add it
    base balance change

  • looks like the hook was not executed with my order. Here is the TX. Pool token is this one

  1. When 'no search results' is displayed, the UI re-renders each 10-20 seconds --> it does not look good
  2. Also, when a hooks is being loaded, it is loaded twice. I mean: the search page appears, then the loader appears once again. It would be nice to show a loaded only once
  3. Could you please remove red background from error messages? :)
    remove the background

Thank you

@yvesfracari yvesfracari requested a review from elena-zh December 12, 2024 13:17
@yvesfracari
Copy link
Contributor Author

@elena-zh Thanks for the feedback. The errors 2-4 are fixed. As I mentioned on our chat, the 1 error is due to an undeployed contract on Base. I think that we can test it again once it is deployed.

@elena-zh
Copy link

Hey @yvesfracari , thank you.

The contract on Base has been deployed, hooks are working there now.

As for issues:

  1. Could you please check why I cannot see my pool tokens on Arbitrum?
    image
    image

  2. Could you please reset search results once I close the Search area and navigate back to the 'Select pool' state?
    quit
    reset

Thanks!

@yvesfracari
Copy link
Contributor Author

Hey @elena-zh !

Currently, there is only 1 Uni v2 pool on the CoW LP Token List for arbitrum. The pools that you presented are CoW AMMs.

image

About resetting the search field, it is done!

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

Thank you

@elena-zh elena-zh requested a review from a team December 17, 2024 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants