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

Add Genshiro network #32

Merged
merged 11 commits into from
Dec 6, 2021
Merged

Add Genshiro network #32

merged 11 commits into from
Dec 6, 2021

Conversation

Overseven
Copy link
Contributor

No description provided.

@Overseven
Copy link
Contributor Author

Overseven commented Oct 27, 2021

We have the same prefix for Equilibrium and Genshiro networks

@Overseven
Copy link
Contributor Author

Overseven commented Nov 12, 2021

We need to be able to connect Ledger devices to both networks.
How can we reach this goal if both networks have the same prefix?

@BenWhiteJam
Copy link

What do you mean by connect Ledger device? You will need to have a Ledger App oder connect through the Polkadot extension to Ledger, no?
How much does the token abbreviation TOKEN make sense for Genshiro, shouldn’t it be more aligned with the project name?

@Overseven
Copy link
Contributor Author

Overseven commented Nov 21, 2021

You will need to have a Ledger App oder connect through the Polkadot extension to Ledger, no?

Yep :)
But the polkadot js extension takes the list of networks from here and filters by ledger availability:
https://github.com/polkadot-js/common/blob/8880e8fa8c715d69a1ba4a7f85f103ffbdbd1bbb/packages/networks/src/interfaces.ts#L55-L64

https://github.com/polkadot-js/common/blob/master/packages/networks/src/substrate.ts#L9-L19

@Overseven
Copy link
Contributor Author

Overseven commented Nov 21, 2021

How much does the token abbreviation TOKEN make sense for Genshiro, shouldn’t it be more aligned with the project name?

Currently our network are using 11 assets and its count will be increased. What is the best solution about symbols naming? Use just one main token name?

@bkchr
Copy link
Member

bkchr commented Nov 21, 2021

How much does the token abbreviation TOKEN make sense for Genshiro, shouldn’t it be more aligned with the project name?

Currently our network are using 11 assets and its count will be increased. What is the best solution about symbols naming? Use just one main token name?

I would say that the idea was to have all of them in the document, because of this reason the tokens are a list.

@Overseven
Copy link
Contributor Author

I would say that the idea was to have all of them in the document, because of this reason the tokens are a list.

Well, then it will look like this:

"symbols": ["GENS", "EQD", "LPT0", "KSM", "BNB", "CRV", "DAI", "ETH", "BUSD", "USDC", "USDT", "WBTC"]

The first three assets are created by ourselves, others are bridged from other networks. Do I need to set only ours or all assets in the 'symbols' field?

@Overseven
Copy link
Contributor Author

I am more concerned about the requirement for the uniqueness of the prefix for networks. We already have our second project Equilibrium. By using the Polkadot js extension we can already connect ledger devices to Equilibrium, but with this limitation, we can't add Genshiro, because both networks have the same prefix

@bkchr
Copy link
Member

bkchr commented Nov 21, 2021

I would say that the idea was to have all of them in the document, because of this reason the tokens are a list.

Well, then it will look like this:

"symbols": ["GENS", "EQD", "LPT0", "KSM", "BNB", "CRV", "DAI", "ETH", "BUSD", "USDC", "USDT", "WBTC"]

The first three assets are created by ourselves, others are bridged from other networks. Do I need to set only ours or all assets in the 'symbols' field?

Wdyt @jacogr ?

@joepetrowski
Copy link

The first three assets are created by ourselves, others are bridged from other networks. Do I need to set only ours or all assets in the 'symbols' field?

IMO This should only set the native tokens of the network. Other assets should come with some asset metadata, either on-chain or in your own Equilibrium/Genshiro registry.

@Overseven
Copy link
Contributor Author

Ok, I updated it.
But I still get an error: "failed to generate code from json: prefixes must be unique but this account’s prefix" from "continuous-integration/gitlab-clippy" check

"network": "genshiro",
"displayName": "Genshiro Network",
"symbols": ["GENS", "EQD", "LPT0"],
"decimals": [9],

Choose a reason for hiding this comment

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

Should be the same length as symbols.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, fixed

@joepetrowski
Copy link

Ok, I updated it. But I still get an error: "failed to generate code from json: prefixes must be unique but this account’s prefix" from "continuous-integration/gitlab-clippy" check

Is there a reason that the two networks have the same prefix? The prefixes should make addresses identifiable as belonging to separate networks.

@Overseven
Copy link
Contributor Author

Is there a reason that the two networks have the same prefix? The prefixes should make addresses identifiable as belonging to separate networks.

Because the same address is more simple to use for our users. We have two projects with started mainnets.

@joepetrowski
Copy link

It's going to be tough to resolve. When I wrote the spec for this registry, I decided that the network name should be unique to a prefix. Changing that could have downstream effects on tools that might use network names to identify entries in this registry.

Changing network from string to [string] is definitely something we could evaluate but is outside the scope of this PR. If you want to merge this ASAP, my recommendation is to choose another prefix for Genshiro (68 looks conveniently available). It mostly affects UI rendering and is something that can be explained in wallets.

@Overseven
Copy link
Contributor Author

Ok, here is our solution: we changed the Equilibrium prefix to resolve this problem. Please, review it ASAP

@joepetrowski
Copy link

LGTM but @BenWhiteJam manages the approvals of these

"website": "https://genshiro.equilibrium.io"
},
{
"prefix": 68,
"network": "equilibrium",
"displayName": "Equilibrium Network",
"symbols": ["Unknown", "USD", "EQ", "ETH", "BTC", "EOS", "DOT", "CRV"],

Choose a reason for hiding this comment

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

@joepetrowski Following your comments, wouldn't it be also recommended to list only chain native tokens for the Equilibrium network then?

@Overseven
Copy link
Contributor Author

Overseven commented Dec 3, 2021

Following your comments, wouldn't it be also recommended to list only chain native tokens for the Equilibrium network then?

@BenWhiteJam done

@bkchr bkchr merged commit d78ad96 into paritytech:main Dec 6, 2021
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.

5 participants