polkadot-js / common

Utilities and base libraries for use across polkadot-js for Polkadot and Substrate. Includes base libraries, crypto helpers and cross-environment helpers.
Apache License 2.0
254 stars 147 forks source link

HydraDX -> Hydration rebrand #1923

Closed jak-pan closed 2 months ago

jak-pan commented 5 months ago

Companion https://github.com/polkadot-js/apps/pull/10669

jak-pan commented 5 months ago

What is the key in the map taken from? Is it rpc.chain? If so should we add new chain to support the transition when we change this?

jak-pan commented 5 months ago

I am talking all of the lines including hydradx as a key to something https://github.com/polkadot-js/common/blob/37fa211fdb141d4f6eb32e8f377a4651ed2d9068/packages/hw-ledger/src/defaults.ts#L21 https://github.com/polkadot-js/common/blob/37fa211fdb141d4f6eb32e8f377a4651ed2d9068/packages/networks/src/defaults/ledger.ts#L25 https://github.com/polkadot-js/common/blob/37fa211fdb141d4f6eb32e8f377a4651ed2d9068/packages/networks/src/defaults/genesis.ts#L68

Is this all mapped first from the genesis hash to the hydradx key and then used throughout other apps? I know that Polkadot Vault uses //hydradx derivation path by default when creating a new wallet.

I was wondering if it would be better to keep the hydradx as well as hydration for some transition period but then I don't know how all the consumers would handle the conflicts.

Can you advise the best route forward here? I would like to get at least display names correct with breaking the least possible amount of things.

jak-pan commented 5 months ago

Test needs to be resolved.

What do you suggest here? It seems that there is a legacy ledger-substrate app which was never released for HydraDX but somehow the @polkadot/dev-test is looking for deps there in some cryptyc way.

TarikGul commented 3 months ago

Hey sorry for the delay, things have been hectic and finally cooled down.

In terms of the new LedgerGeneric wrapper that support the new Polkadot app the naming of your chain only has the affect to see if that chain is supported or not. It checks to see if ledgerApps has the chain name -

https://github.com/polkadot-js/common/blob/7ef452bdf032002fd61e0f95f781b66a88653636/packages/hw-ledger/src/defaults.ts#L6

That being said - For the old Legacy Ledger wrapper it did the same check via the chain name, but also connected to the ledger app via the chain name. This could cause potential issues for older accounts trying to use the legacy app.

https://github.com/polkadot-js/common/blob/7ef452bdf032002fd61e0f95f781b66a88653636/packages/hw-ledger/src/Ledger.ts#L134

Is this all mapped first from the genesis hash to the hydradx key and then used throughout other apps

For the extension - yes. For other apps I don't know.

TarikGul commented 3 months ago

Can you advise the best route forward here? I would like to get at least display names correct with breaking the least possible amount of things.

Question: What is the chain name that the hydra is now returning?

I think on a branding sense in apps, we can totally change it to the new brand. I don't think that will have much affect. Most of the internals in apps, and the extension especially with ledger will use what is received by the chain or what is within these config files to confirm the integrity on connection.

TarikGul commented 3 months ago

For the test, i believe you can adjust the naming here:

https://github.com/paritytech/ss58-registry/blob/f54e1ea030a48404a20dca48db3dd5f671ba2cc1/ss58-registry.json#L545-L551

It's a little annoying because it's another repo so apologies, but that will fix the tests.

I would also test this by adjusting the values in the node_modules for the @substrate/ss58-registry package to ensure there are no mistakes and the tests pass before getting the PR merged in the other repo.

jak-pan commented 3 months ago

https://github.com/paritytech/ss58-registry/blob/f54e1ea030a48404a20dca48db3dd5f671ba2cc1/ss58-registry.json#L545-L551

Maybe I should rename display name here and duplicate this entry with "hydration" network key to be sure we don't break something?

TarikGul commented 3 months ago

Maybe I should rename display name here and duplicate this entry with "hydration" network key to be sure we don't break something?

Makes sense to me! Just to be clear as well, the naming in the default files that contain info like the ledger slip44 or the genesis should reflect the chain name that is being returned by the chain itself.

Little tip as well: Also if you would like to actually see what the effects would look like in apps or the extension etc - you can run yarn polkadot-dev-copy-to <repo_name> Just make sure the other repo shares the same parent directory.

TarikGul commented 3 months ago

Also no need to adjust the ss58 json file in /test for this PR, once we have the ss58-registry package updated this repo will automatically update that file to reflect the changes.

jak-pan commented 2 months ago

@TarikGul can you please rerun the tests? The ss58 has been updated

TarikGul commented 2 months ago

@jak-pan Thanks for pinging me, in order for the tests to pass you will need to pull in master. I updated the ss58-registry for you.

jak-pan commented 2 months ago

@jak-pan Thanks for pinging me, in order for the tests to pass you will need to pull in master. I updated the ss58-registry for you.

Looks like it still failed but https://github.com/paritytech/ss58-registry/blob/0e88f87b185cdee829f624b88d3cfc47eaaf2161/ss58-registry.json#L547 this is updated in master

TarikGul commented 2 months ago

@jak-pan Thanks for pinging me, in order for the tests to pass you will need to pull in master. I updated the ss58-registry for you.

Looks like it still failed but https://github.com/paritytech/ss58-registry/blob/0e88f87b185cdee829f624b88d3cfc47eaaf2161/ss58-registry.json#L547 this is updated in master

Yea looks like we just need to patch that test since it is resolving the the value of the key hydradx: 'hydraDX' for ledger supported apps, but the value is actually 'hydration', just one moment, and I'll post a code snippet that you can add to the test that should solve this

TarikGul commented 2 months ago

@jak-pan Comment above which I think should solve this!

jak-pan commented 2 months ago

@jak-pan Comment above which I think should solve this!

Worked :)

TarikGul commented 2 months ago

Okay so now that the ss58-registry is updated - we can move focus onto Apps now.

polkadot-js-bot commented 2 months ago

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.