Move VM Limits to NeoVM #75
Conversation
- TODO: `MaxStackSize`
|
Amazing change Shargon, Im reviewing.. |
There was a problem hiding this comment.
Finished the review @shargon! Sorry for the many comments, most are related to a single issue... so don't worry about of all them exactly... once reviewing we will decide how to solve the possible inconsistencies. Congratulations for the amazing work, as always!
igormcoelho
dismissed
their stale review
Nice changes, I'll dismiss my current review, and re-review, with new ideas on mind. Now, I'll consider that CheckStackSize automatically increases variable.
|
@erikzhang any idea how we can remove |
|
I have not found a good way to do this for the time being. |
|
@erikzhang I want to add these unit tests https://github.com/CityOfZion/neo-sharp/tree/development/test/NeoSharp.VM.Test/Tests/OpCodes but these tests was prepared for check the Gas cost, scripts, storages. Shall i put this test in neo project or simplify the tests? |
|
Can we remove the gas related content from the test? |
|
Yes, i will! |
|
Let's do some test and merge it. |
|
I'll take a look tomorrow night, to re-review the improved logics. |
|
@shargon Tests failed. |
|
@shargon Is it ready to merge? |
|
Is ready for me, i want to add more UT, but in other PR, will be great if we can configure a coverage report like codecov. |
Related to #73
MaxStackSize