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

Name Service Pallet as Collectives Service #14491

Open
wants to merge 111 commits into
base: master
Choose a base branch
from
Open

Name Service Pallet as Collectives Service #14491

wants to merge 111 commits into from

Conversation

rossbulat
Copy link

@rossbulat rossbulat commented Jul 2, 2023 •
edited
Loading

This PR expands on what was worked on in #11052, getting the name service pallet to a production ready state.

The addition of a register_para and deregister_para will be added, ideally only callable via XCM over OpenGov proposals, as a mechanism for paras to be registered on the service. This will prevent arbitrary / non-existent Para IDs from being stored on the service. The set_address call within the name service resolver can then take a para_id in addition to the address itself, so the service knows which para to route addresses to.

  • Fix up code from Name Service Pallet mark II #11052 to work with master and get tests working.
  • Review of pallet and write-up high level lib doc.
    • Check deposit mechanism and whether deposits should be added to subdomains too (they are).
    • Update header comments to match master.
    • Registrations storage item to have counter.
  • Add register_para and deregister_para calls to store Para IDs that wish to be a part of the name service.
    • Use RegistrationManager and ensure origin is RuntimeOrigin for these calls.
    • Provide a suffix that acts as the source of truth for the para.
  • set_address to also require para_id arg, ensure para id is registered.
  • Revise configs and move to storage where applicable (deposits, tiers) via a set_configs call (similar to Nomination Pools set_configs) for root origin to manage.
  • Make suffixes unique, reverse lookup & set address to take suffix instead of para ID.
    • hash -> address, suffix -> para.
  • Remove controller related code in preparation for a NameService Collectives proxy type.
  • Expand call comments.
  • Don't unreserve deposit on reveal. Instead, assign to owner (if depositor different) to back up registration and incentivise deregistering.
  • Write up benchmarks and plug in call weights.
  • Expand pallet docs with calls.
  • Fix tests to account for domains & configs.
  • Use domain terminology for para registrations, and name terminology for nodes.
  • Revisit tests and determine what needs to be added.
  • Add test proving that a tree of a registered TLD and subnodes can all be deregistered and removed from storage.
  • Bound renew max length & update benchmark.
  • Cumulus Companion (Westend Collectives only): Add pallet to runtime and add a NameService proxy to have access to resolver management and registration of new subdomains.

Other tasks post initial release

  • Explore whether a force_deregister_para is possible, to asynchronously query Relay Chain and remove para from name service if the para is no longer registered.
  • Decentralised / trustless transfer of ownership on-chain process.

@rossbulat rossbulat added A3-in_progress Pull request is in progress. No review needed at this stage. C3-medium PR touches the given topic and has a medium impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited B1-note_worthy Changes should be noted in the release notes T1-runtime This PR/Issue is related to the topic “runtime”. labels Jul 2, 2023
@rossbulat rossbulat changed the title [WIP] Name Service as Collectives Service [WIP] Name Service Pallet as Collectives Service Jul 2, 2023
@rossbulat
Copy link
Author

bot clean

@rossbulat
Copy link
Author

bot rebase

@paritytech-processbot
Copy link

Rebased

@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/name-service-pallet-introduction-and-call-for-frame-devs-to-review/3480/1

@rossbulat
Copy link
Author

bot rebase

@paritytech-processbot
Copy link

Rebased

franciscoaguirre
franciscoaguirre reviewed Jul 21, 2023
View reviewed changes
Copy link
Contributor

@franciscoaguirre franciscoaguirre left a comment

Choose a reason for hiding this comment

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

Love this! 🚀
Left mostly comments about documentation.

frame/name-service/src/lib.rs Outdated Show resolved Hide resolved
frame/name-service/src/lib.rs Outdated Show resolved Hide resolved
frame/name-service/src/lib.rs
//! * node: Either a to-level name hash or a subnode record that exists in the service registry.
//! * name hash: A blake2_256 hash representation of a registered name.
//! * subnode: A child name of a registered name hash. Subnodes of a name can be registered
//! recursively, so the depth a subnode can be registered is unbounded.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the depth unbounded?

Copy link
Author

Choose a reason for hiding this comment

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

The pallet leaves the market to decide how valuable subnode depth will be. Since all subnodes require a deposit, the user is always trading subnode registrations with token value, so this should prevent any flooding of storage.

frame/name-service/src/lib.rs Outdated Show resolved Hide resolved
frame/name-service/src/lib.rs Outdated Show resolved Hide resolved
frame/name-service/src/tests.rs Outdated Show resolved Hide resolved
frame/name-service/src/mock.rs
frame_system::limits::BlockWeights::simple_max(Weight::MAX);
}

impl frame_system::Config for Test {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use the derive_impl macro to use a default for most of these. You can still override the items you need to.

For example:

substrate/frame/examples/split/src/mock.rs

Line 35 in 750fa6b

#[derive_impl(frame_system::config_preludes::TestDefaultConfig as frame_system::DefaultConfig)]

Copy link
Author

Choose a reason for hiding this comment

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

Excellent, just the expertise this pallet needs.
I found that BlockHashCount and AccountData yielded errors when they were left out. Should they be removed from the example in frame support?:

substrate/frame/support/procedural/src/lib.rs

Line 870 in f8c9358

/// #[register_default_impl(TestDefaultConfig)]

frame/name-service/src/commit_reveal.rs Outdated Show resolved Hide resolved
frame/name-service/src/benchmarking.rs
(NameService::<T>::name_hash(&name), owner, caller)
}

benchmarks! {
Copy link
Contributor

Choose a reason for hiding this comment

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

benchmarks v2?

Copy link
Author

Choose a reason for hiding this comment

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

👀 Are you referring to usage of the #[benchmark] maco?

frame/name-service/src/resolver.rs Outdated Show resolved Hide resolved
@paritytech-ci paritytech-ci requested a review from a team July 21, 2023 18:33
Ross Bulat and others added 7 commits July 22, 2023 02:57
@franciscoaguirre
Update frame/name-service/src/lib.rs
55304c4 
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
@franciscoaguirre
Update frame/name-service/src/lib.rs
7a06069 
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
@franciscoaguirre
Update frame/name-service/src/lib.rs
7c01a5d 
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
@franciscoaguirre
Update frame/name-service/src/lib.rs
ddb0a2f 
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
@franciscoaguirre
Update frame/name-service/src/tests.rs
48e0664 
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
@franciscoaguirre feedback
c3c81ff
derive TestDefaultConfig
bf088c1
@rossbulat
Copy link
Author

Love this! 🚀 Left mostly comments about documentation.

Super appreciated @franciscoaguirre , this is exactly the FRAME expertise the pallet needs. 💪

ggwpez added a commit to ggwpez/zepter that referenced this pull request Jul 24, 2023
@ggwpez
Add test for Substrate
f347266 
Testing the feature problem of paritytech/substrate#14491

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
ggwpez added a commit to ggwpez/zepter that referenced this pull request Jul 27, 2023
@ggwpez
Version 0.7.0 (#8)
c3cef05 
* Fix plural (s) and add polkadot#7537 test

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add tests and --feature-enables-dep

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Run CI on self hosted

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Bump to 0.7.0

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add test for Substrate

Testing the feature problem of paritytech/substrate#14491

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Update tests

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Use deterministic commit hash len on UI tests

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add --left-side-feature-missing

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Fix toml comma formatting when adding features

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Update UI tests

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Fmt

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Update to 1.73 nightly

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add MSRV

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Grammar

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3322490

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3322491

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@franciscoaguirre franciscoaguirre franciscoaguirre left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
A3-in_progress Pull request is in progress. No review needed at this stage. B1-note_worthy Changes should be noted in the release notes C3-medium PR touches the given topic and has a medium impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Security Audit (PRs) - SRLabs
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rossbulat @Polkadot-Forum @paritytech-cicd-pr @franciscoaguirre