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

Add functions for MessageDigest and Digest (bouncycastle) #8

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

solonovamax
Copy link
Contributor

Add functions for converting jvm digest classes to HasherFactory

Adds Hasher and HasherFactory wrappers for

  • MessageDigest
  • Digest (bouncycastle)

This also slightly modifies the MicroAmper implementation, to allow for other dependency scopes such as compile-only.
This was required, as the current implementation did not support compile-only dependencies. (you really should consider pulling in a yaml parser for that, tbh)

Adds Hasher and HasherFactory wrappers for
  - MessageDigest
  - Digest (bouncycastle)

This also slightly modifies the `MicroAmper` implementation,
to allow for other dependency scopes such as compile-only.

Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Comment on lines -1012 to +1033
val isWindows = platform.startsWith("mingw")
platform.startsWith("mingw")
Copy link
Member

@soywiz soywiz Mar 4, 2025

Choose a reason for hiding this comment

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

Is this intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm, not at all sure how that happened lol

also looking at it, my ide decided to reformat all the imports as well. I'll rewrite the commit history to remove those changes. (to avoid polluting the history with garbage & changing the git blame)

@soywiz soywiz requested a review from Copilot March 4, 2025 11:46

Choose a reason for hiding this comment

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

PR Overview

This PR adds Hasher and HasherFactory wrappers to support conversions from JVM digest classes (MessageDigest and BouncyCastle's Digest) and adjusts dependency management to allow compile-only dependencies.

  • Adds wrappers for MessageDigest and Digest
  • Updates module.yaml to include the BouncyCastle provider as a compile-only dependency
  • Modifies MicroAmper implementation to support compile-only dependency scopes

Reviewed Changes

File Description
korlibs-crypto/module.yaml Added JVM-specific compile-only dependency for BouncyCastle providers

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Copy link

codecov bot commented Mar 4, 2025

Codecov Report

Attention: Patch coverage is 0% with 20 lines in your changes missing coverage. Please review.

Project coverage is 83.54%. Comparing base (3919b63) to head (94c78e0).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
...ypto/src@jvm/korlibs/crypto/BouncyCastleDigests.kt 0.00% 10 Missing ⚠️
korlibs-crypto/src@jvm/korlibs/crypto/Digests.kt 0.00% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main       #8      +/-   ##
==========================================
- Coverage   85.45%   83.54%   -1.92%     
==========================================
  Files          16       18       +2     
  Lines         770      802      +32     
  Branches       98       98              
==========================================
+ Hits          658      670      +12     
- Misses         89      109      +20     
  Partials       23       23              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@soywiz
Copy link
Member

soywiz commented Mar 4, 2025

@solonovamax thanks for the PR! So instead of creating another artifact / project in the root folder to avoid adding transitive dependencies the idea here is to add extra methods but if people want to use them, they will need to include that library right?
Could you add a runtime dependency for bouncy castle in the test source set and add a test verifying the added functions?

@solonovamax
Copy link
Contributor Author

solonovamax commented Mar 4, 2025

So instead of creating another artifact / project in the root folder to avoid adding transitive dependencies the idea here is to add extra methods but if people want to use them, they will need to include that library right?

yep, exactly

if people do not wish to use it, then it would simply be just a few additional useless classes in the jar. and for the bouncy castle classes, the classes would just have references to bouncy castle classes which don't exist, but since the class never gets loaded & the code is never executed, it's perfectly fine to have this.
but if someone wanted to use it, they'd need to add a dependency on the bouncy castle library, and then would be able to use the extension functions normally.

if this project wasn't using the MicroAmper stuff, it might actually be better to model this using a feature variant rather than optional dependencies, which would then be consumed by doing

implementation("com.soywiz:korlibs-crypto:6.0.0") {
    capabilities {
       requireCapability("com.soywiz:korlibs-crypto-bouncycastle")
    }
}

although, I'm honestly not sure how well feature variants are supported for KMP. (I've asked in the kotlinlang slack and am awaiting a response, as this is something I would find useful for my own projects (note: link requires being in the kotlinlang #gradle slack channel))

Could you add a runtime dependency for bouncy castle in the test source set and add a test verifying the added functions?

sure, I could do that. how would you like me to test it? just computing the results using both the bouncy castle class as well as the wrapper, and then comparing the results?

@soywiz
Copy link
Member

soywiz commented Mar 4, 2025

I'm not much into using special stuff like that in libraries (by past experiences) and prefer plain artifacts.

A new artfiact called:

  • korlibs-crypto-bouncycastle

could be added.

In any case even if we use the compile-only thing, we can still add a test-only dependency to the library and add a test that verifies the newly added function call.

@@ -10,5 +10,8 @@ aliases:
dependencies:
- com.soywiz:korlibs-encoding:6.0.0: exported

dependencies@jvm:
- org.bouncycastle:bcprov-jdk18on:1.80: compile-only

test-dependencies:

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
test-dependencies@jvm:
- org.bouncycastle:bcprov-jdk18on:1.80: exported

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