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

NameService: minor improvements, part 1 #23

Merged
merged 6 commits into from
Sep 15, 2022

Conversation

AnnaShaleva
Copy link
Member

Refactor code a bit and make it closer to https://datatracker.ietf.org/doc/html/rfc1034.

According to [RFC 1034](https://datatracker.ietf.org/doc/html/rfc1034#section-3.1),
"Each node has a label, which is zero to 63 octets in length".

Originally fixed in nspcc-dev/neofs-contract#238.
@roman-khimov
Copy link

Just to give some background, we have an NNS contract in (https://github.com/nspcc-dev/neofs-contract/), it was originally based on C# one, but it diverged over time as we solved some problems related to NeoFS CDN and dApp needs. This contract is used in NeoFS and it also powers dApp examples shown on Aug 30, so it's tested and known to work. We think changes made there are very much relevant for real-life applications and standard public NNS contract will benefit from them.

@roman-khimov
Copy link

@shargon, @superboyiii, @erikzhang? This part is rather trivial, but we have more invasive things in the queue, so maybe we can deal with this one first and then discuss other things.

@@ -353,6 +355,7 @@ public static string GetRecord(string name, RecordType type)
token.EnsureNotExpired();
Copy link
Member

Choose a reason for hiding this comment

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

Returns null if nameMap[tokenKey] is null?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is fixed in #25, see the 20d34ea.

Copy link
Member

Choose a reason for hiding this comment

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

Move this fix to this PR?

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's a lot of other code refactoring made in 20d34ea which is related GetRecord/GetRecords/GetAllRecords, can we keep it inside a separate PR? We can discuss these changes separately in #25.

Choose a reason for hiding this comment

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

I'd really prefer going step by step. The changes in this PR are simple and internally consistent to me, the problem raised is not related to these changes, it always existed and we'll deal with it in #25.

src/NameService/NameService.cs Show resolved Hide resolved
Copy link

@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.

Everything solved, looks beautiful, good to go. Let's merge this one and move on to the part 2.

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.

4 participants