feat: add an environment variable to specify private key location #1312
Conversation
portalnet/src/utils/db.rs
Outdated
| let unsafe_private_key_file = trin_data_dir.join(UNSAFE_PRIVATE_KEY_FILE_NAME); | ||
| if !unsafe_private_key_file.exists() { | ||
| let trin_private_key_dir = match env::var(TRIN_PRIVATE_KEY_ENV_VAR) { | ||
| Ok(val) => PathBuf::from(val), |
There was a problem hiding this comment.
I think this needs better documentation. Especially here. What exactly is a private key file supposed to look like? aka simply a *.hex file with a single hex encoded, 0x-prefixed private key?
It's probably worth updating the help message for the --unsafe-private-key cli flag to say that it will be overridden if the private key is provided via env var. I think a INFO log here detailing the exact location of the private key being used will help prevent cases where: I forgot that I set an env variable previously, but am expecting the cli / default private key to be used.
portalnet/src/utils/db.rs
Outdated
| if !unsafe_private_key_file.exists() { | ||
| let trin_private_key_dir = match env::var(TRIN_PRIVATE_KEY_ENV_VAR) { | ||
| Ok(val) => PathBuf::from(val), | ||
| Err(_) => trin_data_dir.to_path_buf(), |
There was a problem hiding this comment.
Hmmm, I think if somebody sets an invalid private key file path then we should actually throw an error and force the user to correct the env var rather than silently using the default.
There was a problem hiding this comment.
Ok it should be corrected now
env::var can return 2 things a error as in invalid environment input or it will tell you the environment variable isn't present, in the ladder case would would want to return the default trin path
| @@ -98,8 +99,13 @@ fn get_default_data_dir() -> anyhow::Result<PathBuf> { | |||
| /// If the private key does not exist (eg. brand new trin data dir), | |||
There was a problem hiding this comment.
This comment should be updated.
| @@ -16,6 +16,7 @@ use ethportal_api::{ | |||
| }; | |||
|
|
|||
| const TRIN_DATA_ENV_VAR: &str = "TRIN_DATA_PATH"; | |||
| const TRIN_PRIVATE_KEY_ENV_VAR: &str = "TRIN_PRIVATE_KEY_PATH"; | |||
There was a problem hiding this comment.
My assumption is that this env var is for a path to a private key file. aka my_private_key.hex. But in the code it looks like you're interpreting this path as a custom trin data dir?
There was a problem hiding this comment.
The code should reflect this now
|
I took a shot at your concerns, ready for another look and let me know if I missed anything |
| /// If the private key does not exist (eg. brand new trin data dir), | ||
| /// a random pk is generated and stored. | ||
| fn get_application_private_key(trin_data_dir: &Path) -> anyhow::Result<CombinedKey> { | ||
| let unsafe_private_key_file = trin_data_dir.join(UNSAFE_PRIVATE_KEY_FILE_NAME); | ||
| if !unsafe_private_key_file.exists() { | ||
| let unsafe_private_key_file = match env::var(TRIN_PRIVATE_KEY_ENV_VAR) { |
There was a problem hiding this comment.
I'm still not sure this works. The whole idea behind the trin_data_dir is that it's tied directly to the private key identity contained within it. I'm not sure how compatible this concept of just "providing an arbitrary private key via env var" is with the core concept of the trin_data_dir.
With this implementation, if somebody provides a private key via an env var. Then the node is started with a different identity than the data file to which the data is being written.
Honestly, I'm not even sure if this is a feature we should support.
- i'd prefer to minimize the # of conflicting config options for trin (env vars vs. cli args)
- there's been talk of moving all config into a
trin_config.ymlfile, which I think would be nice
If this feature is something that we want to support, then it seems to me like we'll need to create a new trin_data_dir based off of the env var private key, and then write all of the data to the newly created data dir. this just seems a bit overkill imo.
Personally, I would hold off on implementing this until...
- we confirm that there's a significant ask for this feature
- we update to a
config.ymlfile
What was wrong?
How was it fixed?
By adding an environment variable
TRIN_PRIVATE_KEY_PATHwhich overrides the default trin directory if provided