NEP-25 #4043
NEP-25 #4043
Conversation
This reverts commit d392f50.
- Document all implemented features and components - Add usage examples for different type definitions - Confirm backward compatibility with NEP-14 - Mark implementation as complete and tested
- Update FromStackItem_ShouldHandleNullFields test to include all 8 fields - Update ToStackItem_ShouldProduceNullFields test to expect 8 fields - Tests now match the actual ExtendedType implementation which uses 8 fields
I think that now it's solved |
|
According to the meeting circular check will be changed |
|
@ajara87 could you ad an UT for this case: FAIL: "namedtypes": {
"boo": {
"type": "Array",
"namedtype": "boo"
}
}I think should be solved know |
@shargon @roman-khimov Should work this case? |
No. Top-level namedtype can't reference another namedtype. Inner fields/values/whatever of it can. |
Thank you @roman-khimov. If I understand correctly the following should result: Examples that should fail due to invalid format Example that should fail due to circular reference Example should be successful |
These are valid to me. Not meaningful, like an array of abstract pointers, but not breaking anything either. |
* Extend ValidateExtendedTypes to check circle references * remove unused usings * Add more tests and change enum * Add assert to valid tests
| if (NamedTypes != null) | ||
| { | ||
| ret.Add(new Map(NamedTypes.ToDictionary(p => (PrimitiveType)p.Key, p => (StackItem)p.Value.ToStackItem(referenceCounter)), referenceCounter)); | ||
| } |
There was a problem hiding this comment.
I have a concern: manifest serialization format doesn't depend on hardforks, and we don't change native Management in any way. It means that there's a tiny possibility of a statediff during an update to this version: if someone deploys contract with extended types, then deployment transaction will fail on old nodes and will succeed on updated nodes.
This fact may probably be ignored, but at least we should know about it.
There was a problem hiding this comment.
If they don't update the node, the state difference will come later, isn't it?
There was a problem hiding this comment.
the state difference will come later,
At Faun height, due to other Faun-specific updates, right. But my concern is that with the current implementation state difference may happen even before Faun if someone submits extended manifest to the chain on contract deploy/update.
| private static readonly HashSet<ContractParameterType> s_lengthAllowedTypes = | ||
| [ | ||
| ContractParameterType.Integer, | ||
| ContractParameterType.ByteArray, | ||
| ContractParameterType.String, | ||
| ContractParameterType.Array |
There was a problem hiding this comment.
The implementation matches proposal. However, I think we may consider adding ContractParameterType.Map to this list since maps also have length.
When used it specifies the maximum possible byte length of an integer/byte array/string or number of array elements.
So effectively, map's length is the number of key-value pairs in this map. @roman-khimov, what do you think?
| if (Type == ContractParameterType.Array && NamedType is null && (Fields is null || Fields.Length == 0)) | ||
| throw Nep25Error("value, namedtype or fields must be provided for Array type to describe element type."); |
There was a problem hiding this comment.
I think it's allowed to have named structure with zero number of fields. It's an edge-case, but it's not prohibited by the standard directly.
There was a problem hiding this comment.
Done, please check the last changes
| if (Length.HasValue || ForbidNull.HasValue || Interface.HasValue || Key.HasValue || Value is not null || (Fields is not null && Fields.Length > 0)) | ||
| throw Nep25Error("namedtype cannot be combined with other modifiers."); |
There was a problem hiding this comment.
Ditto here, looks like Fields is not null && Fields.Length > 0 check may be reduced to Fields is not null.
| if (Fields is not null && Fields.Length > 0) | ||
| throw Nep25Error("value and fields cannot be used together."); |
Description
Implement NEP-25 neo-project/proposals#160 (review)
Replace of #3272
Type of change
How Has This Been Tested?
Test Configuration:
Checklist: