Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@montekki
Copy link
Contributor

@montekki montekki commented Aug 10, 2020

Mostly a wrapper around paras module, however

  • how does it work together with paras_sudo_wrapper? It seems that they may conflict.
  • how do we manage ParaIds? Atm it's done by hand and I am not sure that the approach existing in the previous registrar would work.
  • The module has to look out for what's going on in paras modules. For instance, it should not allow registration for ids that are outgoing in the Paras module. It's done now by looking into parasmodule but probably I've overlooked something.

This also has to build on #1562

Closes #1508

@montekki montekki added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Aug 10, 2020
@montekki montekki requested a review from rphmeier August 10, 2020 14:19
@montekki
Copy link
Contributor Author

Also it's not quite clear if this should check the limits such as validation code length and head data length.

@montekki montekki added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Aug 10, 2020
Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

Looks very clean, the amount of test code required to verify this is not insignificant

Ok(())
}

/// Swap a parachain with another parachain or parathread. The origin must be a `Parachain`.
Copy link
Contributor

@drahnr drahnr Aug 11, 2020

Choose a reason for hiding this comment

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

What's the reasoning for this limitation? Is it not enough that the types of ParaId x and y are not equivalent?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is about the origin of the Call - parachains can send messages to the relay chain that are executed as calls to modules. So this requirement means that the parachains themselves must initiate the swap

@rphmeier
Copy link
Contributor

how does it work together with paras_sudo_wrapper? It seems that they may conflict.

Yeah, they are not meant to be used together. This registrar is for permissionless deployments, whereas the sudo wrapper is for permissioned (testnet) deployments.

#[derive(PartialEq, Eq, Clone, Encode, Decode)]
#[cfg_attr(feature = "std", derive(Debug))]
#[derive(Debug, PartialEq, Eq, Clone, Encode, Decode)]
// TODO: #[cfg_attr(feature = "std", derive(Debug))]
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO needs to be resolved. Maybe derive(RuntimeDebug) is an option?

///
/// Must be sent from a Signed origin that is able to have `ParathreadDeposit` reserved.
/// `gensis_head` and `validation_code` are used to initalize the parathread's state.
#[weight = 0]
Copy link
Contributor

Choose a reason for hiding this comment

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

did the previous registrar code have 0 weight also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so

impl<T: Trait> Module<T> {
/// Register a parachain with given code. Must be called by root.
/// Fails if given ID is already used.
pub fn register_parachain(
Copy link
Contributor

Choose a reason for hiding this comment

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

I note this doesn't set Debtors. The previous logic used something called SwapAux because deposits for parachains are in the slots module. I would prefer to have all deposits in one place, and this includes XCMP/HRMP channel deposits. So at some point we should have a deposits module where we can implement swapping over. I'm fine with the code committed as is for now, though.

@montekki montekki added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Sep 28, 2020
@rphmeier rphmeier merged commit 9315b62 into paritytech:master Oct 1, 2020
@pepyakin pepyakin mentioned this pull request Oct 5, 2020
ordian pushed a commit that referenced this pull request Oct 6, 2020
* master:
  Make collation an optional return (#1787)
  XCM: Land xcm-handler and xcm-executor (#1771)
  v0.8.25 (#1785)
  add two node local net script (#1781)
  Adjust max nominators down to 128 (from 256) (#1782)
  Companion for substrate/pull/7215 (#1768)
  Remove Stale Upgrades (#1780)
  Update Polkadot Weights for Substrate 2.0 (#1761)
  Parachains v1 registrar module. (#1559)
  Derive `From` for `AllMessages` and simplify `send_msg` (#1774)
  implement remaining subsystem metrics (#1770)
  Companion for paritytech/substrate#7236 (#1773)
  WIP: remove deprecated only/except clauses, build is now manual on PRs (#1769)
  Increase Westend `spec_version` (#1766)
  move Metrics to utils (#1765)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Write a registrar module

4 participants