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

Monorepo ed/x/curve25519 #536

Merged
merged 746 commits into from
Jun 30, 2023
Merged

Conversation

pinkforest
Copy link
Contributor

@pinkforest pinkforest commented Jun 27, 2023

As discussed this is the monorepo PR

  • This would need to be merged - not squashed - in order to retain the combined history from x/ed/curve
  • Renames for each crates in separate commit
  • Merge commits for each combine
  • CI re-organized for Monorepo structure
  • This PR needs to be kept minimal and can follow-up with e.g. proper monorepo root README.md etc.
  • This would help us keep the crates consistent and synchronized across and messing with git patching
  • I was hoping commits could be prefixed-amended based on crate e.g. ed25519 could have ed: prefix and the PR links would be linked to old place - nonetheless this would be the tradeoff with history we would pay for easier consistency across the crates and usually the original commits has all the information from PR's

git combine Procedure (see near last commits)

git mv [curve25519-dalek-stuff] curve25519-dalek/
git commit -m 'Workspace curve25519 under curve25519-dalek'

git remote add ed https://github.com/dalek-cryptography/ed25519-dalek.git
git fetch ed
git merge ed/main --allow-unrelated-histories
git mv [ed25519-dalek-stuff] ed25519-dalek/
git commit -m 'Workspace ed25519 under ed25519-dalek'

git remote add x https://github.com/dalek-cryptography/x25519-dalek.git
git fetch x
git merge x/main --allow-unrelated-histories
git mv [x25519-dalek-stuff] x25519-dalek/
git commit -m 'Workspace x25519 under x25519-dalek'

isislovecruft and others added 30 commits June 30, 2020 22:51
Updates the merlin dependency to ^2 and the correct repo
…ature-error

Impl std::error::Error for SignatureError.
RFC8032 specifies that the context cannot be greater than 255 octets,
but in the previous implementation in ed25519-dalek, this error would
only be caught by a debug_assert.  This changes the sign_prehashed()
function to return a Result so that the error can be handled at
runtime and the library no longer allows misuse by creating signatures
that other libraries cannot handle.
We're now aliasing SignatureError to the error type from the signature
crate.
This is no longer actively breaking our no_std build, but I think
it's still technically a minor bug, and further case of issue dalek-cryptography#108
`rand_core` defines a blanket impl of `RngCore + CryptoRng` for `&mut T` where
`T: RngCore + CryptoRng`, so rather than requiring a borrowed RNG, it's better
to require an owned RNG, as this allows passing either owned or borrowed
values.  In particular, this makes `OsRng` usage much more ergonomic, because
the caller is not forced to do `&mut OsRng` on the zero-sized struct.
Closes dalek-cryptography#59.

The doc examples have code interspersed with text explaining the API.  Because
each doctest executes independently, when these code examples are run as
doctests, they have to include parts of the previous examples with # lines.
These lines are hidden from Rustdoc output and do not appear in the rendered
docs, but they do appear when viewing the README.md on Github.

In order to hide these on Github, the code blocks were made non-executable,
with their content moved to a unit test.  However, this meant that the example
API usage was not tested, and so when the unit test was updated to remove the
deprecated `rand_os`, there was no check that the examples stayed in sync with
the test, causing dalek-cryptography#59.  To prevent this from reocurring in the future, go back
to executable tests of the API examples.
Add .to_bytes() to PublicKey, SharedSecret
…ve-rand-os-example

Remove rand os example
Also does a pass through the docs converting `TypeNames` to Rustdoc links, and
making sure that all the items have consistent summaries.

Closes dalek-cryptography#58
Closes $56
…ify-ephemeral-static

Clarify Ephemeral/StaticSecret docs.
This allows unifying dependencies with other crates using the `3.x` series of
the curve library.  It is a semver patch-level change, because the x25519-dalek
API does not expose any details of the underlying curve implementation.
@pinkforest pinkforest force-pushed the monorepo-t1 branch 8 times, most recently from 97625bd to 6f5281d Compare June 27, 2023 08:00
@rozbb
Copy link
Contributor

rozbb commented Jun 27, 2023

Thank you! Two things:

  1. Is there anything we can do to merge the issues/Pars (current and past) from all the constituent repos? Or are we just stuck keeping or historical record in an archived repo?
  2. If it's not too annoying, I'm gonna do the git merge steps myself. Just bc it's literally impossible to audit on my end. I'll cherry pick the follow-up commits you have here. Saw edited comment

@pinkforest
Copy link
Contributor Author

pinkforest commented Jun 27, 2023

  1. We can transfer the issues, PR's would have to be re-created.
  2. I have other changes too incl. CI and would have to audit in any case :)

This can be audited with ... rsync :)

$ rsync -rcnv a/* b/

-r will recurse into the directories
-c will compare based on file checksum
-n will run it as a "dry run" and make no changes, but just print out the files 
   that would be updated
-v will print the output to stdout verbosely

e.g. like this:

git clone https://github.com/dalek-cryptography/curve25519-dalek.git
git clone https://github.com/dalek-cryptography/ed25519-dalek.git
git clone https://github.com/dalek-cryptography/x5519-dalek.git
pull this pr to "monorepo"

rsync -rcnv monorepo/curve25519-dalek/* curve25519-dalek/
rsync -rcnv monorepo/ed25519-dalek/* ed25519-dalek/
rsync -rcnv monorepo/x25519-dalek/* x25519-dalek/

Above without 'dry run" you can also do "git status" to see if any files changed and how they changed "git diff" if any.

Copy link
Contributor

@rozbb rozbb left a comment

Choose a reason for hiding this comment

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

Left some comments. Verified that the subdirectories match the current main branch of all the relevant repos (outside the necessary Cargo.toml changes). Used

diff -r ./curve25519-dalek ../curve25519-dalek

etc.

.github/workflows/rust.yml Show resolved Hide resolved
.github/workflows/rust.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@rozbb
Copy link
Contributor

rozbb commented Jun 28, 2023

Alright, made a pretty barebones README, but it should be OK for now imo. The dalek image doesn't load correctly anymore bc the URL is gonna change in this PR.

Co-authored-by: Michael Rosenberg <[email protected]>
@pinkforest
Copy link
Contributor Author

pinkforest commented Jun 28, 2023

Ok - I had to re-organize our last commits since this PR needs a merge (not a squash) making it more difficult to change here (suggest doing more in follow-up PR's) and we have renames which need to be kept in separate commits in the middle. I gave you co-authored-by since I had to amend our last change commits on top.

Copy link
Contributor

@rozbb rozbb left a comment

Choose a reason for hiding this comment

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

Gotta re-check the subdirs with diff again bc of the force push. But barring that good to go @tarcieri ?

Update: re-verfified. For concreteness, the current HEADs are:

@rozbb
Copy link
Contributor

rozbb commented Jun 30, 2023

Build's broken now but I have no idea what's causing it 😕

@pinkforest
Copy link
Contributor Author

pinkforest commented Jun 30, 2023

Something broke in transient dependency somewhere for the MSRV build - this should be mergeable in any case as this doesn't cause it.

Found what caused it - raised #538 to track it

@rozbb rozbb merged commit f789810 into dalek-cryptography:main Jun 30, 2023
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.