-
Notifications
You must be signed in to change notification settings - Fork 17
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(solana): cached solana provider #905
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Reinis Martinsons <[email protected]>
Signed-off-by: Reinis Martinsons <[email protected]>
Signed-off-by: Reinis Martinsons <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently this is used in tests only
@@ -13,7 +13,7 @@ export class RateLimitedSolanaRpcFactory extends SolanaClusterRpcFactory { | |||
private queue: QueueObject<SolanaRateLimitTask>; | |||
|
|||
// Holds the underlying transport that the rate limiter wraps. | |||
private readonly defaultTransport: RpcTransport; | |||
protected defaultTransport: RpcTransport; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
had to change these for being able to use in mocks
*/ | ||
export function jsonReplacerWithBigInts(_key: string, value: unknown): unknown { | ||
if (typeof value === "bigint") { | ||
return { type: "BigInt", value: value.toString() }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative would be to encode this as decimal strings followed by n
character which would take bit less space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I favour storing them as strings 👍. Ultimately I wonder if we'll just use BigInt everywhere, in which case we could unconditionally convert strings as numbers back to BigInt.
const getSignatureStatusesResponse = await this.rateLimitedRpcClient | ||
.getSignatureStatuses([params[0]], { | ||
searchTransactionHistory: true, | ||
}) | ||
.send(); | ||
|
||
const getTransactionResponse = await this.rateLimitedTransport<TResponse>(...args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we run getSignatureStatuses
and this.rateLimitedTransport
in parallel using Promise.all(...)
to reduce sequential waits?
// Cache the transaction only if it is finalized. | ||
if (getSignatureStatusesResponse.value[0]?.confirmationStatus === "finalized") { | ||
const redisKey = this.buildRedisKey(method, params); | ||
await this.redisClient?.set( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is no need to await here since it won't affect the returned value
const { method, params } = args[0].payload as { method: string; params?: unknown[] }; | ||
|
||
// Only handles getTransaction right now. | ||
if (method === "getTransaction") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: You could use an early return if (method !== "getTransaction") return this.rateLimitedTransport<TResponse>(...args);
So we don't need the else at the end and improve readability
if (cacheType !== CacheType.NONE) { | ||
const redisKey = this.buildRedisKey(method, params); | ||
|
||
// Attempt to pull the result from the cache. | ||
const redisResult = await this.redisClient?.get<string>(redisKey); | ||
|
||
// If cache has the result, parse the json and return it. | ||
if (redisResult) { | ||
return JSON.parse(redisResult, jsonReviverWithBigInts); | ||
} | ||
|
||
// Cache does not have the result. Query it directly and cache it if finalized. | ||
return this.requestAndCacheFinalized<TResponse>(...args); | ||
} | ||
|
||
return this.rateLimitedTransport<TResponse>(...args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor refactor that Nick figured out with the EVM implementation:
if (cacheType !== CacheType.NONE) { | |
const redisKey = this.buildRedisKey(method, params); | |
// Attempt to pull the result from the cache. | |
const redisResult = await this.redisClient?.get<string>(redisKey); | |
// If cache has the result, parse the json and return it. | |
if (redisResult) { | |
return JSON.parse(redisResult, jsonReviverWithBigInts); | |
} | |
// Cache does not have the result. Query it directly and cache it if finalized. | |
return this.requestAndCacheFinalized<TResponse>(...args); | |
} | |
return this.rateLimitedTransport<TResponse>(...args); | |
if (cacheType === CacheType.NONE) { | |
return this.rateLimitedTransport<TResponse>(...args); | |
} | |
const redisKey = this.buildRedisKey(method, params); | |
// Attempt to pull the result from the cache. | |
const redisResult = await this.redisClient?.get<string>(redisKey); | |
// If cache has the result, parse the json and return it. | |
if (redisResult) { | |
return JSON.parse(redisResult, jsonReviverWithBigInts); | |
} | |
// Cache does not have the result. Query it directly and cache it if finalized. | |
return this.requestAndCacheFinalized<TResponse>(...args); |
typeof value === "object" && | ||
value !== null && | ||
"type" in value && | ||
"value" in value && | ||
value.type === "BigInt" && | ||
typeof value.value === "string" && | ||
/^-?\d+$/.test(value.value) // Ensure it's a valid BigInt value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooc, could superstruct help here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only a couple of minor comments from me - this looks pretty good.
This implements similar cached provider logic as
src/providers/cachedProvider.ts
, but for Solana.Also adds missing tests for the rate limited Solana provider
Fixes: https://linear.app/uma/issue/ACX-3741/solana-cached-provider