Bitcoin algorithm BIP0039: Mnemonic code for generating deterministic keys

This is an implementation of BIP0039 in Rust (I used python-mnemonic as a guide). I'd like my first Rust program to be critiqued.

I would like to know if the program is structured correctly, if I've chosen the right constructs to use where, any other general comments on what to improve to make it cleaner and more Rust-y

main.rs:

```
extern crate getopts;
extern crate core;
extern crate mnemonic;
extern crate "rustc-serialize" as serialize;

use mnemonic::Mnemonic;

use serialize::hex::{FromHex, ToHex};
use getopts::{reqopt,optflag,getopts,OptGroup};

use std::os;
use std::iter::repeat;
use std::rand::{OsRng, Rng};
use std::old_io::File;

//getopts help message
fn print_usage(program: &str, _opts: &[OptGroup]) {
println!("Usage: {} [options]", program);
println!("-s\t\tSeed");
println!("-h --help\tUsage");
}

fn main() {
/ start handling opts /
let args: Vec = os::args();

let program = args[0].clone();

let opts = &[
reqopt("s", "seed", "set mnemonic seed", ""),
optflag("h", "help", "print this help menu")
];
let matches = match getopts(args.tail(), opts) {
Ok(m) => { m }
Err(f) => { panic!(f.to_string()) }
};
if matches.opt_present("h") {
print_usage(program.as_slice(), opts);
return;
}
let seed = match matches.opt_str("s") {
Some(x) => x,
None => panic!("No seed given"),
};
/ end opts, seed value below /
let str_seed:&str = seed.as_slice();

let mut rng = match OsRng::new() {
Ok(g) => g,
Err(e) => panic!("Failed to obtain OS RNG: {}", e)
};
let path = Path::new("src/wordslist/english.txt");
let display = path.display();
let mut file = match File::open(&path) {
Err(why) => panic!("couldn't open {}: {}", display, why.desc),
Ok(file) => file,
};
let words:String = match file.read_to_string() {
Err(why) => panic!("couldn't read {}: {}", display, why.desc),
Ok(strin

(I'm still reading and understanding, so I'll keep updating as I grok more and more.)

General style

• Use spaces after colons denoting types. foo: u8 = 1 instead of foo:u8 = 1.

• Make the spacing between methods consistent and meaningful when it changes. Some of your functions and impls butt right against the previous block.

• No spaces inside of parens. (length) instead of ( length ).

• Spaces after commas. &self, mnemonic instead of &self,mnemonic.

• Use type inference when you can. let mnemonic = Mnemonic::new() instead of let mnemonic: Mnemonic = Mnemonic::new().

• Leave off match arm braces where you can. Ok(m) => m, instead of Ok(m) => { m }.

• Don't create a variable just to return it. 54 instead of let a = 54; a.

• Whenever you are just reading a string, take a &str instead of String.

• Whenever you are reading a slice, take a &[T] instead of Vec.

20000-ft view

I think you are right - your struct doesn't make sense as it is now. None of the struct's methods use self (and there's only one that even takes it as a parameter). Then in process, you directly poke into the guts of Mnemonic to do interesting things. I think that you've really got two things - a Mnemonic (doesn't exist yet) and a MnemonicBuilder (the current Mnemonic).

You also should have some dedicated tests. You have the start of these where you "generate corner cases". However, automated tests will save your bacon! They also would have helped me ensure that I didn't break your code as I was "fixing" it. :-)

main

You should split by words just once, then reuse that:

let word_backing: String = match file.read_to_string() {
    Err(why) => panic!("couldn't read {}: {}", display, why.desc),
    Ok(string) => string,
};
let words: Vec = word_backing.words().collect();

This is the main interesting point. We take &strs and slices of &strs. The middle chunk of this, the loop, probably belongs on Mnemonic. Also, it looks like you are now undoing the binary strings that we created before. If so, then perhaps you should just store a Vec and avoid converting to and from binary. You'd save space and time, the best kind of win!

fn process(random_chars: &str, str_seed: &str, words: &[&str]) {
    println!("random characters: {}", random_chars);

    let mnemonic = Mnemonic::new(random_chars);
    let mut mnem_words = Vec::new();

    for i in range(0us, mnemonic.binary_hash.len() / 11) {
        let bin_idx = &mnemonic.binary_hash[i*11..(i+1)*11];
        let idx: isize = std::num::from_str_radix(bin_idx, 2).unwrap();
        mnem_words.push(words[idx as usize]);
    }

    let str_mnemonic = format!("{:?}", mnem_words);
    println!("mnemonic: {}", str_mnemonic);

    let key_value = mnemonic.to_seed(&str_mnemonic, str_seed);
    println!("key: {}", key_value.to_hex());
}

Mnemonic

new

The big breakthrough here was realizing we were making hash hex for no particular reason. I also beefed up the variable names, giving them a bit more understanding.

pub fn new(seed: &str) -> Mnemonic {
    let hash = Mnemonic::gen_sha256(&seed);
    let hash_binary = Mnemonic::to_binary(&hash);

    let mut seed_binary = Mnemonic::to_binary(seed.as_bytes());
    let length = seed_binary.len() / 32;

    seed_binary.push_str(&hash_binary[..length]);

    Mnemonic {
        binary_hash: seed_binary,
    }
}

to_seed

The main differences here are

• Using vec! to construct a filled vector, this is just a bit simpler to read than the more powerful repeat / take / collect.

• Removing one mut by constructing the string all in one go, using format!

• Changing the static global values to const. The difference is that static items are accessible at run time by reference. const is closer in concept to a value just being inlined at the use-site.

Afterwards:

pub fn to_seed(&self, mnemonic: &str, seed_value: &str) -> Vec {
    let mut mac = Hmac::new(Sha512::new(), mnemonic.as_bytes());
    let salt = format!("mnemonic{}", seed_value);

    let mut result = vec![0u8; PBKDF2_KEY_LEN];
    pbkdf2(&mut mac, salt.as_bytes(), PBKDF2_ROUNDS, &mut result);
    result
}

gen_sha256

After looking at the constructor, I realized that this should just return a vector of bytes, not a string. We just de-hex the string anyway:

fn gen_sha256(hashme: &str) -> Vec {
    let mut sh = Sha256::new();
    sh.input_str(hashme);

    let mut result: Vec = iter::repeat(0).take(sh.output_bytes()).collect();
    sh.result(&mut result);
    result
}

to_binary

-
I feel that this is a poorly-named method. I would expect something called binary to return a slice of bytes, not a string. Maybe to_binary_string would be enough.

-
Instead of creating a string with the character 0, you can use formatting modifiers to have a zero-padded string. Specifically, you want {:08b}.

Here's a slimmer version of this method, but one that allocates more than needed:

```
fn

StackExchange Code Review Q#79593, answer score: 4