feat: extend paymaster follow-up

[natanasow]

Description

There were several items that had to be resolved on #5016, but due to the urgency of it, we decided to create a follow-up PR.

Related issue(s)

Fixes #4977

Testing Guide

Changes from original design (optional)

N/A

Additional work needed (optional)

N/A

Checklist

• I've assigned an assignee to this PR and related issue(s) (if applicable)
• I've assigned a label to this PR and related issue(s) (if applicable)
• I've assigned a milestone to this PR and related issue(s) (if applicable)
• I've updated documentation (code comments, README, etc. if applicable)
• I've done sufficient testing (unit, integration, etc.)

[github-actions]

Test Results

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ❌ ±0 

Results for commit 55be1d3. ± Comparison against base commit 730d785.

♻️ This comment has been updated with latest results.

[jasuwienas]

lgtm

[quiet-node]

One additional issue we should address in this follow-up is the logging of sensitive keys in the config service. At startup, the config service prints all configuration values. Since PAYMASTER_ACCOUNTS may include private keys, those keys should be masked in the logs.

We should ensure that only private keys are masked. Other fields such as account ID or allowance should remain visible, as they can be helpful during debugging.

[natanasow]

One additional issue we should address in this follow-up is the logging of sensitive keys in the config service. At startup, the config service prints all configuration values. Since PAYMASTER_ACCOUNTS may include private keys, those keys should be masked in the logs.

We should ensure that only private keys are masked. Other fields such as account ID or allowance should remain visible, as they can be helpful during debugging.

Fixed.

[simzzz]

Looks good, I left two suggestions

[quiet-node]

Looking back at this, PAYMASTER_ACCOUNTS and PAYMASTER_ACCOUNTS_WHITELISTS are both simple string arrays, but there is no defined contract around the positions. For example, there is nothing enforcing that index 0 must be the account, index 1 the type, index 2 the key, and so on. This means values can be passed in any order and ConfigService will still accept them, only failing at runtime.

We should address this so that invalid configurations are caught at build time instead.

[natanasow]

Added start-up validation.

[jasuwienas]

Now we have to be aware that:

1. Every variable passed to our app through environment variables will be printed (and therefore exposed) by default, whether we intend it or not.
2. We also need to know where and which variable must be updated to prevent it. In practice, you have to modify the variable in loggerService, not in configService, which makes the setup even more confusing (keeping it as it is would make more sense if the logger service depended on an abstraction like configInterface.sensitiveFields instead of the actual definition with hardcoded field names here)

Overall, it's usually safer to do it the other way around. For example, having a public flag next to the variable definition would make it printable, while leaving it omitted would mask it by default.Now we can unintentionally expose critical value (and it can happen in a lots of unpredictable scenarios, for example once we start storing logs in Graylog, even local execution of the application could generate dangerous leaks if a config variable is added before the logger service is properly configured. And when you add new variable you ususally start with that...

[natanasow]

1. That's how it's worked so far, right?
2. It makes more sense to me to let them in loggerService because LoggerService is doing the printing and only it requires this information, whether the field is sensitive or not.

Adding a new flag for each config var is so much copy-pasted code IMO. If the team decides it's a better solution, I'll implement it.

@quiet-node @simzzz, what do you think?

[simzzz]

I think the point is good and having to remember to add sensitive fields to loggerService is not optimal, and opt-in to printing would be more secure, however it's out of scope for this PR and changing the entire logging architecture seems unnecessary. Maybe you could raise a ticket and promote it so that we can discuss it further? @jasuwienas

[codecov]

Codecov Report

❌ Patch coverage is 99.03846% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...kages/config-service/src/services/loggerService.ts	97.56%	0 Missing and 1 partial ⚠️

@@            Coverage Diff             @@
##             main    #5032      +/-   ##
==========================================
+ Coverage   95.81%   95.91%   +0.09%     
==========================================
  Files         144      144              
  Lines       24662    24717      +55     
  Branches     1966     1983      +17     
==========================================
+ Hits        23631    23707      +76     
+ Misses       1003      982      -21     
  Partials       28       28              

Flag	Coverage Δ
config-service	98.48% <98.80%> (-0.17%)	⬇️
relay	92.27% <95.00%> (-0.05%)	⬇️
server	88.39% <ø> (ø)
ws-server	97.90% <ø> (+1.27%)	⬆️

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

Files with missing lines	Coverage Δ
packages/config-service/src/services/index.ts	97.02% <100.00%> (+0.80%)	⬆️
packages/relay/src/lib/clients/sdkClient.ts	96.07% <100.00%> (ø)
...vices/ethService/ethCommonService/CommonService.ts	95.32% <100.00%> (+0.07%)	⬆️
...kages/config-service/src/services/loggerService.ts	94.91% <97.56%> (-5.09%)	⬇️

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.