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

Fixed Rust dependencies + BIP39 support #232

Merged
merged 5 commits into from
Mar 29, 2019
Merged

Conversation

maciejhirsz
Copy link
Contributor

This is squashed #223 to date with fixed Rust dependencies that should hopefully forever solve all of our build issues (no linked C code).

It includes the BIP39 for Ethereum (new phrases are BIP39, recovery attempts BIP39 recovery and falls back to Parity phrases) along with minor UI changes (phrase validation and making the boxes bigger as 24 word BIP39 phrases are just longer).

#223 will be rebased after this is merged.

@maciejhirsz maciejhirsz requested a review from Tbaut March 28, 2019 11:33
maciejhirsz added a commit that referenced this pull request Mar 28, 2019
Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

lgtm, few minor things


[dependencies]
base64 = "0.10.1"
blockies = "0.2"
ethsign = { git = "https://github.com/tomusdrw/ethsign.git", default-features = false, features = ["pure-rust"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's release it and use the crates.io version.

Box::into_raw(Box::new(crypto.into()))
let password = Protected::new((*password).as_str().as_bytes().to_vec());
let crypto = Crypto::encrypt(data.as_bytes(), &password, CRYPTO_ITERATIONS).unwrap();
Box::into_raw(Box::new(serde_json::to_string(&crypto).unwrap()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good to have expects instead of unwraps

Copy link
Contributor Author

@maciejhirsz maciejhirsz Mar 28, 2019

Choose a reason for hiding this comment

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

Unfortunately they aren't very useful here since you never see the message when the mobile app crashes.

I want to abstract out the bridge glue code so type conversions are automatic and have some way of propagating errors up to JS without panicing, then get rid of all unwraps. Another thing is the duplicated logic between iOS and Android that I want to get rid off as well. That said, that will be a separate PR (likely part of #223).

@@ -199,13 +184,12 @@ pub unsafe extern fn decrypt_data(encrypted_data: *mut StringPtr, password: *mut
#[cfg(feature = "jni")]
#[allow(non_snake_case)]
pub mod android {
extern crate jni;
// extern crate jni;
Copy link
Contributor

Choose a reason for hiding this comment

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

remove?

}
}
}
// #[cfg(test)]
Copy link
Contributor

Choose a reason for hiding this comment

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

?

let result = env.new_string("").expect("second result to be created").into_inner();
env.throw(new_exception(&env)).expect("second throw failed");
let result = env.new_string("").expect("Could not create java string").into_inner();
env.throw(new_exception(&env)).expect("second trhow failed");
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

}

impl Keccak256<[u8; 32]> for [u8] {
fn keccak256(&self) -> [u8; 32] {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think such function is already part of tiny_keccak: https://docs.rs/tiny-keccak/1.4.2/tiny_keccak/fn.keccak256.html

}
if (set.size < 11) {

if (words.length < 11) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What happened with the previous validation? It seemed quite nice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It broke for BIP39 since it uses a different dictionary. This is pretty MVP, we want to add the double-dictionary hinting mechanism that figures out which phrase type you are entering after 1-n words, right now the checks are done in the "backend". I did reintroduce checking for extra whitespace just now though, as that is useful for both.

@maciejhirsz
Copy link
Contributor Author

Thanks for the review @tomusdrw, I've addressed all points I think. Will wait for @Tbaut to look over the UI changes too.

Copy link
Contributor

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

👍 built successfully and migrated from r13 using the instructions.
UI change lgtm as well, I let you merge.

@lexfrl
Copy link
Contributor

lexfrl commented Mar 30, 2019

#194 (comment)
😔

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.

4 participants