search

ubiquity / pay.ubq.fi

Generate and claim spender permits (EIP-2612)
https://pay.ubq.fi
8 stars 36 forks source link

wallet provider management #242

Closed Keyrxng closed 3 months ago

Keyrxng commented 3 months ago

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[bot] commented 3 months ago
0848083
d752f72
cdbc892
8ac44f0
67a787c
6f30edb
bf5ca41
github-actions[bot] commented 3 months ago
Preview Deployment
084808396958848ae42a0bf6e6f772840b6d3975
d752f72a760d51c6375b07fd4c4502b8fcb446d4
cdbc8928904a44c269dbc30587fa6171ae0c07a0
8ac44f0d9b2f358ff5812fa383c56cb8beefdd6f
67a787c6c8d0f198ba2fc6f3b8f3734ac8f1987c
[e097379dc3790b272d1aa94d2861813e37f47cd9]()
6f30edb27354e7d49f80aec01055a7aeb4f142e5
Keyrxng commented 3 months ago

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

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

0x4007 commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

Keyrxng commented 3 months ago

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

Keyrxng commented 3 months ago

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

0x4007 commented 3 months ago

@rndquu rfc

rndquu commented 3 months ago

@Keyrxng Pls resolve conflicts and merge this PR

Keyrxng commented 3 months ago

@rndquu Do you think it is right to have a v4 lockfile when CI throws regarding usage of global yarn@1.22 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 commented 3 months ago

@rndquu Do you think it is right to have a v4 lockfile when CI throws regarding usage of global yarn@1.22 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