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

nns: Register pre-defined TLDs for the committee on deployment stage #344

Merged

Conversation

cthulhu-rider
Copy link
Contributor

No description provided.

Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

The concept of "committee" domain is excessive. We need all TLDs to be managed by committee. And then some TLD could be created in _deploy. And then anyone should be able to create regular domains using already existing TLDs (no need to make committee members admins). Like in #334.

@cthulhu-rider cthulhu-rider marked this pull request as draft May 29, 2023 09:52
@cthulhu-rider
Copy link
Contributor Author

lets then explicitly denote the layer of top-level domains controlled by the committee. 2nd level domains can be freely registered by anybody. Any deeper level requires owner or admin authorization.

@cthulhu-rider
Copy link
Contributor Author

cthulhu-rider commented Jun 8, 2023

The concept of "committee" domain is excessive

agree overall, but what behavior do u expect from methods of ownership like https://pkg.go.dev/github.com/nspcc-dev/neofs-contract/nns#OwnerOf called with TLD? Maybe panic?

@cthulhu-rider cthulhu-rider force-pushed the feature/334-committee-tlds branch 2 times, most recently from f83a256 to 4c6580e Compare June 8, 2023 05:32
@cthulhu-rider
Copy link
Contributor Author

@roman-khimov @AnnaShaleva i've updated changes

do u think we need dedicated registerTLD method? Let me remind u that now register method takes the domain owner as a parameter, which from now will ignored during TLD registration.

i'm ok to leave register as it is. With registerTLD, it'd make sense to ban TLD registrations via register

@cthulhu-rider cthulhu-rider marked this pull request as ready for review June 8, 2023 05:39
@roman-khimov
Copy link
Member

OwnerOf called with TLD

It's just not a proper domain, so it should behave as if this domain was never registered. I think that's similar to C# NNS behavior for TLDs.

@cthulhu-rider
Copy link
Contributor Author

should behave as if this domain was never registered

so smth like

OwnerOf("bob.com") -> Bob
OwnerOf("com") -> panic("domain not exists")

?

or it'd be better to panic with TLD not allowed?

@roman-khimov
Copy link
Member

IIUC inexistent.com will fail with panic("token not found"), that's OK for TLD too.

nns/namestate.go Outdated Show resolved Hide resolved
nns/nns_contract.go Outdated Show resolved Hide resolved
nns/nns_contract.go Outdated Show resolved Hide resolved
nns/nns_contract.go Outdated Show resolved Hide resolved
nns/nns_contract.go Show resolved Hide resolved
nns/nns_contract.go Outdated Show resolved Hide resolved
@cthulhu-rider cthulhu-rider marked this pull request as draft June 9, 2023 12:47
@cthulhu-rider
Copy link
Contributor Author

after discussion with @roman-khimov we have approved the behavior of TLD as a non-existent token for which operations like ownerOf work like with missing tokens. We also settled on the need to provide a dedicated registerTLD method and make register to work with "true" tokens, i.e. domains L2+.

@AnnaShaleva ty for the review, some of the discussions will be resolved after update I think

Previously, NNS contract required signature of the TLD owner to grant
access for L2 domain name registration. This didn't allow to properly
manage TLDs by committee (only) since it is multi-account and dynamic
while all domains were owned by the single account. Also, such behavior
made it almost impossible to register L2 domains in practice.

To solve the described problems, the following changes are introduced to
the access rules (`register` method of the NNS contract):
 * TLDs are managed only by the committee only;
 * free L2 domains are free-to-take;
 * all L3+ domains are managed by parent domain owner or administrator
   only.

Refs nspcc-dev#334.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
@cthulhu-rider cthulhu-rider force-pushed the feature/334-committee-tlds branch 2 times, most recently from e0ce477 to a2f67c5 Compare June 10, 2023 08:28
In order to register TLD, committee multi-signature must be gathered.
It is not always easy. For example, on "fresh" network, committee
members may not have a notary role, so they will not be able to use
Notary service to collect a signature. At the same time, Notary service
is the only convenient way to collect a multi-signature in a distributed
mode. To simplify the solution of the described task, it would be
convenient to be able to register pre-known TLDs at the NNS contract
deployment stage (which is performed by the committee).

Support list of (name, e-mail) pairs describing pre-defined TLDs as
optional deployment parameters (passed into `_deploy` callback). Also
make `migration.NewContract` to pre-register 'neofs' TLD.

Refs nspcc-dev#334.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
Top-level domains aren't NFTs, therefore NNS contract must not treat
them as such. At the same time, technically TLDs are valid domains (e.g.
'org'), so nothing will prevent the user from performing operations with
them. Based on this, the best approach would be treating TLDs as
non-existent domains.

Throw 'token not found' exception on TLD input of any method. The
storage model is left the same: this allows us not to perform migration
and implement the behavior logically.

Refs 334.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
@cthulhu-rider cthulhu-rider force-pushed the feature/334-committee-tlds branch 2 times, most recently from 7603fb2 to 5b1f59b Compare June 10, 2023 09:01
Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

I think we need some listTLDs iterator to be able to retrieve them if regular tokens don't work.

nns/nns_contract.go Outdated Show resolved Hide resolved
nns/nns_contract.go Outdated Show resolved Hide resolved
nns/nns_contract.go Outdated Show resolved Hide resolved
nns/nns_contract.go Outdated Show resolved Hide resolved
nns/nns_contract.go Outdated Show resolved Hide resolved
nns/nns_contract.go Show resolved Hide resolved
@cthulhu-rider
Copy link
Contributor Author

I think we need some listTLDs iterator

btw contract provides roots, this is what you need? I've added test 92aa3ed

Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

Why is this still a draft?

nns/nns_contract.go Outdated Show resolved Hide resolved
nns/nns_contract.go Outdated Show resolved Hide resolved
@cthulhu-rider
Copy link
Contributor Author

Why is this still a draft?

waited for meaningful discussions to be resolved. Now it seems to be finished, i'll do #344 (comment) and open

@cthulhu-rider cthulhu-rider marked this pull request as ready for review June 16, 2023 10:16
Top-level domains are controlled by the committee. Previously, NNS
contract provided `register` method that accepted fixed domain owner.
After recent changes, TLDs were forbidden to be treated as regular
tokens in terms of the NNS contract. According to this, `register`
method is no longer well-suited for TLD registration, so, it's worth to
provide dedicated method for this purpose.

Add `registerTLD` method with signature similar to the `register` one
but w/o owner parameter.

Closes nspcc-dev#334.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
After recent changes, some contract methods pre-calculate fragments of
the requested domain name to process TLDs. In most cases, these methods
call `getNameState` function in subsequent instructions which also
performs fragmentation. In order to avoid resource costs for a
duplicated action, an already calculated partition should be reused.

Add `getFragmentedNameState` function which allows to pass
pre-calculated fragments. The function is implemented with the
possibility of a direct call from `getNameState` while maintaining the
behavior of the latter.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
nns/nns_contract.go Show resolved Hide resolved
nns/nns_contract.go Show resolved Hide resolved
@roman-khimov roman-khimov merged commit 6e9b183 into nspcc-dev:master Jun 16, 2023
8 checks passed
roman-khimov added a commit that referenced this pull request Sep 21, 2023
An omission of #344.

Signed-off-by: Roman Khimov <roman@nspcc.ru>
roman-khimov added a commit that referenced this pull request Sep 21, 2023
An omission of #344.

Signed-off-by: Roman Khimov <roman@nspcc.ru>
roman-khimov added a commit that referenced this pull request Sep 21, 2023
An omission of #344.

Signed-off-by: Roman Khimov <roman@nspcc.ru>
roman-khimov added a commit that referenced this pull request Sep 21, 2023
An omission of #344.

Signed-off-by: Roman Khimov <roman@nspcc.ru>
roman-khimov added a commit that referenced this pull request Sep 25, 2023
An omission of #344.

Signed-off-by: Roman Khimov <roman@nspcc.ru>
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.

None yet

3 participants