Skip to content
This repository has been archived by the owner on Mar 11, 2024. It is now read-only.

Remove libsodium and time dependencies #200

Merged
merged 3 commits into from
Jan 10, 2022

Conversation

andrewwhitehead
Copy link
Contributor

No description provided.

@andrewwhitehead
Copy link
Contributor Author

NB I haven't built and tested the docker images.

@andrewwhitehead
Copy link
Contributor Author

Somewhat related but I don't think this repo should have a Cargo.lock. It causes older dependency versions to be pinned when running tests (and the audit) where they would be updated to the latest patch versions when the library is used.

@mac-arrap
Copy link
Contributor

We discussed during the last URSA meeting that we wanted to switch from libsodium to Dalek. This seems to just remove where libsodium is called but doesn't replace it.

@andrewwhitehead
Copy link
Contributor Author

libsodium wasn't used for ed25519 except in tests (verifying the Dalek backend) and in benchmarks.

Copy link
Contributor

@dcmiddle dcmiddle left a comment

Choose a reason for hiding this comment

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

thanks! looks good (just one propagated spelling error)

docs/build-environment.md Outdated Show resolved Hide resolved
@dcmiddle
Copy link
Contributor

dcmiddle commented Jan 6, 2022

Somewhat related but I don't think this repo should have a Cargo.lock. It causes older dependency versions to be pinned when running tests (and the audit) where they would be updated to the latest patch versions when the library is used.

I don't like the lock file in here either. Official guidance is that libraries should not commit the lock file[1] unless the library is a static or cdylib[2]. Ursa is setup as a static, cdylib[3] ... but why? I'm not sure. If you want to remove it, I'm supportive but please create a separate issue so we can make sure we understand why ursa is setup this way.

[1]https://doc.rust-lang.org/cargo/faq.html#why-do-binaries-have-cargolock-in-version-control-but-not-libraries
[2]https://doc.rust-lang.org/cargo/guide/cargo-toml-vs-cargo-lock.html
[3]https://github.com/hyperledger/ursa/blob/main/libursa/Cargo.toml#L35

Signed-off-by: Andrew Whitehead <[email protected]>
@dcmiddle
Copy link
Contributor

dcmiddle commented Jan 7, 2022

fixes #196

@mac-arrap
Copy link
Contributor

If it's agreed that this benchmarking can be removed then I see no problem. The only reason I stated the above was per the request given to me on the last URSA call. But I do agree that it's not useful to just test another libraries backend. @hartm please review

@mikelodder7 mikelodder7 merged commit 8dc4084 into hyperledger-archives:main Jan 10, 2022
@dcmiddle dcmiddle mentioned this pull request Jan 10, 2022
@andrewwhitehead andrewwhitehead deleted the reduce-sodium branch February 7, 2022 05:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants