Fix for many wallet config errors

[Anynomouss]

Previous behavior:

wallet.rs creates config with updated .api_secret .foreign_api_secret and .owner_api_secret
wallet arguments -h (here) ad -t (top_dir) updated the paths in the config, including .api_secret which was wrongly set to top_dir or in case of -h to current directory. Secondly, later on grin_wallet_controller::command::init is called , even though the config file is passed via args, a new config file is created.

New behavior

All wallet config creation happens purely in wallet.rs using functions defined in config.rs.
The node path is determined flexibly, first checking top_dir if .api_secret exist there, otherwise assuming home/.grin
Paths set by top_dir are relative and not absolute. Meaning the user can move around the folders that contains grin, grin-wallet and its data as long as grin binary does not move relative to the grin-wallet data dir. An alternative would be to make the path absolute.

Issues that are fixed (or related). Quite a few issue can be closed.

#478 [related, not fixed]
#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]

I have been thinking, perhaps this pull might need a bit of reworking/discussing @yeastplume.

1. If there is a config in the current directory, grin-wallet loads the path and the config file from the current dir, even when running -init.
   Would it not be more elegant, to load config from current dir and use it as a template for the newly created config file?
   What I mean is to I) Load the config in the current directory as an object with all its particular settings and use it as template II) update that config object by setting the API data paths, wallet data_dir and 3) writing it to the config to the wallet data dir.

2. In this pull request I moved the writing of the config file from command.rs to wallet.rs. The downside of this change is that with a config in the current directory, no new config is created. Only after reworking the config creation I found that the config as object is passed from wallet.rs to the owner API (although it is ignored when running -init). In contrast the config path is not passed along as argument and instead is imported as global variable which is a bit hacky since a global variable can only have one instance and is not intrinsically linked to a particular wallet or its config object. What would be more elegant is to add the config_path as a variable to the config object, do the updating in wallet.rs and let the writing be done by command.rs. The only possible downside is that the config would contain the users path as part of the config_path which might be considered somewhat sensitive data. Still, I think storing the user path in the config object/structures would long be better because it would mean that any call to the owner API contains anything it needs for the wallet, including the config and its path. So encapsulating the config path within the config object might save headaches when supporting multiple wallet instances for example. I see now the GlobalWalletConfig doe actually contain a variable config_file_path.
   The code of grin-wallet can be quite confusing since I see there are conflicting designs in there, e.g. the previous reworking of the config file generation is not in line with the initial design of the software, hence I should probably rework my changes as well.

Let me know what you think.

[aglkm]

I can't compile with this PR:

error[E0061]: this method takes 2 arguments but 1 argument was supplied
   --> impls/src/lifecycle/default.rs:143:18
    |
143 |         default_config.update_paths(&abs_path);
    |                        ^^^^^^^^^^^^----------- an argument of type `&PathBuf` is missing
    |
note: method defined here
   --> /home/user/grin/grin-wallet-sources/grin_wallet_pr731/grin-wallet/config/src/config.rs:332:9
    |
332 |     pub fn update_paths(&mut self, wallet_home: &PathBuf, node_home: &PathBuf) {
    |            ^^^^^^^^^^^^
help: provide the argument
    |
143 |         default_config.update_paths(&abs_path, /* &PathBuf */);
    |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~

[Anynomouss]

@aglkm That can very well be the case, I reverted some code, sloppy that I pulled that to main.
In any case this pull needs some work and is not ready for review. I either have the config by wallet.rs working updating the paths and writing the config file, but the user gets a warning because owner api does not recognize the config is already created and tries to set its own config with default values via defaults.rs. I need to dig a bit deeper to see how I can have all of them working nicely together without any conflicts and unneeded error messages tot he user.

[Anynomouss]

The pull is ready for review.
Default.rs can still be improved, it does not properly work on its own with -top-dir and init -h, but because the config loading and updating now happens in wallet.rs, and defaults.rs only writes the config it is not a problem. If no config is provided defaults creates its own config.

Tested and works for:
a) grin-wallet.exe init
b) grin-wallet.exe init -h
c) grin-wallet.exe -t my_top_dir init . Replaced "/" with "\" to keep the uses of slashes uniform in the config.
d) grin-wallet.exe init (providing a template config in the current directory)

[yeastplume]

Changes look okay so far, but there are still quite a few test failures, see log from runs above

[Anynomouss]

I will have a look at it. I did not yet read the Rust Book chapter on tests, so I simply forgot to run cargo test and adjust existing tests. I expect it to be little work.

[Anynomouss]

@yeastplume Works now, at least on Windows 11.
I had to make a very minor adjustment to one of the test.

[aglkm]

I ran into the following issues on my Linux machine.

1.

./grin-wallet info
Password: 
20250304 15:13:46.799 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?)

In my grin-wallet.toml file I have a different data_file_dir path but it doesn't take into account:

#where to find wallet files (seed, data, etc)
data_file_dir = "/home/user/grin/temp/wallet_data"

2.

./grin-wallet -t d0/d1/d2 init command creates 2 directories:

ls -l

drwxrwxr-x 3 user user     4096 Mar  4 15:30  d0
drwxrwxr-x 2 user user     4096 Mar  4 15:29 'd0\d1\d2'

3.

I'm not sure about changing to relative paths. What about API or 3rd party software relying on the grin-wallet.toml paths?

[yeastplume]

Tests passing, looks good. @aglkm if you have time, can you see if those issues are still occurring in master and create a separate issue for them to keep them straight.

[aglkm]

This PR was not ready for merging.
In it's current state it causes more issues than it fixes.
All pointed problems are still true. #731 (comment)

[yeastplume]

This PR was not ready for merging. In it's current state it causes more issues than it fixes. All pointed problems are still true. #731 (comment)

I'm usually okay with a bit of instability on master when it's just around the fringes of config, but in this case I think you're right and I was a bit too hasty merging. Going to revert and consider a few things here more carefully.

[Anynomouss]

@aglkm
#1
I see what you mean. Interestingly enough I did not change how the data_path gets updated, so that is a mistake that must have been in the code already which indeed always defaulted to home because of default.rs. I will fix it. I will also some test on Linux since what you describe in 2 does not happen on Windows.

#2
On Windows the paths is created with any number of sub-directories in that path. Perhaps on Linux this be different than on Windows. I have to check myself what you exactly mean with that two paths are created. Probably it is because there are multiple functions that create a path when it is not there and since Linux might work differently, e.g. paths instead of dirs. I already have my suspicions (its default.rs who is naughty).

#3
There is no switch to relative paths, nothing changed. Let me explain it since it is rather confusing:
Original the wallet was updated by defaults.rs (made by @yeastplume), then @quentinlesceller implemented a better more explicit config creation with better error handling (and using relative paths) initiated by wallet.rs. Unfortunately the config did not get passed along properly and therefore was always discarded and overwritten by defaults.rs (which uses absolute paths). In the end I chose to stick with the original setup of absolute paths.

I see I need to do more fixing and testing, I will let you know when you can give it another test @aglkm

[aglkm]

Issue 1 can only be re-created with your code changes, no such problem with the old code.

There's more to add to issue 2. I see the following in grin-wallet.toml file:
api_secret_path = "/home/user/grin/temp/d0\\d1\\d2/.owner_api_secret"

I do see the changes related to absolute vs relative paths (issue 3):
Before:

#where to find wallet files (seed, data, etc)
data_file_dir = "/home/user/grin/temp/top_dir_before/wallet_data"

After:

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

[Anynomouss]

@aglkm I also noticed the replacement not working. I must have reverted something at some point or the change were only locally, I would have sworn it worked at some point. I do make the replacement in the code but it does not work for some reason, probably because it contains escape characters.

In any case thank you for taking the time to do testing, it is appreciated.
This time I will do more extensive testing myself on more than one machine before asking for a review. For whatever reason communication between my wallet and node never has worked on my main PC which is not ideal for testing, hence the need to test on more than one machine.

[Anynomouss]

I am going to take more time for this pull request. I have two versions with different dependency versions giving different results ( one with bug 1 fixed and 3 not working and the other exactly the other way) while having the same code. Also one of them cannot make push request to github anymore.
In addition the wallet generates some messages with the relative path later on while in the config we use absolute paths creating extra confusion. Rust is more capricious than I thought.