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

wallet provider management #242

Merged
merged 7 commits into from
Jun 26, 2024

Conversation

Keyrxng
Copy link
Member

@Keyrxng Keyrxng commented Jun 8, 2024

Resolves #241

https://www.github.com/MetaMask/metamask-mobile/issues/9519

A known issue is stopping us from being able to change the in-wallet RPC ourselves right now so the next best thing is to suggest they replace it themselves and serve an infinite toast.

Me trying to use wallet_addEthereumChain and getting {} which is expected when this call succeeds.
image

Repeating the same call with an incorrect args throws an error as expected.
image


This is what a user will see if we need to suggest something to them. The bottom-most toast being infinite.
image


It has been difficult trying to reproduce, debug and think of how to implement effective tests for this. MM won't save an RPC that it cannot connect to itself, so you are relying on connecting to an RPC which is up but often goes down and then it happening while you are spamming refresh.

I was forcing the results through if(true) after I gave up trying to reproduce.

You see here that MM is indeed firing these calls to the in-wallet RPC
valid-stress-test so it is testing it.

On desktop the automatic change goes through as you would expect, no muss no fuss.

So until the known issue is resolved it's best to just suggest that a user manually change their in-wallet RPC.

@ubiquity-os-deployer
Copy link

ubiquity-os-deployer bot commented Jun 8, 2024

@Keyrxng
Copy link
Member Author

Keyrxng commented Jun 8, 2024

I've updated the PR body with some info and debugging infos but the tldr is:

  • can't change provider directly on mobile
  • infinite toast with suggested provider displayed on mobile instead
  • works as expected on desktop

because connectWallet() is called on app load and then again before any claimPermit() I don't think it's necessary to wrap the remaining write ops in fallbacks as we've checked it at least twice at this stage

@Keyrxng Keyrxng marked this pull request as ready for review June 8, 2024 22:19
@0x4007
Copy link
Member

0x4007 commented Jun 10, 2024

Make the logs way less verbose. You shouldn't show every test unless its happening very slowly (it shouldn't)

Also for the RPC that works- can you make it a clickable link to pre-fill the network details on MetaMask? I'm pretty sure i've seen this capability on Chainlist.org etc.

image

In this case I would display something more like:

In case of network issues, please click and add the below RPC...

gnosis.drpc.org

...and thats it.

gnosis.drpc.org should be a link to pre-fill all the RPC information on MetaMask.

@Keyrxng
Copy link
Member Author

Keyrxng commented Jun 10, 2024

@0x4007 I've tried on mobile to see if that feature works but it doesn't it's the same as the app is now.

On desktop chainlist will call eth_requestAccounts followed by eth_addEthereumChain.
On mobile it likely makes the same two calls but you only see the eth_requestAccounts call and then nothing.

Since it works on desktop and does add the network for you the same way chainlist does the only option for mobile realistically is the url to copy paste, maybe a helper button to copy to clipboard but beyond that I don't see how we could make it easier for mobile.

I will make it less verbose

@Keyrxng
Copy link
Member Author

Keyrxng commented Jun 10, 2024

  • Removed the testing... toasts
  • Added a toast if no good RPC can be found.

Because the call to actually add the RPC is a wallet_addEthereumChain call the toast would need to be made interactive and wrapped with a handler to make the call as opposed to it being just a link.

But because of the fact it works on desktop and just not on mobile, I don't think it makes sense to do that because either way it serves no utility.

Insane to me how bad Metamask mobile is.

Never a truer statement

@0x4007
Copy link
Member

0x4007 commented Jun 11, 2024

But because of the fact it works on desktop and just not on mobile, I don't think it makes sense to do that because either way it serves no utility.

What is your conclusion on the solution here?

@Keyrxng
Copy link
Member Author

Keyrxng commented Jun 11, 2024

What is your conclusion on the solution here?

Desktop: it works, so no need to show a toast or manual method.

Mobile: Until the MM mobile browser issue is resolved we cannot directly change their RPC no matter what if we are considering the default browser to be MM. Only they can do it, so improving QOL for them would be making it easier to copy the toast via a "click" as opposed the standard way of copying.

Although personally I don't think that is even required. RPCs can be pretty long so then we need to jam a button/icon for them to press into the already tight-for-space mobile notification and I think it's easy and intuitive enough on mobile for users to press and hold, drag and copy.


There is no excellent solution for this just yet.

I will stay on-top of the MM issue and when it's resolved we should only need to remove the media query checks and both envs should perform the same

@0x4007
Copy link
Member

0x4007 commented Jun 11, 2024

I will stay on-top of the MM issue and when it's resolved we should only need to remove the media query checks and both envs should perform the same

Given that they have 167 open sev2-normal issues and given that it took them two weeks to close out the only sev0-urgent issue I wouldn't hold my breath on it.

So then lets just accomodate users to be able to manually copy and paste the values and be done with this.

@Keyrxng
Copy link
Member Author

Keyrxng commented Jun 12, 2024

That's wild. They could be doing with a Ubiquibot install. And what's the chances that it's you who opened the only sev0 issue 😂

So then lets just accomodate users to be able to manually copy and paste the values and be done with this.

This is how things are at the moment

static/scripts/rewards/web3/connect-wallet.ts Outdated Show resolved Hide resolved
static/scripts/rewards/web3/connect-wallet.ts Outdated Show resolved Hide resolved
static/scripts/rewards/web3/connect-wallet.ts Outdated Show resolved Hide resolved
static/scripts/rewards/web3/connect-wallet.ts Show resolved Hide resolved
static/scripts/rewards/web3/connect-wallet.ts Show resolved Hide resolved
@Keyrxng
Copy link
Member Author

Keyrxng commented Jun 16, 2024

I went with the userAgent regex as that seemed to be the most robust and I verified that it works as expected

Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

Code seems fine. Although I'm not entirely sure what the best way to test this is. @rndquu can you take a quick review to finalize this pull?

@Keyrxng
Copy link
Member Author

Keyrxng commented Jun 18, 2024

I can only test manually with iOS without setting up a testing framework that can handle mobile better than Cypress

@0x4007
Copy link
Member

0x4007 commented Jun 18, 2024

@rndquu rfc

@rndquu rndquu self-requested a review June 25, 2024 09:07
Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

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

Works fine

@rndquu
Copy link
Member

rndquu commented Jun 25, 2024

@Keyrxng Pls resolve conflicts and merge this PR

@Keyrxng Keyrxng merged commit bf5ca41 into ubiquity:development Jun 26, 2024
4 checks passed
@Keyrxng
Copy link
Member Author

Keyrxng commented Jun 26, 2024

@rndquu Do you think it is right to have a v4 lockfile when CI throws regarding usage of global [email protected] when I tried to pin v4? Perhaps it's not an issue since things work as expected across open PRs

If we pin v4 CI throws, if we pin v1.22 we'll get merge conflicts but I'm cautious here because of the previous CI issues we've had with the lockfile. I think if it becomes an issue one more time I will do something about it

@rndquu
Copy link
Member

rndquu commented Jun 26, 2024

@rndquu Do you think it is right to have a v4 lockfile when CI throws regarding usage of global [email protected] when I tried to pin v4? Perhaps it's not an issue since things work as expected across open PRs

If we pin v4 CI throws, if we pin v1.22 we'll get merge conflicts but I'm cautious here because of the previous CI issues we've had with the lockfile. I think if it becomes an issue one more time I will do something about it

As far as I remember this issue is solved with this CI step for yarn v4

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.

Bad Wallet RPC Warning
3 participants