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

feat(sc): Improve ContractParam with fromJson #614

Merged
merged 8 commits into from
Oct 31, 2020

Conversation

snowypowers
Copy link
Member

Rework how ContractParam construction works. Introduce fromJson and restructure the methods to have clear separation of logic. Static methods are now the preferred entry points with fromJson being the common entrypoint.

  • Introduce ContractParamJson which is the direct json output coming from c#.
  • Reduce constructor to type validation.
  • Move parsing and transformation logic into static methods.
  • Add fromJson method as a main entrypoint from external systems.
  • Rework ContractParamLike to be a union of the class and json format.
  • Add tests for parseEnum.

@snowypowers snowypowers requested a review from ixje October 27, 2020 08:00
@snowypowers
Copy link
Member Author

@ixje this pulls out the ContractParam changes from the SmartContract PR. Please take a look.

}

/**
* Creates a Hash160 ContractParam. This is used for containing a scriptHash. Do not reverse the input if using this format.
* Creates a Hash160 ContractParam. This is used for containing a scriptHash. Value field will be a HexString.
* Do not reverse the input if using this format.
Copy link
Member

Choose a reason for hiding this comment

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

I find this a confusing remark, but that probably has to do with neon-js reversing byte arrays in many places I don't expect it to. For clarification purposes only; can you give the rules when neon-js does and does not expect a reversed hex string?

Copy link
Member Author

Choose a reason for hiding this comment

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

Usually it is big endian. In this case, a Hash160's value is parsed as bigendian on C# side while a ByteArray is not. But the rule of thumb in neon-js has always been to keep moving big endian around.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry it is still ambiguous for me. What do you call big endian in this context? I ask this because the NEO C# team has this crazy idea that adding a hexadecimal prefix (0x) suddenly changes the endianness of the data. I couldn't blame you if you follow their convention, but because it deviates from the standard I'm still confused what it means

But the rule of thumb in neon-js has always been to keep moving big endian around.

Does this mean that you do not reverse Hash160/Hash256 types (UInt160/UInt256 in C#) but reverse everything else?

Copy link
Member Author

Choose a reason for hiding this comment

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

The reference case is usually the ByteArray used in C#. This is little-endian. So we extrapolate from this case to all other instances. Im mentioning the reversing because in NEO2, the convention was ContractParam.byteArray(reverseHex(getScriptHashFromAddress(address))).

In neon-js the 0x prefix is usually discarded. Im aware of the convention but currently not adopting it.

What is your definition of the standard?

Copy link
Member

Choose a reason for hiding this comment

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

What is your definition of the standard?

In general my definition is that the only time I care about endianness is when reading primitive types from a byte array where the primitive type is 2 or more bytes in size (e.g. short, int, long etc). Putting this in the NEO context and speaking from my Python project (which is a full port from C#), there are only 2 places in the whole code base where I reverse the data and they only happen for the UInt160 and UInt256 types.

  • from string - taking a UInt160/UInt256 string representation produced by the C# code
  • to string - making sure it produces the same string representation as the C# code (because many other tools expect this format)

Everything else internally can be said to be little endian. So my binary reader/writer classes read all data by default in little endian format. Which is why I previously mentioned that everything in neon-js seems to be in reversed format for me, because your representation seems to be big endian by default. :)

Copy link
Member

@ixje ixje left a comment

Choose a reason for hiding this comment

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

I mostly focused on feature completion. Having said that I noticed the following;

  • hash256 - which I expect to be commonly used for contracts that wish to operation on a specific block or transaction.
    • has no static creation method
    • is not supported in the normal constructor
    • is not supported in fromJSON
  • map - I actually don't expect any contract to use this over an array type. It's going to be more expensive to push the data for no gain other than maybe readability. Regardless, if we want to be complete then
    • the same 3 as hash256 apply here as well, additionally
    • is not supported in toJSON (this is known given the TODO in L: 352
  • signature - never actually seen this used as a param, but for completeness
    • has no static creation method
    • is not supported in fromJSON
    • no explicit support in equals, although maybe the default case is sufficient?
  • interop - I think we can ignore this. C# also doesn't support it in its own constructor, ToJson and FromJSON methods

packages/neon-core/src/sc/ContractParam.ts Outdated Show resolved Hide resolved
@snowypowers
Copy link
Member Author

I have added Hash256 support. I decided to drop Any, Map and Signature support for the moment since they are lesser used and im not too confident how they look like.

@ixje
Copy link
Member

ixje commented Oct 29, 2020

I have added Hash256 support. I decided to drop Any, Map and Signature support for the moment since they are lesser used and im not too confident how they look like.

I think that's a good call 👍

@snowypowers snowypowers merged commit f9ff5ef into CityOfZion:next Oct 31, 2020
@snowypowers snowypowers deleted the contractparam branch October 31, 2020 02:13
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.

2 participants