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: Felt serde #883

Merged
merged 10 commits into from
Aug 7, 2024
Merged

fix: Felt serde #883

merged 10 commits into from
Aug 7, 2024

Conversation

igamigo
Copy link
Contributor

@igamigo igamigo commented Jul 22, 2024

Felt serde fix

(Tested with Rust 1.79)
The prove-miden example was not compiling correctly due to a serde problem with FieldElement.

Description

Fixes compilation by making the type explicit to &str so that the type is Sized and it does not fail based on the serde bounds.

To repro the problem on main: cd examples/prove-miden && cargo run

Type of change

  • Bug fix

Checklist

  • Linked to Github Issue
  • Unit tests added
  • This change requires new documentation.
    • Documentation has been added/updated.
  • This change is an Optimization
    • Benchmarks added/run

@igamigo igamigo requested a review from a team as a code owner July 22, 2024 11:23
@codecov-commenter
Copy link

codecov-commenter commented Jul 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.05%. Comparing base (b2931c3) to head (4aa16e4).
Report is 7 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #883   +/-   ##
=======================================
  Coverage   72.05%   72.05%           
=======================================
  Files         149      149           
  Lines       33408    33408           
=======================================
  Hits        24073    24073           
  Misses       9335     9335           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Oppen
Copy link
Member

Oppen commented Jul 22, 2024

Can we add the examples to the compile pipeline?

@@ -11,7 +11,7 @@ path = "src/main.rs"

[dependencies]
clap = { version = "4.4.6", features = ["derive"] }
lambdaworks-crypto = { workspace = true }
lambdaworks-crypto = { workspace = true, features = ["serde"]}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This came up when building all other examples. The crate relies on serde and it fails to compile if it's not added.

@igamigo igamigo requested a review from Oppen July 22, 2024 15:04
@igamigo igamigo added this pull request to the merge queue Aug 5, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 5, 2024
@Oppen Oppen added this pull request to the merge queue Aug 6, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 6, 2024
@Oppen
Copy link
Member

Oppen commented Aug 6, 2024

@igamigo it seems the compiler updated and now there are new lints. The failing pipelines are due to missing indentation in some doc comments in the fft implementation.

@diegokingston diegokingston added this pull request to the merge queue Aug 7, 2024
Merged via the queue into main with commit e09040f Aug 7, 2024
7 checks passed
@diegokingston diegokingston deleted the igamigo-fix-serde branch August 7, 2024 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants