Skip to content
This repository has been archived by the owner on Oct 31, 2023. It is now read-only.

feat!: serialise RAM and ROM opcodes #153

Merged
merged 9 commits into from
May 15, 2023
Merged

feat!: serialise RAM and ROM opcodes #153

merged 9 commits into from
May 15, 2023

Conversation

guipublic
Copy link
Collaborator

@guipublic guipublic commented May 3, 2023

This PR serialise RAM and ROM opcodes from ACIR into BB acir format.

BEGIN_COMMIT_OVERRIDE
feat!: Add serialization logic for RAM and ROM opcodes (#153)
END_COMMIT_OVERRIDE

@TomAFrench
Copy link
Member

TomAFrench commented May 5, 2023

Can we add a couple of test cases similar to this for a simple block? Ideally we'd have an test for each constraint type so we don't rely on noir integration tests to check we're handling them correctly here.

@guipublic
Copy link
Collaborator Author

I added a unit test, it is failing because it cannot locate barretenberg so I cannot test it, but anyways the relevant barretenberg PR is not merge yet.

@TomAFrench
Copy link
Member

TomAFrench commented May 11, 2023

Looks good. We should be good to merge once the conflicts are resolved.

Huh, looks like the upstream BB PR hasn't been merged yet. Strange that this PR's test is passing in that case.

@guipublic
Copy link
Collaborator Author

Huh, looks like the upstream BB PR hasn't been merged yet. Strange that this PR's test is passing in that case.

I suppose it is because I added the new data at the end when serialising, so if you read less, you still get valid data.

@guipublic guipublic requested a review from vezenovm May 12, 2023 15:32
@TomAFrench
Copy link
Member

I've bumped the BBerg version to include the relevant PRs so the test should be actually testing stuff now.

@TomAFrench
Copy link
Member

It would be good to see a commit which breaks the blocks test case (e.g. use a witness which doesn't satisfy the block constraints) to show that this test is meaningful and then revert it.

@vezenovm
Copy link
Collaborator

It would be good to see a commit which breaks the blocks test case (e.g. use a witness which doesn't satisfy the block constraints) to show that this test is meaningful and then revert it.

Or just leave it in and show verify false

@vezenovm
Copy link
Collaborator

Just a small nit

@guipublic
Copy link
Collaborator Author

It would be good to see a commit which breaks the blocks test case (e.g. use a witness which doesn't satisfy the block constraints) to show that this test is meaningful and then revert it.

Or just leave it in and show verify false

I have added a failing test

src/barretenberg_structures.rs Show resolved Hide resolved
@guipublic guipublic added this pull request to the merge queue May 15, 2023
Merged via the queue into master with commit 3d3847d May 15, 2023
6 checks passed
@guipublic guipublic deleted the gd/up_ram branch May 15, 2023 15:49
@kobyhallx kobyhallx mentioned this pull request May 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants