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

Require result of verify functions to be used #143

Merged
merged 1 commit into from
Oct 16, 2023

Conversation

dr-orlovsky
Copy link
Member

@dr-orlovsky dr-orlovsky commented Oct 14, 2023

Will switch to just Result<(), VerifyError> in v0.11

@dr-orlovsky dr-orlovsky added the enhancement New feature or request label Oct 14, 2023
@dr-orlovsky dr-orlovsky added this to the v0.10.x milestone Oct 14, 2023
cryptoquick
cryptoquick previously approved these changes Oct 14, 2023
Copy link
Member

@cryptoquick cryptoquick left a comment

Choose a reason for hiding this comment

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

ACK

@codecov
Copy link

codecov bot commented Oct 14, 2023

Codecov Report

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

Files Coverage Δ
commit_verify/src/commit.rs 80.8% <ø> (ø)
commit_verify/src/convolve.rs 86.7% <ø> (ø)
commit_verify/src/embed.rs 97.7% <ø> (ø)
commit_verify/src/mpc/tree.rs 98.6% <100.0%> (+0.1%) ⬆️
commit_verify/src/mpc/atoms.rs 36.8% <0.0%> (-5.2%) ⬇️

📢 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.

Build fails with:

error[E0063]: missing field `static_entropy` in initializer of `MultiSource`
   --> /home/zoe/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rgb-wallet-0.10.9/src/psbt/dbc.rs:119:22
    |
119 |         let source = mpc::MultiSource {
    |                      ^^^^^^^^^^^^^^^^ missing `static_entropy`

@dr-orlovsky
Copy link
Member Author

@zoedberg you posted this to a wrong PR

But that PR is targeted for v0.11 so of course it won't compile with 0.10 stack

@zoedberg
Copy link
Member

@dr-orlovsky Sorry, I haven't noticed that! Are you planning to apply this fix also to the 0.10 version?

@dr-orlovsky
Copy link
Member Author

Which fix? From this PR or from the static entropy?

@zoedberg
Copy link
Member

I'm confused. I was talking about this PR. I've patched commit_verify in rgb-lib like this:

commit_verify = { git = "https://github.com/LNP-BP/client_side_validation", branch = "fix/verify_not_used" }

and received the build error I posted here

@dr-orlovsky
Copy link
Member Author

@zoedberg All right, I see. My bad. So I did start master for 0.11 - and did this PR on top of that changes coming from the PRs which were targeting v0.11 and were already merged into the master.

Thus, this PR must be against v0.10 branch and not master. Will fix that.

@dr-orlovsky dr-orlovsky changed the base branch from master to v0.10 October 15, 2023 17:34
@dr-orlovsky
Copy link
Member Author

@zoedberg done. Should work now.

@dr-orlovsky
Copy link
Member Author

Anyway, my tests shows that all these changes are void, since rust enforces must_use only on the top-level type (which is result) and not onto boolean. If the boolean is unused, there still no compiler warning...

@zoedberg zoedberg self-requested a review October 15, 2023 21:41
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.

It builds and all our tests pass, thus ACK

@dr-orlovsky dr-orlovsky merged commit 8a9efab into v0.10 Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants