feat: implement configuration to pre-populate the cache with spending plans #3058
Conversation
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech> # Conflicts: # packages/relay/src/lib/db/repositories/hbarLimiter/hbarSpendingPlanRepository.ts # packages/relay/src/lib/services/hbarLimitService/index.ts # packages/relay/tests/lib/services/hbarLimitService/hbarLimitService.spec.ts
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech> # Conflicts: # packages/relay/src/lib/db/repositories/hbarLimiter/hbarSpendingPlanRepository.ts # packages/relay/src/lib/services/hbarLimitService/index.ts # packages/relay/tests/lib/services/hbarLimitService/hbarLimitService.spec.ts
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech> # Conflicts: # packages/relay/src/lib/db/repositories/hbarLimiter/ethAddressHbarSpendingPlanRepository.ts # packages/relay/src/lib/db/repositories/hbarLimiter/hbarSpendingPlanRepository.ts # packages/relay/src/lib/db/repositories/hbarLimiter/ipAddressHbarSpendingPlanRepository.ts # packages/relay/src/lib/services/hbarLimitService/index.ts # packages/relay/tests/lib/repositories/hbarLimiter/hbarSpendingPlanRepository.spec.ts # packages/relay/tests/lib/sdkClient.spec.ts # packages/relay/tests/lib/services/hbarLimitService/hbarLimitService.spec.ts
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech> # Conflicts: # packages/relay/src/lib/db/repositories/hbarLimiter/ethAddressHbarSpendingPlanRepository.ts # packages/relay/src/lib/db/repositories/hbarLimiter/hbarSpendingPlanRepository.ts # packages/relay/src/lib/db/repositories/hbarLimiter/ipAddressHbarSpendingPlanRepository.ts # packages/relay/src/lib/services/hbarLimitService/index.ts # packages/relay/tests/lib/repositories/hbarLimiter/hbarSpendingPlanRepository.spec.ts # packages/relay/tests/lib/sdkClient.spec.ts # packages/relay/tests/lib/services/hbarLimitService/hbarLimitService.spec.ts
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
… plans Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
victor-yanev
requested review from
AlfredoG87,
ebadiere,
lukelee-sl and
a team
as code owners
victor-yanev
changed the base branch from
main
to
2970-Note-what-configurations-are-used
…ement-configuration-to-pre-populate-the-cache-with-spending-plans' Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
…ement-configuration-to-pre-populate-the-cache-with-spending-plans' Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
…m main branch Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
| @@ -0,0 +1,28 @@ | |||
| [ | |||
| { | |||
| "id": "c758c095-342c-4607-9db5-867d7e90ab9d", | |||
There was a problem hiding this comment.
Hmm, are we now expected to include the plan ID in the config file? How would someone unfamiliar with the codebase, or loading the config file for the first time, come up with such IDs? I was under the impression that these IDs are auto-generated by the Relay and only used internally.
There was a problem hiding this comment.
Well, there are two approaches in which this could be handled:
- Clear all spending plans from the cache on every start-up and re-populate them from the config file:
- in this case we don't need to specify IDs in the config file, they can be randomly generated on each startup
- historical logs on mainnet and testnet will be harder to debug because we would have generated new IDs on redeploys / restarted pods and the IDs we have from the logs before this moment wouldn't be valid anymore
- if Kubernetes pods are restarted - the cache would be re-populated with the same spending plans but with new IDs and we would lose track of the current spending as it will also be restarted
- Only populate the "delta" from the config file (i.e., delete "obsolete" spending plans which are no longer present in the config file and create new spending plans which were not specified before):
- in this case we need to have an ID specified in the config file, otherwise we cannot know if a given spending plan was already populated or not in the cache
- historical logs on mainnet and testnet will be easy to debug since the same ID we are seeing in the logs, will be still valid and we can retrieve the spending plan from the cache by that ID
- the Kubernetes pods can be freely restarted without affecting the behavior of the hbar limiter, as old spending plans wouldn't be cleared and only new ones that have appeared in the config file will be populated
There was a problem hiding this comment.
How would someone unfamiliar with the codebase, or loading the config file for the first time, come up with such IDs
You don't need to be familiar with the codebase to come up with a random UUID: https://www.uuidgenerator.net/version4
I was under the impression that these IDs are auto-generated by the Relay and only used internally.
They are if an "id" field is not specified in the config file.
There was a problem hiding this comment.
If I understand this correctly, with option 2, the config file can be the single source of truth regarding the spending plan.
There was a problem hiding this comment.
discussed offline and agreed on option 2 mentioned above. However, needed to come up with a plan to construct the plan ID beforehand and make it as a required field.
Co-authored-by: Logan Nguyen <logan.nguyen@swirldslabs.com> Signed-off-by: Victor Yanev <161485803+victor-yanev@users.noreply.github.com>
Co-authored-by: Logan Nguyen <logan.nguyen@swirldslabs.com> Signed-off-by: Victor Yanev <161485803+victor-yanev@users.noreply.github.com>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
…ment-configuration-to-pre-populate-the-cache-with-spending-plans Signed-off-by: Victor Yanev <victor.yanev@limechain.tech> # Conflicts: # packages/relay/src/lib/db/repositories/hbarLimiter/hbarSpendingPlanRepository.ts # packages/relay/tests/lib/repositories/hbarLimiter/hbarSpendingPlanRepository.spec.ts
…ment-configuration-to-pre-populate-the-cache-with-spending-plans Signed-off-by: Victor Yanev <victor.yanev@limechain.tech> # Conflicts: # packages/relay/src/lib/db/repositories/hbarLimiter/hbarSpendingPlanRepository.ts # packages/relay/tests/lib/repositories/hbarLimiter/hbarSpendingPlanRepository.spec.ts
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>
|
| ```json | ||
| [ | ||
| { | ||
| "name": "partner name", |
There was a problem hiding this comment.
Just a friendly reminder to ensure we don't forget to add the ID field here in case if we want to make the plan ID a required property for the preconfigured plan.
| ] | ||
| ``` | ||
|
|
||
| ### Spending Limits of Different Tiers |
There was a problem hiding this comment.
Let's maintain consistency with the subscription packages. Currently, "tiers" and "types" are used interchangeably to describe different subscription packages. Let's agree on one term for clarity. I personally prefer "tiers," as it better reflects the hierarchy within the subscription structure.
| const censoredKey = key.replace(Utils.IP_ADDRESS_REGEX, '<REDACTED>'); | ||
| const censoredValue = JSON.stringify(value).replace(/"ipAddress":"[^"]+"/, '"ipAddress":"<REDACTED>"'); |
There was a problem hiding this comment.
I like the idea!
Just a concern but it seems that the key hasn’t been censored up until this point, but it has already been passed into this function, meaning it’s been floating around in the codebase. I just want to ensure that from the moment the key is constructed to when it gets censored, we don’t accidentally log any IP addresses.
| ); | ||
| const censoredKey = key.replace(Utils.IP_ADDRESS_REGEX, '<REDACTED>'); | ||
| const censoredValue = JSON.stringify(value).replace(/"ipAddress":"[^"]+"/, '"ipAddress":"<REDACTED>"'); | ||
| const message = `Returning cached value ${censoredKey}:${censoredValue} on ${callingMethod} call`; |
There was a problem hiding this comment.
Could the ${censoredKey}:${censoredValue} be duplicating or overlapping with the same values?
| @@ -0,0 +1,315 @@ | |||
| /* | |||
There was a problem hiding this comment.
If this is also a service, should we consider moving it into the services/ folder? Perhaps something like services/config/hbarSpendService.ts?
| private readonly TTL = -1; // -1 means no TTL, i.e. the data will not expire | ||
| private readonly DEFAULT_SPENDING_PLANS_CONFIG_FILE = 'spendingPlansConfig.json'; |
There was a problem hiding this comment.
Let's not foget JSDoc for these vars
| private readonly TTL = -1; // -1 means no TTL, i.e. the data will not expire | ||
| private readonly DEFAULT_SPENDING_PLANS_CONFIG_FILE = 'spendingPlansConfig.json'; |
There was a problem hiding this comment.
Since DEFAULT_SPENDING_PLANS_CONFIG_FILE is only used once for the filename in loadSpendingPlansConfig(), can we simplify it to something like:
| private readonly TTL = -1; // -1 means no TTL, i.e. the data will not expire | |
| private readonly DEFAULT_SPENDING_PLANS_CONFIG_FILE = 'spendingPlansConfig.json'; | |
| private readonly TTL = -1; // -1 means no TTL, i.e. the data will not expire | |
| private readonly SPENDING_PLANS_CONFIG_FILE = process.env.HBAR_SPENDING_PLANS_CONFIG_FILE || 'spendingPlansConfig.json'; |
Then instead of initializing a new filename variable in loadSpendingPlansConfig(), we can simply reuse this.SPENDING_PLANS_CONFIG_FILE.
Just a suggestion I'm fine with current flow.
| @@ -27,7 +27,7 @@ import { RequestDetails } from '../../../types'; | |||
|
|
|||
| export class EthAddressHbarSpendingPlanRepository { | |||
| private readonly collectionKey = 'ethAddressHbarSpendingPlan'; | |||
| private readonly threeMonthsInMillis = 90 * 24 * 60 * 60 * 1000; | |||
| private readonly oneDayInMillis = 24 * 60 * 60 * 1000; | |||
Description:
Implements logic to pre-populate the cache with spending plans specified in a JSON file.
Also fixes logs in
HbarSpendingPlanRepository,IPAddressHbarSpendingPlanRepository,EthAddressHbarSpendingPlanRepositoryto log the request ID in messages for better tracking.Related issue(s):
Fixes #3055
Fixes #3025
Notes for reviewer:
Logs without configuration file:
Logs with a configuration file:
Checklist