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: improve custom hook validation messages #5123

Merged
merged 23 commits into from
Nov 27, 2024

Conversation

fairlighteth
Copy link
Contributor

@fairlighteth fairlighteth commented Nov 21, 2024

Summary

  • Addresses Custom hook: wrong validation message when something is wrong with a hook's link #5116
  • Aims to provide more specific error messages. It first check if the domain resolves. And if it does, check if the hook url has CORS issues. This allows for the end-user/developer to better handle their errors.
  • Still handles special protocols (ipfs/ipns/ar) via uriToHttp by converting to a HTTPS accessible URL
  • Converts HTTP to HTTPS on paste/submit. If the user types a HTTP:// url, it shows a specific error validation message for that.
Screenshot 2024-11-21 at 13 04 58 Screenshot 2024-11-21 at 13 19 33 Screenshot 2024-11-21 at 13 12 07

Copy link

vercel bot commented Nov 21, 2024

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

Name Status Preview Updated (UTC)
cosmos ✅ Ready (Inspect) Visit Preview Nov 27, 2024 2:34pm
cowfi ✅ Ready (Inspect) Visit Preview Nov 27, 2024 2:34pm
explorer-dev ✅ Ready (Inspect) Visit Preview Nov 27, 2024 2:34pm
swap-dev ✅ Ready (Inspect) Visit Preview Nov 27, 2024 2:34pm
widget-configurator ✅ Ready (Inspect) Visit Preview Nov 27, 2024 2:34pm

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.

Hey @fairlighteth , sorry to tell you this, but something is not working as expected here:

  1. I'm not able to open a hook that is running locally:
    image
  2. When I'm adding https://cow-hooks-dapps-cow-amm-deposit.vercel.app hook link (it is a valid link), the app says that it is blocked by CORS issues
    cannot add
  3. Some working validations gone missing:
  • post-pre hooks
    pre-post
  • chain:
    chain
  • wallet support:
    wallets support

Could you please take a look at all these issues?

@fairlighteth
Copy link
Contributor Author

@elena-zh could you try again? The only case I couldn't simulate on my end was the 'wrong chain' scenario. The others seemed correct now.

}, options.timeout)

try {
const response = await fetch(url, { signal: controller.signal, ...options })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, use existing getTimeoutAbortController() from @cowprotocol/common-utils

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reviewing. Addressed this now.

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

Awesome! thanks for addressing the comments

@@ -0,0 +1,23 @@
import { getTimeoutAbortController } from '@cowprotocol/common-utils'
Copy link
Contributor

Choose a reason for hiding this comment

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

I was suggesting to move it to @cowprotocol/common-utils, you might want to use this in other front-ends and not just CoW Swap. Anyways, we can always move when needed in other apps if this is adding some overhead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anxolin I see now it was added to the common-utils but didn't cleanup the older util function. Just pushed a fix for this now.

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.

4 participants