interface icon indicating copy to clipboard operation
interface copied to clipboard

Feat: allow users to customize their own rpc urls

Open Jdecristi opened this issue 1 year ago • 20 comments

General Changes

Fixes #440

Developer Notes

I had initially planned to allow users to use their own custom RPC Urls through wallet providers. However, to use WalletConnect, a custom provider is required, which means that the wallet provider alone is not sufficient. To address this issue, I propose creating a modal that allows users to enter an Alchemy or Infura key and customize RPC URLs for each network. This solution provides a secure and flexible way for users to connect their wallets to the application.

Screen shots

Before

Desktop

Screenshot

Mobile

before-mobile
After

Desktop

Screenshot 2023-05-05 at 7 24 49 PM Screenshot 2023-05-05 at 7 27 40 PM Screenshot 2023-05-11 at 2 43 52 AM Screenshot 2023-05-11 at 2 44 21 AM Screenshot 2023-05-05 at 7 58 06 PM .githubusercontent.com/89173284/236584684-589de74b-9a8d-46cc-90a3-db5ddbcb52f4.png">

Mobile

Screenshot 2023-05-05 at 8 00 00 PM Screenshot 2023-05-05 at 8 00 43 PM Screenshot 2023-05-11 at 2 46 21 AM Screenshot 2023-05-11 at 2 46 38 AM Screenshot 2023-05-05 at 8 02 56 PM

Author Checklist

Please ensure you, the author, have gone through this checklist to ensure there is an efficient workflow for the reviewers.

  • [x] The base branch is set to main
  • [x] The title is using Conventional Commit formatting
  • [x] The Github issue has been linked to the PR in the Development section
  • [x] The General Changes section has been filled out
  • [x] Developer Notes have been added (optional)

If the PR is ready for review:

  • [x] The PR is in Open state and not in Draft mode
  • [x] The Ready for Dev Review label has been added

Reviewer Checklist

Please ensure you, as the reviewer(s), have gone through this checklist to ensure that the code changes are ready to ship safely and to help mitigate any downstream issues that may occur.

  • [ ] End-to-end tests are passing without any errors
  • [ ] Code style generally follows existing patterns
  • [ ] Code changes do not significantly increase the application bundle size
  • [ ] If there are new 3rd-party packages, they do not introduce potential security threats
  • [ ] If there are new environment variables being added, they have been added to the .env.example file as well as the pertinant .github/actions/* files
  • [ ] There are no CI changes, or they have been approved by the DevOps and Engineering team(s)
  • [ ] Code changes have been quality checked in the ephemeral URL
  • [ ] QA verification has been completed
  • [ ] There are two or more approvals from the core team
  • [ ] Squash and merge has been checked

Jdecristi avatar May 02 '23 00:05 Jdecristi

This pull request has been linked to 1 task:

💡Tip: Add "Close T-5927" to the pull request title or description, to a commit message, or in a comment to mark this task as "Done" when the pull request is merged.

height[bot] avatar May 02 '23 00:05 height[bot]

Wow, this looks like an awesome feature!

defispartan avatar May 02 '23 05:05 defispartan

❌ CI run has failed! Please see logs at https://github.com/aave/interface/actions/runs/4860245747'

github-actions[bot] avatar May 02 '23 10:05 github-actions[bot]

📦 Next.js Bundle Analysis for aave-ui

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 520.42 KB (🟡 +2.05 KB)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Ten Pages Changed Size

The following pages changed size from the code in this PR compared to its base branch:

Page Size (compressed) First Load
/ 40.77 KB (🟢 -711 B) 561.18 KB
/faucet 22.31 KB (-1 B) 542.73 KB
/governance 82.72 KB (🟡 +1 B) 603.14 KB
/governance/ipfs-preview 111.62 KB (-2 B) 632.04 KB
/governance/proposal 111.78 KB (🟡 +2 B) 632.2 KB
/governance/proposal/[proposalId] 66.49 KB (-1 B) 586.91 KB
/markets 16.95 KB (-1 B) 537.37 KB
/reserve-overview 65.52 KB (🟢 -633 B) 585.93 KB
/staking 20.03 KB (🟢 -709 B) 540.45 KB
/v3-migration 28.78 KB (🟡 +1 B) 549.2 KB
Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.

github-actions[bot] avatar May 02 '23 10:05 github-actions[bot]

  • Ipfs hash: bafybeihxb5lovt3vgitax27ghskenxqneifyuwawgh6dhrwqeah5skubqm
  • Ipfs preview link: https://bafybeihxb5lovt3vgitax27ghskenxqneifyuwawgh6dhrwqeah5skubqm.ipfs.cf-ipfs.com/

github-actions[bot] avatar May 02 '23 10:05 github-actions[bot]

Hey @Jdecristi thanks for the pr! A huge problem would be solved with this one.

Few comments:

  1. I'm a bit confused with switcher as entry point to URLs setup. Maybe we can add another button there to separate enable-button and setup window? Here is how it would look like for zero state, enabled, disabled. Zero state Enabled state Disabled state

  2. I recommend removing gradients from the buttons and replace them with primary colors. Also made some changes to text sizes and placeholders to make it more ~~boring~~ clean 🙃. Dialog 1 Dialog 2

Let me know what you think!

iamanastasia avatar May 02 '23 13:05 iamanastasia

@iamanastasia I have made some changes to your requests and here's what I did:

  1. Instead of the Switcher, I replaced it with a Button. I tested various configurations with both buttons and switchers, and the button configuration proved to be the best.

  2. I made adjustments to the font sizes to match your preferences and also fixed the placeholders.

  3. I removed all the testnets since I realized they were not necessary. Additionally, trying to incorporate the testnets with the mainnets was quite challenging, and their inclusion would not add much value.

Thank you for your feedback, the feature is now much more refined thanks to your input.


Screenshots are fixed at the top

Jdecristi avatar May 03 '23 05:05 Jdecristi

@Jdecristi Thanks! Button seems to be more obvious.

Still have concerns, but not sure about use cases:

  1. I liked your previous idea with switcher because it would be easy to enable/disable custom RPC when user needs it — imagine user wants to repay debt quickly, but network is clogged, it would take user 1 click to switch the connection.
  2. With the button it's not obvious whether I have custom rpc on or off. It says 'Edit', but it doesn't mean it's enabled. Might be confusing, takes some time to learn how it works, some double-checks, adds some friction.

Let's ask for second opinion?

@defispartan @MareskoY @0x4Graham Could you please review screenshots fixed on top and screenshots I attached to previous comment? What do you think? Do you think it's a one time setting and setup button is sufficient?

Here is also another way to show it. Var3

iamanastasia avatar May 03 '23 11:05 iamanastasia

Hello, @Jdecristi

There are a couple of issues that I've noticed with the interface:

  1. It doesn't seem to be adapted for dark mode. Here's a screenshot for reference:
Screenshot 2023-05-03 at 12 38 18
  1. There doesn't appear to be any setup for testnet rpc. It would be great to have this option included, and perhaps we could make it so that the testnet inputs are only visible when testnet is enabled, rather than showing everything on one screen.

MareskoY avatar May 03 '23 11:05 MareskoY

I am a big fan of the slider and would prefer that over the button. The rest looks pretty good to me!

Thanks for picking this task up @Jdecristi!

0x4Graham avatar May 03 '23 11:05 0x4Graham

Awesome feature! I like the idea of being able to active and deactivate the custom RPCs with a simple click and the switcher seems more suitable because it gives the user feedback of being off or on easily. This is something similar to Frame wallet where you can switch the rpc from secondary to primary and its super confortable if one of them is not working correctly.

JoaquinBattilana avatar May 03 '23 11:05 JoaquinBattilana

Hey Ya'll I fixed the issues you brought up

@MareskoY I fixed DarkMode issues, aswell as testnet support through a toggle.

@JoaquinBattilana @0x4Graham @iamanastasia I switched it back to a toggle in the settings menu.


Screenshots are updated above

Jdecristi avatar May 03 '23 15:05 Jdecristi

@Jdecristi Great!

Let's add settings button as well. With only the switch it's hard to edit URLs as you need to disable/enable once again to open setup window. Var3

We'll need an additional logic though for situations when user doesn't have any URLs added yet, so he can't enable RPC. We can hide switcher, or disable it.

Disabled

@0x4Graham @JoaquinBattilana @MareskoY Do you agree with adding both — a button and a switcher?

iamanastasia avatar May 04 '23 15:05 iamanastasia

Agreed on the button and the switcher. When testing, i found it confusing that I would need to turn the switcher off if I want ed to change the RPC URL

0x4Graham avatar May 05 '23 13:05 0x4Graham

link T-5927

0x4Graham avatar May 05 '23 13:05 0x4Graham

@iamanastasia @0x4Graham I fixed your request by adding a button as well, and added the feature that when there are no urls present the switcher is hidden.

Screenshots above

Jdecristi avatar May 06 '23 00:05 Jdecristi

Looks great! Thanks @Jdecristi. We'll do the code review as well as QA review!

0x4Graham avatar May 08 '23 09:05 0x4Graham

Hey @Jdecristi, we've done an initial review and have some validation asks:

  1. Validate that the URL/API key works for the provided market(s). Before closing the modal if there is an error show a error message for which market it doesn't work.
  2. Tex validation: 2(a): You can currently enter in a URL and an api key, but seems like the api key will always take precedence. I think that if there is a valid API key provided, the URL for PRC's should be disabled and a message shown: "API key already provided". 2(b): Text validation: Make sure that only URL's can be entered into the URL text boxes. 3: I don’t think we need the toggle for showing testnets. I think we can just show testnets if testnet mode is enabled, otherwise show prod markets.

Let me know what you think about the above feedback.

0x4Graham avatar May 09 '23 08:05 0x4Graham

Hey @0x4Graham I managed to fix the stuff in your comment

  1. I added validation to both the URL and the Api Key inputs. The modal will only save when urls/keys are tested and passed.
  2. I intended for the user to be able to upload multiple RPC Urls/Api keys but I fixed it so that the URL takes precedence. I think we should keep the feature to allow multiple Urls/Api Keys incase one of their Urls goes down.
  3. I fixed it so that Testnet Inputs show up when testnets are enabled.

Screenshots at the top

Jdecristi avatar May 11 '23 06:05 Jdecristi

Hey @Jdecristi Thanks for the updates!

We will take the PR from here to polish it for final shipping.

Thanks for your input and amazing work on this PR!

0x4Graham avatar May 17 '23 15:05 0x4Graham