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 infinite load on non-web3 friendly browsers #221

Merged
merged 4 commits into from
May 27, 2024

Conversation

Keyrxng
Copy link
Member

@Keyrxng Keyrxng commented Apr 11, 2024

Resolves #220

  • the toaster now creates a persistent toast that does not expire when on a non-web3 browser.
  • when on mobile the toaster is specific and points them towards a Web3-friendly option
  • fully exits the loading state with all details fully rendered

QA:

  • Mobile: tested on safari, brave and MM
  • Desktop: brave with MM

mm
safari
brave

@ubiquity-os-deployer
Copy link

ubiquity-os-deployer bot commented Apr 11, 2024

@Keyrxng
Copy link
Member Author

Keyrxng commented Apr 11, 2024

I spent a little bit last night trying to hack together a solution for combining our signer with our optimal rpc. Anvil gave me a false alarm after it all working locally I'm happy as larry but decided to test on mumbai and low and behold it didn't work.

Likely because when using anvil default accounts it either auto-signs tx or it's just not a requirement at all. Either way it fell through so the only viable options for taking over the user's wallet RPC is:

  • prompting them to replace their current wallet rpc with our optimal one (which may change on every page refresh)
  • Implement AA

Tried this as well as having the user just sign the tx and fire it on the optimal rpc, lots of different variations but in one way or another it either didn't work as intended or would remove feedback such as the wallet tx confirmation request, etc.

Looking into Web3.js it seems possible but also like a step backwards

The ethers library creates a strong division between the operation a Provider can perform and those of a Signer, which Web3.js lumps together.

import { Eip1193Bridge } from "@ethersproject/experimental/retry-provider";

const signer = "... any way you get an ethers Signer...";
const provider = "... any way you get an ethers Provider...";

const eip1193Provider = new Eip1193Provider(signer, provider);

@0x4007
Copy link
Member

0x4007 commented Apr 15, 2024

Preview Deployment
469161d6d9e18a21d61986dc479288985f23b8e5

@gentlementlegen the link doesn't work on pulls it seems

@0x4007
Copy link
Member

0x4007 commented Apr 15, 2024

Hey @Keyrxng thanks for your pull.

Apologies for being pedantic but you need to redo your pull and minimize unnecessary changes. I recall mentioning this before. Please keep this in mind for your future pulls.

The reason your pulls aren't getting merged is because you make it more difficult to review when you make out of scope changes.

The more changes you make, the exponentially slower it is to review, approve, and merge in.

This is also compounded by the issue not being a high priority task to close out as well.

@Keyrxng
Copy link
Member Author

Keyrxng commented Apr 17, 2024

Apologies for being pedantic

No I appreciate it, it can't be enjoyable to be on the receiving end of it so I'll be sure to keep it in mind

@gentlementlegen
Copy link
Member

gentlementlegen commented Apr 17, 2024

Preview Deployment

469161d

@gentlementlegen the link doesn't work on pulls it seems

@0x4007 It seems the CLAIMABLE_URL is not populated in the env anymore? I am pretty sure I tried it but it was within the same repo so it is possible that it doesn't work for external pull requests, will have a look at it.

After investigation, two main issues:

  • one is that we can hit the rate limit as the requests are not made with a logged in account, see here
  • two is that secrets.UBIQUIBOT_PRIVATE_KEY is not populated due to the nature of it being a Pull Request event, so the ERC20 generation fails thus no URL is generated

@0x4007
Copy link
Member

0x4007 commented Apr 17, 2024

@gentlementlegen the simple solution is to hard code a permit made out to ubq.eth

We don't need to test the full flow, just test the table rendering for now.

@rndquu
Copy link
Member

rndquu commented May 22, 2024

@Keyrxng Questions:

  1. This PR adds a toast "Please use a web3 enabled browser..." if there are no web3 providers found, correct?
  2. What time estimate do you propose for this issue better mobile feedback #220 ?
  3. Pls resolve conflicts

@rndquu rndquu marked this pull request as draft May 22, 2024 07:23
@Keyrxng
Copy link
Member Author

Keyrxng commented May 24, 2024

  1. Persistent non-expiring toast, 5 secs is a long time but forever is longer. Buttons remain hidden while unclaimable.
  2. an hour two tops.

With the toasting of all errors, the tests create error toasts that are caused by the mocking/injection of window.ethereum and window.signer.

I tried to find a way to silence them without too much change with little effect. The tests still pass as expected and those toasts can be ignored as they do not block tests and are only present because of the env.

If it would be better handled I will make the relevant changes.

@Keyrxng Keyrxng mentioned this pull request May 24, 2024
@rndquu rndquu marked this pull request as ready for review May 27, 2024 07:13
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 rndquu requested a review from gentlementlegen May 27, 2024 07:14
Copy link
Member

@gentlementlegen gentlementlegen left a comment

Choose a reason for hiding this comment

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

Screenshot_20240527_164115_Chrome.jpg

Toast is appearing and persistent on a mobile non web3 browser. Sorry for the somehow gigantic screenshot.

@rndquu rndquu merged commit df00543 into ubiquity:development May 27, 2024
4 checks passed
@ubiquibot ubiquibot bot mentioned this pull request May 27, 2024
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.

better mobile feedback
4 participants