zkevm-circuits icon indicating copy to clipboard operation
zkevm-circuits copied to clipboard

add unit tests for stackOnlyOp opcode

Open SuccinctPaul opened this issue 3 years ago • 5 comments

Add some stackOnlyOp unit tests. see more on #947

SuccinctPaul avatar Nov 26 '22 13:11 SuccinctPaul

Hi @ChengYueJia is this ready for review? If not, could you leave as Draft until you have the PR ready?

Also, IIRC all of the opcodes that are already implemented as StackOpcode have tests associated. At least things like: 41 | COINBASE 42 | TIMESTAMP 43 | NUMBER 46 | CHAINID

These al have tests associated already. So could you be more specific and review excatly if all the mentioned opcodes require indeed testing?

CPerezz avatar Nov 28 '22 13:11 CPerezz

Also, IIRC all of the opcodes that are already implemented as StackOpcode have tests associated. At least things like: 41 | COINBASE 42 | TIMESTAMP 43 | NUMBER 46 | CHAINID These al have tests associated already. So could you be more specific and review excatly if all the mentioned opcodes require indeed testing?

@CPerezz Thanks for your reminder. I should submit an issue before pr. I've submitted the issue #947 to specify the needed testing.

Hi @ChengYueJia is this ready for review? If not, could you leave as Draft until you have the PR ready?

After specify the need testing, I'll finish it. Before all of that, this will be left as a draft.

SuccinctPaul avatar Nov 29 '22 03:11 SuccinctPaul

@CPerezz Hi, there are completed. Please have a view.

SuccinctPaul avatar Nov 30 '22 12:11 SuccinctPaul

I've added the mock var. Please have another view.

SuccinctPaul avatar Dec 03 '22 06:12 SuccinctPaul

Hi @icemelon can you help us add 1 reviewer from your team? Thanks!

@ChengYueJia FYI needs to be rebased

andyguzmaneth avatar Dec 12 '22 20:12 andyguzmaneth