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

Fix select network and add token requests #11

Open
wants to merge 4 commits into
base: multi
Choose a base branch
from

Conversation

bkbooth
Copy link

@bkbooth bkbooth commented Sep 2, 2022

If Ethereum provider is a class, spreading it onto a new object won't copy methods to the new object

Fixes #10

If ethereum provider is a class, spreading it onto a new object won't copy methods to the new object
@bkbooth
Copy link
Author

bkbooth commented Sep 2, 2022

I've tested this fix locally and switching networks via the AnySwap UI works as expected with the injected provider from our wallet, and still works as expected with MetaMask.

Copy link

@mzndako mzndako left a comment

Choose a reason for hiding this comment

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

No need of setting request method to the same method or an empty string.

Comment on lines 9 to 10
const ethereumFN: any = ethereum;
ethereumFN.request = (ethereum as any).request ?? '';
Copy link

Choose a reason for hiding this comment

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

calling ethereumFN.request will throw an error if ethereum obj is undefined. You dont need to set the request method as it should be part of the ethereum object you are referencing

Suggested change
const ethereumFN: any = ethereum;
ethereumFN.request = (ethereum as any).request ?? '';
const ethereumFN: any = ethereum

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, this is definitely better. I originally raised an issue (#10) about this, I'm not sure why the code is de-structuring ethereumFN from window.ethereum at all, but I tried to keep my PR as close to the original code as possible. But I should have made lines 9 & 10 like this to properly maintain consistency:

const ethereumFN: any = ethereum ?? {};
ethereumFN.request = (ethereum as any).request ?? '';

Copy link
Author

Choose a reason for hiding this comment

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

I simplified the code in my PR slightly which should also fix the concern with window.ethereum being undefined.

Comment on lines 59 to 60
const ethereumFN: any = ethereum;
ethereumFN.request = (ethereum as any).request ?? '';
Copy link

Choose a reason for hiding this comment

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

ditto

Suggested change
const ethereumFN: any = ethereum;
ethereumFN.request = (ethereum as any).request ?? '';
const ethereumFN: any = ethereum

Copy link

@mzndako mzndako left a comment

Choose a reason for hiding this comment

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

utACK

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.

Our injected Ethereum provider never gets the 'wallet_addEthereumChain' request
2 participants