lidofinance / lido-keys-api

Api for fetching node operators keys from modules
MIT License
19 stars 15 forks source link

refactor!: environment variables validation #216

Open AlexanderLukin opened 11 months ago

AlexanderLukin commented 11 months ago

Refactor environment variable validation rules. Apply consistent patterns to env validation.

The main motivation for this change was as follows:

  1. Make env validation more strict and consistent. Stick to the same validation patterns in different projects.

  2. Provide consistent fallback options for optional variables.

Here are the main changes:

  1. Previously if a user set an empty string as the variable name in the .env file like this:

    VARIABLE_NAME=

    not all variable initializers assigned a correct default value to the variable in this case. Now it is fixed. For the purpose of this improvement, now all optional variable initializers always have the @Transform decorator that provides a default value for the variable. Also for this purpose, the toBoolean function has been refactored to be more similar to the toNumber function.

  2. Make validation of boolean variables more strict. CAUTION: This might be a breaking change for users who use values like "yes" to initialize boolean variables.

  3. Implement the new toArrayOfUrls() function to provide a reasonable default value for variables that should have a list of URLs in their values. This function also fixes a bug with default values for such variables. Previously, if a user set an empty string as a value of such variables, the validation worked incorrectly.

  4. Now we validate that values in PROVIDERS_URLS and CL_API_URLS are indeed URLs.

  5. The CHAIN_ID variable now supports only a pre-defined list of testnet IDs. Add the appropriate Network enum for this purpose. CAUTION: Currently the Keys API doesn't officially support testnets other than Goerli, Holesky, and Mainnet. But this might be a breaking change if some users use Keys API for other testnets.

  6. More strict validation for the PORT and DB_PORT values. CAUTION: This might be a breaking change.

  7. Add the @IsNotEmpty decorator to the required variables.

  8. Fix confusing validation rules for the DB_PASSWORD variable. Add default value.

AlexanderLukin commented 11 months ago

OK, I updated the PR to support the suggestions above. The changes are:

  1. Loosen validation rules for boolean variables. Now numbers "1" and "0" and constants "yes" and "no" are accepted as valid boolean values.

  2. Make DB password value required. Remove the default empty DB password value.

  3. Rename the Network interface to Chain.

Anna, please, take a look when you have time. It's not urgent by the way. I'm completely OK if you do this at the beginning of December.

eddort commented 11 months ago

Good job! I suggest creating a module in lido-nestjs, adding these features, and making a standardized solution for all our projects. Usually, config validators can load fields as files (this feature was needed in Council Daemon). This is a feature I have implemented in the Council Daemon itself. If you combine all these features, you will have a very useful library that will allow you to describe configs in a unified style.

AlexanderLukin commented 11 months ago

Yes, that's a good idea! I agree. I implemented a very similar boilerplate code for this validation also in EVM and operators widget backend. Moving this logic to some common place will allow us to reduce this repetition.

Looks like this needs a bit more internal discussion, but I like the idea. If everyone agrees with this, I'll do that and update this PR once we move this validation logic to lido-nestjs.