Name Service Pallet mark II #11052

shawntabrizi · 2022-03-16T14:26:47Z

In progress.

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

…shawntabrizi-name-service

* set_name and bid tests * claim and free tests * assign, unassign, make_permanent tests * fixes * whitespace fixes * remove unused dep Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

ggwpez · 2022-03-28T12:31:24Z

frame/name-service/src/commit_reveal.rs

+
+impl<T: Config> Pallet<T> {
+	/// Get the commitment hash from the raw name and secret.
+	pub fn commitment_hash(name: &[u8], secret: u64) -> CommitmentHash {


I think the secret here should either be a Config type or a [u8; 32], just using a u64 is a but unprofessional IMHO.

Why not? A chain developer can put very large commitment alive periods, in which this would become unsafe.

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

…ytech/substrate into shawntabriz-name-service-2

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

ntn-x2

I think front-running cannot be avoided even with blinded commitments, as someone might discard the reveal tx, and make a new commitment with the same name and a different secret, and then reveal that. Other than that, few nit-picks. Haven't gone over benchmarks nor test cases.

ntn-x2 · 2022-04-04T07:42:23Z

frame/name-service/src/lib.rs

+	pub(super) type AddressResolver<T: Config> =
+		StorageMap<_, Blake2_128Concat, NameHash, T::AccountId>;
+
+	/// This resolver maps name hashes to an account


Copy-paste typo.

ntn-x2 · 2022-04-04T07:42:38Z

frame/name-service/src/lib.rs

+		BytesStorage<T::AccountId, BalanceOf<T>, BoundedNameOf<T>>,
+	>;
+
+	/// This resolver maps name hashes to an account


Typo here as well.

ntn-x2 · 2022-04-04T07:43:33Z

frame/name-service/src/lib.rs

+	pub enum Event<T: Config> {
+		/// A new `Commitment` has taken place.
+		Committed { depositor: T::AccountId, owner: T::AccountId, hash: CommitmentHash },
+		/// A new `Registration` has taken added.


/// A new `Registration` has taken place.

ntn-x2 · 2022-04-04T07:44:07Z

frame/name-service/src/lib.rs

+		/// A new `Registration` has taken added.
+		NameRegistered { name_hash: NameHash, owner: T::AccountId },
+		/// A `Registration` has been transferred to a new owner.
+		NewOwner { from: T::AccountId, to: T::AccountId },


Would it make sense to add the name hash being transferred?

ntn-x2 · 2022-04-04T07:52:51Z

frame/name-service/src/commit_reveal.rs

+		secret: u64,
+		length: T::BlockNumber,
+	) -> DispatchResult {
+		ensure!(name.len() <= T::MaxNameLength::get() as usize, Error::<T>::NameTooLong);


Would T::MaxNameLength::get().saturated_into::<u32>() be more correct?

Also, instead of preventing names that are too short by setting an unresonably high registration fee, why can't the check be performed here?

I think casting a u32 to a usize is not a problem, but I can still change it.

Also, instead of preventing names that are too short by setting an unresonably high registration fee, why can't the check be performed here?

Yea that should be added.

i think casting to usize is fine. this behavior exists a lot of places afaik

ntn-x2 · 2022-04-04T08:42:20Z

frame/name-service/src/misc.rs

+impl<T: Config> Pallet<T> {
+	pub fn registration_fee(name: Vec<u8>, length: T::BlockNumber) -> BalanceOf<T> {
+		let name_length = name.len();
+		let fee_reg = if name_length < 3 {


It doesn't feel completely right to prevent names that are too short by simply returning a very high fee. Also, this is called in a single place, so it might make sense to simply perform the check before this function is called. Furthermore, why is the 3 a magic number and not part of the pallet's configuration?

ntn-x2 · 2022-04-04T08:52:35Z

frame/name-service/src/registrar.rs

+	}
+
+	/// Check if an account is a controller or owner of this name registration.
+	pub fn is_controller(registration: &RegistrationOf<T>, user: &T::AccountId) -> bool {


Since it also checks ownership, perhaps renaming it to can_control would be clearer.

ntn-x2 · 2022-04-05T06:09:13Z

frame/name-service/src/lib.rs

+		/// Register the raw name for a given name hash. This can be used as a reverse lookup for
+		/// front-ends.
+		///
+		/// This is a permissionless function that anyone can call who is willing to place a deposit


Is it correct to talk about deposits here? The NameServiceResolver trait does not really enforce it. This seems a (recommended) implementation detail.

ntn-x2 · 2022-04-05T06:13:22Z

frame/name-service/src/commit_reveal.rs

+
+		// TODO AUDIT WARNING: Can return an error after `withdraw` happens. Needs to be
+		// transactional or refactored.
+		Self::do_register(


I guess this could just be moved up before calling T::RegistrationFeeHandler::on_unbalanced? Then there are no failures after this is executed.

ntn-x2 · 2022-04-05T06:26:02Z

frame/name-service/src/lib.rs

+			name: Vec<u8>,
+		) -> DispatchResult {
+			let sender = ensure_signed(origin)?;
+			ensure!(Registrations::<T>::contains_key(name_hash), Error::<T>::RegistrationNotFound);


Why is there no ensure!(Self::is_controller(&registration, &sender), Error::<T>::NotController); here?

* add conceret types for runtime * Adjusting the PerByteFee Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com> * fix format Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

stale · 2022-06-14T10:51:27Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

shawntabrizi and others added 30 commits September 24, 2020 11:54

Initial Design

fb53e61

Name assignments

5373c40

rename expiration -> bid end

a94e901

Hashed names

71c5115

Update name to PaymentDestination

7c3e76d

attempt at multiaddress

a4f085a

extend_ownership + ExtensionConfig

d59c8da

Basic end to end test

3aa692a

Merge branch 'master' into shawntabrizi-name-service

af5eca2

Update frame/name-service/src/mock.rs

cdb64fb

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

More extensible multiaddress format

a329e32

update name

b81b08d

Don't depend on indices to define multiaddress type

7009e91

Use MultiAddress in Node Template too!

36fdb7b

reduce traits, fix build

3fc64d1

Merge remote-tracking branch 'origin/shawntabrizi-multiaddress' into …

e450ded

…shawntabrizi-name-service

fix merge

08de670

compiles... kinda a hack

b7c120d

Simple implementation of TwoStaticLookup

20d1a70

better impl of multiple lookup sources

64afc14

support multiple StaticLookup

535a512

bump tx version

98bfef6

Merge branch 'master' into shawntabrizi-multiaddress

3a8c1c2

Merge branch 'master' into shawntabrizi-multiaddress

41e3c24

feedback

0e5ffa3

Merge branch 'shawntabrizi-multiaddress' into shawntabrizi-name-service

65c701b

Update lib.rs

04b0e8d

Pallet-name-service tests (#7676)

474c2ea

* set_name and bid tests * claim and free tests * assign, unassign, make_permanent tests * fixes * whitespace fixes * remove unused dep Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

Merge branch 'master' into shawntabrizi-name-service

db31a2d

update to latest substrate

b159137

shawntabrizi added 3 commits March 28, 2022 02:59

more refactor

f6012ac

more refactor

8b79fd8

fix compile

6a68761

ggwpez reviewed Mar 28, 2022

View reviewed changes

shawntabrizi and others added 17 commits March 28, 2022 23:36

fix controller

4621a22

renames

b193060

add some docs

0a18803

add set subnode owner

59fbe4e

introduce resolver

38d4e37

Add first benchmarks

1db41fa

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

Add weights file

22eabf1

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

Increase max commit alive

e58611b

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

Merge branch 'shawntabriz-name-service-2' of https://github.com/parit…

930d0c7

…ytech/substrate into shawntabriz-name-service-2

periods -> length, renew takes expiry

75b5aaa

Add MaxNameLength

cd2ae23

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

make balance max in benchmark

161d39c

Merge branch 'master' into shawntabriz-name-service-2

0c24553

introduce resolver trait

2c72d4c

tweaks

2d4ccb3

add text resolvers

b184a60

comments and tweaks

bbdc59d

ntn-x2 reviewed Apr 5, 2022

View reviewed changes

hamidra and others added 3 commits May 2, 2022 21:15

add conceret types for runtime (#11336)

cc384c8

* add conceret types for runtime * Adjusting the PerByteFee Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com> * fix format Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

some integrity tests

2262e3e

integrity_test

61784c7

shawntabrizi mentioned this pull request May 16, 2022

PSP for Name Service on Polkadot w3f/PSPs#43

Open

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jun 14, 2022

stale bot closed this Jun 28, 2022

rossbulat mentioned this pull request Jul 2, 2023

Name Service Pallet as Collectives Service #14491

Open

23 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Name Service Pallet mark II #11052

Name Service Pallet mark II #11052

shawntabrizi commented Mar 16, 2022

ggwpez Mar 28, 2022

shawntabrizi Mar 29, 2022

ggwpez Mar 29, 2022 •

edited

Loading

ntn-x2 left a comment

ntn-x2 Apr 4, 2022

ntn-x2 Apr 4, 2022

ntn-x2 Apr 4, 2022

ntn-x2 Apr 4, 2022

ntn-x2 Apr 4, 2022

ntn-x2 Apr 5, 2022

ggwpez Apr 5, 2022

shawntabrizi Apr 22, 2022

ntn-x2 Apr 4, 2022

ntn-x2 Apr 4, 2022

ntn-x2 Apr 5, 2022

ntn-x2 Apr 5, 2022

ntn-x2 Apr 5, 2022

stale bot commented Jun 14, 2022

Name Service Pallet mark II #11052

Name Service Pallet mark II #11052

Conversation

shawntabrizi commented Mar 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggwpez Mar 29, 2022 • edited Loading

Choose a reason for hiding this comment

ntn-x2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Jun 14, 2022

ggwpez Mar 29, 2022 •

edited

Loading