Skip to content
This repository has been archived by the owner on Nov 15, 2021. It is now read-only.

Add VerificationContract + tests #123

Merged
merged 8 commits into from Dec 21, 2017
Merged

Add VerificationContract + tests #123

merged 8 commits into from Dec 21, 2017

Conversation

ixje
Copy link
Member

@ixje ixje commented Dec 20, 2017

Add IO Helper utility needed in more classes to come

@property
def IsStandard(self):
"""
XXXUnknown.
Copy link
Member Author

Choose a reason for hiding this comment

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

It seems to do sanity checking on the Script, which makes me think it's more like a IsValid check. Eitherway, I don't know what could generate a 'non-standard'. If you have a good description please let me know

Copy link
Collaborator

Choose a reason for hiding this comment

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

Any smart contract associated with an address is 'non-standard'

)

@property
def IsStandard(self):
Copy link
Member Author

Choose a reason for hiding this comment

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

#overwriting Contract.IsStandard because Script should not need to be unhexlified

return True

@property
def ScriptHash(self):
Copy link
Member Author

Choose a reason for hiding this comment

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

note I'm overriding this method from the VerificationCode class because self.Script should not be a hex string. I'm being strict on data types to prevent mismatches in the future (which i've already encountered quite some).

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 68.51% when pulling 0178089 on ixje:feature-consensus into 4554cbb on CityOfZion:feature-consensus.

expected_address = 'AWLYWXB8C9Lt1nHdDZJnC5cpYJjgRDLk17'
expected_isStandard = True
# should we make this a proper ENUM like C#?
expected_ParameterList = [bytes([ContractParameterType.Signature])]
Copy link
Member Author

Choose a reason for hiding this comment

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

Need an opinion on the ContractParameter type. It's currently a class, but I think a proper enum could do as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

FYI; it's actually an enum in C#

@localhuman
Copy link
Collaborator

Does VerificationContract do anything different or more than neo.SmartContract.Contract ?

@ixje
Copy link
Member Author

ixje commented Dec 20, 2017

Yes,
self.Script are actual bytes instead of a hexstring encapsulted in a bytes() object. This matches matches the C# reference closer and prevents future issues with data type mismatches.
self.ParameterList is actually a list now instead of just a bytes object. This also required changing the serialization parts. Again reflects C# closer.
Size is implemented

@localhuman
Copy link
Collaborator

If you plan on replacing the existing neo.SmartContract.Contract implementation ( Which is how the c# was originally implemented :) ) please be aware that there is a lot of complexity involved with contract invocations, multi sig contracts, etc.
This is double the case for changing ContractParameterType

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 68.424% when pulling 207f257 on ixje:feature-consensus into 4554cbb on CityOfZion:feature-consensus.

@ixje
Copy link
Member Author

ixje commented Dec 21, 2017

I hope to not touch neo.SmartContract.Contract, and as far as I can see now it's not needed. I simply operate on the fact that in the past when I asked for refactoring you said you want to stay as close to the C# implementation as possible. In the end it's not my call, so if you think the current implementation is wrong and should be changed then just let me know what I should do and I'll try to update and work around it.

@localhuman
Copy link
Collaborator

Looks good. I just worry about having very similar functionality implemented twice, once in VerificationContract and the other in Contract. If we can get rid of the neo.SmartContract.Contract implementation and follow the C# implementation closer that would be ideal. My only hesitation is that this has implications throughout the codebase.

If possible for now, we should keep ContractParameterType as it is.

@localhuman localhuman merged commit b60a021 into CityOfZion:feature-consensus Dec 21, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants