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: Emit DepositFailed in L2 Bridge #3450

Merged
merged 4 commits into from
Sep 19, 2022
Merged

fix: Emit DepositFailed in L2 Bridge #3450

merged 4 commits into from
Sep 19, 2022

Conversation

maurelian
Copy link
Contributor

Description

The L2StandardBridge contract did not emit a DepositFailed event when a
finalizeDeposit call fails and instead always emitted a DepositFinalized event.

This was fixed by adding a boolean return value to StandardBridge.finalizeBridgeERC20, and emitting the correct event based on the return value.

The L1 bridge is not affected by this issue because it did not previously have a failure case which could occur in the minting of the deposited token.

Tests

Existing test cases have been extended to validate that the proper events are emitted.

@changeset-bot
Copy link

changeset-bot bot commented Sep 13, 2022

🦋 Changeset detected

Latest commit: 866780d

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

This PR includes changesets to release 5 packages
Name Type
@eth-optimism/contracts-bedrock Patch
@eth-optimism/actor-tests Patch
@eth-optimism/sdk Patch
@eth-optimism/drippie-mon Patch
@eth-optimism/message-relayer 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

@github-actions github-actions bot added 2-reviewers A-pkg-contracts-bedrock Area: packages/contracts-bedrock labels Sep 13, 2022
@maurelian maurelian force-pushed the ctb/m-05 branch 2 times, most recently from b3ba82b to 5fe814e Compare September 13, 2022 20:04
@mergify
Copy link
Contributor

mergify bot commented Sep 13, 2022

This PR changes implementation code, but doesn't include a changeset. Did you forget to add one?

@mergify
Copy link
Contributor

mergify bot commented Sep 14, 2022

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

@mergify
Copy link
Contributor

mergify bot commented Sep 15, 2022

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

@mergify mergify bot added the conflict label Sep 15, 2022
@mergify
Copy link
Contributor

mergify bot commented Sep 16, 2022

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

@mergify mergify bot added the conflict label Sep 16, 2022
@tynes
Copy link
Contributor

tynes commented Sep 19, 2022

Will approve after snapshot and bindings are rebuilt

@mergify mergify bot removed the conflict label Sep 19, 2022
@mergify
Copy link
Contributor

mergify bot commented Sep 19, 2022

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

@mergify mergify bot merged commit 249a8ed into develop Sep 19, 2022
@mergify mergify bot deleted the ctb/m-05 branch September 19, 2022 23:43
@mergify
Copy link
Contributor

mergify bot commented Sep 19, 2022

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 Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-pkg-contracts-bedrock Area: packages/contracts-bedrock
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants