-
Notifications
You must be signed in to change notification settings - Fork 2
Simon/test utils folder #217
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #217 +/- ##
==========================================
- Coverage 87.92% 87.84% -0.08%
==========================================
Files 41 48 +7
Lines 9046 9077 +31
Branches 9046 9077 +31
==========================================
+ Hits 7954 7974 +20
- Misses 645 656 +11
Partials 447 447 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
netrome
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks alright to me, but we should be much more mindful about introducing breaking changes. We need better versioning and changelog management to make sure we can communicate breaking changes better.
For this PR, please re-export any existing types you've moved in the top-level module and we should be fine.
| @@ -0,0 +1,224 @@ | |||
| // This module provides generic functions to be used in the mpc repository | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these are intended only for the MPC repository, why are they defined here and not in that repo?
Also this module seems to revolve around the TestGenerators struct, why not name it test_generators?
Finally, since these are used by the MPC repo it would be nice if we can re-export them from the test_utils module so that this refactor is not breaking the MPC repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were functions that we decided recently to move from the MPC repo because they were purely testing functions for threshold signatures. The original thread for reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah they seem usable to have in this repo, but naturally this repo should not have a module called mpc_interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @netrome this is why I have been recently wondering whether it is going to be dealt with soon or not https://nearone.slack.com/archives/C07UW93JVQ8/p1762723559814599
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is also a needed change. The file should not be named mpc_interface, as these functions are purely test utils functions offered by this crate (albeit currently only used by the mpc node, that's why we still need some unification in the future)
| #[cfg(feature = "test-utils")] | ||
| pub mod test_utils_impls; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Afaik this has the same problem as before. To be backwards compatible, you need to expose the same types and functions in the same location. If you change the name of the module, that's not the case. The functions/structs need to be re-exported in the same files test-utils.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the location is still the same: see that there is still test_utils.rs file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, in that file you have:
pub use crate::test_utils_impls::*;
pub use mpc_interface::*;
so this looks like a hack? Why doing so instead of keeping the old folder name test_utils? Also I think only the previous public types needed to be exported.
netrome
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not following the distinction between test_utils_impls and test_utils. Any module suffixed _impls sounds like it should primarily contain trait implementations in my view.
Secondly we still have the mpc_interface module. Please rename it to something more appropriate, like test_generators.
Closes #216