Skip to content

add xKSM & heiko-runtime#134

Closed
0x8f701 wants to merge 20 commits intomasterfrom
feat/heiko-runtime
Closed

add xKSM & heiko-runtime#134
0x8f701 wants to merge 20 commits intomasterfrom
feat/heiko-runtime

Conversation

@0x8f701
Copy link
Copy Markdown
Contributor

@0x8f701 0x8f701 commented May 22, 2021

close: #74

TODO in next PR:

  • use currency decimal instead of TOKEN_DECIMAL

@0x8f701 0x8f701 force-pushed the feat/heiko-runtime branch 2 times, most recently from 53146a4 to 665eeca Compare May 22, 2021 02:52
@0x8f701 0x8f701 requested review from RainFallsSilent, ayushmishra2005, kaichaosun and yz89 and removed request for RainFallsSilent May 22, 2021 03:45
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 23, 2021

Codecov Report

Merging #134 (a234da7) into master (afe8886) will decrease coverage by 0.57%.
The diff coverage is 49.01%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #134      +/-   ##
==========================================
- Coverage   69.82%   69.24%   -0.58%     
==========================================
  Files          23       25       +2     
  Lines        2237     2312      +75     
==========================================
+ Hits         1562     1601      +39     
- Misses        675      711      +36     
Impacted Files Coverage Δ
node/parallel/src/chain_spec.rs 0.00% <ø> (ø)
node/parallel/src/command.rs 0.00% <0.00%> (ø)
pallets/loans/src/loan.rs 76.55% <0.00%> (ø)
pallets/loans/src/mock.rs 88.00% <ø> (ø)
pallets/prices/src/mock.rs 78.57% <ø> (ø)
pallets/prices/src/tests.rs 100.00% <ø> (ø)
runtime/heiko/src/lib.rs 0.00% <0.00%> (ø)
runtime/parallel/src/lib.rs 0.00% <0.00%> (ø)
runtime/vanilla/src/lib.rs 0.00% <0.00%> (ø)
primitives/src/currency.rs 36.84% <36.84%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update afe8886...a234da7. Read the comment docs.

@0x8f701 0x8f701 force-pushed the feat/heiko-runtime branch from b823740 to a234da7 Compare May 23, 2021 13:24
@0x8f701
Copy link
Copy Markdown
Contributor Author

0x8f701 commented May 24, 2021

@yz89 @kaichaosun shall we merge this?


use cumulus_primitives_core::ParaId;
use parallel_runtime::currency::DOLLARS;
#[cfg(feature = "runtime-heiko")]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I forgot why we have this conditional build feature, is it also used in statemine or polkadot?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@kaichaosun no actually there are two solutions:

  1. build all runtimes while compiling, and then use --chain= to specify different chainspec, the disadvantage of this is: it compiles very slow

  2. use feature gate to build only one time

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we use a better server to improve the compilation?
Change implementation code to solve build issue doesn't make much sense to me.

T::PalletId::get().into_account()
}

fn ensure_currency(currency_id: &CurrencyId) -> DispatchResult {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Need test case.

#[derive(Encode, Decode, Eq, PartialEq, Copy, Clone, RuntimeDebug, PartialOrd, Ord)]
#[cfg_attr(feature = "std", derive(Serialize, Deserialize, Hash))]
pub enum CurrencyId {
DOT("Polkadot", 10),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How is the decimal used?

Copy link
Copy Markdown
Contributor Author

@0x8f701 0x8f701 May 24, 2021

Choose a reason for hiding this comment

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

@kaichaosun it's currently not used, I'd like to do it in next PR. Basically it should replace TOKEN_DECIMAL

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's discuss this one in code diff.
Some thoughts from me,
if it's an asset issuance platform, the storage needs to maintain the decimals for a specific asset, frame-assets pallet,
but if it is the native asset for a substrate chain, the source of truth of the decimals locates in chain_spec.json, if we maintain the decimals here, if dot change its decimal, we need to upgrade at the same time, this can be struggle.

@0x8f701 0x8f701 force-pushed the feat/heiko-runtime branch from a234da7 to ff64485 Compare May 24, 2021 12:54
@0x8f701
Copy link
Copy Markdown
Contributor Author

0x8f701 commented May 25, 2021

@yz89 how should I test it? because it's an obvious thing

@0x8f701 0x8f701 force-pushed the feat/heiko-runtime branch from dd6463b to a73ee55 Compare May 25, 2021 03:53
@0x8f701 0x8f701 force-pushed the feat/heiko-runtime branch from a73ee55 to 6b8ac12 Compare May 25, 2021 09:31
@0x8f701 0x8f701 marked this pull request as draft May 26, 2021 02:00
@0x8f701 0x8f701 closed this May 27, 2021
@0x8f701 0x8f701 deleted the feat/heiko-runtime branch June 8, 2021 06:47
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.

currency_id abstration

5 participants