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

Domain registry for domains v2 #1614

Merged
merged 12 commits into from
Jul 3, 2023
Merged

Domain registry for domains v2 #1614

merged 12 commits into from
Jul 3, 2023

Conversation

NingLin-P
Copy link
Member

#close #1591

This PR implements Domain registry as described in the V2 Domains spec

There are some TODOs left regarding to #1612.

Code contributor checklist:

crates/pallet-domains/src/domain_registry.rs Outdated Show resolved Hide resolved
crates/pallet-domains/src/domain_registry.rs Outdated Show resolved Hide resolved
crates/pallet-domains/src/domain_registry.rs Outdated Show resolved Hide resolved
crates/subspace-runtime/src/lib.rs Outdated Show resolved Hide resolved
crates/sp-domains/Cargo.toml Show resolved Hide resolved
crates/pallet-domains/src/lib.rs Outdated Show resolved Hide resolved
crates/pallet-domains/src/domain_registry.rs Outdated Show resolved Hide resolved
Comment on lines +165 to +146
// TODO: initialize the genesis block in the domain block tree once we can drive the
// genesis ER from genesis config through host function
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as genesis_block_hash, the genesis ER is initialized on the instantiation, not the genesis block.

Copy link
Member Author

Choose a reason for hiding this comment

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

The genesis block here is referring to the block in the block tree, not the actual domain block.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's obviously confusing, I know the confusion essentially comes from the block tree though 🤷‍♂️

Copy link
Contributor

@vedhavyas vedhavyas left a comment

Choose a reason for hiding this comment

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

Will do another round in a bit. Overall make sense.

Please leave stake related things with a TODO and I'll those gaps in my PR.

crates/pallet-domains/src/domain_registry.rs Outdated Show resolved Hide resolved
@NingLin-P
Copy link
Member Author

This PR was rebased regarding to a lot of renaming and resolving conflicts with the main branch.

These items will be used in later commit

Signed-off-by: linning <[email protected]>
Required by upcoming changes, also considered changing DomainId to a
type alias of u32, will be great to have some feedbacks

Signed-off-by: linning <[email protected]>
Copy link
Contributor

@vedhavyas vedhavyas left a comment

Choose a reason for hiding this comment

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

Some nits and suggestions but overall make sense!
Also noticed that there are no tests for this. Please add tests

crates/pallet-domains/src/domain_registry.rs Show resolved Hide resolved
#[derive(TypeInfo, Debug, Encode, Decode, Clone, PartialEq, Eq)]
pub struct DomainConfig {
/// A user defined name for this domain, should be a human-readable UTF-8 encoded string.
pub domain_name: Vec<u8>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't there a BoundedVec for this purpose ?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, the domain_name is constructed by the user not by the runtime, thus we still need to check it manually, and after the domain is instantiated, domain_name is not supposed to change.

@@ -211,6 +266,10 @@ mod pallet {
DomainRuntimeUpgraded {
runtime_id: RuntimeId,
},
DomainInstantiated {
domain_id: DomainId,
runtime_id: RuntimeId,
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if the runtime_id is useful here as this is part of the domain object, no ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Runtime event is used to send notifications about changes or conditions in the runtime to external entities like users, chain explorers, or dApps. I think runtime_id is important info about the domain instance that is worth delivery (just like runtime_type in DomainRuntimeCreated event).

Copy link
Contributor

@vedhavyas vedhavyas Jul 3, 2023

Choose a reason for hiding this comment

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

Not sure I agree here. Events should signal that domain was created with a specific DomainId. Rest of the information should be fetched from the state as required. If we add runtime_id then we should add runtime_type as well as just runtime_id is useless by itself since users or chain explorers or dapps will have to make state call to fetch that information.

IMO, events should hold as minimum details as possible but still points to bigger information on the state

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I don't have strong opinion here, the runtime_id is removed now PTAL.

crates/pallet-domains/src/lib.rs Show resolved Hide resolved
Copy link
Contributor

@liuchengxu liuchengxu left a comment

Choose a reason for hiding this comment

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

A few nits, otherwise lgtm.

crates/pallet-domains/src/domain_registry.rs Outdated Show resolved Hide resolved
Comment on lines +165 to +146
// TODO: initialize the genesis block in the domain block tree once we can drive the
// genesis ER from genesis config through host function
Copy link
Contributor

Choose a reason for hiding this comment

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

It's obviously confusing, I know the confusion essentially comes from the block tree though 🤷‍♂️

crates/pallet-domains/src/domain_registry.rs Outdated Show resolved Hide resolved
liuchengxu
liuchengxu previously approved these changes Jul 3, 2023
type DomainInstantiationDeposit: Get<BalanceOf<Self>>;

/// The currency trait.
type Currency: LockableCurrency<Self::AccountId, Moment = Self::BlockNumber>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I just realize that we shouldn't continue using LockableCurrency as it has been deprecated since paritytech/substrate#12951. Now that we're starting the domains from scratch, better not use the deprecated interfaces from day one. Non-blocker, can be resolved in another PR after. cc @vedhavyas

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, will resolve it in my upcoming PR.

@NingLin-P NingLin-P enabled auto-merge July 3, 2023 06:08
@NingLin-P NingLin-P merged commit e1df001 into main Jul 3, 2023
9 checks passed
@NingLin-P NingLin-P deleted the domain-registry branch July 3, 2023 14:55
ParthDesai added a commit to autonomys/subspace-pulsar-sdk that referenced this pull request Jul 13, 2023
ParthDesai added a commit to autonomys/subspace-pulsar-sdk that referenced this pull request Aug 15, 2023
ParthDesai added a commit to autonomys/subspace-pulsar-sdk that referenced this pull request Aug 15, 2023
ParthDesai added a commit to autonomys/subspace-pulsar-sdk that referenced this pull request Aug 15, 2023
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.

Domain Registry
3 participants