SDK NewMnemonic Passphrase Issue

[araskachoi]

Summary of Bug

Issue with the account that is created using NewMnemonic. NewMnemonic does not require a passphrase argument and the passphrase passed to NewAccount inside the method is set to DefaultBIP39Passphrase. The issue that this causes is that the passphrase is now set automatically to an empty string ("") and it cannot be changed. If we decide the use the mnemonic that is outputted by the NewMnemonic method into NewAccount, this causes two accounts to be created, where the first one would have the empty string as the passphrase and the second account will have the passphrase set.

Proposed Solution:
Remove the NewAccount method inside NewMnemonic and have the NewMnemonic method only output the bip39 mnemonic and err (nil if none). Then that mnemonic can be used inside NewAccount - which can be left the way it is.

Would like to discuss here before opening any PRs. Thanks guys

Version

v0.41.3

Steps to Reproduce

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[aaronc]

This looks like a keyring issue. @alessio ?

[alessio]

I need to investigate into this. We've used an empty value for DefaultBIP39Passphrase for ages.

If we decide the use the mnemonic that is outputted by the NewMnemonic method into NewAccount,

But that seems wrong. NewMnemonic already uses NewAccount to generate a private key. Thus what do you mean by that?

[araskachoi]

@alessio that is the exact issue here. the NewMnemonic generates an account that only uses the DefaultBIP39Passphrase or aka an emptystring (""). thus, the account generated by NewMnemonic will always create an account that is unlockable by anyone as long as the node's rpc endpoint is public.

Can we fix this so that we separate the logic? NewMnemonic should only return the mnemonic and err. and then we can use NewAccount separately, that way we can pass in the passphrase properly. Or another way to fix this is to add an argument to the NewMnemonic method to pass in a passphrase, which will be passed to NewAccount.

here is an example (adding passphrase as an argument):

func (ks keystore) NewMnemonic(uid string, language Language, passphrase, hdPath string, algo SignatureAlgo) (Info, string, error) {
	if language != English {
		return nil, "", ErrUnsupportedLanguage
	}

	if !ks.isSupportedSigningAlgo(algo) {
		return nil, "", ErrUnsupportedSigningAlgo
	}

	// Default number of words (24): This generates a mnemonic directly from the
	// number of words by reading system entropy.
	entropy, err := bip39.NewEntropy(defaultEntropySize)
	if err != nil {
		return nil, "", err
	}

	mnemonic, err := bip39.NewMnemonic(entropy)
	if err != nil {
		return nil, "", err
	}


        var info keyring.Info
        if passphrase == "" {
            info, err := ks.NewAccount(uid, mnemonic, DefaultBIP39Passphrase, hdPath, algo)
	    if err != nil {
		return nil, "", err
	    }
        } else {
            info, err := ks.NewAccount(uid, mnemonic, passphrase, hdPath, algo)
	    if err != nil {
		return nil, "", err
    	    }
        }
        
	return info, mnemonic, err
}

[fedekunze]

I agree with the proposed solution here. It's also not clear from the description that the NewMnemonic function will create a new account

[fedekunze]

If we decide the use the mnemonic that is outputted by the NewMnemonic method into NewAccount, this causes two accounts to be created, where the first one would have the empty string as the passphrase and the second account will have the passphrase set.

This is another issue because the NewAccount should check if there's an existing key with the same derived address before creating the account.