Lifecycle Error: Error recovering wallet seed

[bladedoyle]

Describe the bug I get this error when trying to get the recovery seed phrase for a wallet.

To Reproduce This wallet was created using the wallets owner_api "create_wallet" call using the following parameters: {"password": "supersecret", "name": null, "mnemonic": null, "mnemonic_length": 0}

The create_wallet call returns success and the wallet appears to work. I can scan it, and get "info", etc.

Getting the recovery phrase fails with:

grin@grinwallet:~$ grin-wallet recover
Password: 
Wallet command failed: LibWallet Error: Lifecycle Error: Error recovering wallet seed

Expected behavior A recovery phrase should be returned and no error

Additional context This is the seed in case it helps. Password is supersecret

grin@grinwallet:~/.grin/main/wallet_data$ cat wallet.seed 
{
  "encrypted_seed": "f7dfdfbdf73faba595f43d530078a093",
  "salt": "693ff1f71d398171",
  "nonce": "deeaed12ae8443978e495977"
}

Note: If I provide a valid "mnemonic" when calling create_wallet then the "grin-wallet recover" command works as expected.

[deevope]

@bladedoyle We should probably just accept a minimum mnemonic_length = 16 when someone calls create_wallet otherwise we returns an errors. It's a non-sense to "create" a wallet with mnemonic_length = 0

[cliik]

From the documentation on create_wallet():

/// * `mnemonic_length`: Desired length of mnemonic in bytes (16 or 32, either 12 or 24 words).
/// Use 0 if mnemonic isn't being used.

From this, it seems using mnemonic_length = 0 is an expected scenario, if the user wants to create a wallet which cannot export a recovery phrase. I can't think of a reason this scenario would be desired though. It seems reasonable to me to require a minimum length of 16 to eliminate this confusing error. If nobody objects to this change, I can volunteer myself to implement it.

[cliik]

After thinking this over some more, I don't think this should be changed. It makes sense to leave advanced features like this (generating a wallet that cannot export a mnemonic) in the API. I would feel differently if this was an option in the CLI intended to be consumed by less technical users, but its not. The API is inherently a feature-rich advanced toolset, and I think this feature (although quirky) is fine to stay there. Just my $0.02