Skip to content

feat(txwrapper-polkadot): Add support for Statemint & Statemine#75

Merged
emostov merged 20 commits intomainfrom
tarik-txwrapper-statemint
May 11, 2021
Merged

feat(txwrapper-polkadot): Add support for Statemint & Statemine#75
emostov merged 20 commits intomainfrom
tarik-txwrapper-statemint

Conversation

@TarikGul
Copy link
Member

@TarikGul TarikGul commented Apr 5, 2021

Closes: #68

This PR adds support for the Statemint & Statemine chains.

@TarikGul TarikGul added the inProgress PR that is actively being worked on label Apr 5, 2021

const KNOWN_CHAIN_PROPERTIES = {
statemint: {
ss58Format: PolkadotSS58Format.polkadot,
Copy link
Member Author

Choose a reason for hiding this comment

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

@emostov @joepetrowski, I know the ss58Format is 0, and it shares that same property of Polkadot (assuming all parachains will share the same ss58Format as their relay chain). Just want to make confirm the tokenDecimals and tokenSymbol are the same. I saw in the statemint repo, that it calculates Dollars different from DOT ie: using 1/100 instead of 1/10.

Also Zeke should the key be statemint here as well. Just not sure how parachains will deal with

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah good question. On the first part, I will defer final answers to Joe. But in general all non-development chains should ideally have there own ss58 prefix, so it does not make sense to me that it is 0 here. (Dev chains should have prefix 42).

To your second part, yes this is correct. Should not be a difference for parachains.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@emostov - the reason that this uses 0 as its SS58 is that it's a common good chain, so it uses DOT as its native currency. It's basically part of Polkadot, but rather than bloat the Relay Chain with this logic, we're putting it in a parachain.

@TarikGul TarikGul requested review from emostov and joepetrowski April 5, 2021 04:27
Copy link
Contributor

@emostov emostov left a comment

Choose a reason for hiding this comment

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

Looks good.
Lets

  1. wait to hear back on the chain properties before merging
  2. Re-export the utility pallet methods


const KNOWN_CHAIN_PROPERTIES = {
statemint: {
ss58Format: PolkadotSS58Format.polkadot,
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah good question. On the first part, I will defer final answers to Joe. But in general all non-development chains should ideally have there own ss58 prefix, so it does not make sense to me that it is 0 here. (Dev chains should have prefix 42).

To your second part, yes this is correct. Should not be a difference for parachains.

@joepetrowski
Copy link
Contributor

Looks good, would be good to add another instance for Statemine, Statemint's Kusama cousin.

@emostov
Copy link
Contributor

emostov commented Apr 29, 2021

@joepetrowski & @TarikGul (cc @dvdplm) I think we should ship methods for system parachains in txwrapper-polkadot. Seems redundant to have packages which overlap in everything expect for ~1 pallet.

To do this:

  1. make sure the registry in txwrapper-polkadot supports every system parachain + relay chains.
  2. re-export methods for all pallets that system parachains + relay chains touch
  3. make a clear note in txwrapper-polkadot README, this repos README, and somewhere in the polkadot example that txwrapper-polkadot aims to support both system parachains and relay chains. Additionally we should be clear that they should look at a chains runtime to see what exact pallets it has and that not all methods in txwrapper-polkadot apply to all supported chains

Any concerns with this plan? If not I think we can convert this PR to comply with the above

@joepetrowski
Copy link
Contributor

That sounds good. As logic becomes more specialized I think we can code in some helpful warnings. For example, if Statemint is the only chain with pallet-assets, and a governance chain is the only one with pallet-democracy, we can hardcode those genesis hashes into txwrapper-polkadot and log a warning if someone builds a tx with the genesis hash of the wrong chain. Will also be an indicator that you are probably fetching version info from the wrong node.

@emostov emostov changed the title feat(txwrapper-statemint): Add support for txwrapper-statemint feat(txwrapper-polkadot): Add support for Statemint & Statemine May 11, 2021
Co-authored-by: David <dvdplm@gmail.com>
@emostov emostov requested review from dvdplm and niklasad1 May 11, 2021 15:35
Copy link
Contributor

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

LGTM

@TarikGul
Copy link
Member Author

LGTM @emostov! Great job!

Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

lgtm

// The default type registry has polkadot types
const registry = new TypeRegistry();

// As of now statemine is not a supported specName in the default polkadot-js/api type registry.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know if there's an issue for that upstream? If there is, maybe link to it here?

@emostov emostov removed the inProgress PR that is actively being worked on label May 11, 2021
@emostov emostov merged commit 8958d30 into main May 11, 2021
@emostov emostov deleted the tarik-txwrapper-statemint branch May 11, 2021 20:42
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.

Create txwrapper-statemint

5 participants