Skip to content

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.

string tokenId = name[^(fragments[^2].Length + fragments[^1].Length + 1)..];
ByteString tokenKey = GetKey(tokenId);
NameState token = (NameState)StdLib.Deserialize(nameMap[tokenKey]);
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.

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