Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MEX-527] Bulk get methods #1535

Open
wants to merge 5 commits into
base: feat/memory-store
Choose a base branch
from

Conversation

mad2sm0key
Copy link
Contributor

@mad2sm0key mad2sm0key commented Dec 11, 2024

Reasoning

  • optimize fetching of data from cache using redis MGET
  • these methods will be used by the memory store cron jobs

Proposed Changes

  • add bulk get methods (using getAllKeys util function) for various fields of pairs, farms, staking farms
  • update dataloaders where needed

How to test

  • N/A

- bulk get methods for various fields
- update 'feesUSD' compute method signature to enable creating a dataloader for the field; update cache warmer + setter services; update usage in router service
- fix unit tests
@mad2sm0key mad2sm0key marked this pull request as ready for review December 12, 2024 12:34
(tokenID) => tokenID === lpTokenID,
);

if (farmAddressIndex === -1) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here you used:

 if index === -1 { return undefined } else { return from array }

and in the function below you used:

return index === -1 { return undefined } else { return from array }

maybe keep consistency and choose a single way

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

return await getAllKeys<string>(
this.cachingService,
stakingProxyAddresses,
'stakeProxy.stakingFarmAddress',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would recommend putting all the keys in a cache.info.ts because:

  • we can easily see all the caching keys in a single place
  • we can easily reuse the keys somewhere else
  • it helps the overall code by avoiding magic strings in the code

@@ -507,8 +573,8 @@ export class PairComputeService implements IPairComputeService {
remoteTtl: CacheTtlInfo.Analytics.remoteTtl,
localTtl: CacheTtlInfo.Analytics.localTtl,
})
async feesUSD(pairAddress: string, time: string): Promise<string> {
return await this.computeFeesUSD(pairAddress, time);
async feesUSD(pairAddress: string): Promise<string> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to update the IPairComputeService interface

@@ -814,6 +910,30 @@ export class PairComputeService implements IPairComputeService {
return addresses[farmAddressIndex];
}

async getAllPairsFarmAddress(pairAddresses: string[]): Promise<string[]> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove some duplicated code. We have two methods that does almost the same thing: computeHasFarms and getPairFarmAddress. We can use the second method to determine if a pair has a farm

@@ -814,6 +910,30 @@ export class PairComputeService implements IPairComputeService {
return addresses[farmAddressIndex];
}

async getAllPairsFarmAddress(pairAddresses: string[]): Promise<string[]> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove the methods that find a farm address for a single pair address. We can use this bulk methods with a single element in the array and we would have the same output

});
}

private calculateBoostedApr(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename into computeBoostedApr

return boostedAPR.toFixed();
}

private calculateMaxBoostedApr(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename into computeMaxBoostedApr

@@ -991,4 +992,74 @@ export class StakingComputeService {

return true;
}

async getAllBaseAndMaxBoostedAPRs(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use the existing compute methods for base and boosted APR: computeBoostedAPR computeMaxBoostedApr

- update Pair compute service interface and mock
- remove duplicate logic for computing pair farms
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants