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

Neo Name Service #2201

Merged
merged 22 commits into from
Jan 10, 2021
Merged

Neo Name Service #2201

merged 22 commits into from
Jan 10, 2021

Conversation

erikzhang
Copy link
Member

@erikzhang erikzhang commented Jan 5, 2021

Close #1545
Replace #1644

@erikzhang erikzhang added this to the NEO 3.0 milestone Jan 5, 2021
@erikzhang erikzhang marked this pull request as ready for review January 5, 2021 17:48
@erikzhang
Copy link
Member Author

Ready for review.

uint now = (uint)(engine.Snapshot.PersistingBlock.Timestamp / 1000) + 1;
byte[] start = CreateStorageKey(Prefix_Expiration).AddBigEndian(0).ToArray();
byte[] end = CreateStorageKey(Prefix_Expiration).AddBigEndian(now).ToArray();
foreach (var (key, _) in engine.Snapshot.Storages.FindRange(start, end))
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it is possible to just delete those expired within the last couple of seconds.
The start can be the last end.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no difference when doing db search.

if (!IPAddress.TryParse(data, out IPAddress address)) throw new FormatException();
if (address.AddressFamily != AddressFamily.InterNetwork) throw new FormatException();
break;
case RecordType.CNAME:
Copy link
Contributor

Choose a reason for hiding this comment

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

It is better to check whether the original domain name exists.

Copy link
Member Author

Choose a reason for hiding this comment

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

But the original name can be external.

Copy link
Contributor

Choose a reason for hiding this comment

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

what do you mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

An external name, for example, google.com.

@erikzhang erikzhang merged commit b87df5d into master Jan 10, 2021
@erikzhang erikzhang deleted the nns branch January 10, 2021 11:05
@cloud8little
Copy link
Contributor

Tested PASS with following scenaroes:

  • addRoot with committee members

  • addRoot with non-committee member

  • addRoot and register name, such as neo.org

  • register name with longer than 62 characters, Upper-Case, special characters.

  • setPrice and then register name, the price changed.

  • Two accounts can not register the same name

  • The same account can not register the same name twice.

  • Account A transfer the NNS token to another account, A not able to setRecord, the new account is able to setRecord. A not able to setAdmin, the new account is able to setRecord.

  • nns1 account register org.neo, add CNAME record onchain.com;
    nns2 account register onchain.com; add CNAME record onchain.org,
    nns3 account register onchain.org; add CNAME record leo.com,
    nns4 account register leo.com; add IPV6 record "message",
    resolve leo.com with IPV6 type,get "message"
    resolve onchain.org with IPV6 type get "message"
    resolve onchain.com的IPV6 type, get "message"
    resolve org.neo with IPV6 record ;with FAULT result.

  • more scenaroes to be added.

@erikzhang
Copy link
Member Author

@cloud8little The cases can be added to UT.

@cloud8little
Copy link
Contributor

cloud8little commented Jan 13, 2021

@cloud8little The cases can be added to UT.

Sure, @erikzhang what do you think of following two questions?

  1. why do we check the admin witness when setAdmin(L149)? I think the owner can decide whom to be the administrator of the domain name, no need to check the admin account.

    private void SetAdmin(ApplicationEngine engine, string name, UInt160 admin)
    {
    if (!nameRegex.IsMatch(name)) throw new ArgumentException(null, nameof(name));
    string[] names = name.Split('.');
    if (names.Length != 2) throw new ArgumentException(null, nameof(name));
    if (admin != null && !engine.CheckWitnessInternal(admin)) throw new InvalidOperationException();
    NameState state = engine.Snapshot.Storages.GetAndChange(CreateStorageKey(Prefix_Token).Add(GetKey(Utility.StrictUTF8.GetBytes(name)))).GetInteroperable<NameState>();
    if (!engine.CheckWitnessInternal(state.Owner)) throw new InvalidOperationException();
    state.Admin = admin;
    }

  2. seems there is no API to find out who is the admin. properties only return the domain name, description, and expiration data.

{"name":"testown1.neo","description":"","expiration":1642052284}

Thought providing contract method or RPC API is both OK. because the owner of the domain can transfer the domain to the other account, and the new account may have no idea who is the admin.

@erikzhang
Copy link
Member Author

erikzhang commented Jan 13, 2021

  1. Because it checks that it is a valid address.
  2. If you are the owner, you know who is the administrator. If you are not, why do you want to know it? Maybe we should set the Admin to null automatically after transfer. What do you think?

@cloud8little
Copy link
Contributor

  1. Because it checks that it is a valid address.
  2. If you are the owner, you know who is the administrator. If you are not, why do you want to know it? Maybe we should set the Admin to null automatically after transfer. What do you think?

1 is ok to me.
2. I think it is ok and reasonable to set the admin to null after transfer, then the old admin is not able to manipulate the domain name. It is up to the new account to reset the admin.

@erikzhang
Copy link
Member Author

Please check #2222

@superboyiii superboyiii mentioned this pull request Jan 14, 2021
36 tasks
cloud8little pushed a commit to cloud8little/neo that referenced this pull request Jan 24, 2021
ixje added a commit to CityOfZion/neo-mamba that referenced this pull request Feb 12, 2021
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.

NFT assets in NNS scenarios
5 participants