paritytech / ss58-registry

Registry for SS58 account types
Apache License 2.0
55 stars 150 forks source link

Move sub-token to this repository #82

Closed wigy-opensource-developer closed 2 years ago

wigy-opensource-developer commented 2 years ago

The sub-tokens crate is still being used from an already archived repository. The goal of this PR is to move it near to the single source of truth about tokens in the ecosystem and give it a place to maintain it further.

wigy-opensource-developer commented 2 years ago

@kianenigma suggested that sub-token could grow from having just 3 hardwired tokens DOT, WND and KSM (and the possibility of making your own static or dynamic instances) into a crate that is kept up-to-date with the whole ecosystem through the JSON file updates here. Every release of this crate could add new tokens to provide currency output formatting.

As a first step I wanted to understand what is this registry, and make space for new functionality added to it. I see that networks and tokens are 2 distinct entities in this simplified DB and it is not normalized at the moment (e.g. the information that DCK token has 6 digits is duplicated in 2 different networks).

To avoid doing too much, in this PR I just plan to add impl_token and DynamicToken into this crate and do not change the build.rs to create an enumeration of networks and tokens yet, but I think that is a completely viable direction.

bkchr commented 2 years ago

This code is currently only used in the staking miner as far as I have seen. This is not giving enough reasoning why we should merge this macro here. This could just be merged into the staking miner.

kianenigma commented 2 years ago

@bkchr Currently the Rust tooling in the ecosystem does not have a proper way to format tokens. I think we def. need something like this, and to do it you need to know both the token names, and the decimal points of each substrate-based chain's token. Both of these information are in this repo, so my opinion is that this should be either in this repo, or use it as dependency. Probably we can start with one and decouple if needed.

(Note: I have not read the PR code changes yet. Ideally this PR would incorporate the list of all tokens registered here, and generate some token type for them that can be nicely formatted)

bkchr commented 2 years ago

Okay, but then we should only support the static tokens. I don't want to pull in the dynamic tokens especially as it uses thread locals etc.

So, we should need to change the code in the following way.

  1. Ensure that the we sanity check all token definitions in the registry that the same name always maps to the same number of decimals.
  2. Then we should generate an enum TokenTypes or just Tokens? IDK. I'm open for ideas.
  3. This enum should provide a function:
    
    impl TokenTypes {
     pub fn create_token(&self, amount: u128) -> Token { 
         match self {
              Self::Dot => Token::new(amount, 10),
         }
     }
    }

struct Token { decimals: u8, amount: u128, }

/// The rest of the implementation the macro has provided

4. Add a function to `Ss58AddressFormatRegistry `:

impl Ss58AddressFormatRegistry { pub fn tokens(&self) -> &[TokenTypes] { match self { Self::Polkadot => &[TokenSymbol::Dot] } } }



Does this makes sense @kianenigma. Then we have information about the tokens also being exposed. And if you need some "dynamic" you can just use the `Token` type directly.
kianenigma commented 2 years ago

we certainly don't need the DynamicToken anymore here. That was exactly a placeholder for when we don't know the information because of lacking this repo!

Your ideas sounds good. I am glad we came to agreement about having this.

As for @wigy-opensource-developer's original PR:

  1. I think this is a cool task and if you want to do it as Basti describe, you are more than welcome to do so.
  2. If you want a shortcut, as noted offline as well, I don't want this to block the upgrade of scale codec, so feel free to make an issue out of our discussion here, and just patch staking miner in any way (probably just move sub-tokens there).

Arguably option 1 is the much better way forward, not creating debt along the way.

bkchr commented 2 years ago

SCALE codec is already merged everywhere or at least in Substrate, Polkadot & Cumulus. So, @wigy-opensource-developer can take over this task. Shouldn't be that hard :)

wigy-opensource-developer commented 2 years ago

I know it is a big change to this repo, but I tried to separate changes to smaller commits at least. I hope it matches both Basti's specification and Kian's needs for the staking repo.