added UpdatePc tests #61
Conversation
I think they are the same, because when jzn -> res = op1 |
Offsets can be negative, but it is express according to Field Elements for example, -1 would be: Because operations with felt are made modulus a prime P where P = x + 1. Then x + 3 = 2, for example |
|
Don't worry about negative offset nonetheless, I am fixing them as part of a feature I am working on |
Do you mean in the update res part of the pseudo code res = op1? If so, isn't that when pc_update is case 0,1 or 2? |
You were right from the beginning, it was a bug, good catch! I think it should be already patched in |
* added UpdatePc tests and fixed a bug in UpdatePc * Fixed accessing field value without Read() * fixed failing tests * small refactor for TestUpdatePcJump
* edited comments todo list * verify Compute Result Addition via more test results. Minor update on add function on memory_value.go * Verify Test for ComputeRes Addition * improve better format for test function * separate each cases to have own functions * add Mulitple operand test cases and also Unconstrained, Op1 * wip memory addition * Fixes add function with felt-to-address addition * added UpdatePc tests (#61) * added UpdatePc tests and fixed a bug in UpdatePc * Fixed accessing field value without Read() * fixed failing tests * small refactor for TestUpdatePcJump * Chore: Readme hotfix * separate each cases to have own functions * Fix as per comments of prev. PR. Fixes issue #56 adding test cases and code change * integration test done * minor change on comments * Remove factorial_compiled.json * Fix add implementation --------- Co-authored-by: M. Mahdi Khosravi <mmk1776@gmail.com> Co-authored-by: Rodrigo <rodrodpino@gmail.com>
I have added 5 tests for UpdatePc function. I could not add tests with negative offsets because the offset should be uint64. In case there is any way to address this, please point it out and I'll add negative offset test variants too.
Also while I was writing tests, I noticed a potential bug. Based on the Cairo whitepaper, in the case of JNZ (conditional relative jump), the next_pc should be
pc + op1notpc + res, so I fixed that too. If I understood incorrectly please lmk.