-
Notifications
You must be signed in to change notification settings - Fork 14
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
Conversation
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.
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. |
@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(); |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
Move commonly used checks to a separate methods.
According to the [RFC 1034](https://datatracker.ietf.org/doc/html/rfc1034#section-3.5), labels are forbidden to have hyphen as the first and the last characters, but it's allowed to have it in the middle. Originally fixed in nspcc-dev/neofs-contract#183.
Originally implemented in nspcc-dev/neofs-contract@4041924.
05d5c22
to
b5f7a16
Compare
There was a problem hiding this 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.
Refactor code a bit and make it closer to https://datatracker.ietf.org/doc/html/rfc1034.