search

0xPolygonMiden / miden-vm

STARK-based virtual machine
MIT License
632 stars 161 forks source link

Fix `Program` deserialization #1543

Closed Fumuran closed 1 month ago

Fumuran commented 1 month ago

This small PR fixes a bug in the Program deserialization. It also adds some integration tests to check the correctness of the serialization and deserialization of the Program structure.

Fumuran commented 1 month ago

The main question is where we should locate the tests for the Program serde. I think that it would be better to construct the Program from source string to make sure we don't have any bug on the stage of creation of the original Program instance, but in that case we should be able to use Assembler and test-utils. I decided to put tests in the miden integration tests folder, but not sure that this is the best option.

bobbinth commented 1 month ago

The main question is where we should locate the tests for the Program serde. I think that it would be better to construct the Program from source string to make sure we don't have any bug on the stage of creation of the original Program instance, but in that case we should be able to use Assembler and test-utils. I decided to put tests in the miden integration tests folder, but not sure that this is the best option.

Could it not go into the assembly crate?

Fumuran commented 1 month ago

Could it not go into the assembly crate?

Essentially we are testing the functionality of the Program structure, which is part of the core, so it's strange for me to add tests to the assembly. But it is strange to add it anywhere, so probably we just should chose where it will suit better. I can move it to the assembly if you think that it will be better.

bobbinth commented 1 month ago

Essentially we are testing the functionality of the Program structure, which is part of the core, so it's strange for me to add tests to the assembly. But it is strange to add it anywhere, so probably we just should chose where it will suit better. I can move it to the assembly if you think that it will be better.

Agreed that ideally it should be in core - but I think putting it into assembly simplifies testing - so, let's put it there.