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

NNS update for 0.20.0 #419

Merged
merged 19 commits into from
Jul 30, 2024
Merged

NNS update for 0.20.0 #419

merged 19 commits into from
Jul 30, 2024

Conversation

roman-khimov
Copy link
Member

It's a rebase of #266 with two additional commits.

AnnaShaleva and others added 15 commits July 29, 2024 15:47
Reuse getAllRecords for GetAllRecords.

Signed-off-by: Anna Shaleva <[email protected]>
Do not include CNAME to the resulting list if we're looking for another
record type. If it's CNAME than it must be resolved.

Signed-off-by: Anna Shaleva <[email protected]>
Do not move parts of SetRecord/AddRecord to a separate functions,
it makes the contract code more complicated. Also, improve
documentation a bit.

Signed-off-by: Anna Shaleva <[email protected]>
a.b.c.d can be an "a.b" record of "c.d" and can be "a" record of "b.c.d", the
first one would fail to setRecord currently:

  at instruction 4287 (THROW): unhandled exception: "parent domain has expired"

which is a 5758c84 regression.

Signed-off-by: Roman Khimov <[email protected]>
Don't repeat it each time.

Signed-off-by: Anna Shaleva <[email protected]>
It doesn't save VM opcodes, but allows to keep record key creation
logic in a single place which prevents the code from bugs appearance.

Signed-off-by: Anna Shaleva <[email protected]>
Less code repeating.

Signed-off-by: Anna Shaleva <[email protected]>
If conflicting records '*.domain' are present on new domain
registration, then `isAvailable` should return false for this
domain. Ref.
f25296b.

Signed-off-by: Anna Shaleva <[email protected]>
@roman-khimov roman-khimov added the nns NNS contract related issue label Jul 29, 2024
@roman-khimov roman-khimov added this to the v0.20.0 milestone Jul 29, 2024
@roman-khimov roman-khimov force-pushed the nns-upd-0.20.0 branch 2 times, most recently from d0d687a to 405162e Compare July 29, 2024 13:52
carpawell
carpawell previously approved these changes Jul 29, 2024
Copy link
Member

@carpawell carpawell left a comment

Choose a reason for hiding this comment

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

Did not test.

contracts/nns/contract.go Show resolved Hide resolved
@@ -60,7 +60,7 @@ const (
maxTXTRecordLength = 255
// maxRecordID is the maximum value of record ID (the upper bound for the number
// of records with the same type).
maxRecordID = 255
maxRecordID = 16
Copy link
Member

Choose a reason for hiding this comment

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

do we have some ref why this number is good? is it your idea? is it from some original version of NNS?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. Yes. No. We definitely need to fix ID overflow problem, but using a lot of records can be a problem performance-wise (likely signifying some design issues). So start small, extend if needed.

contracts/nns/contract.go Show resolved Hide resolved
contracts/nns/contract.go Outdated Show resolved Hide resolved
These numbers are pretty much arbitrary, but we currently don't need more than
that.

Signed-off-by: Roman Khimov <[email protected]>
1. splitAndCheck doesn't need allowMultipleFragments since there is exactly
   one user of it and it checks for the number of fragments anyway.
2. We can panic right inside of it with a bit more details, nil fragments are
   never valid.
3. Except for one case in checkRecord where the error must be different, so
   safeSplitAndCheck is introduced.

Fixes #411.

Signed-off-by: Roman Khimov <[email protected]>
cthulhu-rider
cthulhu-rider previously approved these changes Jul 30, 2024
Sometimes 10 is exactly the value, so changing it doesn't make the ID different:

              	Error Trace:	/home/runner/go/pkg/mod/github.com/nspcc-dev/[email protected]/pkg/neotest/basic.go:218
        	            				/home/runner/go/pkg/mod/github.com/nspcc-dev/[email protected]/pkg/neotest/client.go:108
        	            				/home/runner/work/neofs-contract/neofs-contract/tests/container_test.go:237
        	Error:      	Not equal:
        	            	expected: 0x1
        	            	actual  : 0x2
        	Test:       	TestContainerPut/standard_deploy/with_nice_names/register_in_advance
        	Messages:   	at instruction 2149 (THROW): unhandled exception: "container was previously deleted"

Signed-off-by: Roman Khimov <[email protected]>
@cthulhu-rider cthulhu-rider merged commit 5907ac0 into master Jul 30, 2024
10 checks passed
@cthulhu-rider cthulhu-rider deleted the nns-upd-0.20.0 branch July 30, 2024 08:47
@roman-khimov roman-khimov mentioned this pull request Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nns NNS contract related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants