-
Notifications
You must be signed in to change notification settings - Fork 142
Conversation
Signed-off-by: Nathan Nguyen <nathan.nguyen@evernym.com>
Signed-off-by: Nathan Nguyen <nathan.nguyen@evernym.com>
Signed-off-by: Nathan Nguyen <nathan.nguyen@evernym.com>
Signed-off-by: Nathan Nguyen <nathan.nguyen@evernym.com>
Signed-off-by: Nathan Nguyen <nathan.nguyen@evernym.com>
Signed-off-by: Nathan Nguyen <nathan.nguyen@evernym.com>
This is great. Thanks for catching & fixing the compatibility test. For the workspace, I think the endgame is that libzmix is gone. I think Mike's intent is that libursa gets split up into the other components, like ursa_signatures so libursa goes away too. Until that's all refactored, maybe we should just put libursa into the workspace members list so it gets built and tested? |
libursa/src/kex/secp256k1.rs
Outdated
@@ -130,6 +130,7 @@ mod ecdh_secp256k1 { | |||
use sha2::digest::generic_array::typenum::U32; | |||
use sha2::Digest; | |||
use zeroize::Zeroize; | |||
#[allow(dead_code)] |
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.
Probably better to remove the dead code. What was coming up?
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.
It's not used anywhere, I was just taking a super conservative approach for my changes. I'll go ahead and remove it now.
If we want to put Also, adding As for actually getting Do we want to build and test all the packages simultaneously, or do we want some sort of granularity? (to be clear, I think having everything in the workspace makes sense -- I just want to make sure these changes don't get in the way later) |
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 have nothing to add more than @dcmiddle has mentioned about the code review. Thank you for contributing this. I will look into what you mentioned above
@mikelodder7 Do you have any input for the concerns listed above? |
Based on the conversation in today's community meeting we should rely on the Operation Oso issue description. Essentially don't "fix" libursa in the workspace and CI. Instead to get those components back into testing they should be refactored into the new structure e.g. signatures subproject. The fixes for k256k1 etc. should still remain in this PR though (recognizing that it's awkward that those changes aren't validated in CI until the refactoring happens) |
Signed-off-by: Nathan Nguyen <nathan.nguyen@evernym.com>
Signed-off-by: Nathan Nguyen <nathan.nguyen@evernym.com>
I've reverted the changes to the workspace/CI as per what we discussed in Wednesday's meeting. At this point, this PR just fixes that one regression test and some compiler warnings, so I'll be opening up more PRs geared at #151 to follow up on this one. |
This PR adds
libursa
-specific jobs to GH Actions (namely buildinglibursa
and running its tests).One thing I'm not sure about is the workaround for getting
libursa
to build on its own -- currently, runningcargo build
in thelibursa
directory on themain
branch will yield this error:I heeded the second suggestion and excluded
libzmix
andlibursa
from the workspace, but I'm not sure if this is the correct course of action. Feel free to request/make changes!As for the changes to
libursa/src/kex/secp256k1.rs
, the compatibility test was never updated afterlibsecp256k1
was replaced withk256
in #171, so this PR also fixes that.