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: rpc networks are tested after selection to make sure they are functional #209

Merged
merged 2 commits into from
Mar 25, 2024

Conversation

gentlementlegen
Copy link
Member

Resolves #205

@ubiquity-os-deployer
Copy link

ubiquity-os-deployer bot commented Mar 24, 2024

@gentlementlegen
Copy link
Member Author

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.

image

@gentlementlegen gentlementlegen marked this pull request as ready for review March 24, 2024 21:17
@gentlementlegen gentlementlegen requested a review from 0x4007 as a code owner March 24, 2024 21:17
@0x4007
Copy link
Member

0x4007 commented Mar 25, 2024

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.

image

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?

app.provider = await useFastestRpc(app);
} catch (e) {
toaster.create("error", `${e}`);
}
if (ethereum) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm having second thoughts about myself having had renamed window.ethereum to ethereum. Could be shadowed easily 😰

@0x4007
Copy link
Member

0x4007 commented Mar 25, 2024

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
Copy link
Member Author

gentlementlegen commented Mar 25, 2024

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.
image

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
Copy link
Member

0x4007 commented Mar 25, 2024

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

@0x4007 0x4007 merged commit 3c163d0 into ubiquity:development Mar 25, 2024
3 checks passed
@ubiquibot ubiquibot bot mentioned this pull request Mar 25, 2024
@gentlementlegen gentlementlegen deleted the fix/failed-network branch March 25, 2024 06:11
@Keyrxng
Copy link
Contributor

Keyrxng commented Mar 25, 2024

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
Copy link
Member

0x4007 commented Mar 25, 2024

@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
Copy link
Member

0x4007 commented Mar 28, 2024

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
Copy link
Member Author

gentlementlegen commented Mar 28, 2024

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.

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.

Claims Broken
3 participants