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

feat: language detection for mnemonic seed words #3590

Merged

Conversation

StriderDM
Copy link
Contributor

Description

Implementation of a basic language detection algorithm for mnemonic words

Motivation and Context

Improve language detection for mnemonic words

How Has This Been Tested?

cargo test --all

Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

Think something like this may be simpler

fn detect_language(words: &[&str]) -> Result<MnemonicLanguage, MnemonicError> {
    let mut langs = MnemonicLanguage::iterator().collect::<Vec<_>>();
    for word in words {
        langs = langs
            .drain(..)
            .filter(|lang| find_mnemonic_index_from_word(word, lang).is_ok())
            .collect();
        if langs.is_empty() {
          return  Err(MnemonicError::UnknownLanguage);
        }
        if langs.len() == 1 {
            return Ok(langs.remove(0));
        }
    }
    Err(MnemonicError::UnknownLanguage)
}

(probably could be made to have zero allocations but that's a bonus)

This language detection also needs to be in MneumonicLanguage::from

base_layer/key_manager/src/mnemonic.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@philipr-za philipr-za left a comment

Choose a reason for hiding this comment

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

A bit of a brute force approach in my opinion. In 99% of cases the first word would be good enough. This approach checks all 24 words against all languages. But it probably works. The issue is I can't tell if it works because it is not tested at all. Please provide tests that show it working and testing the edge cases it is trying to solve.

Also you have left the old detection method in MnemonicLanguage::from(...) which is also where I would actually put this implementation.

@StriderDM
Copy link
Contributor Author

Thanks, @sdbondi, and @philipr-za. I have switched it to a scanning-based approach.

@StriderDM StriderDM force-pushed the mnemonic_language_detection branch 7 times, most recently from 148e5e7 to 16c38ba Compare November 19, 2021 18:02
Implementation of a basic language detection algorithm for mnemonic words

Review comments

Added tests

Simplification
@StriderDM StriderDM force-pushed the mnemonic_language_detection branch from 16c38ba to d3aba9e Compare November 20, 2021 21:24
@aviator-app aviator-app bot merged commit 57f51bc into tari-project:development Nov 21, 2021
sdbondi added a commit to sdbondi/tari that referenced this pull request Nov 22, 2021
* development:
  feat: language detection for mnemonic seed words (tari-project#3590)
  chore: minor clippy fixes (tari-project#3576)
  fix: be more permissive of responses for the incorrect request_id (tari-project#3588)
  feat: track ping failures and disconnect (tari-project#3597)
  chore: upgrade tokio deps tari-project#3581 (tari-project#3595)
  feat: standardize output hash for unblinded output, transaction output and transaction input (tari-project#3592)
  fix: allow bullet proof value only rewinding off one-sided transaction (tari-project#3587)
  refactor: update miningcore repository links (tari-project#3593)
  refactor: clean up unwraps in wallet_ffi (tari-project#3585)
  fix: update daily test start times and seed phrase (tari-project#3584)
  fix: allow bullet proof value only rewinding in atomic swaps (tari-project#3586)
  v0.21.2
  fix: remove delay from last request latency call
sdbondi added a commit to sdbondi/tari that referenced this pull request Nov 23, 2021
* development: (46 commits)
  refactor: remove tari_common dependency from tari_comms (tari-project#3580)
  feat: language detection for mnemonic seed words (tari-project#3590)
  chore: minor clippy fixes (tari-project#3576)
  fix: be more permissive of responses for the incorrect request_id (tari-project#3588)
  feat: track ping failures and disconnect (tari-project#3597)
  chore: upgrade tokio deps tari-project#3581 (tari-project#3595)
  feat: standardize output hash for unblinded output, transaction output and transaction input (tari-project#3592)
  fix: allow bullet proof value only rewinding off one-sided transaction (tari-project#3587)
  refactor: update miningcore repository links (tari-project#3593)
  refactor: clean up unwraps in wallet_ffi (tari-project#3585)
  fix: update daily test start times and seed phrase (tari-project#3584)
  fix: allow bullet proof value only rewinding in atomic swaps (tari-project#3586)
  v0.21.2
  feat: add atomic swap refund transaction handling (tari-project#3573)
  feat: improve wallet connectivity status for console wallet (tari-project#3577)
  v0.21.1
  feat: add error codes to LibWallet for CipherSeed errors (tari-project#3578)
  ci: split cucumber job into two (tari-project#3583)
  feat(wallet): import utxo’s as EncumberedToBeReceived rather than Unspent (tari-project#3575)
  docs: rfc 0250_Covenants (tari-project#3574)
  ...
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