Update NNS contract #266
Update NNS contract #266
Conversation
And adjust method usages along the way.
Reuse getAllRecords for GetAllRecords.
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.
AnnaShaleva
force-pushed
the
nns-upd
branch
from
06bcd6c
to
7d3d5de
Compare
| _ = getNameState(ctx, tokenID) // ensure not expired | ||
| recordsKey := getRecordsKey(tokenID, name) | ||
| return storage.Find(ctx, recordsKey, storage.ValuesOnly|storage.DeserializeValues) | ||
| return getAllRecords(ctx, name) |
There was a problem hiding this comment.
If we have a maxRecordID and a limited set of types, maybe we can avoid having iterator.Iterator? Although we still should have some reasonable number of items in the end. Just an idea.
There was a problem hiding this comment.
Maybe we firstly need to discuss maxRecordID value? I've added this constraint because of the record ID type: it was byte, but all int-like go types are BiGInteger in the VM, so technically, the previous implementation didn't restrict the maximum number of records of the same type. I suppose, it's not the initial intention, thus let's decide on maxRecordID.
And if we're to keep MaxRecordID set to be 255 (not that much), then I vote for converting the result of getAllecords to []stirng.
There was a problem hiding this comment.
DNS doesn't have this limit. C# NNS doesn't have it too. But I see what you're talking about now, id byte overflow can be a problem in some cases. It can be fixed at the same time to have unlimited number of records of any type. Practically I'd rather have some limit (16-64), at the moment C# contract has a limit of 1 IIRC and it's kinda OK.
There was a problem hiding this comment.
@fyrchik, what do you think about it?
There was a problem hiding this comment.
I think []string is easier to work with. In our scenarios we have at most 2 records per type, so 255 is more than enough.
There was a problem hiding this comment.
Getting back to this discussion, the profit of Iterator returned value here is that it returns not only record name, but the complete record information which includes record's name, type, ID and data. Currently it's the only method allowing to retrieve the whole RecordState from the contract, so I've changed my mind and suggest to keep Iterator. @roman-khimov, somehow I've missed this point previously.
There was a problem hiding this comment.
OK for iterator, but some lower limit is needed. 16?
|
@AnnaShaleva we agreed to use capital letters in NeoFS repos after |
In case if no records of the specified type found. Signed-off-by: Anna Shaleva <[email protected]>
`getRecord` doesn't exist since nspcc-dev@6ea4573. Signed-off-by: Anna Shaleva <[email protected]>
And adjust method usages along the way. Signed-off-by: Anna Shaleva <[email protected]>
Signed-off-by: Anna Shaleva <[email protected]>
…same type Signed-off-by: Anna Shaleva <[email protected]>
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]>
Signed-off-by: Anna Shaleva <[email protected]>
AnnaShaleva
force-pushed
the
nns-upd
branch
from
e9a6ff7
to
ac4af51
Compare
In case if no records of the specified type found. Signed-off-by: Anna Shaleva <[email protected]>
`getRecord` doesn't exist since nspcc-dev@6ea4573. Signed-off-by: Anna Shaleva <[email protected]>
And adjust method usages along the way. Signed-off-by: Anna Shaleva <[email protected]>
Signed-off-by: Anna Shaleva <[email protected]>
…same type Signed-off-by: Anna Shaleva <[email protected]>
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]>
AnnaShaleva
force-pushed
the
nns-upd
branch
from
ac4af51
to
288369a
Compare
Signed-off-by: Anna Shaleva <[email protected]>
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. nspcc-dev@f25296b. Signed-off-by: Anna Shaleva <[email protected]>
AnnaShaleva
force-pushed
the
nns-upd
branch
from
4260555
to
02fcfa8
Compare
…iate Signed-off-by: Anna Shaleva <[email protected]>
…iate Signed-off-by: Anna Shaleva <[email protected]>
AnnaShaleva
force-pushed
the
nns-upd
branch
from
02fcfa8
to
37d5f56
Compare
cthulhu-rider
requested review from
carpawell and
cthulhu-rider
as code owners
|
Hey, what's up here? The anniversary coming up soon. |
Preparing for celebration, as usual. |
|
So are the conflicts gonna be solved? Is the PR's idea actual? Haven't seen the code yet, should I review this? |
| @@ -443,7 +443,8 @@ func DeleteRecords(name string, typ RecordType) { | |||
| // Resolve resolves given name (not more then three redirects are allowed). | |||
| func Resolve(name string, typ RecordType) []string { | |||
| ctx := storage.GetReadOnlyContext() | |||
| return resolve(ctx, nil, name, typ, 2) | |||
| res := []string{} | |||
| return resolve(ctx, res, name, typ, 2) | |||
There was a problem hiding this comment.
For better unwrap compatibility? nspcc-dev/neo-go#2795, maybe?
| _ = getNameState(ctx, tokenID) // ensure not expired | ||
| recordsKey := getRecordsKey(tokenID, name) | ||
| return storage.Find(ctx, recordsKey, storage.ValuesOnly|storage.DeserializeValues) | ||
| return getAllRecords(ctx, name) |
There was a problem hiding this comment.
OK for iterator, but some lower limit is needed. 16?
Refactor code and fix a couple of bugs. Thes PR may affect other services, so @fyrchik, @carpawell, @alexvanin please, review it carefully.
@roman-khimov, these changes are ported to nspcc-dev/neo-go#2687 and to the set of C# PRs (neo-project/non-native-contracts#27 and others).