Skip to content
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(store-sync): add status and block number to return type of waitForTransaction #2668

Merged
merged 4 commits into from
Apr 16, 2024

Conversation

alvrs
Copy link
Member

@alvrs alvrs commented Apr 16, 2024

No description provided.

Copy link

changeset-bot bot commented Apr 16, 2024

🦋 Changeset detected

Latest commit: 4e35c97

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 24 packages
Name Type
@latticexyz/store-sync Patch
@latticexyz/dev-tools Patch
@latticexyz/store-indexer Patch
@latticexyz/abi-ts Patch
@latticexyz/block-logs-stream Patch
@latticexyz/cli Patch
@latticexyz/common Patch
@latticexyz/config Patch
create-mud Patch
@latticexyz/faucet Patch
@latticexyz/gas-report Patch
@latticexyz/protocol-parser Patch
@latticexyz/query Patch
@latticexyz/react Patch
@latticexyz/recs Patch
@latticexyz/schema-type Patch
@latticexyz/services Patch
solhint-config-mud Patch
solhint-plugin-mud Patch
@latticexyz/store Patch
@latticexyz/utils Patch
@latticexyz/world-modules Patch
@latticexyz/world Patch
mock-game-contracts Patch

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

@alvrs alvrs marked this pull request as ready for review April 16, 2024 13:20
@alvrs alvrs requested review from holic and yonadaa as code owners April 16, 2024 13:20
yonadaa
yonadaa previously approved these changes Apr 16, 2024
return lastBlock.blockNumber >= receipt.blockNumber;
const { status, blockNumber } = await publicClient.getTransactionReceipt({ hash: tx });
if (lastBlock.blockNumber >= blockNumber) {
return { status, blockNumber };
Copy link
Contributor

@yonadaa yonadaa Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not return everything from getTransactionReceipt? Then this is like a wrapper for that function and users can use everything it returns

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because we don't have access to most of this information in the early return case above and it feels wrong to only include this information in some cases based on a race condition

for (const block of blocks) {
const txs = block.logs.map((op) => op.transactionHash);
// If the transaction caused a log, it must have succeeded
if (txs.includes(tx)) return { blockNumber: block.blockNumber, status: "success" as const };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (txs.includes(tx)) return { blockNumber: block.blockNumber, status: "success" as const };
if (txs.includes(tx)) {
return { blockNumber: block.blockNumber, status: "success" as const };
}

const receipt = await publicClient.getTransactionReceipt({ hash: tx });
return lastBlock.blockNumber >= receipt.blockNumber;
const { status, blockNumber, transactionHash } = await publicClient.getTransactionReceipt({ hash: tx });
if (lastBlock.blockNumber >= blockNumber) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking at this now, I wonder if this is maybe the cause of infinite spinning sometimes

if we get a receipt, we only sometimes return it?

I think this might be here to make sure the syncing is caught up to where the block for this receipt came through, but just want to check/confirm that we would refetch this transaction receipt while waiting, or if this would cause it to sort of "fall out" of the loop and ~wait indefinitely

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would refetch it when processing the next block. Agree we should add caching to fetching tx receipts though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alvrs alvrs merged commit 8c3dcf7 into main Apr 16, 2024
12 checks passed
@alvrs alvrs deleted the alvrs/wait-for-tx-result branch April 16, 2024 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants