Skip to content
This repository has been archived by the owner on Apr 9, 2024. It is now read-only.

chore!: Remove manual serialization of Opcodes in favour of serde #286

Merged
merged 3 commits into from
May 19, 2023

Conversation

kevaundray
Copy link
Contributor

Related issue(s)

(If it does not already exist, first create a GitHub issue that describes the problem this Pull Request (PR) solves before creating the PR and link it here.)

Resolves (link to issue)

Description

Discussed internally: We originally wanted an encoding which could be implemented in other languages easier. I thought that having a manual encoding and then specifying this in some documentation would have been the best way for this. Even if this was the case, we have had two serialization bugs and the effort for folks to modify this code has become unproductive.

There are still solutions to the problem of having other languages implement a deserialization strategy, they can choose to use rmp/json for example, which is available in most languages. For now, we use rmp by default with the read and write methods.

Summary of changes

(Describe the changes in this PR. Point out breaking changes if any.)

Dependency additions / changes

(If applicable.)

Test additions / changes

(If applicable.)

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Additional context

(If applicable.)

@kevaundray
Copy link
Contributor Author

Closes #228

@kevaundray kevaundray requested a review from TomAFrench May 16, 2023 20:19
@vezenovm vezenovm mentioned this pull request May 16, 2023
5 tasks
@vezenovm
Copy link
Contributor

I think you can remove to_u16() and from_u16() in black box func file

@kevaundray kevaundray marked this pull request as ready for review May 17, 2023 17:16
@kevaundray
Copy link
Contributor Author

I think you can remove to_u16() and from_u16() in black box func file

This was there prior to the manual serialization work so we probably want to check if this is being used in other places.

@vezenovm
Copy link
Contributor

This was there prior to the manual serialization work so we probably want to check if this is being used in other places.

I see it only being used in the consistent_index test in black_box_functions.rs

@kevaundray
Copy link
Contributor Author

This was there prior to the manual serialization work so we probably want to check if this is being used in other places.

I see it only being used in the consistent_index test in black_box_functions.rs

Checked Noir too and they are not being used, so I removed it

@TomAFrench TomAFrench changed the title chore!: Remove manual serialization for Serde chore!: Remove manual serialization of Opcodes in favour of serde May 19, 2023
@TomAFrench TomAFrench added this pull request to the merge queue May 19, 2023
Merged via the queue into master with commit 8a3812f May 19, 2023
@github-actions github-actions bot mentioned this pull request May 19, 2023
@TomAFrench TomAFrench deleted the kw/back-to-serde branch May 19, 2023 08:30
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.

3 participants