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

Feature: Fetch Fastest RPC For Rewards dApp #151

Merged
merged 8 commits into from
Feb 15, 2024
Merged

Feature: Fetch Fastest RPC For Rewards dApp #151

merged 8 commits into from
Feb 15, 2024

Conversation

barebind
Copy link
Contributor

@barebind barebind commented Feb 7, 2024

Resolves #130

  • Using integers instead of hexes for networkId/chainId is less confusing and also is compatible with specs.
  • Fixed some missing parts/fields for scripts/typescript/generate-permit2-url.ts
  • network parameter in the url is not used so I removed it. networkId fromClaimTx is used instead.
  • More structured multi-chain network config might be a better use.
  • onboarding has it's own switchNetwork functions etc. Some functions are shared across different UIs and some are not. Definitely confusing for newcomers.
  • isNonceClaimed always returns true for a new setup(new ubiquibot account) on Goerli. Something must be missing in the documentation. Don't know what and why.
  • For getOptimalRpc() functionality. The current approach is definitely not the best one as it always calls whenever it needs rpc provider. There must be some context to store optimal provider and fetching optimal RPC must be on page load or something.
  • Optimal RPC as a concept cannot be applied to Metamask because if an already added network has a slow rpc provider on Metamask side, there is no edit option as far as I know. So using optimal RPC provider is for only backend calls like transaction data rendering, fetching nonces etc. but sending a transaction through Metamask is via it's own stored rpc provider for chainId.

Copy link

ubiquibot bot commented Feb 7, 2024

@barebind barebind marked this pull request as ready for review February 8, 2024 07:10
@barebind barebind requested a review from 0x4007 as a code owner February 8, 2024 07:10
@rndquu rndquu self-requested a review February 8, 2024 07:50
@rndquu
Copy link
Member

rndquu commented Feb 8, 2024

Regarding the issue, as far as I understand, metamask doesn't allow adding RPCs programmatically so as a part of the current issue we could:

  1. Use custom (fastest) RPC provider and utilize signer from metamask
  2. Meticulously update all pages (onboarding, audit, payouts, etc...) to use the fastest provider

Time estimate for the current issue is ~1 day. Overall I would save time and close current issue as "not planned" since current metamask RPCs work pretty fine both for mainnet and gnosis.

@barebind
Regarding the PR:

  1. The code is formatted pretty heavily which is really hard to review
  2. Pls follow the "1 PR 1 solved issue" rule. If you want to fix some other issues pls open a separate PR.

@barebind
Copy link
Contributor Author

Regarding the issue, as far as I understand, metamask doesn't allow adding RPCs programmatically so as a part of the current issue we could

What I meant actually is that, you cannot add additional provider urls for already added specific networkId on metamask. So if you have Gnosis added to metamask with a slow rpc already, you cannot update or add additional faster providers to it.

Use custom (fastest) RPC provider and utilize signer from metamask

I think it's the current state right now with rpc provider (not necessarily the fastest though) saved in constants.ts. As a side note, afaik it's not possible to sign but not broadcast the transaction with metamask. It broadcasts automatically. So when broadcasting a transaction after signing, it has to be the one that is saved on metamask.

I agree with a structural all-pages update for fastest rpc usage like you said.

The code is formatted pretty heavily which is really hard to review

I've used project's prettier settings(.prettierrc) on the root. Should I disable it for later use?

Pls follow the "1 PR 1 solved issue" rule. If you want to fix some other issues pls open a separate PR.

Sure.

@0x4007
Copy link
Member

0x4007 commented Feb 13, 2024

What I meant actually is that, you cannot add additional provider urls for already added specific networkId on metamask. So if you have Gnosis added to metamask with a slow rpc already, you cannot update or add additional faster providers to it.

For enhanced UX we can time the RPC request and if its slow e.g. longer than 1 second, we can have a message pop up that explains to the user to change the RPC endpoint to one that we recommend. From there the user can click to add it.

So when broadcasting a transaction after signing, it has to be the one that is saved on metamask.

After the user attempts the first request, they will receive this notification to change the RPC and then they no longer have to worry about this problem.

I've used project's prettier settings(.prettierrc) on the root. Should I disable it for later use?

I recently updated the prettier and linter settings across several repositories to match our ts-template repository's settings so this would explain why there is some code that is not formatted correctly! I think we can run yarn format once and commit to ensure that the codebase is formatted correctly.

@0x4007 0x4007 changed the base branch from development to main February 13, 2024 08:35
@0x4007 0x4007 changed the base branch from main to development February 13, 2024 08:35
@0x4007
Copy link
Member

0x4007 commented Feb 13, 2024

The code is formatted pretty heavily which is really hard to review

I formatted the codebase and tried forcing GitHub Files view to update by toggling the branch to no avail. @barebind If you can handle the new merge conflicts then we can merge this in!

@0x4007 0x4007 merged commit a23be4c into ubiquity:development Feb 15, 2024
1 check passed
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.

Fetch Fastest RPC For Rewards dApp
4 participants