Add Create LlamaPay Vesting iframe hook

[JeanNeiverth]

Summary

Add Create LlamaPay Vesting hook

To Test

1. (Access /#/100/swap/hooks/ running locally) or (access https://dev.swap.cow.fi/#/100/swap/hooks/ and add custom post-hook with URL https://cow-hooks-dapps-create-vesting.vercel.app)
2. Connect your wallet and switch to a chain with deployed LlamaPay vesting factory (Mainnet, Gnosis or Arbitrum)
3. Fill some swap order
4. In Hooks > Add Post-Hook Action, the Create Vesting Hook should appear in the "All hooks" section
5. Fill the form data and add the post-hook

Issues that are still being fixed:

• Tenderly simulations are currently not working
• If you try to manually input a vesting amount that is larger than your balance, the encoding will fail

Other info

Old PR: #5026

[JeanNeiverth]

Hi @elena-zh, thanks for your feedback! I had to open this new PR because I unintentionally closed the first one, sorry about that. Despite some issues are still on progress, I'll let some updates down here. Feel free to point more issues in the meantime.

1. Icons changed. What do you think of the new ones?
2. Fixed
3. Fixed
4. Fixed
5. Fixed
6. I tried to replicate the component, what's your opinion about the new version?
7. Fixed
8. Fixed
9. This will be pending for now since there's an update by CoW side coming soon that will help us solve this
10. Removed the address display. About the signing page, since we use different styling tools than CoW, replicating their exact spinner would be challenging and time-consuming. While we could get close, it might not be identical. Could you let me know if this is a critical requirement, or if our current spinner adequately matches CoW's style?
11. It is the expected behavior and happens because the hook changes the swap recipient (user or proxy) in order to save gas consumption depending on the parameters (if it will vest the user input, all from swap or all from swap + user balance). The most important in a vesting contract is the recipient, but it is also possible to verify the sender this way just comparing its proxy with the vesting contracts creator

[elena-zh]

Hey @JeanNeiverth , great job, thank you!

Some my notes:

1. Max button (nitpick): it would be nice to hide it once 100% amount is selected
   

2. Warning UI: great job! But I'd colored it with Yellow as it is more like an informational message, not like a warning that should protect from doing smth wrong. (again, it is a nitpick!)
   

3. Buy/Sell order type: required changes from our side are in Develop now, So you should be unblocked with fixing p.9

4. signing page layout: totally understand. It looks good. The only one thing to improve here is to make the stepper elements center-aligned
   

It is the expected behavior and happens because the hook changes the swap recipient (user or proxy) in order to save gas consumption depending on the parameters (if it will vest the user input, all from swap or all from swap + user balance). The most important in a vesting contract is the recipient, but it is also possible to verify the sender this way just comparing its proxy with the vesting contracts creator

Thank you for explanation, but my question here is related to a bit different thing: if proxy address is used for all these 3 cases, then, I think, the same behavior should be applied on the UI, and the recipient address (with the appropriate warning in the confirm modal) should appear when I pick option 2 and 3. WDYT?

Thanks!

[JeanNeiverth]

Hi @elena-zh,

Thanks again for your reviews! I've addressed the first 4 topics. Regarding the 5th point about the proxy/address warning:

Currently, this notification appears only during the swap step. Since the hook summary UI exists outside the hook dapp, we have limited ability to modify these warning displays from the dapp side.

In my point of view, the warning's purpose is to inform users: "Hey, be careful! The tokens from this swap will be directed to a different address than your main account." In cases where we're adding, for example, a create vesting hook, this warning may not be as necessary since users would expect their tokens to be sent to a vesting contract (and the proxy is just a sort of technical step inside this to make the operation safer).

I understand that, if we think this way, then it would be better to remove the recipient warning for the 1st case, but I assume it can tricky to handle this on the swap UI side. WDYT @shoom3301 @alfetopito?

[elena-zh]

Hey @JeanNeiverth , thanks for fixes.
However, I don't see warnings now at all: both for Sell and Buy orders it does not appear when I pick options 1 and 2. Could you please double check this?

[shoom3301]

@JeanNeiverth I think the hook is supposed to change recepient to the proxy address always, isn't it?
If yes, then it seems to be a bug in the hook dapp, because now when you edit hook and change the option to "
Use all your tokens after swap" then the recepient is resetted.

[yvesfracari]

@shoom3301 on my understanding this is not a bug. Using the entire balance after the swap represents the current balance + executed buy amount. In that sense, even if we change the receiver to the proxy address we still have to pull the current balance from the user's wallet, so it is easier to not override the recipient and pull all the tokens after the swap.

[JeanNeiverth]

Hi @elena-zh!

About the lack of warning in sell orders, that's strange 🤔... For me it's working fine here

I am testing it the following way: if I want a sell order, I edit the swap amount of the token I'm selling in the swap section (then the warning appears). If I want a buy order, just the opposite, I set the amount of the tokens I'm buying (and it works, no more warnings)

Could you provide more details about how are you testing this?

[elena-zh]

Hi @elena-zh!

About the lack of warning in sell orders, that's strange 🤔... For me it's working fine here

I am testing it the following way: if I want a sell order, I edit the swap amount of the token I'm selling in the swap section (then the warning appears). If I want a buy order, just the opposite, I set the amount of the tokens I'm buying (and it works, no more warnings)

Could you provide more details about how are you testing this?

Hey @JeanNeiverth ,
I use this link to test https://swap-jewatjjbn-cowswap.vercel.app/#/100/swap/WXDAI, the only one available for me in this PR.
I connect to a wallet on Gnosis chain, specify a sell order and trying to add 'Create LLama vesting' hook.

As for this statement:

. Using the entire balance after the swap represents the current balance + executed buy amount

but the entire swap outcome will be sent to another (proxy) address: isn't it a 'send to recipient' option, no? So a user should be warned about it like it is done for the 1st option

[vercel]

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

A member of the Team first needs to authorize it.

[JeanNeiverth]

Hi @elena-zh, thanks for providing more information

I understand the issue now: this PR was created before the order kind started being passed to the hooks. That's why if you try to access the deployment link you have here this feature won't work (I'm merging with develop to fix this, but someone will have to authorize deploy first).

For now, you can see it working if you access develop https://dev.swap.cow.fi/#/100/swap/hooks/ and add custom post-hook with URL https://cow-hooks-dapps-create-vesting.vercel.app

[yvesfracari]

@elena-zh About this:

but the entire swap outcome will be sent to another (proxy) address: isn't it a 'send to recipient' option, no? So a user should be warned about it like it is done for the 1st option

I talked with @shoom3301 and @alfetopito about this and it is a different behavior in my opinion. The tokens are being sent to another address but via the hook and not via the swap, and that is why there isn't a warning. However, I get your point that could be clear to the user that his funds are being sent to another address and that is the case for all hooks that are interacting with the proxy (including pre-hooks). So, we are thinking of adding another kind of warning to show this information to the user in a future PR. How does this sound to you?

[vercel]

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

Name	Status	Preview	Updated (UTC)
swap-dev	✅ Ready (Inspect)	Visit Preview	Nov 6, 2024 7:57am

[elena-zh]

Hey @JeanNeiverth , @yvesfracari , thank you for clarification.
I'm OK with adding an additional message in a separate PR.

As for the current PR:

1. I've noticed that the hook cannot be added in Safe (or any other SC wallet). So it would be great to let a user know about it here
   
   and add 'Unsupported wallet' message when a user tries to add this hook there. At least, for now, until SC wallet support for hooks will be implemented.
2. It would be nice to improve a bit amount field view in a mobile resolution:
   
3. (nitpick!) font in the amount field differs from the one we have elsewhere. Could you please improve it a bit?
   

Thanks

[yvesfracari]

Hey @elena-zh ! Thanks for the feedbacks!

1- Thanks for noticing this! I marked the app as not supporting Safe (not sure if it will reflect on the next deployment or not). However, that flow of connecting via Wallet connect might happen again. However, as was mentioned on the Withdraw Hook review, this should be handle on cowswap side (outside the context of this PR).

2- Done.
3- We're using the same font on the entire app actually. We only change the font weight but on this example that you show is the same weight as well I think.

[elena-zh]

Hey!

Thank you for fixes. But:

1. Mobile view with the Max button still does not look good
   
2. I'd really appreciate it if you change the font-weight for an amount
   
3. It would be nice to hide Max button for 0 :)
   
4. Could you please update the PR with changes in Develop, so some performance issues will be fixed:
   
5. Question: should we allow to add a hook when there is insufficient balance for a trade? Currently, we don't allow when pick an unsupported token, so here I'd add this validation as well
   

Thanks

[yvesfracari]

Thanks for the feedback @elena-zh !

1- Done.
2- Done.
3- Done.
4- Done.
5- I agree that there are some flows that could be improved for all hooks. We're exploring an option to include all of them in a new grant. Thanks for reporting this, I think that we can work on this on a separate task.

[elena-zh]

Hey @yvesfracari , great, thank you!

My last 2 UI improvements I'd like to report:

1. Could you please place Balance line under a token, so it will not create a cluttered effect in the field like this:
   
   See an example:
   
2. When there is a long value in the field, it is glued to the token, and is truncated in the end:
   
   Could you please improve it by adding a space after a token and ellipsis in the end like we do in other fields? See an example:
   

[yvesfracari]

Thanks for catching this @elena-zh ! I tried to apply these layout changes, let me know if there is something else to be done!

[elena-zh]

Thank you!

[elena-zh]

The only thing that I want to mention is that in your PRs it is impossible to connect to MM atm:

But I will report it in #5039 PR where it is reproducible as well.

[yvesfracari]

Thanks for the review @elena-zh ! Regarding the Metamask warning, this doesn't seem related to none of these PRs (just need to update the package maybe), but I think this should be made on a separate PR cc @shoom3301 @alfetopito @anxolin