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  244 suites   32m 5s ⏱️
605 tests 589 ✔️ 4 💤 12 ❌
733 runs  717 ✔️ 4 💤 12 ❌

Results for commit ff24221.

♻️ This comment has been updated with latest results.

[github-actions]

Tests

       4 files     299 suites   19s ⏱️
1 358 tests 1 357 ✔️ 1 💤 0 ❌
1 367 runs  1 366 ✔️ 1 💤 0 ❌

Results for commit ff24221.

♻️ This comment has been updated with latest results.

[sonarcloud]

Quality Gate failed

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

See analysis details on SonarCloud

[sonarcloud]

Quality Gate failed

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

See analysis details on SonarCloud

[codecov]

Codecov Report

Attention: Patch coverage is 86.39053% with 23 lines in your changes missing coverage. Please review.

Project coverage is 89.90%. Comparing base (b160863) to head (ff24221).

Files with missing lines	Patch %	Lines
packages/relay/src/lib/constants.ts	55.55%	0 Missing and 4 partials ⚠️
.../relay/src/lib/services/hapiService/hapiService.ts	75.00%	4 Missing ⚠️
...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/relay/src/lib/relay.ts	71.42%	0 Missing and 2 partials ⚠️
packages/env-provider/src/services/index.ts	93.75%	1 Missing ⚠️
...kages/relay/src/lib/clients/cache/localLRUCache.ts	66.66%	0 Missing and 1 partial ⚠️
packages/relay/src/lib/clients/cache/redisCache.ts	75.00%	0 Missing and 1 partial ⚠️
packages/relay/src/lib/clients/mirrorNodeClient.ts	96.55%	0 Missing and 1 partial ⚠️
packages/relay/src/lib/clients/sdkClient.ts	85.71%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2956      +/-   ##
==========================================
- Coverage   89.92%   89.90%   -0.02%     
==========================================
  Files          58       59       +1     
  Lines        3900     3952      +52     
  Branches      780      786       +6     
==========================================
+ Hits         3507     3553      +46     
- Misses        346      348       +2     
- Partials       47       51       +4     

Flag	Coverage Δ
env-provider	93.75% <93.75%> (?)
relay	90.17% <85.12%> (+0.03%)	⬆️
server	87.77% <85.18%> (-0.38%)	⬇️
ws-server	100.00% <100.00%> (ø)

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

Files with missing lines	Coverage Δ
packages/relay/src/lib/eth.ts	88.98% <100.00%> (+0.01%)	⬆️
packages/relay/src/lib/hbarlimiter/index.ts	100.00% <100.00%> (ø)
packages/relay/src/lib/net.ts	100.00% <100.00%> (ø)
...elay/src/lib/services/cacheService/cacheService.ts	95.55% <100.00%> (+0.06%)	⬆️
...kages/relay/src/lib/services/debugService/index.ts	81.42% <100.00%> (+0.54%)	⬆️
.../lib/services/ethService/ethCommonService/index.ts	95.38% <100.00%> (+0.03%)	⬆️
.../lib/services/ethService/ethFilterService/index.ts	85.36% <100.00%> (+0.36%)	⬆️
...ay/src/lib/services/metricService/metricService.ts	97.82% <100.00%> (+0.04%)	⬆️
packages/relay/src/lib/subscriptionController.ts	100.00% <100.00%> (ø)
packages/relay/src/lib/web3.ts	100.00% <100.00%> (ø)
... and 20 more

[victor-yanev]

Mostly looks good, thank you for putting in the effort! Let's keep in mind that the whole purpose of this issue is to make the env variables readonly and unmodifiable, hence I've left some suggestions:

[victor-yanev]

This property is declared as type JSON, which is incorrect. JSON is not a valid type for an object; instead, you should type it as Record<string, string | undefined> or NodeJS.ProcessEnv since process.env is inherently a Record<string, string | undefined>.

[victor-yanev]

Let's also make it readonly - we should never want to override this.

Suggested change

	private envs: JSON;
	private readonly envs: Record<string, string | undefined>;

[victor-yanev]

Let's make this method private and add a static get method which uses it:

public static get(name: string): string | undefined {
  return EnvProvider.getInstance().get(name);
}

In this way usages of this EnvProvider class would be shorter and simpler to follow and we avoid the need to chain getInstance() every time we want to use any of the methods:

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

[victor-yanev]

Let's make this shorter: EnvProviderService -> EnvProvider

[victor-yanev]

QQ: Why are we stringifying and then parsing the process.env?

If the purpose is to copy the process.env object, you can use object de-structuring since it's only containing strings, we don't need to do a "deep" clone with JSON.parse and JSON.stringify:

Suggested change

	this.envs = JSON.parse(JSON.stringify(process.env));
	this.envs = { ...process.env };

[victor-yanev]

QQ: Why would we ever want to delete an env variable, isn't hotReload used for reloading the original configurations in those scenarios?

Also declaring it as a public method which makes it accessible through any class in the source code only for testing purposes is not really advisable.

[victor-yanev]

Again, we don't need to use JSON.parse and JSON.stringify here, and we shouldn't replace the this.envs object, but only modify it:

Suggested change

	public appendEnvsFromPath(configPath: string): void {
	dotenv.config({ path: configPath, override: true });
	const envsToAppend = JSON.parse(JSON.stringify(process.env));
	const merged = Object.assign(this.envs, envsToAppend);
	this.envs = JSON.parse(JSON.stringify(merged));
	}
	public appendEnvsFromPath(configPath: string): void {
	dotenv.config({ path: configPath, override: true });
	const instance = EnvProvider.getInstance();
	Object.entries(process.env).forEach(([key, value]) => {
	instance.dynamicOverride(key, value);
	});
	}

[victor-yanev]

We should move this together with hotReload, remove and appendEnvsFromPath to a helper class in the test directory, these shouldn't be accessible as public methods in the source code.

[victor-yanev]

You are using findConfig('.env') || '', which means that if no .env file is found, an empty string is passed to dotenv.config(). This isn't necessarily a problem, but it might be clearer to raise an error or log a warning when .env is not found, as this might not be the desired behavior.

[victor-yanev]

We can implement a default value or fallback mechanism to handle missing environment variables:

public static getWithDefault(name: string, defaultValue: string): string {
  const value = EnvProvider.get(name);
  if (!value) {
    console.warn(`Environment variable ${name} is not set, using default: ${defaultValue}`);
    return defaultValue;
  }
  return value;
}

We can also throw an error or log a warning when a required environment variable is not set:

public static getRequired(name: string): string {
  const value = EnvProvider.get(name);
  if (!value) {
    throw new Error(`Missing required environment variable: ${name}`);
  }
  return value;
}