Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Program deserialization #1543

Merged
merged 2 commits into from
Oct 22, 2024
Merged

Fix Program deserialization #1543

merged 2 commits into from
Oct 22, 2024

Conversation

Fumuran
Copy link
Contributor

@Fumuran Fumuran commented Oct 22, 2024

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 Fumuran added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label Oct 22, 2024
@Fumuran
Copy link
Contributor Author

Fumuran commented Oct 22, 2024

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.

@Fumuran Fumuran requested a review from bobbinth October 22, 2024 13:18
@bobbinth bobbinth requested a review from plafer October 22, 2024 15:59
@bobbinth
Copy link
Contributor

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
Copy link
Contributor Author

Fumuran commented Oct 22, 2024

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
Copy link
Contributor

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.

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you!

@bobbinth bobbinth merged commit 2a97d8d into next Oct 22, 2024
9 checks passed
@bobbinth bobbinth deleted the andrew-fix-program-serde branch October 22, 2024 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog This PR does not require an entry in the `CHANGELOG.md` file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants