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

Add UInt160, UInt256 and ECPoint to SmartContract Framework #362

Merged
merged 44 commits into from
Nov 10, 2020
Merged

Add UInt160, UInt256 and ECPoint to SmartContract Framework #362

merged 44 commits into from
Nov 10, 2020

Conversation

devhawk
Copy link
Contributor

@devhawk devhawk commented Sep 19, 2020

fixes #208, part of neo-project/neo#1866
Obsoletes #347

This PR introduces Neo.UInt160, Neo.UInt256 and Neo.Cryptography.ECC.ECPoint to the Neo.SmartContract.Framework assembly. Note, these type namespaces were explicitly chosen to match their corresponding types in Neo.dll. Over the next few weeks, I'll be introducing additional PRs to further unify types in Neo and Neo.SmartContract.Framework as part of neo-project/neo#1866.

Because these types in the two assemblies have the same fully qualified namespace, an assembly that needs to reference Neo and Neo.SmartContract.Framework needs to use a reference alias to disambiguate. I've added appropriate reference aliases in Neo.Compiler.MSIL and the relevant unit tests projects.

Under the hood, these types are simple byte arrays, like how strings are handled in Neo smart contracts. These types are merely there to provide a small amount of compile time type safety + familiarity to the developer. Relevant service methods (such as Runtime.CheckWitness) and domain properties (such as Block.Hash) have been updated where appropriate to be strongly typed as UInt160, UInt256 or ECPoint.

Assorted Notes:

  • SmartContract Framework types that wrap native contracts (such as NEO and Oracle) have been updated to expose their script hash as a static Hash property. To avoid declaring the script hash twice, NEON has been updated to look for a new NativeContractHashAttribute as a marker for the appropriate method to return the contract script hash.
  • ConvertNewObj in NEON has been updated to support the default and byte array constructors of UInt160 and UInt256. Default constructor pushes a byte array of appropriate size onto to stack. Byte array constructors inject VM code to validate the byte array on the evaluation stack is of the expected size
  • ECPoint in SmartContract.Framework has neither default or byte array constructors. I wasn't sure if these types were ever constructed from byte arrays in contracts. It would be straightforward to include these constructors similar to UInt160/UInt256's constructors.
  • UInt160/UInt256 each have a static Zero property that pushes a zero array of appropriate length onto the stack via OpCodeAttribute
  • FuncExport.ConvType recognizes the new types and returns the appropriate ContractParameterType value as a string
  • ModuleConverter.IsContractCall was updated to return contract hash out parameter as UInt160 instead of a byte array

@devhawk
Copy link
Contributor Author

devhawk commented Sep 19, 2020

@ShawnYun GitHub wouldn't let me request you as an official reviewer. Please review this PR if you have a chance.

@devhawk devhawk changed the title Devhawk/unify1 Add UInt160, UInt256 and ECPoint to SmartContract Framework Sep 19, 2020
@devhawk devhawk mentioned this pull request Sep 22, 2020
44 tasks
@erikzhang
Copy link
Member

We should add ByteString too.

@devhawk
Copy link
Contributor Author

devhawk commented Nov 2, 2020

@shargon Shouldn't IsEmpty be called IsZero? UInts and ECPoints aren't really ever empty, right?

@devhawk devhawk dismissed stale reviews from Tommo-L and shargon via d7509e3 November 2, 2020 16:28
Tommo-L
Tommo-L previously approved these changes Nov 3, 2020
shargon
shargon previously approved these changes Nov 3, 2020
src/Neo.SmartContract.Framework/UInt160.cs Outdated Show resolved Hide resolved
src/Neo.SmartContract.Framework/UInt160.cs Outdated Show resolved Hide resolved
src/Neo.SmartContract.Framework/UInt256.cs Outdated Show resolved Hide resolved
src/Neo.SmartContract.Framework/UInt256.cs Outdated Show resolved Hide resolved
erikzhang
erikzhang previously approved these changes Nov 8, 2020
@devhawk
Copy link
Contributor Author

devhawk commented Nov 9, 2020

Am I supposed to squash and merge this PR now that it's approved?

@ProDog
Copy link
Contributor

ProDog commented Nov 10, 2020

Am I supposed to squash and merge this PR now that it's approved?

I have tested it again, calling all methods using the contract, everything is normal, except for one place it should be UnregisterCandidate rather than UnRegisterCandidate, needs to be consistent with Neo core.
Maybe we can fix it in another PR, we need merge this as soon as possible.

@erikzhang erikzhang merged commit 6051662 into neo-project:master Nov 10, 2020
@devhawk devhawk deleted the devhawk/unify1 branch November 10, 2020 22:26
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.

Add UInt160 and UInt256 data types
7 participants