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

fix(sdk): check for both v0 and v1 messages when looking for RelayedMessages #6042

Merged
merged 2 commits into from
Jun 29, 2023

Conversation

roninjin10
Copy link
Contributor

@roninjin10 roninjin10 commented Jun 16, 2023

Fixes #6035

Bug explanation

  • Made a breaking change after goerli bedrock release to migrate legacy withdrawals message hashesto v1.
  • the message hash what is used from l2 withdrawal logs to generate the message hash on l1
  • Any legacy withdrawal made prebedrock will be v0
  • any legacy withdrawal made post bedrock will be v1

To figure out if a message is relayed we check for RelayedMessage event for message hash

Fix

Check for both v0 and v1

@changeset-bot
Copy link

changeset-bot bot commented Jun 16, 2023

🦋 Changeset detected

Latest commit: 650eba7

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 Jun 16, 2023

Deploy Preview for opstack-docs canceled.

Name Link
🔨 Latest commit 650eba7
🔍 Latest deploy log https://app.netlify.com/sites/opstack-docs/deploys/649ce92976bef0000861dffc

@roninjin10
Copy link
Contributor Author

roninjin10 commented Jun 16, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@mergify mergify bot added the sdk label Jun 16, 2023
@roninjin10 roninjin10 marked this pull request as ready for review June 16, 2023 18:50
@roninjin10 roninjin10 requested review from a team as code owners June 16, 2023 18:50
@roninjin10 roninjin10 requested a review from twoshark June 16, 2023 18:51
@roninjin10 roninjin10 marked this pull request as draft June 16, 2023 19:06
@roninjin10 roninjin10 marked this pull request as ready for review June 16, 2023 19:09
.circleci/config.yml Outdated Show resolved Hide resolved
@semgrep-app
Copy link
Contributor

semgrep-app bot commented Jun 17, 2023

Semgrep found 2 unchecked-type-assertion findings:

  • op-bindings/bindings/disputegamefactory.go: L308, L370

Unchecked type assertion.

Ignore this finding from unchecked-type-assertion.

@roninjin10 roninjin10 requested a review from a team as a code owner June 17, 2023 01:05
@mergify
Copy link
Contributor

mergify bot commented Jun 17, 2023

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

@mergify mergify bot added the conflict label Jun 17, 2023
@github-actions github-actions bot added the C-protocol-critical Category: Modifies protocol-critical code label Jun 17, 2023
@github-actions github-actions bot removed the C-protocol-critical Category: Modifies protocol-critical code label Jun 17, 2023
@mergify mergify bot removed the conflict label Jun 17, 2023
@codecov
Copy link

codecov bot commented Jun 17, 2023

Codecov Report

Merging #6042 (39800c6) into develop (b168350) will increase coverage by 0.00%.
The diff coverage is 66.66%.

❗ Current head 39800c6 differs from pull request most recent head ab3a539. Consider uploading reports for the commit ab3a539 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #6042   +/-   ##
========================================
  Coverage    44.56%   44.56%           
========================================
  Files          412      412           
  Lines        26268    26269    +1     
  Branches       684      684           
========================================
+ Hits         11706    11707    +1     
  Misses       13573    13573           
  Partials       989      989           
Flag Coverage Δ
bedrock-go-tests 44.71% <ø> (ø)
common-ts-tests 26.82% <ø> (ø)
contracts-bedrock-tests 56.95% <ø> (ø)
core-utils-tests 48.47% <0.00%> (ø)
fault-detector-tests 29.51% <ø> (ø)
sdk-tests 39.87% <100.00%> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
packages/core-utils/src/optimism/hashing.ts 33.33% <0.00%> (ø)
packages/sdk/src/cross-chain-messenger.ts 52.04% <100.00%> (+0.09%) ⬆️

@mergify
Copy link
Contributor

mergify bot commented Jun 27, 2023

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

@mergify
Copy link
Contributor

mergify bot commented Jun 27, 2023

This PR is next in line to be merged, and will be merged as soon as checks pass.

1 similar comment
@mergify
Copy link
Contributor

mergify bot commented Jun 27, 2023

This PR is next in line to be merged, and will be merged as soon as checks pass.

@mergify mergify bot removed the on-merge-train label Jun 27, 2023
…essages

test: add one

fix: linter

fix: Failed to save file to change failed ones too

fix: linter

Fix --fork-block-number to correct network block number

changeset
@mergify
Copy link
Contributor

mergify bot commented Jun 29, 2023

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

@mergify
Copy link
Contributor

mergify bot commented Jun 29, 2023

This PR is next in line to be merged, and will be merged as soon as checks pass.

1 similar comment
@mergify
Copy link
Contributor

mergify bot commented Jun 29, 2023

This PR is next in line to be merged, and will be merged as soon as checks pass.

@OptimismBot OptimismBot merged commit ff6d298 into develop Jun 29, 2023
8 of 9 checks passed
@OptimismBot OptimismBot deleted the willc/prebedrockbug branch June 29, 2023 02:28
@mergify mergify bot removed the on-merge-train label Jun 29, 2023
This was referenced Jul 18, 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.

sdk: getMessageStatus is returning wrong message status for finalized withdrawals
5 participants