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

Illegal invocation after using wallet_requestSnaps #1345

Closed
alanorwick opened this issue Apr 12, 2023 · 24 comments
Closed

Illegal invocation after using wallet_requestSnaps #1345

alanorwick opened this issue Apr 12, 2023 · 24 comments

Comments

@alanorwick
Copy link

Function we are calling:

    async function connect() {
      await ethereum.request({
        method: 'wallet_requestSnaps',
        params: {
          [snapId]: {},
        },
      });
    }

Error we are seeing:
Screen Shot 2023-04-12 at 2 22 00 PM

@alanorwick
Copy link
Author

alanorwick commented Apr 12, 2023

Versions:

    "@metamask/auto-changelog": "^3.0.0",
    "@metamask/eslint-config": "^8.0.0",
    "@metamask/eslint-config-jest": "^8.0.0",
    "@metamask/eslint-config-nodejs": "^8.0.0",
    "@metamask/snaps-cli": "0.32.0",

@pragmatos
Copy link

It happens after update MetaMask Flask in Chrome to 10.28.2-flask.0, previous version was working correctly.
Any news?

@FrederikBolding
Copy link
Member

This may be a known bug with usage of atob and btoa. If you are comfortable with it, you could try building the MM extension locally from https://github.com/MetaMask/metamask-extension to verify that it is fixed in an upcoming release.

@pragmatos
Copy link

pragmatos commented Apr 13, 2023

This may be a known bug with usage of atob and btoa. If you are comfortable with it, you could try building the MM extension locally from https://github.com/MetaMask/metamask-extension to verify that it is fixed in an upcoming release.

Could you help?
How correct to build last version of flask in https://github.com/MetaMask/metamask-extension. I'm getting extension errors in Chrome. Thanks
image

@alanorwick
Copy link
Author

Any updates here?

@Montoya
Copy link
Collaborator

Montoya commented Apr 17, 2023

Hi @alanorwick , what is the value of snapId in your example? Is it a variable?

@zwilling
Copy link

I have the same issue with my snap since the flask 10.26->10.28.2-flask.0 update.

It works with an empty snap (same permissions, processRpcRequest empty) and breaks as soon as I add a reference to import { stringToBytes, bytesToHex } from '@metamask/utils'; back in (tried @metamask/utils versions 3.5.0 and 5.0.1).
As @FrederikBolding suggested, it also breaks the same way when using atob.

This issue seems also related: MetaMask/metamask-extension#18590

@Montoya The snapId I use is a string variable containing local:http://localhost:8080.

@FrederikBolding I built Flask 10.28.3-flask.0 and got the same error. When I try the same with the develop branch, it just loads forever after accepting permissions.

@pragmatos I built it with yarn install; yarn dist --build-type flask.

@alanorwick
Copy link
Author

Hi @alanorwick , what is the value of snapId in your example? Is it a variable?

Similar to @zwilling my snapId is coming in as local:http://localhost:8083. Broken on both local and hosted snapIds via npm. Example snapId for npm npm:@quainetwork/quai-snap

@FrederikBolding
Copy link
Member

FrederikBolding commented Apr 18, 2023

Looking into this to verify whether this is fixed on develop. Will respond back when I have more information

@FrederikBolding
Copy link
Member

I'm seeing atob and btoa working on develop when building with yarn dist --build-type flask.

Will look into the @metamask/utils problem that @zwilling mentioned.

@FrederikBolding
Copy link
Member

I cannot reproduce the @metamask/utils problem. Is there a repository I can try to repro from @zwilling ?

@pragmatos
Copy link

I'm seeing atob and btoa working on develop when building with yarn dist --build-type flask.

Will look into the @metamask/utils problem that @zwilling mentioned.

I tried develop, atob and btoa work. But there is another issue.
I included "polyfills" there are some functions override Object methods.
I got Error. With code bellow you could reproduce it.
const TestObj = { testProp: {}, }; TestObj.testProp.toLocaleString = () => '';
Screenshot 2023-04-18 at 19 47 11

Between Extension has errors

image

Any ideas?

@FrederikBolding
Copy link
Member

@pragmatos Properties such as toLocaleString are protected, but you can modify them as such instead:

Reflect.defineProperty(obj, 'toLocaleString', { value: function toLocaleString(formatOpts, opts) { ... } });

See this patch as an example: https://github.com/MetaMask/test-snaps/blob/021bfda5f30f1f229dee0f18dc3731bb15ca1875/.yarn/patches/luxon-npm-3.1.0-16e2508500.patch

You may need to patch your dependencies if they are modifying props such as toLocaleString

@alanorwick
Copy link
Author

@FrederikBolding With bare minimal dependencies I'm seeing @metamask/snaps-utils uses "cron-parser": "^4.5.0" which has a dependency on luxon. Just as you patched luxon in the test-snaps example, is it expected that all snaps run the luxon patch on toLocaleString?

Can't confirm it resolves after running the patch as well. Still working through applying the patch via yarn for the nested dependency.

@pragmatos
Copy link

@pragmatos Properties such as toLocaleString are protected, but you can modify them as such instead:

Reflect.defineProperty(obj, 'toLocaleString', { value: function toLocaleString(formatOpts, opts) { ... } });

See this patch as an example: https://github.com/MetaMask/test-snaps/blob/021bfda5f30f1f229dee0f18dc3731bb15ca1875/.yarn/patches/luxon-npm-3.1.0-16e2508500.patch

You may need to patch your dependencies if they are modifying props such as toLocaleString

It's clear. But I use dependencies with Intl. Intl is not able in the snap.
Will `Intl' added in snaps? Thanks

@Montoya
Copy link
Collaborator

Montoya commented Apr 19, 2023

@pragmatos you may have to patch Intl or find a different library that works under SES. Here's a guide to patching: https://docs.metamask.io/snaps/how-to/troubleshoot#patch-dependencies

@zwilling
Copy link

I cannot reproduce the @metamask/utils problem. Is there a repository I can try to repro from @zwilling ?

Thanks for checking @FrederikBolding . Trying to compile a minimal example reproducing it, I noticed that the issue is not caused by @metamask/utils. Instead it is caused by the library from where I imported a constant which is passed to the utils function.
Sorry for the false alarm. I did not expect such side effects from using a constant 🙈

Will further investigate it. I also have luxon in my dependencies.

@FrederikBolding
Copy link
Member

FrederikBolding commented Apr 19, 2023

@FrederikBolding With bare minimal dependencies I'm seeing @metamask/snaps-utils uses "cron-parser": "^4.5.0" which has a dependency on luxon. Just as you patched luxon in the test-snaps example, is it expected that all snaps run the luxon patch on toLocaleString?

Can't confirm it resolves after running the patch as well. Still working through applying the patch via yarn for the nested dependency.

@alanorwick Hmm, it is not intended for all snaps to need to patch luxon. What dependencies does your snap have that bring in @metamask/snaps-utils? Or are you using @metamask/snaps-utils directly in your snap?

Also, what are you using for bundling? Because @metamask/snaps-utils may sometimes be brought in for type reasons, but usually is not included in the final snap bundle

@alanorwick
Copy link
Author

@FrederikBolding from what I can tell @metamask/snaps-uiuses @metamask/snaps-utils.

We use @metamask/snaps-ui directly in the project. If @metamask/snaps-ui and @metamask/key-tree build okay as a base then it is likely a problem with another dependency. I've seen a few errors with ethers@6.3.0 as well in regards to private methods. Going to dive into that more before opening up another issue.

@alanorwick
Copy link
Author

alanorwick commented Apr 19, 2023

@FrederikBolding from what I can tell @metamask/snaps-uiuses @metamask/snaps-utils.

We use @metamask/snaps-ui directly in the project. If @metamask/snaps-ui and @metamask/key-tree build okay as a base then it is likely a problem with another dependency. I've seen a few errors with ethers@6.3.0 as well in regards to private methods. Going to dive into that more before opening up another issue.

So with only these dependencies I can build and connect fine:

    "@metamask/key-tree": "7.0.0",
    "@metamask/snaps-ui": "^0.32.2",
    "patch-package": "^6.5.1",
    "postinstall-postinstall": "^2.1.0"

After adding v5.4.3 of Ethers I see the Illegal Invocation again. If I upgrade Ethers to 6.3.0 I see an error on mm-build from the use of # to indicate a private method on the ens-resolver.

If I had to guess where v5.4.3 would error it would be on the buffer package calling Buffer.prototype.toLocaleString = Buffer.prototype.toString.

@himanshuchawla009
Copy link

himanshuchawla009 commented Apr 21, 2023

My Snap was working till last week and all of a sudden I am getting this error while making an API call inside snap. Don't know what I need to change here.
Screenshot 2023-04-21 at 12 21 43 PM

@FrederikBolding
Copy link
Member

A fix is part of Flask 10.29.0, which we just submitted to Chrome and Firefox. So a fix should be rolling out as soon as they approve it.

@FrederikBolding
Copy link
Member

Flask version 10.29.0 is rolling out on Chrome now. Let me know if this fixes your problems with illegal invocation

@alanorwick
Copy link
Author

Flask version 10.29.0 is rolling out on Chrome now. Let me know if this fixes your problems with illegal invocation

Fixed it, thank you.

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

No branches or pull requests

6 participants