fix: rpc networks are tested after selection to make sure they are functional

ubiquity / pay.ubq.fi

Generate and claim spender permits (EIP-2612)

https://pay.ubq.fi

8 stars 28 forks source link

fix: rpc networks are tested after selection to make sure they are functional #209

Closed gentlementlegen closed 3 months ago

gentlementlegen commented 3 months ago

Resolves #205

ubiquibot-continuous-deploys[bot] commented 3 months ago

24d55c0

3c163d0

gentlementlegen commented 3 months ago

I sadly can only get a screenshot for the QA since there is no tests inside this project (yet). When a network is not reachable, another would be tried instead for a max of 10 retries. If 10 retries didn't work, a toast shows up.

0x4007 commented 3 months ago

I sadly can only get a screenshot for the QA since there is no tests inside this project (yet). When a network is not reachable, another would be tried instead for a max of 10 retries. If 10 retries didn't work, a toast shows up.

Injected wallet (metamask) has a built in provider. Perhaps we can make the app default to the built in one? It's really slow but I think pretty reliable.

I think if you add it to the defaults for the RPC picker it should be pretty quick to accommodate this?

0x4007 commented 3 months ago

I'm not sure if there are any pitfalls with the built in RPC provider like if the wallet MUST be connected to the app already.

If it's a big project I'm ready to merge this pull as is. Otherwise please include that adjustment!

gentlementlegen commented 3 months ago

I sadly can only get a screenshot for the QA since there is no tests inside this project (yet). When a network is not reachable, another would be tried instead for a max of 10 retries. If 10 retries didn't work, a toast shows up.

Injected wallet (metamask) has a built in provider. Perhaps we can make the app default to the built in one? It's really slow but I think pretty reliable.

I think if you add it to the defaults for the RPC picker it should be pretty quick to accommodate this?

Maybe could be done, but in my case I use Coinbase not Metamask, and I don't know if they all have the same built-ins, we cannot assume users do no use a different app. But this fix should (hopefully) fix 90% of the problems we have regarding the network issue, as it covers both unreachable networks but also 429 errors (too many requests) which were the two main reasons why the transaction got denied, at least from my tests. Could be done as a separate task since some research might be needed.

0x4007 commented 3 months ago

I think the api is standardized. It's like window.ethereum.signer.provider or something like that, although this implies that a wallet is connected. There might be one that's just like window.ethereum.provider

Keyrxng commented 3 months ago

it's standardized across providers and if multiple are used then [EIP-6963) (https://eips.ethereum.org/EIPS/eip-6963) can help with that but not really applicable here

signer is only available if the user actually connects the wallet but so long as a provider extension is installed you can still access the provider (i.e obtain the block number or allowances of accounts/tokens, you just cannot write anything) although it'll be whatever the last network selected in their wallet was and unless they log in with pass there is no way to change it

0x4007 commented 3 months ago

@gentlementlegen just wanted to confirm that this fix works and I'm happy to see it self healed my claims ability in my iOS MetaMask!

0x4007 commented 3 months ago

I'm not sure if there are any pitfalls with the built in RPC provider like if the wallet MUST be connected to the app already.

If it's a big project I'm ready to merge this pull as is. Otherwise please include that adjustment!

@gentlementlegen this is probably why we have this problem.

Currently manually testing each commit locally.

I can't reproduce this from a IP address URL.

gentlementlegen commented 3 months ago

I think I understand the root of the issue. Because there is no wallet connected the fetch to the chain always fails and it ends up having no instance returned hence leading to the infinite loading screen. I think the whole flow should be reviewed to avoid this. Quick fix would be by default to return the first item of the list. But this also means that on network failure or on unreachable rpc we will have the same network issue as we had before this fix.

To reproduce this on a PC disconnecting the wallet should trigger a similar behavior probably. Otherwise locally connect with a mobile device.