nns: Allow 'Register' register TLD from nested domains #117

Merged
fyrchik merged 1 commit from achuprov/frostfs-contract:feat/allow_register_nested_domain into master 2024-11-02 14:21:47 +00:00
Member

Allow registration of TLD domains when calling aa.bb.cc

Signed-off-by: Alexander Chuprov a.chuprov@yadro.com

Allow registration of `TLD` domains when calling `aa.bb.cc` Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>
achuprov added 1 commit 2024-09-30 19:08:37 +00:00
[#115] nns: Allow 'Register' register TLD from nested domains
Some checks failed
Code generation / Generate wrappers (pull_request) Successful in 54s
DCO action / DCO (pull_request) Successful in 58s
Tests / Tests (pull_request) Failing after 1m28s
d449e0adf1
Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>
achuprov force-pushed feat/allow_register_nested_domain from d449e0adf1 to 833326318d 2024-10-04 11:00:37 +00:00 Compare
achuprov force-pushed feat/allow_register_nested_domain from 833326318d to 2508ec4c17 2024-10-09 09:58:25 +00:00 Compare
achuprov force-pushed feat/allow_register_nested_domain from 2508ec4c17 to 81853bd242 2024-10-10 21:18:31 +00:00 Compare
achuprov added 2 commits 2024-10-10 22:20:28 +00:00
Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>
[#115] nns: Make 'DeleteDomain' faster
All checks were successful
DCO action / DCO (pull_request) Successful in 1m10s
Code generation / Generate wrappers (pull_request) Successful in 1m35s
Tests / Tests (pull_request) Successful in 1m40s
7b59530eaf
Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>
achuprov force-pushed feat/allow_register_nested_domain from 7b59530eaf to b004b05b17 2024-10-10 22:24:45 +00:00 Compare
achuprov changed title from WIP: nns: Allow 'Register' register TLD from nested domains to nns: Allow 'Register' register TLD from nested domains 2024-10-10 22:26:59 +00:00
achuprov requested review from storage-core-committers 2024-10-10 22:27:10 +00:00
achuprov requested review from storage-core-developers 2024-10-10 22:27:11 +00:00
fyrchik requested changes 2024-10-11 12:56:42 +00:00
Dismissed
nns/doc.go Outdated
@ -16,0 +12,4 @@
| 0x21 + tokenKey | ByteArray | serialized NameState struct |
| 0x22 + tokenKey + Hash160(tokenName) | Hash160 | container contract hash |
| 0x23 + tokenKey + Hash160(tokenName) | string | global domain flag |
| 0x24 + tokenKey | int | number of subdomains for the TLD domain |
Owner

If I register bb.com and then aa.bb.com, will it be equal to 2?
I.e. TLD is NOT included and lower level domains are taken into account regardless of what zone they belong to?

If I register `bb.com` and then `aa.bb.com`, will it be equal to 2? I.e. TLD is NOT included and lower level domains are taken into account regardless of what zone they belong to?
Author
Member

No, 3. The number of domains in the zone is counted (including the zone). Clarified the wording

No, 3. The number of domains in the zone is counted (including the zone). Clarified the wording
@ -277,7 +279,6 @@ func getGlobalDomain(ctx storage.Context, domain string) string {
// extractCnametgt returns the value of the Cnametgt TXT record.
func extractCnametgt(ctx storage.Context, name, domain string) string {
fragments := splitAndCheck(domain)
Owner

useless change

useless change
Author
Member

fixed

fixed
achuprov marked this conversation as resolved
@ -45,2 +45,4 @@
//This is necessary to distinguish it from regular CNAME records.
prefixGlobalDomain byte = 0x23
// prefixCountSubDomains contains information about the number of subdomains for the TLD domain.
// If it is nil, it will definitely be calculated on the first removal
Owner

Comment should end with a dot (we have no linter for contracts for other reasons)

Comment should end with a dot (we have no linter for contracts for other reasons)
achuprov marked this conversation as resolved
@ -326,2 +320,3 @@
return name
continue
}
Owner

blank line

blank line
Author
Member

fixed

fixed
achuprov marked this conversation as resolved
@ -409,2 +413,4 @@
postTransfer(oldOwner, owner, []byte(name), nil)
runtime.Notify("RegisterDomain", name)
countSubDomain := 0
Owner

Could you move this to a separate updateSubdomainCounter function?
The l == 1 condition could be a isTLD parameter.
Would be easier to read.

Could you move this to a separate `updateSubdomainCounter` function? The `l == 1` condition could be a `isTLD` parameter. Would be easier to read.
achuprov marked this conversation as resolved
@ -578,3 +594,3 @@
}
func deleteDomain(ctx storage.Context, name string) {
func restoreCountSubDomains(ctx storage.Context, name string) int {
Owner

What does restore mean here?
Looks like countSubdomains()?

What does `restore` mean here? Looks like `countSubdomains()`?
achuprov marked this conversation as resolved
@ -588,0 +618,4 @@
countSubDomain = common.FromFixedWidth64(countSubDomainRaw.([]byte))
} else {
countSubDomain = restoreCountSubDomains(ctx, fragments[len(fragments)-1])
storage.Put(ctx, countSubDomainsKey, std.Serialize(common.ToFixedWidth64(countSubDomain)))
Owner
  1. Why do we put it here if we will update the value later anyway?
  2. Why do we read it here and not closer to where the counter is updated?
1. Why do we put it here if we will update the value later anyway? 2. Why do we read it here and not closer to where the counter is updated?
achuprov marked this conversation as resolved
@ -609,2 +651,4 @@
storage.Delete(ctx, nsKey)
storage.Delete(ctx, append([]byte{prefixRoot}, []byte(name)...))
isAutoCrated := storage.Get(ctx, autoCreatedPrefix)
Owner

crEated

crEated
Author
Member

fixed

fixed
achuprov marked this conversation as resolved
@ -610,1 +652,4 @@
storage.Delete(ctx, append([]byte{prefixRoot}, []byte(name)...))
isAutoCrated := storage.Get(ctx, autoCreatedPrefix)
if countSubDomain == 1 && isAutoCrated != nil {
Owner

isAutoCreated looks like a leaky abstraction to me, why do we care whether is was autocreated or by hand?

`isAutoCreated` looks like a leaky abstraction to me, why do we care whether is was autocreated or by hand?
Author
Member

If the user has created aa.bb.cc, they may not know anything about cc. Therefore, there is a chance of forming garbage zones.

If the user has created `aa.bb.cc`, they may not know anything about `cc`. Therefore, there is a chance of forming garbage zones.
achuprov force-pushed feat/allow_register_nested_domain from b004b05b17 to 2947de0d87 2024-10-11 15:13:44 +00:00 Compare
achuprov force-pushed feat/allow_register_nested_domain from 2947de0d87 to 10ae9bafc1 2024-10-11 16:02:26 +00:00 Compare
fyrchik reviewed 2024-10-15 10:25:54 +00:00
@ -344,3 +340,2 @@
fragments := splitAndCheck(name)
l := len(fragments)
tldKey := append([]byte{prefixRoot}, []byte(fragments[l-1])...)
isTLD := len(fragments)
Owner

is* is more like a name for boolean variable than an integer

`is*` is more like a name for boolean variable than an integer
Author
Member

fixed

fixed
achuprov marked this conversation as resolved
@ -610,1 +638,4 @@
storage.Delete(ctx, append([]byte{prefixRoot}, []byte(name)...))
isAutoCreated := storage.Get(ctx, autoCreatedPrefix)
if countSubDomain == 1 && isAutoCreated != nil {
Owner

Could you add && isAutoCreated.(bool)?
There would be fewer chances to break in future.

Could you add `&& isAutoCreated.(bool)`? There would be fewer chances to break in future.
Author
Member

fixed

fixed
achuprov marked this conversation as resolved
achuprov force-pushed feat/allow_register_nested_domain from 10ae9bafc1 to 85c0a044ef 2024-10-15 13:43:36 +00:00 Compare
achuprov force-pushed feat/allow_register_nested_domain from 85c0a044ef to 3f4f8feca7 2024-10-15 14:08:25 +00:00 Compare
fyrchik approved these changes 2024-10-16 05:50:51 +00:00
fyrchik merged commit 3f4f8feca7 into master 2024-10-16 05:50:58 +00:00
fyrchik added this to the v0.21.0 milestone 2024-10-17 14:18:09 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-core-developers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-contract#117
No description provided.