feat: capture env variables into sigleton and reusable constants

[natanasow]

Description:

Currently, we are dynamically getting the environment variables and parsing/casting them every time. This is inefficient and also, more importantly, allows env configurations to be dynamically changed, which can modify the behavior of the relay based on the current values of the env vars, which can be a potential exploit.

Solution:

We should change this to only capture them once when the relay is initialized and reuse those as constants

Related issue(s):

Fixes #2686

Notes for reviewer:

• created a new package that holds the env-provider singleton
• the env-provider package is added as a dependency to relay, server, and ws-server
• in the perfect world, the env-provider must be immutable and should not provide an interface for overriding or extending once-loaded environment variables but there are many tests where we dynamically override envs in order to test specific cases so we should add these interfaces as well
• the main review should be applied to the env-provider package
• almost all of the changes are just replaced values from process.env.<VAR_NAME> to EnvProviderService.getInstance().get(<VAR_NAME>)
• in the test cases if overrides are needed, they are done like that - process.env.CHAIN_ID = '0x160c' is transformed to EnvProviderService.getInstance().dynamicOverride('CHAIN_ID', '0x160c')

Checklist

• Documented (Code comments, README, etc.)
• Tested (unit, integration, etc.)

[github-actions]

Acceptance Tests

  19 files  248 suites   32m 55s ⏱️
606 tests 595 ✔️ 4 💤 7 ❌
689 runs  678 ✔️ 4 💤 7 ❌

Results for commit 877f95d.

♻️ This comment has been updated with latest results.

[github-actions]

Tests

       4 files     299 suites   19s ⏱️
1 355 tests 1 354 ✔️ 1 💤 0 ❌
1 364 runs  1 363 ✔️ 1 💤 0 ❌

Results for commit 877f95d.

♻️ This comment has been updated with latest results.

[sonarqubecloud]

Quality Gate failed

Failed conditions
5.8% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

[Nana-EC]

Nice work.
I agree with most of Victors comments

[ebadiere]

Thank you for doing this. I think we should also have a --help option that can further explain this environment variables and perhaps include examples. It should probably done in another github issue.

[AlfredoG87]

I agree with all comments from @victor-yanev and @Nana-EC

maybe I was not part of the discussion and design however, would like to express my opinions.

I agree that this should be used by all other packages ( relay, server and ws-server) hence the need to keep it as a separate package itself and not only as an internal service of the server package.

When I thought about the need of this, did so thinking it as a a Service and calling it ConfigService that had the following responsibilities:

1. Read .env file
2. Make sure all required configurations are present.
3. Store defaults and parse override.
4. "Know" what configurations are available for what services/packages.
5. Print at startup the onto the logs the FINAL values for all needed configurations so is easy to debug and troubleshoot, and using the logs an operator is able to see what configurations are being used. (Secrets should be obscured before printing, so the configService provider should also know which ones are secrets)
6. Provide an interface for getting such values.

I understand that the scope of this PR might be different and as I said, am late to the discussion, however would like my comments to be taken into consideration to see what it makes sense and what not, if anything I would strongly argue against as a just passtrough service to GET the env values, but to make it a full fledged Configuration Service provider that is aware of the config values, its defaults and exposes an Enum or record with an existing list of configs, this way we minimize the risk of duplications, we fail fast if parsing fails, and we print the values for the envs (except secrets those should be oscured)

example of usage in my mind:
instead of:

const shouldDefaultToConsensus = EnvProvider.get('ETH_CALL_DEFAULT_TO_CONSENSUS_NODE') === 'true';

it would be:

const shouldDefaultToConsensus = ConfigService.get(RelayConfigs.ETH_CALL_DEFAULT_TO_CONSENSUS_NODE);

(where RelayConfigs is a known enum) and the values are guaranteed to be present by either defaults overrides or fast failing at (ServiceConfig startup).

[natanasow]

I agree with all comments from @victor-yanev and @Nana-EC

maybe I was not part of the discussion and design however, would like to express my opinions.

I agree that this should be used by all other packages ( relay, server and ws-server) hence the need to keep it as a separate package itself and not only as an internal service of the server package.

When I thought about the need of this, did so thinking it as a a Service and calling it ConfigService that had the following responsibilities:

1. Read `.env` file

2. Make sure all required configurations are present.

3. Store defaults and parse override.

4. "Know" what configurations are available for what services/packages.

5. Print at startup the onto the logs the `FINAL` values for all needed configurations so is easy to debug and troubleshoot, and using the logs an operator is able to see what configurations are being used. (Secrets should be obscured before printing, so the configService provider should also know which ones are secrets)

6. Provide an interface for getting such values.

I understand that the scope of this PR might be different and as I said, am late to the discussion, however would like my comments to be taken into consideration to see what it makes sense and what not, if anything I would strongly argue against as a just passtrough service to GET the env values, but to make it a full fledged Configuration Service provider that is aware of the config values, its defaults and exposes an Enum or record with an existing list of configs, this way we minimize the risk of duplications, we fail fast if parsing fails, and we print the values for the envs (except secrets those should be oscured)

example of usage in my mind: instead of:

const shouldDefaultToConsensus = EnvProvider.get('ETH_CALL_DEFAULT_TO_CONSENSUS_NODE') === 'true';

it would be:

const shouldDefaultToConsensus = ConfigService.get(RelayConfigs.ETH_CALL_DEFAULT_TO_CONSENSUS_NODE);

(where RelayConfigs is a known enum) and the values are guaranteed to be present by either defaults overrides or fast failing at (ServiceConfig startup).

I agree with all comments from @victor-yanev and @Nana-EC

maybe I was not part of the discussion and design however, would like to express my opinions.

I agree that this should be used by all other packages ( relay, server and ws-server) hence the need to keep it as a separate package itself and not only as an internal service of the server package.

When I thought about the need of this, did so thinking it as a a Service and calling it ConfigService that had the following responsibilities:

1. Read `.env` file

2. Make sure all required configurations are present.

3. Store defaults and parse override.

4. "Know" what configurations are available for what services/packages.

5. Print at startup the onto the logs the `FINAL` values for all needed configurations so is easy to debug and troubleshoot, and using the logs an operator is able to see what configurations are being used. (Secrets should be obscured before printing, so the configService provider should also know which ones are secrets)

6. Provide an interface for getting such values.

I understand that the scope of this PR might be different and as I said, am late to the discussion, however would like my comments to be taken into consideration to see what it makes sense and what not, if anything I would strongly argue against as a just passtrough service to GET the env values, but to make it a full fledged Configuration Service provider that is aware of the config values, its defaults and exposes an Enum or record with an existing list of configs, this way we minimize the risk of duplications, we fail fast if parsing fails, and we print the values for the envs (except secrets those should be oscured)

example of usage in my mind: instead of:

const shouldDefaultToConsensus = EnvProvider.get('ETH_CALL_DEFAULT_TO_CONSENSUS_NODE') === 'true';

it would be:

const shouldDefaultToConsensus = ConfigService.get(RelayConfigs.ETH_CALL_DEFAULT_TO_CONSENSUS_NODE);

(where RelayConfigs is a known enum) and the values are guaranteed to be present by either defaults overrides or fast failing at (ServiceConfig startup).

Solid observations and proposals 🙏. Our next actions are defined here #3023 and they cover almost all of your concerns. We'll stick to your proposals and ping you for a PR review, once we're ready. It'll be done in another PR because that one will become enormously bigger.

cc: @ebadiere

[konstantinabl]

Can you add a test for non-existing env?

[sonarqubecloud]

Quality Gate failed

Failed conditions
3.9% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

[codecov]

Codecov Report

Attention: Patch coverage is 74.17219% with 39 lines in your changes missing coverage. Please review.

Project coverage is 84.93%. Comparing base (f1794d8) to head (877f95d).

Files with missing lines	Patch %	Lines
.../relay/src/lib/services/hapiService/hapiService.ts	30.76%	4 Missing and 5 partials ⚠️
packages/relay/src/lib/clients/mirrorNodeClient.ts	76.19%	0 Missing and 5 partials ⚠️
packages/relay/src/lib/constants.ts	55.55%	0 Missing and 4 partials ⚠️
packages/relay/src/lib/eth.ts	50.00%	0 Missing and 4 partials ⚠️
...s/relay/src/lib/services/hbarLimitService/index.ts	25.00%	0 Missing and 3 partials ⚠️
...s/server/src/koaJsonRpc/lib/methodConfiguration.ts	62.50%	0 Missing and 3 partials ⚠️
packages/config-service/src/services/index.ts	86.66%	1 Missing and 1 partial ⚠️
packages/relay/src/lib/clients/sdkClient.ts	71.42%	1 Missing and 1 partial ⚠️
packages/relay/src/lib/poller.ts	33.33%	0 Missing and 2 partials ⚠️
packages/relay/src/lib/relay.ts	66.66%	0 Missing and 2 partials ⚠️
... and 3 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2956      +/-   ##
==========================================
+ Coverage   84.83%   84.93%   +0.09%     
==========================================
  Files          59       60       +1     
  Lines        3937     3975      +38     
  Branches      786      788       +2     
==========================================
+ Hits         3340     3376      +36     
- Misses        357      359       +2     
  Partials      240      240              

Flag	Coverage Δ
config-service	86.66% <86.66%> (?)
relay	85.03% <68.86%> (+0.09%)	⬆️
server	83.48% <84.00%> (+0.04%)	⬆️
ws-server	97.91% <100.00%> (+0.04%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
packages/relay/src/lib/hbarlimiter/index.ts	100.00% <100.00%> (ø)
packages/relay/src/lib/net.ts	100.00% <100.00%> (+12.50%)	⬆️
...elay/src/lib/services/cacheService/cacheService.ts	92.53% <100.00%> (+0.05%)	⬆️
...kages/relay/src/lib/services/debugService/index.ts	76.81% <100.00%> (+0.34%)	⬆️
.../lib/services/ethService/ethCommonService/index.ts	89.23% <100.00%> (+0.08%)	⬆️
.../lib/services/ethService/ethFilterService/index.ts	79.76% <100.00%> (+0.24%)	⬆️
...ay/src/lib/services/metricService/metricService.ts	92.68% <100.00%> (+0.18%)	⬆️
packages/relay/src/lib/subscriptionController.ts	100.00% <100.00%> (ø)
packages/relay/src/lib/web3.ts	100.00% <100.00%> (ø)
packages/relay/src/utils.ts	100.00% <100.00%> (ø)
... and 20 more