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

Add claim airdrop hook app #4897

Conversation

JeanNeiverth
Copy link
Contributor

@JeanNeiverth JeanNeiverth commented Sep 16, 2024

Summary

  • Add Claim Airdrop Hook as a PreHook app;
  • Add one example airdrop in order to test the application. The name in the selector is COW, which is only configured for Sepolia network;
  • The example follows the ERC20 Merkle Distributor pattern (repo link). The idea is that in order to be compatible with the hook app, future airdrop deployers will need to use this standard.

image

image

image

image

image

image

To Test

  1. Open the page /#/11155111/swap/hooks/WETH
  • You should be able to see the swap with hooks screen
  1. Connect your wallet and switch to Sepolia Network

  2. Click 'Add PreHook action' > 'Claim Airdrop' and test the app

  3. If your wallet is in this csv you should be able to see your claimable tokens. Else, you will probably get the message 'No tokens to claim'

JeanNeiverth and others added 18 commits August 29, 2024 16:26
…ook-ui

Jean/cow 345 create minimal airdrop hook UI
* fix: aidropHookApp -> move UI components to a separate folder

* feat: hook to get user claimable tokens

* feat: add COW database url in airdrop hook constants

Co-authored-by: Pedro Yves Fracari <[email protected]>

* fix: switch name 'link' to 'dataBaseUrl'

* feat: (wip) add test function for findIntervalKey

* docs: comment findIntervalKeys function in Airdrop hook

* fix: typescript issues

* feat: add message to the user about claimable tokens status

* wip: call isClaimed verification on contract

* wip: use preview token output and add airdrop contract hook

* feat: add preview of claimable tokens in Airdrop hook

* style: add empty line in the end of component files

* fix: change token formatting to cow standard

* refactor: move dropdownMenu from  AidropHookApp/index to components

* fix: no more claimable tokens messages when user connects without airdrop selected

* wip: review

* refactor: add claimed call on the usePreviewTokens

* chore: remove duplicated virtual token abi

* refactor: transform test script in ts test

* refactor: rename preview tokens error variable

---------

Co-authored-by: Pedro Yves Fracari <[email protected]>
Co-authored-by: Pedro Yves Fracari <[email protected]>
* wip: hook encoding

* chore: add airdrop claim hook

* wip: add cowshed hooks

* refactor: rename components folder to styles

* refactor: simplife hooks and fix cow shed init code

* refactor: components hook messages and hooks

* chore: replace airdrop contract for uniswap interface

* fix: airdrop conctract merkle proof

---------

Co-authored-by: Jean Neiverth <[email protected]>
* wip: fixing airdrop address

* chore: add airdrop abi

* fix: use new airdrop contract on useClaimData

* chore: remove todo comment

---------

Co-authored-by: Jean Neiverth <[email protected]>
Copy link

vercel bot commented Sep 16, 2024

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

A member of the Team first needs to authorize it.

Copy link
Contributor

github-actions bot commented Sep 16, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@JeanNeiverth JeanNeiverth changed the title Jean/cow 346 airdrop hook UI refactoring based on designs Add claim airdrop hook app Sep 16, 2024
@ribeirojose
Copy link
Contributor

I have read the CLA Document and I hereby sign the CLA

yarn.lock Outdated Show resolved Hide resolved
Copy link

vercel bot commented Sep 17, 2024

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

Name Status Preview Updated (UTC)
swap-dev ✅ Ready (Inspect) Visit Preview Sep 23, 2024 7:53am

@JeanNeiverth
Copy link
Contributor Author

Build is failing: image

I don't know if it is because we did some changes, but here yarn build is running

image

The yarn build:all triggers some errors, but it doesn't seem it is because anything we changed

image

Can you guys check this again?

@alfetopito
Copy link
Collaborator

Build is failing: image

I don't know if it is because we did some changes, but here yarn build is running

image

The yarn build:all triggers some errors, but it doesn't seem it is because anything we changed

image

Can you guys check this again?

I get the same running both develop and this branch locally.
Must be something with CI, then.
I'll take a look.

@elena-zh
Copy link
Contributor

Hey @JeanNeiverth , great!

I've not been able to run all my tests till the end as I don't have a PK of any whitelisted account, but I'd love to have it and check how the balances are changes after a trade is executed..

Anyways, here is the list of issues I've faced today:

  1. Is this OK to add the same hook for the same account several times?
  2. I add an airdrop hook, press on the Edit button --> no details of the hook added there
    image
  3. (continuation of the 2nd point): I fill in the details on the Edit form and save changes, I see a new hook added to the form
  4. I can add 'Claim airdrop' pre-hook in the post-hook section.
    image
  5. Might it be possible to show an airdrop amount on the hook details once it is added to the form?
    image
  6. The hook's icon looks a bit blurred in Win11
    blurred icon
  7. Add a hook: I'd hide this field until a hook is selected, and there is an available amount to claim
    image
  8. Wallet is not connected: I'd hide all controls there and show 'Connect to a wallet' message like it is implemented
    connect wallet
    for the Claim GNO hook
  9. As for list of available claims, I'd reuse this UI component from the Tokens page (personal opinion ;))
    image
    image
    Besides, it would be great to have a possibility to close this dropdown without picking any option.

Thanks

@JeanNeiverth
Copy link
Contributor Author

JeanNeiverth commented Sep 18, 2024

Hi @elena-zh, thank you for your feedbacks! I made some changes and will try to answer each point you brought.

First of all, we decided during some discussions of this PR that now we will have one hook per airdrop, which means we won't have the airdrop dropdown selector anymore. I apologize for not updating the PR description right after we took this decision.

  1. I understand that concern, let me circle back to the team to see how we can improve here, but I'm afraid there's no practical way to avoid that. I know there are plans to work on simulations that will make clear to the user when something's about to go wrong. Do you think this is critical to solve now?
  2. this is outdated, since we don't have input parameters anymore;
  3. Fixed, now the "add button" becomes a "return to swap" button in the edit mode;
  4. Could you elaborate more here? The team from CoW and us thought it might be a good idea to have it as a post hook as well. Do you think that might not make much sense at this point?;
  5. The situation is pretty similar to 1, we would love to help, but I fear that working on this affects other scopes too. How important do you think it is?;
  6. Fixed, now using svg icon;
  7. Fixed
  8. Fixed
  9. outdated, same case as (2)

By the way, we've been working from a specific UI design (Link here). Could you confirm if your team has had a chance to review it? If not, how could we make this easier for you?

Please feel free to take another look in the new hook and tell us if there are other points to change,
Thank you

@elena-zh
Copy link
Contributor

Hey @JeanNeiverth , thank you for clarifications and fixes!

Still, I have some issues to report :)

  1. Edit mode: I don't see 'Return to swap' button there
    image
  2. Not eligible to claim: we still display the 'Total available' field. Could you please add 0 COW amount into it, so it will make a bit more sense why this field is displayed on the form?
    0
  3. Not connected wallet: I'd hide this field from the form at all
    not connected
  4. Post hooks: if the hook should be displayed there, IMO, it would be great to change its description and remove 'before' from texts (or changed to 'before and/or after')
    post - description
    post-hook
  5. Still, I can see a blurred icon in the hook :(
    blurred

Is this OK to add the same hook for the same account several times?

I understand that concern, let me circle back to the team to see how we can improve here, but I'm afraid there's no practical way
to avoid that. I know there are plans to work on simulations that will make clear to the user when something's about to go wrong. Do you think this is critical to solve now?

I'm thinking that it would be great to protect users from stupid actions resulting in paying twice for the same transactions.
@shoom3301 , WDYT? Should this case be fixed here or added as a separate task for addressing later?

Might it be possible to show an airdrop amount on the hook details once it is added to the form?
The situation is pretty similar to 1, we would love to help, but I fear that working on this affects other scopes too. How important do you think it is?;

For me, personally, it is important to show a user an amount to claim and then to swap, if needed.
@fairlighteth , @shoom3301 , WDYT about this case?

Thank you!

@yvesfracari
Copy link
Contributor

Hey @elena-zh . I think that you are not using the latest version. I am not sure if you are running locally or approving the deployments, but since we will start to open some PRs I am wondering if we could have authorization for the CI deployment cc @shoom3301 @fairlighteth

@shoom3301
Copy link
Collaborator

shoom3301 commented Sep 19, 2024

WDYT? Should this case be fixed here or added as a separate task for addressing later?

@elena-zh
One thing I would like to specify about hooks store - hooks validation and adequacy.
We discussed many times cases like:

  • what if I add the same hook 3 times?
  • what if I claim GNO as a prehook, but my sell token will be USDC?
  • should we allow adding a pre-hook with permitting COW, but selling WETH?

So mainly, those questions are about hooks ordering and hooks influence on the order parameters.
To cover all those questions, I would like to introduce two conceptions.
Hooks are “programming language”. It means, that hooks provide a way to program some actions before and after trade. Using javascript we can write “console.log(1);console.log(1);console.log(1);”. Is this code valid? Yes, it is. Does it make sense? Not so much. Same with hooks, if a hook callData is syntactically valid (Tenderly simulation is successful) - we accept it, but we don’t judge is it logical or not.
Hooks presets / hooks chain / scenarios / etc. This abstraction is responsible for providing a sequence of related on meaning hooks. For example - liquidity migration from Uniswap to CoW AMM. To do this we need: withdraw lp tokens from Uni, swap them, approve spending tokens, transfer them into CoW AMM pool. This scenario must be validated once it’s ready, and it should be validated by a human, I don’t think we can do it programmatically.
In conclusion, there are two independent jobs:

  1. Hooks scenario writing
  2. Hooks scenario usage

First one mainly is supposed to be done by a person with some technical knowledge.
Second one is supposed to be done by any user who interested in its results.

Answering on your question - for now we should keep it as it is

@shoom3301
Copy link
Collaborator

Might it be possible to show an airdrop amount on the hook details once it is added to the form?

We have been discussing this with @fairlighteth , it supposed to be displayed as a standardized list of results, like:

 - added 30 COW
 - changed recipient to 0x90000...11
 - changed sell token to WETH

@elena-zh this is out of this task scope, we will work on it later

@elena-zh
Copy link
Contributor

elena-zh commented Sep 19, 2024

@yvesfracari , @shoom3301 for clarifications.

I've been able to run the full airdrop flow! Works great!

Some issues that I can still see:

  1. Still. the icon does not look good :(
    icon

  2. Posthooks: need to change its description to 'before and/or after' here
    image

  3. Once I've claimed all my available airdrop, the hook is still displayed on the form. However, not sure, maybe it is an expected behavior :)
    image

image

@JeanNeiverth
Copy link
Contributor Author

@elena-zh Thanks again for your careful review!

  1. We changed the hook image so now its background is transparent and the color is a little lighter. We did this because the background color is automatically set based on the dark / light mode, and it is standardized for all hooks. By turning the image lighter, we are able to work around the problem, because it works for both themes. Do you think this color tone works great?

image

image

Please note that you will probably not be able to run the latest versions until another deploy, but you can take a look here:

  1. Fixed

  2. I understand that it would provide a very nice experience to the user. However, this is another issue which solution resides in more complex questions. By solving that, we would possibly be creating scalability problems. Imagine getting the claim status of every user entering in the page, or keeping the data in their local storage. Now apply this logic for every hook in a future where we are expecting to have a lot of hooks: at some point the memory or processing would be overcharged. Please, let us know if you think this feature is critical, or if there are any other issues you see.

Copy link
Contributor

@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 for the fixes and explanation. Approved!

@JeanNeiverth
Copy link
Contributor Author

Hey @shoom3301 @alfetopito, just to say I had to merge develop into my branch to resolve some conflicts. Now, I would like to kindly ask you guys to deploy again xD, so we can merge the PR.

@alfetopito
Copy link
Collaborator

Now lint is complaining 🙈
I'll merge as is and address it on our side since it'll be faster, given the GH fork deployment rules.

@alfetopito alfetopito merged commit cab2407 into cowprotocol:develop Sep 23, 2024
6 of 12 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants