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

Add support for static entropy #140

Merged
merged 1 commit into from
Oct 10, 2023
Merged

Add support for static entropy #140

merged 1 commit into from
Oct 10, 2023

Conversation

dr-orlovsky
Copy link
Member

We have to target it for v0.11 since it breaks APIs - and there is no way of adding support for the feature without the API break

@dr-orlovsky dr-orlovsky added feature New feature request refactoring Refactoring of the existing code labels Oct 8, 2023
@dr-orlovsky dr-orlovsky requested a review from zoedberg October 8, 2023 10:05
@dr-orlovsky dr-orlovsky added this to the v0.11.0 milestone Oct 8, 2023
@codecov
Copy link

codecov bot commented Oct 8, 2023

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Files Coverage Δ
commit_verify/src/mpc/tree.rs 98.6% <100.0%> (+0.1%) ⬆️
commit_verify/src/mpc/atoms.rs 69.9% <0.0%> (-7.4%) ⬇️

📢 Thoughts on this report? Let us know!.

Copy link
Member

@zoedberg zoedberg 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 but there are some warnings that may be addressed:

warning: unnecessary `pub(self)`
  --> commit_verify/src/lib.rs:48:1
   |
48 | pub(self) mod commit;
   | ^^^^^^^^^ help: remove it
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_pub_self
   = note: `#[warn(clippy::needless_pub_self)]` on by default

warning: unnecessary `pub(self)`
  --> commit_verify/src/lib.rs:51:1
   |
51 | pub(self) mod embed;
   | ^^^^^^^^^ help: remove it
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_pub_self

warning: methods with the following characteristics: (`to_*` and `self` type is `Copy`) usually take `self` by value
   --> commit_verify/src/mpc/block.rs:123:27
    |
123 |     pub fn to_merkle_node(&self) -> MerkleNode {
    |                           ^^^^^
    |
    = help: consider choosing a less ambiguous name
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
    = note: `#[warn(clippy::wrong_self_convention)]` on by default

As said in BP-WG/bp-core#56 (comment) I think clippy should fail when there are warnings

@cryptoquick
Copy link
Member

This should help with unit testing, I would think

@dr-orlovsky
Copy link
Member Author

@zoedberg already fixed in previously merged PRs

Copy link
Member

@zoedberg zoedberg left a comment

Choose a reason for hiding this comment

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

LGTM

@dr-orlovsky dr-orlovsky merged commit d9161f0 into master Oct 10, 2023
23 checks passed
@dr-orlovsky dr-orlovsky deleted the static_entropy branch September 4, 2024 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature request refactoring Refactoring of the existing code
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants