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

[5.3]Add custom networks #3783

Merged
merged 40 commits into from
Jun 9, 2022
Merged

Conversation

blackdevelopa
Copy link
Contributor

@blackdevelopa blackdevelopa commented Feb 19, 2022

Description

Can now add a non-default network.

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

Issue

Resolves #3545

@blackdevelopa blackdevelopa requested a review from a team as a code owner February 19, 2022 19:56
@github-actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@blackdevelopa blackdevelopa self-assigned this Feb 19, 2022
@blackdevelopa blackdevelopa added Code Impact - Medium Average task code change that can relatively safely being applied to the codebase type-enhancement New feature or request labels Feb 19, 2022
@blackdevelopa blackdevelopa marked this pull request as draft February 19, 2022 20:02
@blackdevelopa blackdevelopa marked this pull request as ready for review February 22, 2022 16:30
@blackdevelopa blackdevelopa added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Feb 22, 2022
Copy link
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

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

Did you optimize the pngs that you uploaded? @andreahaku Could help you if you need to go through that process.

Let me know if you have questions about anything.

@sethkfman sethkfman removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Mar 2, 2022
@blackdevelopa blackdevelopa force-pushed the feature/3545-add-custom-networks branch from 5526461 to 4a1ea9b Compare March 3, 2022 15:03
@cortisiko cortisiko added QA in Progress QA has started on the feature. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Mar 4, 2022
@blackdevelopa blackdevelopa force-pushed the feature/3545-add-custom-networks branch from dee0b71 to a1e1610 Compare March 15, 2022 12:31
Copy link
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

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

@blackdevelopa sending this back your way!

Issue 1

I can add the same custom network twice.

To reproduce:
Go to chainlist.org
Search for and add Binance smart chain to your list of networks
From the settings view, Go to the networks view
Tap “Add custom network”
Tap on add Binance smart chain
Approve adding the network.
Go back to the wallet view, then tap on the network in the top nav
Notice Binance smart chain appears twice
Return to the network view
Notice Binance smart chain exists

see here: http://recordit.co/cNidnXEHUY

Issue 2

If I were to:
Go to the networks view
Add a custom network such as polygon
Return to the wallet view
Confirm the network switched correctly
Go back to the networks view
Search for ‘polygon’
From the search list, tap and hold ‘polygon’ then delete the network
Notice the network name still appears in the search list:

Issue 3
from the switch network modal shouldn’t the modal disappear and I should still be on the same view (in this case, the custom network view) ? Currently what happens is after I hit “close” I am taken back to the wallet view. This is the same behavior as hitting the “switch network” button. When a user taps switch network, the users is taken back to the wallet view with the newly set network.
See here: http://recordit.co/TU4amzB0Un

If I were to repeat the above on chainlist.org the switch network modal disappears upon tapping “cancel”: http://recordit.co/LnN7dBLT3v

To reproduce:
Tap on any custom network
Notice the approval modal pops up
Tap approve
Now the switch network Modal pops up
Tap “close”. Notice you are taken back to the wallet view.

Issue 4
As Seth pointed out, the RPC URL has “undefined” at the end of it: https://www.screencast.com/t/WqMwBsPEh

Issue 5
The native currency token on a popular network(such as avalanche and matic) does not update with the correct image: http://recordit.co/PQn6TixjJD

To reproduce:
Add Avalanche from the popular network view
On the wallet view switch to main net
Switch back to Avalanche. Notice the AVAX token image is that of ETH

Issue 6
If I already added a popular network when I go back to the add popular network view, shouldn’t the “Add” button be disabled ? http://recordit.co/fP1T0yMnX4

To reproduce:
Add a popular network
Go to the networks view. Notice the popular network is visible
Now go to the popular network view. Notice the “Add” button is still active.

Issue 7

After I manually added a custom network I noticed in the network list modal, the custom network is the second in the list. Usually custom networks go to the bottom of that list

open the drawer
go to the networks page and tap on "add a network"
Switch to the custom networks tab
Input these into the empty fields: Network name: xDai, RPC url: https://dai.poa.network/, Symbol: xDai, Chain ID: 100
Add the network
In the switch networks modal, switch to XDAI
Now you should be on the wallet view. On that view, open the network lists
Notice xDAI is second in the list.

see here: http://recordit.co/cMrxeW5Gfx

Issue 8

Are we going to “show/hide” test networks ? In the network list modal I do not see an option to show/hide:
https://www.screencast.com/t/tg3DLkwsf

@cortisiko cortisiko added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed QA in Progress QA has started on the feature. labels Mar 15, 2022
@blackdevelopa blackdevelopa force-pushed the feature/3545-add-custom-networks branch from a003a7c to 4d71849 Compare March 18, 2022 13:22
@blackdevelopa blackdevelopa force-pushed the feature/3545-add-custom-networks branch 2 times, most recently from a4e2eab to cc65fad Compare March 28, 2022 18:24
Copy link
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

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

@blackdevelopa Adding a couple more issues:

Issue 9
looking at the figma designs: https://www.figma.com/file/wfOwLmE52OKY0bSrsQwHPL/Add-popular-custom-network?node-id=468%3A7325 i noticed some inconsistencies. If i were to add all the popular networks I do not see the copy “Great! You have added all the popular networks .. etc etc” See here: https://www.screencast.com/t/0MAIA7askOp

To reproduce:
add all custom networks
notice you just see a blank screen

Issue 10
If i were to:
add a custom network such a polygon via chain-list
go to the networks view
I see that polygon is added. Fine.
I then tap on “Add Networks”
From the popular network screen i notice polygon is still visible.
I can go ahead and re-add polygon. So now my network list shows 2 polygon networks.

see here: http://recordit.co/CYS9iTUUva

Having two of the same networks might lead to some confusion.
I noticed Chainlist has a different RPC url (in the case of Arbitrium One, Palm, Polygon) compared to what we have for those three custom networks.

Shouldn't the network education modal appear if i were to:
go to the networks view
add a new network
switch networks?
see what I mean: http://recordit.co/a3N8GpU45I

Currently this is not happening.

Issue 11

My token balance is incorrect when I am on main net with funds:
Go to the popular networks screen
add a popular network (in my case I added Binance smart chain)
instead of "switching" the network I tap "close"
upon doing so I am taken back to the wallet view with the incorrect token balance on main net.
see here: http://recordit.co/CyIBGklPdR

P.S. please pull origin before committing because I pushed some e2e changes.

@blackdevelopa blackdevelopa force-pushed the feature/3545-add-custom-networks branch from d0389f5 to b75c644 Compare April 12, 2022 14:49
@cortisiko cortisiko removed the QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed label Apr 19, 2022
@blackdevelopa blackdevelopa force-pushed the feature/3545-add-custom-networks branch from 483cfcb to b99b8ea Compare June 9, 2022 10:23
@blackdevelopa blackdevelopa merged commit 2bf68d7 into main Jun 9, 2022
@blackdevelopa blackdevelopa deleted the feature/3545-add-custom-networks branch June 9, 2022 18:34
@github-actions github-actions bot locked and limited conversation to collaborators Jun 9, 2022
@mobularay mobularay added the release-5.3.0 Issue or pull request that will be included in release 5.3.0 label Jun 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Code Impact - Medium Average task code change that can relatively safely being applied to the codebase QA Passed A successful QA run through has been done release-5.3.0 Issue or pull request that will be included in release 5.3.0 type-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENHANCEMENT] Make it easy for me to add custom networks
5 participants