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

Proposal of VM-UT #80

Merged
merged 8 commits into from
Feb 28, 2019
Merged

Proposal of VM-UT #80

merged 8 commits into from
Feb 28, 2019

Conversation

shargon
Copy link
Member

@shargon shargon commented Feb 24, 2019

  • Added 2 json

I will add more json here if we decide that this is the right way

@igormcoelho
Copy link
Contributor

Will review asap @shargon, congratulations for the initiative!

erikzhang
erikzhang previously approved these changes Feb 27, 2019
@shargon
Copy link
Member Author

shargon commented Feb 27, 2019

@erikzhang ,BREAK state should be only when hit a breakpoint?

Copy link
Contributor

@igormcoelho igormcoelho left a comment

Choose a reason for hiding this comment

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

@shargon I noticed you ported cryptography basics to NeoVM. Can we remove all this code and rely only on native C# implementation?

P.S.: I couldn't take enough time to review it in details yet, but I'm loving the idea of having JSON basis for tests... this is exactly what we need to achieve cross-compatibility between vm implementations. 1 thousand congratulations for you, for the nice idea!

@erikzhang
Copy link
Member

BREAK state should be only when hit a breakpoint?

StepInto(), StepOut(), StepOver() or hitting a breakpoint.

@shargon
Copy link
Member Author

shargon commented Feb 28, 2019

@igormcoelho i know, i don't like too, but the other way is include neo package, and this package have neo-vm dependency... the solution should move this code to other project or to neo-vm, so i copy this code to test :P

using System.Linq;
using System.Security.Cryptography;
using System.Threading;
using Neo.Test.Cryptography;
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove this Neo.Test.Cryptography include, since it looks like only RIPEMD160Managed is included, right?
I checked the implementation of RIPEMD160Managed and it is equivalent to the standard one in System.Security.Cryptography. So, we can use standard, the same way we do for SHA256.

Looks like ECC code pushed here is on Neo.Cryptography.ECC namespace, which is also not imported anywhere. So it can also be safely removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just found it... you recreated Crypto lib too...

Ok then. Let's approve this first, then we decide how to deal with this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@igormcoelho igormcoelho Feb 28, 2019

Choose a reason for hiding this comment

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

oh, I didn't know that... O.o ok, one more reason to leave that for now. In the future we come back to this discussion.

@shargon shargon merged commit fabeb9f into neo-project:master Feb 28, 2019
@shargon shargon deleted the vm-json-ut branch February 28, 2019 18:44
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.

3 participants