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

refactor network name output #421

Merged
merged 9 commits into from
Mar 4, 2021
Merged

Conversation

kremalicious
Copy link
Contributor

@kremalicious kremalicious commented Mar 2, 2021

Fixes #412.

Changes proposed in this PR:

  • move network detection and warning out of generic Web3 feedback component, so it's not displayed in popover anymore
  • output beside account button, based on app config or user network
  • output nothing when connected to Mainnet
  • fetch chain metadata from ethereum-lists/chains on build time, for mapping networkId to display name among other things
  • detect supported networks based on ocean.js ConfigHelper configs, and error icon output based on it
  • cleanup utils/wallet.ts, remove unused or redundant methods

Resulting in:

Supported & test network:

Screen Shot 2021-03-03 at 03 12 34

Unsupported & test network:

Screen Shot 2021-03-03 at 03 38 18

@vercel
Copy link

vercel bot commented Mar 2, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/oceanprotocol/market/8mBysJtbBRfFhSR4QNA7J7Z99Q5f
✅ Preview: https://market-git-feature-network-name-output-oceanprotocol.vercel.app

@mihaisc
Copy link
Contributor

mihaisc commented Mar 2, 2021

Not really solves the issue, or maybe i am misunderstanding something.
The main idea is that in the future we will have multiple "mainnet" for each chain that we implement. My suggestion was something across these lines:

  • eth mainnet
    image
  • eth test net (any of them) and continue to show network in popup like we used to do
    image
  • polygon mainnet ( or any other chains)
    image

We can add the chain name in ConfigHelper with any other stuff that we need

@kremalicious
Copy link
Contributor Author

kremalicious commented Mar 2, 2021

right, each chain can have multiple networks so to speak, and PR only accounts for current status quo.

Warning icon:
Logic as in your mockups makes sense then, it's basically warning when stuff is not "real", as in tokens representing real money value being transacted. And that would be then true for other chain's main networks too.

Another possibility would be to redefine the warning condition. Was also thinking of not using warning icon at all for all supported networks. But use it to signal when user is connected to an unsupported network, for which we can still output the network name.

As for network name label:

  • would still want to hide that for ETH mainnet, it's our default, and also reflects how other dapps handle that. User expectation in dapps is that it works against ETH mainnet.
  • for ETH test networks think it's fine showing the respective network name. They are usually well known and attached to ETH in user's minds.
  • for every other networks should be fine only showing the chain name, like Polygon as in the mock. That is unless they also have and encourage to use their own test networks. In that case we should also add e.g. Testnet after the network name

@kremalicious
Copy link
Contributor Author

After some researching, tending towards outsourcing the network naming standardization and just fetch all metadata from https://github.com/ethereum-lists/chains

They capture chain & network which we could combine, so e.g. ETH Rinkeby or Matic Mainnet based on their data structure: https://chainid.network/chains.json. This should then cover all future chains/networks as long as they are in that list, which they probably are.

There's also nativeCurrency which might help with #418

@mihaisc
Copy link
Contributor

mihaisc commented Mar 2, 2021

I think i agree with everything you said. Summary :

  • don't show chain for eth main, show only for other chains. for now we don't really want to add other test networks. The purpose of rinkeby and ropsten is to test the market not the chain itself. So we already have 2 testnets no need for more, unless some specific scenario
    image

  • change warning icon for testnets, something like (obviously make it look proper)
    image

  • integrate with https://chainid.network/

@kremalicious
Copy link
Contributor Author

kremalicious commented Mar 3, 2021

alrighty, with chainid.network we get those network naming combinations if we want to keep it flexible:

Screen Shot 2021-03-03 at 01 05 21

As per https://chainid.network/chains/#matic-mainnet--, for Matic it would be Matic Mainnet then, we could still remove all Mainnet strings though.

The goal I had in mind was to make this network name output completely network agnostic, meaning we simply output whatever user is connected to, regardless if we support it or not. So the source of truth for all network metadata becomes that community-maintained repo.

Like that idea of using test label but would keep it around network name, that account element is now exclusively for the account info. The condition based on https://chainid.network response data, if (network.network !== 'mainnet') return label, should be safe for that.

Which gives us this for all test networks:

Screen Shot 2021-03-03 at 01 37 12

With that, maybe it makes sense to use the warning icon then for networks we do not support. Although I guess everything crashes anyway right now when user is connected to unsupported network but ideally user should be able to connect to whatever network and our UI should give hints if it works or not for which warning icon seems a better use. Could be also solved as part of #401 instead of in here, but this could already work:

Screen Shot 2021-03-03 at 03 38 18

@mihaisc
Copy link
Contributor

mihaisc commented Mar 3, 2021

Not crashing can be patched easily. It's not a proper fix, but it will not crash.
Agree with everything.

@mihaisc
Copy link
Contributor

mihaisc commented Mar 3, 2021

So... just slapping a if (!initialNewConfig) return in NetworkMonitor will make the app not crash but then you don't detect the new network so it's kinda useless

app.config.js Outdated Show resolved Hide resolved
* remove supportedNetworks app config
@kremalicious
Copy link
Contributor Author

one remaining thing: response from https://chainid.network can be slow, it's just Jekyll site on GitHub Pages. So seems better to fetch the list on build time into Gatsby data structure, and query that on client side so it's instant

@kremalicious
Copy link
Contributor Author

works pretty well by now in my testing, getting name is now instant. Will not touch any other network init stuff in here and leave that to #401.

@mihaisc mihaisc self-requested a review March 4, 2021 16:45
@kremalicious kremalicious merged commit 8737264 into main Mar 4, 2021
@kremalicious kremalicious deleted the feature/network-name-output branch March 4, 2021 17:16
kremalicious added a commit that referenced this pull request Mar 4, 2021
* was broken in non-web3 browsers
* missed in #421
kremalicious added a commit that referenced this pull request Mar 4, 2021
* was broken in non-web3 browsers
* missed in #421
T0psecurity added a commit to T0psecurity/data_market that referenced this pull request May 10, 2023
* was broken in non-web3 browsers
* missed in oceanprotocol/market#421
mjtechworks pushed a commit to mjtechworks/data_market that referenced this pull request May 10, 2023
* was broken in non-web3 browsers
* missed in oceanprotocol/market#421
LoveNuna added a commit to LoveNuna/data_market that referenced this pull request May 24, 2023
* was broken in non-web3 browsers
* missed in oceanprotocol/market#421
morgan1119 added a commit to morgan1119/data_market that referenced this pull request Apr 10, 2024
* was broken in non-web3 browsers
* missed in oceanprotocol/market#421
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.

Better representation of the selected chain/network
2 participants