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.

Sign up for GitHub

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

Jump to bottom

feat: impl Deserialize on Program by changing builtins field type to enum #896

Merged
merged 1 commit into from
Mar 14, 2023
Merged

feat: impl Deserialize on Program by changing builtins field type to enum #896

merged 1 commit into from
Mar 14, 2023

Conversation

tdelabro
Copy link
Contributor

@tdelabro tdelabro commented Mar 13, 2023 •
edited
Loading

close #895

@tdelabro tdelabro force-pushed the deserialize-program branch from b8af962 to d838ff6 Compare March 13, 2023 17:49
@tdelabro
Copy link
Contributor Author

@fmoletta I think this is what you described. wdyt?

@codecov
Copy link

codecov bot commented Mar 13, 2023

Codecov Report

Merging #896 (d838ff6) into main (2ec04a4) will increase coverage by 0.00%.
The diff coverage is 98.52%.

@@           Coverage Diff           @@
##             main     #896   +/-   ##
=======================================
  Coverage   97.46%   97.47%           
=======================================
  Files          69       69           
  Lines       29070    29071    +1     
=======================================
+ Hits        28333    28336    +3     
+ Misses        737      735    -2
Impacted Files Coverage Δ
src/vm/runners/cairo_runner.rs 97.51% <98.13%> (+0.03%) ⬆️
src/serde/deserialize_program.rs 97.87% <100.00%> (+0.08%) ⬆️
src/types/program.rs 98.70% <100.00%> (ø)
src/utils.rs 99.48% <100.00%> (ø)
src/vm/runners/builtin_runner/bitwise.rs 93.34% <100.00%> (ø)
src/vm/runners/builtin_runner/ec_op.rs 96.70% <100.00%> (ø)
src/vm/runners/builtin_runner/hash.rs 95.20% <100.00%> (ø)
src/vm/runners/builtin_runner/mod.rs 99.31% <100.00%> (ø)
src/vm/runners/builtin_runner/range_check.rs 96.45% <100.00%> (ø)
src/vm/security.rs 98.43% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

fmoletta
fmoletta approved these changes Mar 13, 2023
View reviewed changes
Copy link
Contributor

@fmoletta fmoletta left a comment

Choose a reason for hiding this comment

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

LGTM, left a small comment

Oppen
Oppen reviewed Mar 14, 2023
View reviewed changes
Copy link
Contributor

@Oppen Oppen left a comment

Choose a reason for hiding this comment

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

Minor comment, but otherwise LGTM.

@Oppen Oppen added this pull request to the merge queue Mar 14, 2023
Merged via the queue into lambdaclass:main with commit e32e435 Mar 14, 2023
@tdelabro tdelabro deleted the deserialize-program branch March 15, 2023 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmoletta fmoletta fmoletta approved these changes

@Oppen Oppen Oppen approved these changes

@igaray igaray Awaiting requested review from igaray igaray is a code owner

@entropidelic entropidelic Awaiting requested review from entropidelic

@HermanObst HermanObst Awaiting requested review from HermanObst

@juanbono juanbono Awaiting requested review from juanbono juanbono is a code owner

@Jrigada Jrigada Awaiting requested review from Jrigada

@pefontana pefontana Awaiting requested review from pefontana pefontana is a code owner

@Juan-M-V Juan-M-V Awaiting requested review from Juan-M-V

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Derive Deserialize on Program
3 participants
@tdelabro @Oppen @fmoletta