[Bug]: "NODE_HOME" is not the correct environment variable to get the home path from.

[SpicyLemon]

Is there an existing issue for this?

• I have searched the existing issues

What happened?

In GetNodeHomeDirectory (in client/v2/helpers/home.go), if no --home flag is provided, it then checks the NODE_HOME environment variable.

However, with viper (and the current simapp setup), the correct environment variable would be just HOME, or if the chain uses the SetEnvPrefix viper functionality (which is used in client.Context#WithViper), it will be <prefix>_HOME.

Since GetNodeHomeDirectory was written to be called in an init(), you can't rely on viper being set up yet. Use of the global viper instance is bad anyway (because any call to viper.New() will overwrite it). So it'd be better to just provide a mechanism that allows for a different env var to be used, or maybe just a different prefix (e.g. <prefix>_HOME if provided, or just HOME if not).

Cosmos SDK Version

v0.50.8 and main at f772a0a2fc

How to reproduce?

I'm not sure what to do to actually demonstrate problems with this, but there will be a discrepancy between the home dirs used for various parts of the app depending on if a --home flag is provided, or the HOME environment variable is set.

E.g. In the absence of a --home flag, in app.New, the home directory is looked up from viper which will use the HOME env var, but the client config will ultimately be looking at the NODE_HOME env var and thus will just end up using the default location.

[julienrbrt]

Yes, I know about this small dispertancy. However as you said at init and before any cobra logic we don't have any flag nor a viper (so we don't know any prefix).
Falling back to simply $HOME is as well not correct and can even be wrong as it defaults to the home directory of the user (and resetting this may cause problem -- try to do a simd config get home)

What we did in simapp/v2, is set in app.go home on viper to the default folder got by the helper. I'd advise you to do the same on your chain.
Because of the way things are init, we cannot rely on getting the home so late in the flow.

[julienrbrt]

I can try again to update the helper to just use HOME but that still leaves the prefix issue, and I didn't really want to make it a global variable as it makes the UX weird as well.

Currently if you don't use the default value nor a flag, you just need to set both env variable

[SpicyLemon]

I guess my sticking point is that NODE_HOME is always the wrong environment variable there.
Maybe the env-var prefix could also provided to GetNodeHomeDirectory. It's hard for me to think of a scenario where that string isn't just hard-coded in the root command anyway and will almost never change over the life of the chain. Yeah, hard-coding the same string in a couple places stinks a bit, but since it's solidly static anyway, it's not too bad.

[julienrbrt]

If you find it okay UX, then let's do it 👍🏾

[julienrbrt]

I guess my sticking point is that NODE_HOME is always the wrong environment variable there.

That's only true if you use a prefix. Otherwise, the issue is that you cannot know if you are reading an environment variable or the actual home directory (an issue that existed before too), as they are both called HOME on linux and darwin.