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

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

Merged
merged 20 commits into from
May 11, 2021

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.

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

packages/txwrapper-statemint/CHANGELOG.md Outdated Show resolved Hide resolved

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.

packages/txwrapper-statemint/src/index.ts Outdated Show resolved Hide resolved
packages/txwrapper-statemint/src/index.ts Outdated Show resolved Hide resolved
@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
README.md Outdated Show resolved Hide resolved
Co-authored-by: David <dvdplm@gmail.com>
@emostov emostov requested review from dvdplm and niklasad1 May 11, 2021 15:35
Copy link
Member

@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

@@ -75,9 +88,18 @@ export function getRegistry({
// 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