Skip to content

Merge the pact-crypto lib to the main tree - #3

Open
kdafriend wants to merge 3 commits into
masterfrom
remove_crypto_lib
Open

Merge the pact-crypto lib to the main tree#3
kdafriend wants to merge 3 commits into
masterfrom
remove_crypto_lib

Conversation

@kdafriend

Copy link
Copy Markdown

I've merged the the pact-cypto lib into the main tree, moving everything from Pact.Core.Crypto -> to Pact.Crypto (like the WebAuthn implementation).
Base64 has been moved moved to the directly Pact.Core, and indirect import through Pact.Hash removed.

Preprocessor directive WITHOUT_CRYPTO (and related cases) have been removed.

A warning (shadowed variable) has been fixed in Keccak.

I hope you like the color of bicycle shed @jmcardon .. 😃

@jmcardon jmcardon left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

So:

WITHOUT_CRYPTO macro had a use: building for profiling builds with cost centers. At the time when we tried this, GHC had a bug which prevented profiling builds to be used with our lib.

Please prove that profiling builds work in this PR after removing this macro.

@CryptoPascal31

Copy link
Copy Markdown
Member

WITHOUT_CRYPTO macro had a use: building for profiling builds with cost centers. At the time when we tried this, GHC had a bug which prevented profiling builds to be used with our lib.

Probably you didn't use the WITHOUT_CRYPTO flags since at least 2 years… 😄 😉 ..
Because Pact doesn't build at all with WITHOUT_CRYPTO.
For at least 2 reasons:

  • pact-crypto is referenced as a "whatever the flag is" dependency of pact-repl in the cabal build file.
  • and more important, the Base64 codec routines are in pact-crypto.. And are used in hundreds of places in Pact core. In fact pact-crypto is not separable at all from Pact.

That's why I believe, it doesn't make sense anymore to have it separated.

@CryptoPascal31

Copy link
Copy Markdown
Member

Please prove that profiling builds work in this PR after removing this macro.

I remember I already did it.. But let me check again.

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.

3 participants