feat: local file metadata provider #56
Conversation
| throw new FetchError(`Failed to fetch chains metadata: ${e.message}`); | ||
| }); | ||
| let cachedData = await this.cache.get<ZKChainMetadata | undefined>( | ||
| `${GITHUB_METADATA_PREFIX}-chains`, |
There was a problem hiding this comment.
should we use some sort of unique id here ? like UUID?
There was a problem hiding this comment.
any suggestions here ? @0xyaco
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I agree that it's too much, let's just be aware of this issue when defining cache keys.
There was a problem hiding this comment.
lets keep it as it is nigiri, no need for uuid
There was a problem hiding this comment.
i was wondering if we want to index the token list by address like { [key:address] : Token<"erc20">} for example...
There was a problem hiding this comment.
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?
| throw new FileNotFound(chainJsonPath); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Lets add natspec on IMetadataProvider and reuse it on the classes that implement the interface
| private readonly tokenJsonPath: string, | ||
| private readonly chainJsonPath: string, | ||
| private readonly logger: ILogger, | ||
| private readonly cache: Cache, |
There was a problem hiding this comment.
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. 😎
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 )
There was a problem hiding this comment.
we'll go with this option ser, makes sense
🤖 Linear
Closes ZKS-221
Description
LocalFileMetadataProviderCachetoGithubMetadataProvider