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

feat: pricing & metadata caching #51

Merged
merged 4 commits into from
Jan 9, 2025
Merged

Conversation

0xnigir1
Copy link
Collaborator

@0xnigir1 0xnigir1 commented Jan 8, 2025

🤖 Linear

Closes GIT-218 GIT-219 GIT-220

Description

We create two repositories for pricing and metadata that will act as cache (note: is not the usual cache for fast retrieval that works in a Redis way, but instead is a layer that provides a significant improvement when reindexing so we don't fetch data again)

Checklist before requesting a review

  • I have conducted a self-review of my code.
  • I have conducted a QA.
  • If it is a core feature, I have included comprehensive tests.

@0xnigir1 0xnigir1 requested review from jahabeebs and 0xyaco January 8, 2025 17:25
@0xnigir1 0xnigir1 self-assigned this Jan 8, 2025
Copy link

linear bot commented Jan 8, 2025

Copy link
Collaborator

@0xyaco 0xyaco left a comment

Choose a reason for hiding this comment

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

The only question that I got here is: can the already-cached data in the underlying data source change after the indexer has cached the data point?


expect(result).toBeUndefined();
expect(mockCache.set).not.toHaveBeenCalled();
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another case you might want to include as a spec: what happens when the provider fails? Should the caching proxy re-throw the error?

@0xnigir1
Copy link
Collaborator Author

0xnigir1 commented Jan 8, 2025

The only question that I got here is: can the already-cached data in the underlying data source change after the indexer has cached the data point?

no, is always data from the past that isn't supposed to change

0xyaco
0xyaco previously approved these changes Jan 8, 2025
const metadataProvider = new IpfsProvider(env.IPFS_GATEWAYS_URL, logger);
const metadataRepository = new KyselyMetadataCache(kyselyDatabase, env.DATABASE_SCHEMA);
const _metadataProvider = new IpfsProvider(env.IPFS_GATEWAYS_URL, logger);
const metadataProvider = new CachingMetadataProvider(
Copy link
Collaborator

Choose a reason for hiding this comment

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

it might be good to name _metadataProvider more specific to differentiate it from metadataProvider--maybe ipfsProvider, ipfsInstance?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

* If not found in cache, fetches from the underlying provider and caches the result before returning.
* Cache failures (both reads and writes) are logged but do not prevent the provider from functioning.
*/
export class CachingPricingProvider implements IPricingProvider {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can this data ever become stale? do we need TTL/eviction logic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no, is always data from the past that isn't supposed to change


export type PriceCacheKey = { tokenCode: TokenCode; timestampMs: number };

export class KyselyPricingCache implements ICache<PriceCacheKey, TokenPrice> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

docs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -0,0 +1,67 @@
import { ICache, PriceCacheKey } from "@grants-stack-indexer/repository";
import { ILogger, TokenCode } from "@grants-stack-indexer/shared";
Copy link
Collaborator

Choose a reason for hiding this comment

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

it doesn't have to be in this PR but probably we should rename tokencode to tokensymbol, they are different things (and I've never heard of token code used to describe a symbol like "ETH")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

could be, TokenCode is just a way to identify token generically (ie. not tied to the ID on a pricing provider like Coingecko) or to the symbol (since many tokens can have the same symbol), but in this case we decided to pick the token symbol as TokenCode but could be anything else. what name do you suggest?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh gotcha, I think that name is good then

Copy link
Collaborator

@jahabeebs jahabeebs left a comment

Choose a reason for hiding this comment

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

👌👌

@0xnigir1 0xnigir1 merged commit f26134f into dev Jan 9, 2025
6 checks passed
@0xnigir1 0xnigir1 deleted the feat/pricing-metadata-caching branch January 9, 2025 15:00
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