-
Notifications
You must be signed in to change notification settings - Fork 102
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: upgrade api layer #1910
base: main
Are you sure you want to change the base?
feat: upgrade api layer #1910
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
25e066e
to
7be35ba
Compare
7be35ba
to
6025c52
Compare
6025c52
to
8d9e372
Compare
8d9e372
to
8f5a43f
Compare
8f5a43f
to
9d76acd
Compare
9d76acd
to
d307216
Compare
d307216
to
7ae81b7
Compare
7ae81b7
to
7e16fa5
Compare
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
7e16fa5
to
58b12b5
Compare
58b12b5
to
d001bb7
Compare
combine: (response: UseQueryResult<NakamotoBlockListResponse, Error>[]) => { | ||
const result = response.flatMap(data => data.data?.results || []); | ||
return result; | ||
combine: (response: UseQueryResult<any, Error>[]) => { |
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 don;t like using any here
smart_contract: number; | ||
contract_call: number; | ||
}; | ||
filteredTxTypeCounts: Record<string, number>; |
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.
Did we lose types here?
@@ -239,7 +240,7 @@ export function MempoolFeeStats({ tokenPrice }: { tokenPrice: TokenPrice }) { | |||
</Flex> | |||
<VStack gap={3} alignItems="flex-start"> | |||
{Object.entries(filteredTxTypeCounts).map(([key, value]) => { | |||
const icon = getTxTypeIcon(key as keyof typeof filteredTxTypeCounts); | |||
const icon = getTxTypeIcon(key as any); |
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 any?
@@ -13,11 +13,11 @@ import { MenuList } from '../../ui/MenuList'; | |||
|
|||
interface MenuItem { | |||
onClick: () => void; | |||
label: string; | |||
label: string | 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.
How can a menu item not have a label? Wouldn't that make it invisible/empty?
return useQuery({ | ||
queryKey: [ACCOUNT_BALANCE_QUERY_KEY, address], | ||
queryFn: () => { | ||
queryFn: async () => { | ||
if (!address) return 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.
not needed because of enabled prop
}); | ||
return response; | ||
if (error) { |
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.
Have we made sure all these errors are caught properly. I think it would be a great idea to use the logError util func that will log this error in sentry. I would probably add the aility to rethrow the error to that func. And perhaps we can create a util function to do all this when making these apiclient requests so that we don't need to do this in every api hook. This comment equally applies to all these hooks. Hopefully it will make it easier if I don't bog down your PR by repeating the same comment :)
}, | ||
staleTime: ONE_MINUTE, | ||
...options, | ||
refetchOnWindowFocus: true, |
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.
let's add a comment here why this is always set to true, such as we want the ui to always try to update the principal balance.
}), | ||
queryKey: [ADDRESS_CONFIRMED_TXS_WITH_TRANSFERS_INFINITE_QUERY_KEY, principal], | ||
queryFn: async ({ pageParam }: { pageParam: number }) => { | ||
if (!principal) return 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.
not needed with the enale prop
offset: pageParam || 0, | ||
}), | ||
queryFn: async ({ pageParam }: { pageParam: number }) => { | ||
if (!address || !txId) return 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.
not needed with the enable prop
offset: pageParam, | ||
}), | ||
queryFn: async ({ pageParam }: { pageParam: number }) => { | ||
if (!address) return 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.
I keep seeing this so I won't keep making redundanct commentsm but I would like to know why we are using these checks and also the enabled prop
staleTime: Infinity, | ||
...options, | ||
refetchOnWindowFocus: true, |
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.
put reason in comment?
import { useApi } from '../api/useApi'; | ||
import { useGlobalContext } from '../context/useGlobalContext'; | ||
import { getErrorMessage } from '../../api/getErrorMessage'; | ||
import { useApiClient } from '../../api/useApiClient'; | ||
|
||
const BLOCK_QUERY_KEY = 'block'; | ||
|
||
export function useBlockByHash( |
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.
So we need this if we have the blockbyheightorhash endpoint?
limit: numStxBlocksperBtcBlock, | ||
}), | ||
queryFn: async () => { | ||
if (!heightOrHash) return 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.
use enabled?
return useQuery({ | ||
queryKey: ['contract-ft-metadata', contractId], | ||
queryFn: () => api.tokenMetadataApi?.getFtMetadata(contractId!), | ||
queryFn: () => tokenMetadataApi?.getFtMetadata(contractId!), |
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.
Is the metadata api going to transistion to the endpoint model too?
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.
not for now, current version still follows the old convention
return useQuery({ | ||
queryKey: ['coreApiInfo'], | ||
queryFn: () => api.infoApi.getCoreApiInfo(), | ||
queryFn: () => fetch(`${activeNetworkUrl}/v2/info`).then(res => res.json()), |
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.
not using apiClient?
const api = useApi(); | ||
return useQuery<CoreNodeInfoResponse, Error>({ | ||
export function useCustomNetworkApiInfo(url: string, options: any = {}) { | ||
return useQuery({ |
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 feel like this can be combined with useCoreAPIInfo
return useQuery({ | ||
queryKey: ['transfer-fees'], | ||
queryFn: () => api.feesApi.getFeeTransfer(), | ||
queryFn: () => fetch(`${activeNetworkUrl}/v2/fees/transfer`).then(res => res.json()), |
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.
no apiclient
return useSuspenseQuery({ | ||
queryKey: ['poxInfo'], | ||
queryFn: () => api.infoApi.getPoxInfo(), | ||
queryFn: () => fetch(`${activeNetworkUrl}/v2/pox`).then(res => res.json()), |
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.
no api client?
return { | ||
results: tx.events, | ||
total: tx.event_count, | ||
results: (tx as Transaction).events, |
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.
did we lose typings?
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.
Left some comments
7ee41a7
to
cb61633
Compare
cb61633
to
6ce1bcb
Compare
6ce1bcb
to
affec16
Compare
What type of PR is this? (check all applicable)
Description
@stacks/blockchain-api-client no longer exports API call wrappers, we need to upgrade to latest stacks API JS client replace all API calls across Explorer.
https://github.com/hirosystems/stacks-blockchain-api/blob/HEAD/client/MIGRATION.md
Issue ticket number and link
closes #1896
Checklist:
Screenshots (if appropriate):
N/A