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(sdk): Add event filter params to sdk as options #6332

Conversation

wilsoncusack
Copy link
Contributor

@wilsoncusack wilsoncusack commented Jul 18, 2023

Description

This PR adds optional args fromBlockOrBlockHash and toBlockOrHash to getMessageStatus, getMessageReceipt, waitForMessageReceipt, waitForMessageStatus, and estimateMessageWaitTimeSeconds.

This was motivated by having issues when bridging due to the massive block range.

Tests

I am not sure how we can test this. I ran the test suite locally. Maybe I can build the SDK from this branch in my project and try it?

Additional context

Add any other context about the problem you're solving.

Metadata

  • Fixes #[Link to Issue]

@wilsoncusack wilsoncusack requested a review from a team as a code owner July 18, 2023 19:50
@changeset-bot
Copy link

changeset-bot bot commented Jul 18, 2023

🦋 Changeset detected

Latest commit: 8003aa6

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

This PR includes changesets to release 2 packages
Name Type
@eth-optimism/sdk Minor
@eth-optimism/chain-mon 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

@netlify
Copy link

netlify bot commented Jul 18, 2023

Deploy Preview for opstack-docs canceled.

Name Link
🔨 Latest commit 8003aa6
🔍 Latest deploy log https://app.netlify.com/sites/opstack-docs/deploys/64b8c876088e3d0008daecd9

@mergify
Copy link
Contributor

mergify bot commented Jul 18, 2023

Hey @wilsoncusack! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot requested a review from roninjin10 July 18, 2023 19:50
@wilsoncusack wilsoncusack force-pushed the wilson/wait-for-receipt-start-block branch from 5e6f0f5 to a333a2a Compare July 18, 2023 20:10
@mergify mergify bot removed the conflict label Jul 18, 2023
Copy link

@lauchness lauchness left a comment

Choose a reason for hiding this comment

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

🚀

@tynes
Copy link
Contributor

tynes commented Jul 18, 2023

There is currently no sane way to test the sdk in some cases, def talk to @roninjin10 about the sdk future, he made the sdk-next tests

@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Merging #6332 (4e2c44a) into develop (1b43cbd) will decrease coverage by 0.02%.
The diff coverage is 90.00%.

❗ Current head 4e2c44a differs from pull request most recent head 8003aa6. Consider uploading reports for the commit 8003aa6 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #6332      +/-   ##
===========================================
- Coverage    44.89%   44.88%   -0.02%     
===========================================
  Files          449      449              
  Lines        29241    29244       +3     
  Branches       748      748              
===========================================
- Hits         13128    13126       -2     
- Misses       15046    15051       +5     
  Partials      1067     1067              
Flag Coverage Δ
bedrock-go-tests 43.59% <ø> (ø)
cannon-go-tests 62.58% <ø> (ø)
common-ts-tests 26.82% <ø> (ø)
contracts-bedrock-tests 62.45% <ø> (-0.46%) ⬇️
core-utils-tests 44.24% <ø> (ø)
fault-detector-tests 26.95% <ø> (ø)
sdk-next-tests 42.60% <90.00%> (+0.11%) ⬆️
sdk-tests 42.60% <90.00%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/sdk/src/cross-chain-messenger.ts 52.09% <90.00%> (+0.27%) ⬆️

... and 1 file with indirect coverage changes

@roninjin10 roninjin10 changed the title Wilson/wait for receipt start block feat(sdk): Add event filter params to sdk as options Jul 18, 2023
Copy link
Contributor

@roninjin10 roninjin10 left a comment

Choose a reason for hiding this comment

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

image

packages/sdk/src/cross-chain-messenger.ts Show resolved Hide resolved
packages/sdk/src/cross-chain-messenger.ts Show resolved Hide resolved
Copy link
Contributor

@roninjin10 roninjin10 left a comment

Choose a reason for hiding this comment

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

Ah good call out @tynes! Would it be possible to add a test? There are existing sdk tests in packages/sdk/test-next/message-status.spec.ts would just need to use any message from goerli/op-goerli older than the blocks we fork
9256679 for l1
11276409 for l2

@roninjin10
Copy link
Contributor

Testing waitForMessageStatus is a little involved so maybe just test that getMessageStatus returns a tx when in the block range and then doesn't return a tx when the blockrange is outside of the block the tx is in

@wilsoncusack
Copy link
Contributor Author

Testing waitForMessageStatus is a little involved so maybe just test that getMessageStatus returns a tx when in the block range and then doesn't return a tx when the blockrange is outside of the block the tx is in

Like a manual test or are you making real network calls in your tests?

Copy link
Contributor

@roninjin10 roninjin10 left a comment

Choose a reason for hiding this comment

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

image

packages/sdk/example.env Show resolved Hide resolved
packages/sdk/src/cross-chain-messenger.ts Show resolved Hide resolved
packages/sdk/src/cross-chain-messenger.ts Show resolved Hide resolved
packages/sdk/test-next/messageStatus.spec.ts Show resolved Hide resolved
packages/sdk/test-next/messageStatus.spec.ts Show resolved Hide resolved
@wilsoncusack
Copy link
Contributor Author

@roninjin10 could we get this merged pls 🙏

@mergify
Copy link
Contributor

mergify bot commented Jul 20, 2023

This PR has been added to the merge queue, and will be merged soon.

@mergify
Copy link
Contributor

mergify bot commented Jul 20, 2023

Hey @wilsoncusack, this pull request failed to merge and has been dequeued from the merge train. If you believe your PR failed in the merge train because of a flaky test, requeue it by commenting with @mergifyio requeue.
More details can be found on the Queue: Embarked in merge train check-run.

@mergify mergify bot removed the on-merge-train label Jul 20, 2023
@roninjin10 roninjin10 merged commit bbd27dd into ethereum-optimism:develop Jul 20, 2023
6 of 9 checks passed
@github-actions github-actions bot mentioned this pull request Jul 20, 2023
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.

4 participants