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: use mm provider as default #2

Merged
merged 1 commit into from
Mar 17, 2024

Conversation

Keyrxng
Copy link

@Keyrxng Keyrxng commented Mar 13, 2024

Resolves #1

  • The issue stemmed from the fact a fallback provider was being used and it failed to reach quorum on occasion. Removed the fallback which was solved to solve single point of failure by providing multiple endpoints lmao but /credits also requires a connected wallet and markets are just ext links

  • staking isn't displayed until the wallet is connected so it makes sense to just use the MM provider

  • development needs will be forking mainnet likely although if not set the provider to localhost

  • otherwise use a websocket connection

QA:

  • I've spammed refresh and it won't reproduce
  • Been think of how I can test it with cypress but that would need some type of UI handling if the provider(s) fail
  • would benefit from rpc-racer

@Keyrxng
Copy link
Author

Keyrxng commented Mar 13, 2024

Will continue to try to reproduce but if you have any input for QA other than manual testing lmk, I'll test the deploy preview once you approve the workflow but I'm confident it's been achieved internally

@Keyrxng Keyrxng marked this pull request as draft March 13, 2024 19:10
@Keyrxng Keyrxng marked this pull request as ready for review March 15, 2024 13:38
@0x4007
Copy link
Owner

0x4007 commented Mar 16, 2024

Been think of how I can test it with cypress but that would need some type of UI handling if the provider(s) fail

You can ask our in house Cypressooooor @fernandVEYRIER

I think rndquu broke our deploys so I'll just need to run it locally.

@Keyrxng
Copy link
Author

Keyrxng commented Mar 16, 2024

What I mean is, a UI component or UI state would need built in order to display something if the provider(s) fail, similar to the work.ubq rate limit toaster

I could add the rpc-racer and display the staking stats etc without needing to connect the wallet, then swap out the JsonRpcProvider for a Web3Provider if they decide to connect their wallet?

@Keyrxng
Copy link
Author

Keyrxng commented Mar 16, 2024

I've spent a couple hours today on this trying to embed the rpc-handler here's where I'm at with it, I think this would be a much larger codebase change than you'd like to see with the current state of things or would require compromise (such as retesting bad rpcs)

  • embedding it into useWeb3() as a hook requires both versions due to how it's used throughout the codebase meaning one has storage and the other doesn't

  • because of this replacing provider with a new fallback provider with each latency used as config option plus how often useWeb3 is called you see a silly amount of network requests

  • as an API route I was returning just the latencies and setting up a fallback/json provider, although it means no storage

  • as a hook we have storage and it's visible by the lack of failed network requests but the dapp doesn't want to play ball with any provider setup that I've tried so far and unless it's the provider returned by useProvider() the UI just shits the bed and doesn't load the modals


I'll try a few non-breaking change package bumps I hope the issue lies there (wagmi v0.8 lmao), had to bring Next up to v14 it was killing me (fixed what issues appeared, still on pages router)

Is this UI being replaced by this or is there plans to refactor this codebase or will this remain as close to as-is as possible for the forseeable?

@0x4007
Copy link
Owner

0x4007 commented Mar 17, 2024

I don't want to invest heavily into our short term UI efforts for the dApp. I think later this year I might want to break them off into simple pages (separate repos) for ease of development, or take a new approach entirely (like dynamically generated UIs based on smart contracts.)

It seems like the smallest amount of effort would be to just edit our current UI for now and let it be for the next year or so.

} else if (IS_DEV) {
provider = new JsonRpcProvider(LOCAL_NODE_ADDRESS);
} else {
provider = new WebSocketProvider("wss://ethereum-rpc.publicnode.com", 1);
Copy link
Owner

Choose a reason for hiding this comment

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

I remember we had issues with the amount of requests we were making for the audit app. Maybe websockets are the solution.

@0x4007
Copy link
Owner

0x4007 commented Mar 17, 2024

First attempt the APY failed to load (not super essential but I expected that all RPC errors should be addressed)

image

Second attempt, I refreshed the staking page and nothing worked

image

Third attempt, I refreshed the staking page and it matches the first attempt.

It doesn't seem like anything is different.

Copy link
Owner

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

Behavior is the same

@Keyrxng
Copy link
Author

Keyrxng commented Mar 17, 2024

(like dynamically generated UIs based on smart contracts.)

modern wagmi simplifies the heck out of this

It seems like the smallest amount of effort would be to just edit our current UI for now and let it be for the next year or so.

understood, I didn't think you'd be wanting extensive changes considering it's a stop-gap

Hmmm strange it never failed for me once in my 30+ attempts but I'll see a fix to this today

Silly question but are you sure that you had pulled my changes? I'm seeing `Cloudflare.eth`` in your screenshot but my changes removed that

  if (window.ethereum) {
      provider = new Web3Provider(window.ethereum);
    } else if (IS_DEV) {
      provider = new JsonRpcProvider(LOCAL_NODE_ADDRESS);
    } else {
      provider = new WebSocketProvider("wss://ethereum-rpc.publicnode.com", 1);
    }

Logging the provider

image


I'll add logic to handle a failed RPC regardless

@0x4007
Copy link
Owner

0x4007 commented Mar 17, 2024

I was looking at my screenshot for the build number. I set up the project again and now I'm at build ubiquity@0acdf283 will need to test again.


Seems to be fast now!

@0x4007
Copy link
Owner

0x4007 commented Mar 17, 2024

Our CI is all broken but I think this fixes the problem. Thanks @Keyrxng

@0x4007 0x4007 merged commit 2a849ab into 0x4007:development Mar 17, 2024
0 of 2 checks passed
@0x4007
Copy link
Owner

0x4007 commented Mar 17, 2024

@rndquu looks like this also relies on the deploys please fix

@Keyrxng Keyrxng mentioned this pull request Mar 17, 2024
@rndquu
Copy link
Collaborator

rndquu commented Mar 19, 2024

@rndquu looks like this also relies on the deploys please fix

Can't push a new branch to this repo, getting permission denied error

@0x4007
Copy link
Owner

0x4007 commented Mar 20, 2024

@rndquu looks like this also relies on the deploys please fix

Can't push a new branch to this repo, getting permission denied error

Added you

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.

Make Staking Stable
3 participants