WP: Fix for many wallet config errors including solution for -h and -t wrongly setting the node path

[Anynomouss]

#478 [Partly Fixed/answered, cannot be solved easily since generate by toml crate ]
#728 [fixed]
mimblewimble/grin#3803 [fixed]
mimblewimble/grin#3394 [fixed]
mimblewimble/grin#3420 [Abandoned Pull request, my pull replace this one]
mimblewimble/grin#3002 [Partly fixed, not default values part]
mimblewimble/grin#2300 [Could be closed, not dependent on my pull]

Note
The wallet firsts checks a) current directory for config, b) top_dir for config, then c) assumes its in home/.grin.
Option a) suppresses generation of a config file when running grin-wallet.exe init, not sure if that is intentional behavior or that the config should be crated but the one in current dir should be loaded.

[Anynomouss]

Update
#581
#728 [fixed]
mimblewimble/grin#3803 [fixed]
mimblewimble/grin#3394 [fixed]
mimblewimble/grin#3420 [Pull, my own pull replaces this one]
mimblewimble/grin#3002 [Partly fixed, not default values part]
mimblewimble/grin#2300 [Could be closed, not dependent on my pull]
• Fix/workaround for a “bug” in the fs::create_dir_alll create to not recognize backslashes as directory separator on Linux. Fixed by converting the top_dir to always use OS_SEPARATOR. This bug only occurs on Linux and was uncovered by @aglkm
• Fix all Seed generation message, to format the path with OS specific separator in seed.rs and default.rs
• Removing config exists check from wallet_args, all logic is now handled by the cofig::initial_wallet_setup
• Added smart functionality: If run with a config in the current directory, the wallet will load the config file and if run with ‘init’ will use as a template for config generation, meaning port settings etc. get copied from the template config.

Possible things to improve

1. I did not manage to change the top_dir arguments stored in the arguments. Therefore it is update at multiple places in the code. There is a lot of formatting going on in various message functions, but since you normally only get a few messages this should not slow things down much. Perhaps there are ways to reduce some of these formatting calls, I am open to suggestions.
   I used a helper function to format the path to use OS_SEPARATOR. This helper function is used in seed.rs, defaults.rs and wallet.rs, each with a new definition of the function. If there is such things as a crate where other helper functions are put, perhaps I could put it there and avoid redefining it.

2. Config generation for wallet created using the command line are now handled by the wallet.rs. This means that if defaults.rs is called to generate a config, e.g. via the owner API, the behaviors is as before not distinguishing node and wallet directory. It needed I can also adjust the code in default.rs to use config::get_node_path and config::get_wallet_path , but I would have to convert the Config Errors to normal Errors. It here are reasons to make change there as well I could still do it, but if not needed I will move on to look at the node header syncing.

@aglkm If you have time, can you give it another test?

[aglkm]

Issues reported here: #731 (comment)
Fixed: 2 and 3.
Not fixed: 1.

Steps to re-create:

1. Compile the wallet with the PR
2. Init a wallet: ./target/release/grin-wallet -t top-dir init
3. Copy executable into the top dir: cp target/release/grin-wallet ./top-dir/.
4. CD into the top dir: cd top-dir/
5. Make sure there's no wallet seed in the default directory (/home/user/.grin/main/wallet_data/) and run the wallet: ./grin-wallet info
   You will see the following:

20250518 12:32:59.411 ERROR grin_wallet_impls::lifecycle::seed - wallet seed file /home/user/.grin/main/wallet_data/wallet.seed could not be opened (grin-wallet init). Run "grin-wallet init" to initialize a new wallet.
Wallet command failed: LibWallet Error: Lifecycle Error: Error opening wallet (is password correct?)

So the wallet is ignoring the "top-dir" directory (/home/user/grin/grin-wallet-sources/grin-wallet-pr732/grin-wallet/top-dir) and still is trying to use the default one.

In grin-wallet.toml file the path is correct:

#where to find wallet files (seed, data, etc)
data_file_dir = "/home/user/grin/grin-wallet-sources/grin-wallet-pr732/grin-wallet/top-dir/wallet_data"

Another issue:
Make sure you are one level up from the "top-dir" directory, then run: ./top-dir/grin-wallet -t top-dir info
You will get:

Unable to load wallet configuration: Error serializing configuration: /home/user/grin/grin-wallet-sources/grin-wallet-pr732/grin-wallet/top-dir/grin-wallet.toml already exists in the target directory (/home/user/grin/grin-wallet-sources/grin-wallet-pr732/grin-wallet/top-dir/wallet_data). Please remove it first (Run `grin-wallet init` to create a new wallet)

[Anynomouss]

@aglkm Thanks for explaining issue 1, I misunderstood you before.

Basically grin-wallet will always assume that if you do not provide the '-t' top-dir command that it should use the wallet in the default directory. What you are proposing is a smart auto-detect wallet_data directory function.
This new smart function would mean grin-wallet would first test if the current directory contains a wallet_data folder, if present grin-wallet will assume this is the wallet you want to use, if not grin-wallet will default to the default wallet data directory. This would be new wallet behavior, I could implement it if all think this is desirable.
The advantage I can think of is that it would make it much easier for beginning user to run for example grin-wallet -h in a directory and afterwards the user does not need to specify the directory, grin-wallet will always use the wallet in the current directory of his executable.
The downside I can think of is that this smart behavior is implicit and not explicit, a user might not know it and even get confused why not his default wallet is loaded. I could implement this smart functionality but I think at least I should combine it with a warning warning to the users. Something along the line 'wallet_data' detected in current dir, defaulting to use this wallet. What do you think @yeastplume and @aglkm , would that be the desired behavior, or just keep it as is?

Another issue:
Make sure you are one level up from the "top-dir" directory, then run: ./top-dir/grin-wallet -t top-dir info
You will get:

This is actually desired behavior. What I mean is that all paths you provide to grin-wallet should be given relative to your current working directory, not relatively to the executable. For example, if you are in the top_dir, you would have to run ../grin-wallet.exe -t ../top-dir info. Additionally using paths relative to the working directory allows power users to do some cool tricks. For example:

1. I run grin-wallet.exe -t top_dir init (with a template config file in the current directory of grin-wallet.exe)
   What will happen is that grin-wallet will create a new config file in the top-dir which uses the config file in the working directory as a template, for example keeping specific peers, ports etc. in the config file. Only the node path, wallet path and tor path will be updated.
2. cd top_dir
3. run ..grin-wallet.exe -t ../top_dir info
   Now there will be no template file in your current working directory since you are in directory top-dir, so the wallet will run using your new config file

I could change this behavior, but I think it is customary for executable to use paths relative to the current working directory and not paths relative to the directory of the executable. Therefore I would propose not to change this behavior and keep it as is. What do you think @yeastplume?

[aglkm]

It is documented in grin-wallet docs that the wallet will check the current directory first and then the
default one.

When running any grin-wallet command, grin will check the working directory if these files exist. If not, it will use the default location ~/.grin.

This is how it works currently.

What is this PR proposing: if you wish to use local (non default) config files, you have to explicitly specifying current working directory every time you execute a wallet, while you are already locating there. I disagree with this.

./top-dir/grin-wallet -t top-dir info

Here ☝️ I've specified the path to grin-wallet executable relatively to the current working directory and provided the path to "top-dir" relatively to the current working directory as well, not an executable. The current wallet behaviour has no such issue.

You wrote:

What will happen is that grin-wallet will create a new config file in the top-dir which uses the config file in the working directory as a template, for example keeping specific peers, ports etc. in the config file. Only the node path, wallet path and tor path will be updated.

Wallet shouldn't try to think instead of a user, because a desired behaviour could be different. I could want a fresh config file, but instead wallet will create it from an old "template" - a config file which is placed in the current directory. It's more straightforward to let user to copy the old config file if he wishes to preserve the settings.

You wrote:

Now there will be no template file in your current working directory since you are in directory top-dir, so the wallet will run using your new config file

The difference between a template file and a config file: a config file becoming a template file if the user is trying to create a new wallet while also the config file exists in the current directory. If we want to have "template" files it could be better to introduce a new option, so the user can directly specify what he wants to do and do not let the wallet to decide.

As a user, it could be confusing, that the wallet is silently trying to use a config file as a template file.

There's another thing that need attention. When you are creating a config file from a "template" it will create a relative path in grin-wallet.toml file, like that:

#where to find wallet files (seed, data, etc)
data_file_dir = "top_dr"

What's the purpose of this change? This could potentially break the things, e.g. API.

[Anynomouss]

It is documented in grin-wallet docs that the wallet will check the current directory first and then the
default one.

Interesting, this functionality is not present in the code. It does make sense to add it since many users might expect the wallet to be smart enough to detect it. I will add it to the code, it is similar smart behavior as I have implemented for detecting the path to the node.

Wallet shouldn't try to think instead of a user, because a desired behavior could be different. I could want a fresh config file, but instead wallet will create it from an old "template" - a config file which is placed in the current directory. It's more straightforward to let user to copy the old config file if he wishes to preserve the settings.

This sounds very contradicting when you are saying that you want smart behavior for the wallet to detect the path to the node and smart behavior to detect if a local wallet folder is present, yet you do not want smart behavior? The whole point of this pull request is to fix -t and -here and add smart behavior to grin-wallet to let it behave like the user expect it should.

Smart behavior:

• Looking where to find the node, first in the current directory, if not there in the default directory
• Looking which wallet to use, first try current directory if not there, use the default directory (smart behavior that should have been there already according to the documentation)
• Detecting config file in the executable path and using its to open the right wallet, or as template when creating a new wallet (extension on existing behavior)

Using a config in the executable dir with -init as template is an extension of the behavior that when running without -init the config file will indicate which wallet to open and with what settings. Again, this is not me making real change, just extending on existing behavior. To give you an example, lets say you copy grin-wallet.ex inside the 'wallet_data' directory, it will exactly perform the smart behavior you are asking for, it will detect the config and as such load the wallet in which data directory the executable resides. So this is in fact the described behavior you are asking for.
If we keep this behavior and add detection of the 'wallet_data' in the current working directory, this will mean that both if you put the executable in the data directory itself, or in its top directory, it will automatically detect and load the right wallet. I think that is indeed good user friendly behavior.

As a user, it could be confusing, that the wallet is silently trying to use a config file as a template file.
I think this is the case for all smart behavior. Smart behavior is implicit since the user does not explicitly ask for it using an input argument. The solution is simple, I will ad Warning or Info level messages to the user to notify them each time any smart behavior is triggered.
The alternative is not having any smart behavior at all which is very, very bad for the user experience and conflicts with the behavior described in the readme file.
Also we have to be realistic, what is the chance of a user copying a config file in the working directory of without knowing about this smart behavior creating a new wallet? I think the chance of that happening is very small.

More importantly, lets make grin-wallet user friendly for those 99% of user that expect the grin-wallet to emulated some minimal level of intelligence. And also lets not forget that the current situation is that -t and -here are completely broken, which is the worst user experience you can offer. We can only go uphill from the current situation.