-
Notifications
You must be signed in to change notification settings - Fork 2
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
Refactor and add SVM support #4
base: main
Are you sure you want to change the base?
Conversation
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.
Some small comments
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "@duneanalytics/hooks", | |||
"version": "1.0.8", | |||
"version": "2.0.0", |
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.
Are we sure we want to bump to 2? Another approach is keep the useTransactions and pass an object config for vm: [evm|svm] with evm as the default.
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've included some breaking changes, hence the major bump, but I could revert those. There are some inconsistencies in the API I'd like us to fix, but it's not necessary.
@@ -0,0 +1,85 @@ | |||
export type TokenBalancesParams = { |
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.
We should double check these params. I think all is right, but something to keep an eye on as we continue to evolve.
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.
Yeah we should generate this from OpenAPI
@@ -0,0 +1,57 @@ | |||
import { useEffect, useState } from "react"; |
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.
are these all copy pasta over from the previous files? No changes?
@@ -0,0 +1,63 @@ | |||
export type QueryParamValue = string | number | boolean; |
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.
we should add a test for this
export * as evm from "./evm"; | ||
export * as svm from "./svm"; |
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.
are there any downfalls of this from a splitting standpoint?
} | ||
|
||
const DuneContext = createContext<DuneContextType>({duneApiKey: ""}) | ||
const DuneContext = createContext<DuneContextType | undefined>(undefined); |
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.
why now allow undefined?
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.
Because duneApiKey: ""
does not really make sense 😅 In the current code this would be allowed, but fail in the API request without telling the user how to solve it. Now instead the useDuneContext
hook will fail early and let the user know what's wrong (missing DuneProvider).
const clientRef = useRef<HttpClient>(); | ||
|
||
if ( | ||
clientRef.current === undefined || | ||
clientRef.current.apiKey !== apiKey || | ||
clientRef.current.baseUrl !== baseUrl | ||
) { | ||
clientRef.current = new HttpClient(baseUrl, apiKey); | ||
} | ||
|
||
return ( | ||
<DuneContext.Provider value={{ duneApiKey }}> | ||
<DuneContext.Provider value={{ client: clientRef.current }}> |
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.
Why'd we add this?
@@ -25,17 +25,31 @@ export function useDeepMemo<TKey, TValue>( | |||
|
|||
// Custom deep equality function | |||
function deepEqual(obj1: any, obj2: any): boolean { |
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.
we should add tests for these too
Breaking changes
fetchBalances
->fetchTokenBalances
for consistencyevm
moduleAdditions