BLOCK-40: Add indexer to the SDK #3741
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Your org requires the Graphite merge queue for merging into mainAdd the label “merge-queue” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
AmineAfia
changed the title
CodSpeed Performance ReportMerging #3741 will not alter performanceComparing Summary
|
| export { getIndexerClient } from "../indexer/indexer"; | ||
| export { getBlocks } from "../indexer/endpoints/getBlocks"; | ||
| export { getEvents } from "../indexer/endpoints/getEvents"; | ||
| export { getLatestBlock } from "../indexer/endpoints/getLatestBlock"; | ||
| export { getReceipts } from "../indexer/endpoints/getReceipts"; | ||
| export { getNFTs } from "../indexer/endpoints/getNFTs"; |
There was a problem hiding this comment.
I wonder if these should be explicit exports or if we should instead use them "under the hood" in less indexer-specific places. I guess there is always a likely requirement to also explicitly be able to ask the indexer for data
There was a problem hiding this comment.
We can both export and use them under the hood
| export async function getReceipts(client: any, params: any) { | ||
| const blocksResponse = await client(params); | ||
| // TODO: Calrify structured return type through the indexer | ||
| return await blocksResponse.json(); |
There was a problem hiding this comment.
at first thought this should (maybe?) return the same type as the RPC method? that way it can be used interchangeably? Maybe there's additional optional fields?
There was a problem hiding this comment.
Converted transactions and blocks to the closest possible formats with extended data
size-limit report 📦
|
gregfromstl
removed
the
Dashboard
Graphite App
There was a problem hiding this comment.
pretty close
a couple of high level observations (in addition to inline comments)
- this should be split into 2 parts (stack it?): SDK changes + dashboard changes - makes reviewing easier and we can ship it incrementally
- note to self to add a lint rule to enforce avoiding
const foo () => ....function definitions whenever possible (mostly style) - do not use
.d.tsfiles if u do not have to - consider chainsaw support - any "sdk method" that is not clearly a "chainsaw" method has to work across all EVM chain (looking at
useOwnedNFTs()) - is production deployed? If we merge this will it work?
- can we add some tests to formatting etc?
- can we find a way to use the same return types for "block" "transaction" etc that are returned from existing SDK methods?
apps/dashboard/src/contract-ui/tabs/transactions/components/transactions-feed.tsx
Fixed
Show resolved
Hide resolved
There was a problem hiding this comment.
do not use .d.ts files in src - this is completely fine to be a .ts file
| groupBy?: GetNFTsGroupBy; | ||
| } & ChainsawPagingParams; | ||
|
|
||
| export type GetNFTsByCollectionResult = { |
There was a problem hiding this comment.
for live data you probably need to use cursor based pagination but I agree that in the short term this will likely be OK
There was a problem hiding this comment.
do all these formatters in end result output the same data type as if I directly called RPC using the SDK?
if not is that a technical limitation or can we get it there?
| @@ -0,0 +1,14 @@ | |||
| import type { ChainsawPagingParams } from "./types.js"; | |||
|
|
|||
| export const addPagingToRequest = ( | |||
There was a problem hiding this comment.
prefer function foo over const foo () => wherever possible
There was a problem hiding this comment.
suggestion: export function addRequestPagination(...
There was a problem hiding this comment.
nvm, saw the mostly style comment
| type GetLatestBlockNumberParams, | ||
| } from "../chainsaw/endpoints/getLatestBlockNumber.js"; | ||
|
|
||
| export type { |
There was a problem hiding this comment.
can we unify the return types? is there a reason that return types (not input params) have to be different than existing block transaction etc types?
| return useQuery<ChainsawNFTs, Error>({ | ||
| queryKey: ["useGetOwnedNFTs", params], | ||
| queryFn: async () => { | ||
| const result = await getNFTsByOwner(params); |
There was a problem hiding this comment.
what about chains without chainsaw support?
| queryFn: () => | ||
| fetchNFTs(props.client, nft.chain, nft.address, activeAccount?.address), | ||
| })), | ||
| const { data, isLoading } = useGetOwnedNFTs({ |
There was a problem hiding this comment.
what about chains without chainsaw support?
Co-authored-by: Jonas Daniels <[email protected]> Signed-off-by: Toomas Oosalu <[email protected]>
Co-authored-by: Jonas Daniels <[email protected]> Signed-off-by: Toomas Oosalu <[email protected]>
Co-authored-by: Jonas Daniels <[email protected]> Signed-off-by: Toomas Oosalu <[email protected]>
Problem solved
Short description of the bug fixed or feature added
Changes made
How to test
Contributor NFT
Paste in your wallet address below and we will airdrop you a special NFT when your pull request is merged.
Address:PR-Codex overview
This PR adds pagination functionality to Chainsaw API requests, introduces new hooks for fetching NFTs and transactions, and updates Chainsaw endpoints and URLs.
Detailed summary
useGetOwnedNFTsanduseGetTransactionshooks