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

upgrade rpc logic #3

Merged
merged 4 commits into from
Jun 16, 2024
Merged

Conversation

Keyrxng
Copy link

@Keyrxng Keyrxng commented Mar 17, 2024

Relates to #2

  • bumped next to v14 as it was throwing errors with how it was handling the esm package
  • upgraded the rpc logic to use rpc-handler
ignore the background handler calls I just swapped out the providers for QA

use_provider_handling_w_background_handler_calls

works as expected but does require multi dynamic import (which I now see the fix for shortening the rpc-handler import path by exporting one file that handles multi-import like the way the hook does here)

rpc_handler_handling

@Keyrxng Keyrxng changed the title Make staking stable upgrade rpc logic Mar 17, 2024
@Keyrxng
Copy link
Author

Keyrxng commented Mar 17, 2024

@pavlovcik just the finishing touches I was working on for the other PR

@0x4007
Copy link
Owner

0x4007 commented Mar 19, 2024

I can't really tell the difference when I run this. Whats the purpose of this in the scope of getting the new RPCs working? What was the problem with the original deliverable?

@Keyrxng
Copy link
Author

Keyrxng commented Mar 20, 2024

In essence nothing other than now it's not one singular websocket endpoint being used when not MM connected.

The UI is still using the MM provider for the most part but thought it prudent to upgrade the RPC logic here too seeing as it's happening across the board

@0x4007
Copy link
Owner

0x4007 commented Mar 20, 2024

It seems like there's a lot of out of scope changes to the react code. They should be done in a separate pull

@Keyrxng
Copy link
Author

Keyrxng commented Mar 20, 2024

It seems like there's a lot of out of scope changes to the react code. They should be done in a separate pull

  • 69493c4 this is required as Nextjs@14 handles <a> inside <Link> and Nextjs@14 is required as it was a blocker for the rpc-handler package, removing <a> is the only change

  • cb01d94 using useMemo here prevents re-initialized of the provider state once it's already set unless one of the values change

I'll open a new PR with the useMemo stuff

@Keyrxng
Copy link
Author

Keyrxng commented Mar 20, 2024

It shaves about 1-1.5 secs off of the loading time memo-izing (i mean the time it takes for the APR to display)

@Keyrxng
Copy link
Author

Keyrxng commented Mar 20, 2024

next build passing but deploy to cloudflare is failing

@Keyrxng
Copy link
Author

Keyrxng commented Jun 15, 2024

@0x4007 the trouble with this is that useWeb3 is called all throughout the codebase meaning that it's receiving a different provider for various parts of the code and because without ubiquity/rpc-handler#24 the providers are still susceptible to potential call failure.

So this would benefit from rpc-handler returning a proxy and would benefit from memo-izing the web3State so that it prevents it re-rendering multiple providers and drops the external calls made.


Having said that, on the prod instance right now I can repro a bunch of call failures just by accessing credits.

image

@0x4007
Copy link
Owner

0x4007 commented Jun 15, 2024

Oh I'm surprised you are working on this codebase. I almost think we should toss it out and start fresh. Anyways if you are making a lot of changes, consider doing it across several tightly scoped pulls.

@Keyrxng
Copy link
Author

Keyrxng commented Jun 15, 2024

Oh I'm surprised you are working on this codebase. I almost think we should toss it out and start fresh. Anyways if you are making a lot of changes, consider doing it across several tightly scoped pulls.

ubiquity/.github#100 (comment)

This is the last place I believe that rpc-handler was to be implemented


Other than memo-izing these are the only changes needed as a proxied provider will handle retry etc. I removed the dirty commits I made memo-izing in this PR previously so it's cleaner

Noticed audit does use hardcoded RPCs, will do that now.

The RPCs are not used in audit, neither of the two functions which use RPC_URLS are called anywhere.

@0x4007
Copy link
Owner

0x4007 commented Jun 15, 2024

I am surprised that audit isn't using RPCs. @rndquu rfc

@Keyrxng
Copy link
Author

Keyrxng commented Jun 15, 2024

as far as I can tell audit is relying on the DB having all of the data needed other than what can be taken directly from parsing the repo.

Because of this issue I adjusted airdrop-cli so that, as best I could, match up permits to their corresponding repo, issue number with the tx hash which I thought would make life a bit easier and certainly would for audit if the locations table is due to be removed.

@0x4007 0x4007 merged commit 9da9a4c into 0x4007:development Jun 16, 2024
1 of 2 checks passed
@rndquu
Copy link
Collaborator

rndquu commented Jun 26, 2024

I am surprised that audit isn't using RPCs. @rndquu rfc

as far as I can tell audit is relying on the DB

Yes, https://github.com/ubiquity/audit.ubq.fi relies on a DB right now since all permits (at least in bot v2) are saved in a DB right now and it's easier to fetch them from a DB instead of parsing etherscan/github/etc... Besides there's an ongoing task to save already generated (i.e. old) permits to a DB to make the permits table "whole".

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.

None yet

3 participants