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

[BUG FIX]: Axios to Fetch Migration #11936

Closed
wants to merge 1 commit into from
Closed

Conversation

proffapt
Copy link

@proffapt proffapt commented Feb 7, 2024

What?

Migrated to use Fetch for making network calls instead of Axios. This is the fix which allows the creation of Metamask Snaps.

Why?

https://docs.metamask.io/snaps/how-to/troubleshoot/#axios

As mentioned clearly in the above section of official documentation of Metamask Snaps, they run in a sandboxed environment in which axios doesn't work, always leading to this very error. In addition, none of the solution provided in that issue thread works in this case either.

Test Plan

Can be tested on a template snap

Copy link

trunk-io bot commented Feb 7, 2024

⏱️ 14s total CI duration on this PR

Job Cumulative Duration Recent Runs
permission-check 4s 🟥
permission-check 4s 🟥
permission-check 4s 🟥
permission-check 2s 🟥

settingsfeedbackdocs ⋅ learn more about trunk.io

@0xmaayan
Copy link
Contributor

@proffapt Thank you for the PR.

The Aptos SDK provides a custom client configuration where one can override the existing client (i.e axios), can check out this example where we use fetch as a custom client https://github.com/aptos-labs/aptos-ts-sdk/blob/main/examples/typescript/custom_client.ts#L21

Additionally, this change requires comprehensive testing of the SDK, and we should explore the tradeoff between axios and fetch more.

@proffapt
Copy link
Author

proffapt commented Apr 6, 2024

Hi @0xmaayan ,

I didn't know that aptos provides custom client configurations. The reason for making this PR is mentioned in the PR message. This custom client should solve my problem now (for which I wrote this migration from axios to fetch).

I am closing the PR now.

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.

None yet

2 participants