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

[crypto] Changes `types::SignedTransaction` to use the nextgen crypto API #429

Merged
merged 1 commit into from Aug 3, 2019

Conversation

@huitseeker
Copy link
Contributor

huitseeker commented Aug 2, 2019

This changes the following behavior:

  • Propagates the change onto the rest of the code base

Why this is better

  • continues the nextgen_crypto API
  • removes copies of private key material effected through copies of Accounts
  • makes tests deterministic, with exception of the functional_tests, where this diff outlines the dependency on e2e_tests::Account::new() returning different values (see #430)

Why this is worse

  • breaking change to Transaction serialization format

Future work

  • remove remaining instances of crypto::signing (just a few of them, exclusively in tests)
  • switch the network identity key to the nextgen_crypto implementation of X25519
@huitseeker huitseeker changed the title Changes `types::SignedTransaction` to use the nextgen crypto API [crypto] Changes `types::SignedTransaction` to use the nextgen crypto API Aug 2, 2019
@huitseeker huitseeker force-pushed the huitseeker:nextgen_ledgerinfo branch from 586131b to a124732 Aug 2, 2019
@huitseeker huitseeker requested review from kchalkias, kevinlewi, gedigi and mimoo Aug 2, 2019
Copy link
Contributor

kchalkias left a comment

LGTM, ignore the minor rand version comment on this PR.

@@ -10,6 +10,7 @@ edition = "2018"
bincode = "1.1.1"
clap = "2.32"
hex = "0.3.2"
rand = "0.6.5"

This comment has been minimized.

Copy link
@kchalkias

kchalkias Aug 3, 2019

Contributor

why not 0.7?

This comment has been minimized.

Copy link
@huitseeker

huitseeker Aug 3, 2019

Author Contributor

Because it's better to upgrade in one fell swoop and I believe I wrote a script at some point (scripts/cargo_update_outdated.sh) and that thing hasn't told me it's time yet :)

@@ -468,6 +468,10 @@ pub mod compat {

use rand::{rngs::StdRng, SeedableRng};
/// Generate an arbitrary key pair, with possible Rng input
///
/// Warning: if you pass in None, this will not return distinct

This comment has been minimized.

Copy link
@kchalkias

kchalkias Aug 3, 2019

Contributor

Useful comment <3

*This changes the following behavior:*
- Propagates the change onto the rest of the code base

*Why this is better*
- continues the nextgen_crypto API
- removes copies of private key material effected through copies of `Account`s
- makes tests deterministic, with exception of the functional_tests, outlines the dependency on `e2e_tests::Account::new()` returning differnet values

*Why this is worse*
- n/a

*Future work*
- remove remaining instances of crypto::signing (just a few of them, exclusively in tests)
- switch the network identity key to the nextgen_crypto implementation of X25519

Traduces `types/transaction.rs` and dependents to nextgen_crypto API
@huitseeker huitseeker force-pushed the huitseeker:nextgen_ledgerinfo branch from a124732 to 5ad3762 Aug 3, 2019
@huitseeker huitseeker merged commit 09817d3 into libra:master Aug 3, 2019
1 check passed
1 check passed
commit-workflow Workflow: commit-workflow
Details
types = { path = "../types", features = ["testing"]}

[features]
default = []

This comment has been minimized.

Copy link
@lightmark

lightmark Aug 6, 2019

Contributor

What do these two lines mean here?

This comment has been minimized.

Copy link
@mimoo

mimoo Aug 6, 2019

Contributor

this is the list of configuration flags that you can use to build this module.
By default you don't use any (default = []), but you can use the testing flag when building, which will make use of two other features defined in different crates (line below).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.