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

Let users browse among all possible RPC networks #3464

Closed
colourfreak opened this issue Nov 23, 2021 · 41 comments · Fixed by #3595
Closed

Let users browse among all possible RPC networks #3464

colourfreak opened this issue Nov 23, 2021 · 41 comments · Fixed by #3595
Assignees

Comments

@colourfreak
Copy link

Problem: users have to enter RPC networks manually, where in most cases it can be automated.

Changes:

  • We add a second line in the list where we mention ChainID
  • We add tabs in "Add Custom RPC Network". One is to Enter it manually, second to browse among all existing networks (we can use chainlist)
  • Users should be able to hide any network they want except Ethereum Mainnet, but by default, we keep it as it is

Prototype:
https://invis.io/MW1220T36ET3

Screen.Recording.2021-11-23.at.12.26.04.mov
@colourfreak colourfreak added the Design UI Change label Nov 23, 2021
@hboon
Copy link
Member

hboon commented Nov 24, 2021

We add a second line in the list where we mention ChainID

This looks useful. I think we spoke about (or I wanted to but forgot) showing the chain icon here too. Do you think we could add both as a separate change, separate from this issue?


The chain data can be pulled from https://github.com/ethereum-lists/chains so it's not a problem.

But the chain data is sometimes wrong or possibly outdated and is community maintained. We can help to fix the data as we discover them (e.g ethereum-lists/chains#440). Do you think we should have a disclaimer message somewhere in the Browser tab?

Another thing is the repo doesn't say whether each chain is a testnet or not (eg. https://github.com/AlphaWallet/chains/blob/0f95f16c35fa6794a03941453a397480798ce02f/_data/chains/eip155-42161.json). So I suppose we can only show a single list and then when the user taps one, switch back to the Enter tab, prepopulating the fields or alternatively, display a prompt along this lines:

Screenshot 2021-11-24 at 11 45 22 AM

@colourfreak
Copy link
Author

We add a second line in the list where we mention ChainID

This looks useful. I think we spoke about (or I wanted to but forgot) showing the chain icon here too. Do you think we could add both as a separate change, separate from this issue?

Yeah, we need two issues.

The chain data can be pulled from https://github.com/ethereum-lists/chains so it's not a problem.

But the chain data is sometimes wrong or possibly outdated and is community maintained. We can help to fix the data as we discover them (e.g ethereum-lists/chains#440). Do you think we should have a disclaimer message somewhere in the Browser tab?

I would not overcomplicate this. We can connect (message) to each one of these networks and let them know that this is available in the AlphaWallet app now. And we can kindly request them to file an issue if something has changed.

Another thing is the repo doesn't say whether each chain is a testnet or not (eg. https://github.com/AlphaWallet/chains/blob/0f95f16c35fa6794a03941453a397480798ce02f/_data/chains/eip155-42161.json). So I suppose we can only show a single list and then when the user taps one, switch back to the Enter tab, prepopulating the fields or alternatively, display a prompt along this lines:

Screenshot 2021-11-24 at 11 45 22 AM

You can figure this out with their names. If hasTestnet = true, then put this into Testnets. We need to write simple scripts that will automate the work for us.

@hboon
Copy link
Member

hboon commented Nov 26, 2021

Ah yes, and there's actually a "network" field that says whether it is testnet or not, though the data is dirty too. But we can make a guess.

@hboon
Copy link
Member

hboon commented Dec 7, 2021

@eviltofu can you help to work on this? But excluding: #3511, i.e. don't change that screen to add the icon/chain ID.

Probably need to look at https://github.com/ethereum-lists/chains and see if we should include it as a submodule (how big is it?) or download it on-demand

@hboon hboon assigned eviltofu and unassigned hboon Dec 7, 2021
@hboon hboon removed the Design UI Change label Dec 7, 2021
@eviltofu
Copy link
Contributor

eviltofu commented Dec 9, 2021

@hboon @colourfreak Some notes and clarifications.

Browse Screen

  1. The user can select and deselect multiple networks in the browse screen.
  2. Networks which are already selected do not appear in the browse screen (otherwise this would be a manage and not add function). This is done by comparing ChainIDs.
  3. The network list should be cached and a modification date of some kind be used to reload new network list.
  4. Is the network list from our own source (downloaded from https://github.com/ethereum-lists/chains and processed with our own scripts) or downloaded directly from the source and processed on the app? I'd prefer the former so that the Android client won't have to duplicated the same work.
  5. I would like to split the manual entry from the list selection entry. If the user selects multiple networks in the browse tab and then switches to the manual entry tab, what data is shown? What if the user selects multiple networks, switches to the manual entry tab, enters network data and hits update? What happens if an error occurs? How do we recover?

@hboon
Copy link
Member

hboon commented Dec 9, 2021

  1. The network list should be cached and a modification date of some kind be used to reload new network list.

You might be referring to caching either of these:

A. chains that are added through Browse
B. the list of chains in the Browse tab

A. Chains that are added through Browse

After the user adds their selections, we must not automatically/silently modify the copy they have added to

B. The list of chains in the Browse tab

I am leaning towards just embedding it as a git submodule, for us to refresh when we update the app but need to see how much it adds to the app size

  1. Is the network list from our own source (downloaded from https://github.com/ethereum-lists/chains and processed with our own scripts) or downloaded directly from the source and processed on the app? I'd prefer the former so that the Android client won't have to duplicated the same work.

It's a third party repo, but community maintained. But we are free to include a script to created an amalgamation JSON based on https://github.com/ethereum-lists/chains/tree/master/_data/chains. If you create such a file, just include it in the PR and we can probably create a separate repo for it

I would like to split the manual entry from the list selection entry. If the user selects multiple networks in the browse tab and then switches to the manual entry tab, what data is shown? What if the user selects multiple networks, switches to the manual entry tab, enters network data and hits update?

Seems like we are missing an "Add" button in the top right when the "Browse" tab is selected. Tapping that button (which is hidden for the Enter tab) will add the selected chains ignorig whatever was entered in the Enter tab. Similiarly, tapping "Save Network" in the Enter tab ignores selection made in the Browser tab. Simpler UI and code

What happens if an error occurs? How do we recover?

Run through the processed chains like how we process the one submitted chain during manual entry and display an error dialog with the first error in the first selected chain in the simplest manner for now? It's a rarely used screen, don't waste too much time on it (because the better version is much more fancier). If we discover that it turns out to be a popular function, we can come back and improve it.

@eviltofu
Copy link
Contributor

eviltofu commented Dec 9, 2021

@colourfreak Can we have a pop up appear after you tap the + button which then lets the user pick manual or browse and then just go to that screen?

@colourfreak
Copy link
Author

@hboon @eviltofu

I updated the prototype:
https://invis.io/MW1220T36ET3

Changes:

  • Switch Browse and Enter tabs, so Browse appear as the first one
  • When you go to the Browse tab, you will see only these networks that are not in the main Select Custom RPC screen
  • In the Browse tab, you can select multiple networks and the "Add Network" button appears in the bottom sheet to confirm
  • If you select multiple networks in the Browse tab but then switch to Enter, then nothing happens since you have not confirmed with the bottom sheet button "Add Network"

Video
https://user-images.githubusercontent.com/51817359/145555798-1ad93377-fb50-4de9-9dbd-a4efdbac2bc6.mov

@eviltofu
Copy link
Contributor

@hboon We embed https://github.com/ethereum-lists/chains as a git submodule and run the Kotlin code to generate "chains.json" which we then embed into our app as the source for RPC networks?

@hboon
Copy link
Member

hboon commented Dec 16, 2021

@eviltofu Ah. Didn't realise there's already a chains.json . How about we just wget https://chainid.network/chains.json (or https://chainid.network/chains_mini.json) when the user access that screen and cache it in memory? Less work and developer-side dependency and it's updated. If that fails, then we still about building it or caching it somewhere?

@eviltofu
Copy link
Contributor

eviltofu commented Dec 16, 2021

@eviltofu Ah. Didn't realise there's already a chains.json . How about we just wget https://chainid.network/chains.json (or https://chainid.network/chains_mini.json) when the user access that screen and cache it in memory? Less work and developer-side dependency and it's updated. If that fails, then we still about building it or caching it somewhere?

We should provide a base chains.json file in the app and then the app should download and save this new version of the file when it is determines that there is an update at the GitHub site. With a base chains.json the user can start immediately without waiting for the file to be download. Before any release, we update the chains.json file in the app.

We can use the HTTP header last-modified: Wed, 15 Dec 2021 15:04:49 GMT to determine when the file is changed. The app should check once every X days so as to not overload the GitHub server?

@hboon
Copy link
Member

hboon commented Dec 16, 2021

We can do the caching as you suggested.

But I don't think we should include the file. It's huge for what it does, but can you verify how big it is if compressed? It seems unnecessary to include it when most users will never access it in the app.

@eviltofu
Copy link
Contributor

eviltofu commented Dec 16, 2021

curl https://chainid.network/chains_mini.json > chains.json shows chains.json is 74k uncompressed. 13k compressed with macOS Finder Compress Function.

Is this the complete list? In the _data directory for the chains git repository, https://github.com/ethereum-lists/chains, (I cloned it) ls | wc -l shows 281 different eip155-xxx.json files.

@hboon
Copy link
Member

hboon commented Dec 16, 2021

I don't know. Can you compare chains.json and chains_mini.json to see what's the difference? If it is small enough to bundle (say 30k or less, zipped?), then let's just download a copy and bundle and skip the runtime caching entirely. Simplest approach.

@eviltofu
Copy link
Contributor

Files Size Compressed size
chains.json 99k 27k
chains_mini.json 74k 21k

@hboon
Copy link
Member

hboon commented Dec 16, 2021

Ah, pick one. The mini if possible and let's just bundle.

@eviltofu
Copy link
Contributor

From the Kotlin source code, the mini version only contains "name", "chainId", "shortName", "networkId", "nativeCurrency", "rpc", "faucets", "infoURL" fields while the other one contains everything.

@eviltofu
Copy link
Contributor

Some of the entries in chains_mini.json have empty attributes for rpc. The rpc attribute maps to rpcEndpoint for CustomRPC. In SaveCustomRpcViewModel, the rpcEndpoint cannot be blank. Do we ignore this validation check?

@hboon
Copy link
Member

hboon commented Dec 20, 2021

Filter out those that aren’t valid for uss

@eviltofu
Copy link
Contributor

I'll use the validation routines in the SaveCustomRpcViewModel.

@eviltofu
Copy link
Contributor

Having trouble with using UISearchController in a child container. Temporarily using a SearchBar to do the filtering.

@eviltofu
Copy link
Contributor

eviltofu commented Dec 22, 2021

@hboon Is there a reason why we have switches in section headers to select Mainnet or Testnet Custom RPC Servers?

Simulator Screen Shot - iPhone 12 - 2021-12-22 at 23 13 29 copy

@hboon
Copy link
Member

hboon commented Dec 22, 2021

@eviltofu UI design decision. But we wanted to be clear it is mainnet vs testnet, and it worked in that way.

@eviltofu
Copy link
Contributor

@colourfreak Can you recommend the phrase / position of the empty search result message? Currently, it is placed in the middle of the screen. (Also font, colour, etc)

Simulator Screen Shot - iPhone 12 - 2021-12-23 at 10 00 58 copy

@eviltofu
Copy link
Contributor

eviltofu commented Dec 23, 2021

@hboon What happens when someone adds multiple CustomRPCs but one of the CustomRPCs is invalid? For error recovery, do we want the state to be exactly as before the add operation or allow those added to remain? When we return to the Browse screen, is the entry which caused the error unchecked?

@colourfreak
Copy link
Author

network-search-no-results

https://zpl.io/VxjBXQg

@eviltofu
Copy link
Contributor

@hboon @colourfreak Adding a single CustomRPC takes some time. Adding multiple will take a long time. Do we want to limit the number of servers that can be added? We also need to be able to allow the user to cancel in the middle of the add operation if it takes too long.

@eviltofu
Copy link
Contributor

@hboon Any solutions to something like this?

Simulator.Screen.Recording.-.iPhone.12.-.2021-12-24.at.18.18.51.mp4

@eviltofu
Copy link
Contributor

What we could do is pop up a modal view that lists all the customRPCs in a tableview and have an animation indicating which customRPC is being added (we automatically do the check on the blockchain explorer url). Once added, we add a check mark at the end of the row. There are buttons that will allow the user to cancel the entire operation. All added networks are actually only added after all networks have been cleared.

@hboon
Copy link
Member

hboon commented Dec 28, 2021

@hboon Any solutions to something like this?

If you are referring to the layout of the UI elements being animated when it appears, they have to be laid out before the screen is animated. The keyboard showing up might be causing it. Try disable the call to becomeFirstResponder() if there is one and trace from there?

@hboon
Copy link
Member

hboon commented Dec 28, 2021

Presuming these 2 are about the same point:

Adding a single CustomRPC takes some time. Adding multiple will take a long time.

What we could do is pop up a modal view that lists all the customRPCs in a tableview and have an animation indicating which customRPC is being added (we automatically do the check on the blockchain explorer url). Once added, we add a check mark at the end of the row. There are buttons that will allow the user to cancel the entire operation. All added networks are actually only added after all networks have been cleared.

Just block the app with a UIActivityView for now? If the user tries to add 20 chains at a go, they have to make extra efforts to do it (we don't have a Select All button). But we do have to make sure that if the app is terminated while processing the chains that we don't end up in an inconsistent state, but just adding X out of 20 chain is fine. It's not a common operation and we don't know if it's going to be used much yet and we can come back and polish the UI up if we need something better.

I think you asked somewhere about what happens if there's an error, if you don't have an answer yet try this:

if totalChainsToAdd.count > 1
    continue to add the other chains silently. (we can tweak this to show a final error summary message after this is done or an in-progress message)
else
    show error message
end

@hboon
Copy link
Member

hboon commented Dec 28, 2021

@hboon Any solutions to something like this?

Simulator.Screen.Recording.-.iPhone.12.-.2021-12-24.at.18.18.51.mp4

BTW, are we still changing this to a pop up that lets the user choose between browsing and manual entry?

@eviltofu
Copy link
Contributor

@hboon Any solutions to something like this?
Simulator.Screen.Recording.-.iPhone.12.-.2021-12-24.at.18.18.51.mp4

BTW, are we still changing this to a pop up that lets the user choose between browsing and manual entry?

No, it is now a combined screen.

@hboon
Copy link
Member

hboon commented Dec 28, 2021

Ok. BTW, according to https://projects.invisionapp.com/share/MW1220T36ET3#/screens/461348027, the tab title should be "Enter", not "Manual"

@eviltofu
Copy link
Contributor

eviltofu commented Dec 29, 2021

@hboon Any solutions to something like this?

If you are referring to the layout of the UI elements being animated when it appears, they have to be laid out before the screen is animated. The keyboard showing up might be causing it. Try disable the call to becomeFirstResponder() if there is one and trace from there?

It's not setting the keyboard for firstResponder. I think I made a design mistake by adding/removing the child view controllers. I'm going to try and add them both child view controllers' views to the container view and then show/hide the correct view.

Yes, the weird animation went away once I added both view controllers and just changed the relevant view's isHidden var.

@eviltofu
Copy link
Contributor

eviltofu commented Dec 30, 2021

Just block the app with a UIActivityView for now? If the user tries to add 20 chains at a go, they have to make extra efforts to do it (we don't have a Select All button). But we do have to make sure that if the app is terminated while processing the chains that we don't end up in an inconsistent state, but just adding X out of 20 chain is fine. It's not a common operation and we don't know if it's going to be used much yet and we can come back and polish the UI up if we need something better.

I think you asked somewhere about what happens if there's an error, if you don't have an answer yet try this:

if totalChainsToAdd.count > 1
    continue to add the other chains silently. (we can tweak this to show a final error summary message after this is done or an in-progress message)
else
    show error message
end

@hboon I'm going to make a simple view controller to handle this. Just a simple progress bar with a "x/y" caption so no localisation will be needed, and a cancel button. There is no difference if the user adds a few main nets and test nets networks together?

@hboon
Copy link
Member

hboon commented Dec 30, 2021

There is no difference if the user adds a few main nets and test nets networks together?

Does browsing to add chains have a UISwitch in the mainnet and testnet headers as shown in #3464 (comment) ? If there is, then we should only allow them to add mainnets or testnets separately each time, i.e. X mainnet or Y testnets, but not X mainnet + y testnets to mirror the behavior enabling/disabling screen.

If there are no UISwitchs, then it should be fine.

@eviltofu
Copy link
Contributor

Does browsing to add chains have a UISwitch in the mainnet and testnet headers as shown in #3464 (comment) ? If there is, then we should only allow them to add mainnets or testnets separately each time, i.e. X mainnet or Y testnets, but not X mainnet + y testnets to mirror the behavior enabling/disabling screen.

@hboon Currently, the user is able to switch from mainnet to testnet in both browse and search modes. What happens to the selected networks when I switch sections?

Simulator.Screen.Recording.-.iPhone.12.-.2021-12-30.at.10.33.54.mp4

@hboon
Copy link
Member

hboon commented Dec 31, 2021

Remember them so they remain selected when the user toggles back, but don't add them unless the user switchs back to that tab.

Unrelated: seems a bit odd the user has to tap Cancel to add the selected chains, but not a big issue. Don't worry about it.

@eviltofu
Copy link
Contributor

eviltofu commented Jan 4, 2022

@colourfreak @hboon What should the error message be when some networks failed when being added?

@hboon
Copy link
Member

hboon commented Jan 5, 2022

Reuse something that we already have in the manual/enter screen if we can. Otherwise propose something. Faster that way, because it's not so critical. We can tweak it after that.

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 a pull request may close this issue.

3 participants