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: add ffi get mnemonic wordlist #3538

Merged

Conversation

hansieodendaal
Copy link
Contributor

@hansieodendaal hansieodendaal commented Nov 5, 2021

Description

Added an FFI method to get the entire mnemonic word list for a given language.

Motivation and Context

See above.

How Has This Been Tested?

  • Added a unit test to test this functionality.
  • Added a cucumber test to verify the mnemonic word list is retrieved.

Added an FFI method to get the entire mnemonic wordlist
for a given language.
@hansieodendaal hansieodendaal changed the title feat: add ffi get mnemonic wordlist [WIP adding cucumber test] feat: add ffi get mnemonic wordlist Nov 5, 2021
@hansieodendaal hansieodendaal changed the title [WIP adding cucumber test] feat: add ffi get mnemonic wordlist feat: add ffi get mnemonic wordlist Nov 8, 2021
base_layer/wallet_ffi/wallet.h Outdated Show resolved Hide resolved
base_layer/wallet_ffi/src/lib.rs Show resolved Hide resolved
@hansieodendaal
Copy link
Contributor Author

Although TariSeedWords and TariMnemonicWordList looks the same, they are different things. All methods that operate on TariSeedWords is not valid for TariMnemonicWordList, for example seed_words_push_word. One might retrieve the mnemonic word list and then continue to work with it as if it is the seed words.

@philipr-za
Copy link
Contributor

philipr-za commented Nov 8, 2021

Although TariSeedWords and TariMnemonicWordList looks the same, they are different things. All methods that operate on TariSeedWords is not valid for TariMnemonicWordList, for example seed_words_push_word. One might retrieve the mnemonic word list and then continue to work with it as if it is the seed words.

So you would rather repeat all the helper functions except that single one (which returns feedback that something is wrong)? We are dealing with a very clunky FFI interface here which is unsafe on many levels so needs careful handling by the client and this is a case where I believe that adding 3 completely repeated helpers functions for a SLIGHTLY more appropriate API interface is definitely the wrong move.

Edit: To be clear the original PR already missed 2 of these functions which was get_length and get_at. I feel this already demonstrates the issue of adding more types that are not really necessary.

@hansieodendaal
Copy link
Contributor Author

Thank you @delta1, @philipr-za and @StriderDM for the review comments; applied all. Unit tests and cucumber test working properly.

base_layer/wallet_ffi/src/lib.rs Outdated Show resolved Hide resolved
base_layer/wallet_ffi/wallet.h Outdated Show resolved Hide resolved
@hansieodendaal
Copy link
Contributor Author

@StriderDM the code panics if position == len, thus the change from> to >=. I tested this by accident.

        if position >= len as u32 {
            error = LibWalletError::from(InterfaceError::PositionInvalidError).code;
            ptr::swap(error_out, &mut error as *mut c_int);
        } else {
            word = CString::new((*seed_words).0[position as usize].clone()).unwrap()
        }

@StriderDM
Copy link
Contributor

@hansieodendaal Good find. Yeah I realized that right after posting my comment.

@philipr-za philipr-za dismissed StriderDM’s stale review November 9, 2021 06:51

Addressed and holding up merge queue maybe

@aviator-app aviator-app bot merged commit d8e0ced into tari-project:development Nov 9, 2021
@hansieodendaal hansieodendaal deleted the ho_ffi_mnemonic_language branch November 9, 2021 07:34
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.

None yet

4 participants