-
Notifications
You must be signed in to change notification settings - Fork 62
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
Remove PassthroughDigest and update crypto dependencies #385
Conversation
a430169
to
5a74c89
Compare
Checked on this again, it seems the original issue is resolved, but now |
Ran `cargo hack test --each-feature --workspace --exclude-features "tor-tests" -- --test-threads=8` 3 tests are failing, maybe from older changes Update CI to run tests with cargo hack
5a74c89
to
ab205fa
Compare
#[error("Curve not implemented: '{0}'")] | ||
CurveNotImplemented(String), | ||
/// Errors from p256, k256 and ed25519-dalek | ||
#[cfg(feature = "k256")] |
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.
should these features be "secp256k1"
and "secp256r1"
?
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 was asking myself the same question -- I came to the conclusion that it doesn't matter as long as we make sure to always use features and never optional dependencies as features
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.
never mind I forgot about dependencies being enabled by other crates
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've opened #472 to reduce the scope
let hash = openssl::hash::hash(digest, &data)?; | ||
let sig = openssl::ecdsa::EcdsaSig::sign(&hash, &sk)?; | ||
[sig.r().to_vec(), sig.s().to_vec()].concat() | ||
#[cfg(feature = "p256")] |
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 like an oversight from #457, should this say "secp256r1"
?
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.
same as above
ssi-vc/src/lib.rs
Outdated
@@ -2925,7 +2925,9 @@ _:c14n0 <https://w3id.org/security#verificationMethod> <https://example.org/foo/ | |||
assert_eq!(n_proofs, 4); | |||
} | |||
|
|||
// Can't deserialise the URIs as verification methods (?) |
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.
this relies on the testing
flag for ssi-dids
to be enabled (see ssi-dids/src/lib.rs#973
)
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.
cheers
.github/workflows/build.yml
Outdated
|
||
- name: Test DID Resolution HTTP(S) Binding | ||
run: cargo test --manifest-path ssi-dids/Cargo.toml --features http | ||
run: cargo hack test --each-feature --workspace --exclude-features "tor-tests" -- --test-threads=4 |
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.
do we want to do --feature-powerset
or --group-features
instead of --each-feature
? I think this may exclude some combinations and tests
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.
Depends if you want to run tests for 17,421 combinations -- I checked and there didn't seem to be obvious combination of features. I will double check on tests though, making sure e.g. secp256k1 tests run for both the secp256k1
and ring
features
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 added another step in the CI to run the tests with all features enabled, that should cover any build/test problem
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.
big fan of these updates. does anything important change when using sequoia-pgp
default features (apart from dynamic lib dependancies)? assuming no, LGTM
Ah my mistake, I thought it didn't do any crypto stuff (only decoding/parsing keys) as we are using ssi-jws but here's the default default = ["compression", "crypto-nettle"]
|
I replaced the pgp library for one with more users and more recent versions of RustCrypto crates. Had to modify a tests assertions but the changes are valid, and tested with gpg. |
This test was purely a regression test. k256 version change in #385 changed the recoverable/ethereum signatures generated (both variants are valid and verify successfully). Ref: RustCrypto/elliptic-curves#552
This test was purely a regression test. k256 version change in #385 changed the recoverable/ethereum signatures generated (both variants are valid and verify successfully). Ref: RustCrypto/elliptic-curves#552
This test was purely a regression test. k256 version change in #385 changed the recoverable/ethereum signatures generated (both variants are valid and verify successfully). Ref: RustCrypto/elliptic-curves#552
This test was purely a regression test. k256 version change in #385 changed the recoverable/ethereum signatures generated (both variants are valid and verify successfully). Ref: RustCrypto/elliptic-curves#552
This test was purely a regression test. k256 version change in #385 changed the recoverable/ethereum signatures generated (both variants are valid and verify successfully). Ref: RustCrypto/elliptic-curves#552
Ran
cargo hack test --each-feature --workspace --exclude-features "tor-tests" -- --test-threads=8
3 tests are failing, maybe from older changes
Update CI to run tests with cargo hack