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

Improve simulation UX #14

Merged
merged 22 commits into from
Dec 4, 2024
Merged

Improve simulation UX #14

merged 22 commits into from
Dec 4, 2024

Conversation

survived
Copy link
Contributor

@survived survived commented Nov 26, 2024

Writing tests for MPC protocols involves copy-pasting the same boilerplate over and over again. This PR improves ergonomics by moving the most common logic into the lib.

You can see how it improves the tests in LFDT-Lockness/cggmp21#120, specifically in 81191c0:

  • A lot less code required:
    • No need to manually poll futures via futures::future::try_join_all
    • No need to write a check each time that all outputs are equally the same (e.g. in signing, we need to check that all sigs are the same, and we used to write a loop each time, now we just use .expect_eq())
  • No need of async runtime. All tests are sync now.

Other changes:

  • Remove std feature. All errors implement core::error::Error trait.

@survived survived marked this pull request as ready for review November 27, 2024 13:50
@survived survived requested a review from maurges November 27, 2024 13:50
round-based/CHANGELOG.md Outdated Show resolved Hide resolved
round-based/Cargo.toml Outdated Show resolved Hide resolved
Signed-off-by: Denis Varlakov <[email protected]>
Signed-off-by: Denis Varlakov <[email protected]>
Signed-off-by: Denis Varlakov <[email protected]>
Signed-off-by: Denis Varlakov <[email protected]>
Copy link
Contributor

@maurges maurges left a comment

Choose a reason for hiding this comment

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

One part of me wants to say that we should keep the old api, on the other hand I'm not going to use it anywhere.

One nitpick is that I'd prefer using "simulation" instead of "sim"

Signed-off-by: Denis Varlakov <[email protected]>
maurges
maurges previously approved these changes Nov 28, 2024
@survived
Copy link
Contributor Author

One part of me wants to say that we should keep the old api

How do we improve UX and evolve the lib if we commit to keep old api? 😃

@survived
Copy link
Contributor Author

One nitpick is that I'd prefer using "simulation" instead of "sim"

I also thought about it, though writing "simulation" wasn't an issue when I was updating all the dependent lib... I think I'll go ahead and use shorter name

Signed-off-by: Denis Varlakov <[email protected]>
Signed-off-by: Denis Varlakov <[email protected]>
maurges
maurges previously approved these changes Nov 29, 2024
@survived survived marked this pull request as draft December 3, 2024 12:34
Signed-off-by: Denis Varlakov <[email protected]>
@survived
Copy link
Contributor Author

survived commented Dec 3, 2024

@maurges that's a bit out of scope of the PR, but I also removed uses of std::error::Error trait, and replaced with core::error::Error in 5d53357

@survived survived marked this pull request as ready for review December 3, 2024 12:45
@survived survived requested a review from maurges December 3, 2024 12:45
Signed-off-by: Denis Varlakov <[email protected]>
Signed-off-by: Denis Varlakov <[email protected]>
maurges
maurges previously approved these changes Dec 3, 2024
@survived
Copy link
Contributor Author

survived commented Dec 3, 2024

Too soon! @maurges Clippy suddenly started complaining on unmodified part of the library

@survived survived requested a review from maurges December 3, 2024 15:15
@survived
Copy link
Contributor Author

survived commented Dec 4, 2024

That's a large PR...

@survived survived merged commit 100b267 into m Dec 4, 2024
13 checks passed
@survived survived deleted the impr-sim branch December 4, 2024 12:15
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.

2 participants