-
Notifications
You must be signed in to change notification settings - Fork 193
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(explorer): show transactions #3062
Conversation
karooolis
commented
Aug 26, 2024
•
edited
Loading
edited
🦋 Changeset detectedLatest commit: 0e929d2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 26 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
packages/explorer/src/app/(explorer)/[chainName]/worlds/[worldAddress]/observe/TimeAgoCell.tsx
Outdated
Show resolved
Hide resolved
return Array.from(mergedMap.values()).reverse(); | ||
}, [transactions, observerWrites]); | ||
|
||
async function handleTransaction(hash: Hex, timestamp: bigint) { |
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.
hmm, feels weird that we're not writing to the transaction list until after all the things below resolve (getting tx, getting receipt, simulating result
wouldn't we want to render it right away, then enrich it with data as it comes in?
could we do this all inline with individual components rather than in a big handler like this? or does that impact the resulting order?
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.
good point, did it like that initially for simplicity reasons but definitely better to update state as soon as it's available, will change.
As for individual components, I explored that approach but it didn't quite work due to how tanstack table works. It expects all the data to be passed to the useReactTable
beforehand, and then provides utilities for rendering rows and cells. We could do without the tanstack table ofc but wanted to use it in case we need table functionalities like filter, sorting, and such. Of course, all of that can be achieved custom. Also, as you noted, there would be other considerations when it comes to sorting that would be need handling, mainly seems like handling incoming txs from observer would need more thought where tx hash is not immediately available.
Anyhow, to improve the situation regarding the size of handleTransaction
separated TransactionTable
into TransactionTableContainer
to handle all the logic, and TransactionTableView
for display.
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 do we need two components? seems like we could do one component and one hook? since most of the logic in TransactionTableContainer
is now hook-like and we only wrap it in a component to render TransactionTableView
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 good suggestion. I couldn't figure out at first why using the hook way caused infinite re-renders but now figured out the culprit was const observerWrites = useStore(store, (state) => Object.values(state.writes))
which then messed with useMemo
due to being a new object every time. Switching to const observerWrites = useStore(store, (state) => state.writes
did the trick.
Co-authored-by: Kevin Ingersoll <[email protected]>
…Address]/observe/BlockExplorerLink.tsx Co-authored-by: Kevin Ingersoll <[email protected]>
…Address]/observe/TimeAgoCell.tsx Co-authored-by: Kevin Ingersoll <[email protected]>
…Address]/observe/BlockExplorerLink.tsx Co-authored-by: Kevin Ingersoll <[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.
no action needed on the spacing comment, just a note as we continue to build out and refine pages
prob worth moving the TransactionTableContainer
logic into a hook then use it in TransactionsTableView
(which could prob get back to being TransactionsTable
)
otherwise lgtm!
if (attempt < maxRetries - 1) { | ||
await new Promise((resolve) => setTimeout(resolve, retryInterval)); | ||
} | ||
} |
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.
this is basically what viem's waitForTransactionReceipt
does, but with caching/batching - can we use that 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.
oh so nice!!! great suggestion, changed