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.

Sign up for GitHub

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: local file metadata provider #56

Merged
merged 4 commits into from
Aug 26, 2024
Merged

feat: local file metadata provider #56

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ebd0d57
feat: local file metadata provider
0xnigir1 Aug 23, 2024
f9ae12b
feat: add cache to github provider
0xnigir1 Aug 23, 2024
3c39170
docs: add tsdocs
0xnigir1 Aug 23, 2024
7f5b664
feat: remove cache from local provider and save in instance variable
0xnigir1 Aug 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/metadata/src/exceptions/fileNotFound.exception.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export class FileNotFound extends Error {
constructor(path: string) {
super(`File not found at path: ${path}`);
}
}
1 change: 1 addition & 0 deletions packages/metadata/src/exceptions/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
export * from "./invalidSchema.exception.js";
export * from "./fetchError.exception.js";
export * from "./fileNotFound.exception.js";
8 changes: 6 additions & 2 deletions packages/metadata/src/external.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
export type { IMetadataProvider } from "./internal.js";

export { InvalidSchema, FetchError } from "./internal.js";
export { InvalidSchema, FetchError, FileNotFound } from "./internal.js";

export { StaticMetadataProvider, GithubMetadataProvider } from "./internal.js";
export {
StaticMetadataProvider,
GithubMetadataProvider,
LocalFileMetadataProvider,
} from "./internal.js";
78 changes: 50 additions & 28 deletions packages/metadata/src/providers/githubMetadata.provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import axios, { AxiosInstance } from "axios";
import { z } from "zod";

import {
Cache,
ILogger,
Token,
TokenType,
Expand All @@ -13,6 +14,8 @@ import { IMetadataProvider } from "../interfaces/index.js";
import { FetchError, InvalidSchema } from "../internal.js";
import { ChainSchema, TokenSchema } from "../schemas/index.js";

export const GITHUB_METADATA_PREFIX = "github-metadata";

/**
* Represents a provider for retrieving metadata from GitHub.
*/
Expand All @@ -22,6 +25,7 @@ export class GithubMetadataProvider implements IMetadataProvider {
private readonly tokenJsonUrl: string,
private readonly chainJsonUrl: string,
private readonly logger: ILogger,
private readonly cache: Cache,
) {
this.axios = axios.create({
headers: {
Expand All @@ -31,43 +35,61 @@ export class GithubMetadataProvider implements IMetadataProvider {
}

async getChainsMetadata(): Promise<ZKChainMetadata> {
const { data } = await this.axios.get(this.chainJsonUrl).catch((e) => {
this.logger.error(
`Failed to fetch chains metadata from ${this.chainJsonUrl}: ${e.message}`,
);
throw new FetchError(`Failed to fetch chains metadata: ${e.message}`);
});
let cachedData = await this.cache.get<ZKChainMetadata | undefined>(
`${GITHUB_METADATA_PREFIX}-chains`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we use some sort of unique id here ? like UUID?

Copy link
Collaborator

Choose a reason for hiding this comment

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

any suggestions here ? @0xyaco

Copy link
Collaborator

@0xyaco 0xyaco Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I guess there would be a possibility of, given a shared Cache instance between different classes, to overwrite a cached value by accident if someone uses the same key in multiple classes.

I can't come up with anything simple as a workaround to that issue right now lol so I think we can keep it as is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

an alternative im thinking now would be a wrapper around the cache, like BrandedCache or smth like that:

BrandedCache is Cache {
constructor(cache: Cache, prefix: string)
...wraps calls
}

and that this class wraps the cache methods and automatically adds the prefix to the key

but seems too much at this point

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that it's too much, let's just be aware of this issue when defining cache keys.

Copy link
Collaborator

Choose a reason for hiding this comment

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

lets keep it as it is nigiri, no need for uuid

);
if (!cachedData) {
const { data } = await this.axios.get(this.chainJsonUrl).catch((e) => {
this.logger.error(
`Failed to fetch chains metadata from ${this.chainJsonUrl}: ${e.message}`,
);
throw new FetchError(`Failed to fetch chains metadata: ${e.message}`);
});

const validatedData = z.array(ChainSchema).safeParse(data);

const validatedData = z.array(ChainSchema).safeParse(data);
if (!validatedData.success) {
this.logger.error(`Invalid ZKChain metadata: ${validatedData.error.errors}`);
throw new InvalidSchema("Invalid ZKChain metadata");
}

if (!validatedData.success) {
this.logger.error(`Invalid ZKChain metadata: ${validatedData.error.errors}`);
throw new InvalidSchema("Invalid ZKChain metadata");
cachedData = validatedData.data.reduce((acc, chain) => {
const { chainId, ...rest } = chain;
const chainIdBn = BigInt(chainId);
acc.set(chainIdBn, { ...rest, chainId: chainIdBn });
return acc;
}, new Map<bigint, ZKChainMetadataItem>());

await this.cache.set(`${GITHUB_METADATA_PREFIX}-chains`, cachedData);
}

return validatedData.data.reduce((acc, chain) => {
const { chainId, ...rest } = chain;
const chainIdBn = BigInt(chainId);
acc.set(chainIdBn, { ...rest, chainId: chainIdBn });
return acc;
}, new Map<bigint, ZKChainMetadataItem>());
return cachedData;
}

async getTokensMetadata(): Promise<Token<TokenType>[]> {
const { data } = await this.axios.get(this.tokenJsonUrl).catch((e) => {
this.logger.error(
`Failed to fetch chains metadata from ${this.chainJsonUrl}: ${e.message}`,
);
throw new FetchError(`Failed to fetch chains metadata: ${e.message}`);
});
let cachedData = await this.cache.get<Token<TokenType>[] | undefined>(
`${GITHUB_METADATA_PREFIX}-tokens`,
);

const validatedData = z.array(TokenSchema).safeParse(data);
if (!cachedData) {
const { data } = await this.axios.get(this.tokenJsonUrl).catch((e) => {
this.logger.error(
`Failed to fetch chains metadata from ${this.chainJsonUrl}: ${e.message}`,
);
throw new FetchError(`Failed to fetch chains metadata: ${e.message}`);
});

if (!validatedData.success) {
this.logger.error(`Invalid Token metadata: ${validatedData.error.errors}`);
throw new InvalidSchema("Invalid Token metadata");
}
const validatedData = z.array(TokenSchema).safeParse(data);

if (!validatedData.success) {
this.logger.error(`Invalid Token metadata: ${validatedData.error.errors}`);
throw new InvalidSchema("Invalid Token metadata");
}

return validatedData.data;
cachedData = validatedData.data;

await this.cache.set(`${GITHUB_METADATA_PREFIX}-tokens`, cachedData);
}
return cachedData;
}
}
1 change: 1 addition & 0 deletions packages/metadata/src/providers/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
export * from "./githubMetadata.provider.js";
export * from "./staticMetadata.provider.js";
export * from "./localFileMetadata.provider.js";
95 changes: 95 additions & 0 deletions packages/metadata/src/providers/localFileMetadata.provider.ts
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

i was wondering if we want to index the token list by address like { [key:address] : Token<"erc20">} for example...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i thought about it too but i would leave that task for when we refactor the pricing service, so now everything still works as it is, wdyt?

Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
import { existsSync, readFileSync } from "fs";
import { z } from "zod";

import {
Cache,
ILogger,
Token,
TokenType,
ZKChainMetadata,
ZKChainMetadataItem,
} from "@zkchainhub/shared";

import { FileNotFound, IMetadataProvider, InvalidSchema } from "../internal.js";
import { ChainSchema, TokenSchema } from "../schemas/index.js";

export const LOCALFILE_METADATA_PREFIX = "local-metadata";

/**
* Represents a local file metadata provider.
*/
export class LocalFileMetadataProvider implements IMetadataProvider {
/**
* Constructs a new instance of the LocalFileMetadataProvider class.
* @param tokenJsonPath The path to the token JSON file.
* @param chainJsonPath The path to the chain JSON file.
* @param logger The logger instance.
* @param cache The cache instance.
* @throws {FileNotFound} if any of the files is not found.
*/
constructor(
private readonly tokenJsonPath: string,
private readonly chainJsonPath: string,
private readonly logger: ILogger,
private readonly cache: Cache,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are the local JSON files going to be live-updated while the app is running? If not, do we need the cache for local files? Wdyt about using two instance variables and reading the file contents inside this constructor?

Something along these lines:

constructor() {
  if (!existsSync(...)) {
    ...
  }


  if (!existsSync(...)) {
    ...
  }

  this.chainsMetadata = readMetadata()
  this.tokensMetadata = tokensMetadata()
}

async getChainsMetadata() {
  return Promise.resolve(this.chainsMetadata);
}

If for some business rule this provider needs to keep reading the local file from time to time (or if there's any other reason) dismiss this comment. 😎

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we had the exact same discussion with @0xkenj1 jajajaj, we ended up agreeing in make it work like the GithubMetadata as a matter of consistency

Copy link
Collaborator

Choose a reason for hiding this comment

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

we can reconsider this 😅

Copy link
Collaborator

@0xkenj1 0xkenj1 Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think i didn't consider the case in which u redeploy the chainhub instance, if you keep redis running and you had updated the JSON file, then it will pick the data from redis instead of the updated file. (Supposing infinite ttl )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we'll go with this option ser, makes sense

) {
if (!existsSync(tokenJsonPath)) {
throw new FileNotFound(tokenJsonPath);
}

if (!existsSync(chainJsonPath)) {
throw new FileNotFound(chainJsonPath);
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets add natspec on IMetadataProvider and reuse it on the classes that implement the interface

async getChainsMetadata(): Promise<ZKChainMetadata> {
let cachedData = await this.cache.get<ZKChainMetadata>(
`${LOCALFILE_METADATA_PREFIX}-chains`,
);
if (!cachedData) {
const jsonData = readFileSync(this.chainJsonPath, "utf-8");
const parsed = JSON.parse(jsonData);

const validatedData = z.array(ChainSchema).safeParse(parsed);

if (!validatedData.success) {
this.logger.error(`Invalid ZKChains metadata: ${validatedData.error.errors}`);
throw new InvalidSchema("Invalid ZKChains metadata");
}

cachedData = validatedData.data.reduce((acc, chain) => {
const { chainId, ...rest } = chain;
const chainIdBn = BigInt(chainId);
acc.set(chainIdBn, { ...rest, chainId: chainIdBn });
return acc;
}, new Map<bigint, ZKChainMetadataItem>());

await this.cache.set(`${LOCALFILE_METADATA_PREFIX}-chains`, cachedData);
}

return cachedData;
}

async getTokensMetadata(): Promise<Token<TokenType>[]> {
let cachedData = await this.cache.get<Token<TokenType>[]>(
`${LOCALFILE_METADATA_PREFIX}-tokens`,
);
if (!cachedData) {
const jsonData = readFileSync(this.tokenJsonPath, "utf-8");
const parsed = JSON.parse(jsonData);

const validatedData = z.array(TokenSchema).safeParse(parsed);

if (!validatedData.success) {
this.logger.error(`Invalid Tokens metadata: ${validatedData.error.errors}`);
throw new InvalidSchema("Invalid Tokens metadata");
}

cachedData = validatedData.data;

await this.cache.set(`${LOCALFILE_METADATA_PREFIX}-tokens`, cachedData);
}

return cachedData;
}
}
6 changes: 4 additions & 2 deletions packages/metadata/test/fixtures/metadata.fixtures.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { Token, TokenType } from "@zkchainhub/shared";

export const tokenJsonUrl = "https://example.com/tokens.json";
export const chainJsonUrl = "https://example.com/chains.json";
export const mockTokenData = [
export const mockTokenData: Token<TokenType>[] = [
{
name: "Ethereum",
symbol: "ETH",
Expand All @@ -20,7 +22,7 @@ export const mockTokenData = [
type: "erc20",
decimals: 18,
},
];
] as const;
export const mockChainData = [
{
chainId: 324,
Expand Down
Loading