getL2Output reverts if not found by maurelian · Pull Request #2979 · ethereum-optimism/optimism

maurelian · 2022-07-11T20:22:43Z

Description

Calls to oracle.getL2Output revert if no output is found for the given block number.

Pros:

Reverting on blocks which are not found forces 3rd parties to consider and handle this failure mode, instead of possibly missing this 'edge case'. This feels more semantically correct, and is similar to how many of ERC721's functions handle non-existent token IDs (ex. getApproved).

Cons:

This change forces us to introduce some pretty ugly try/catch code in isOutputFinalized().

Metadata

Fixes #ENG-2279

changeset-bot · 2022-07-11T20:22:46Z

🦋 Changeset detected

Latest commit: ad77328

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

This PR includes changesets to release 1 package

Name	Type
@eth-optimism/contracts-bedrock	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

mergify · 2022-07-11T20:23:17Z

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

maurelian · 2022-07-12T02:21:37Z

@smartcontracts: I discussed this change with @tynes. Would appreciate your input before I spend time debuggin the go tests.

mergify · 2022-07-12T17:43:11Z

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

packages/contracts-bedrock/contracts/L1/OptimismPortal.sol

mergify · 2022-07-13T20:33:15Z

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

maurelian · 2022-07-14T20:48:13Z

@mergify refresh

mergify · 2022-07-14T20:48:33Z

refresh

✅ Pull request refreshed

mergify · 2022-07-14T21:15:15Z

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

mergify · 2022-07-15T02:22:53Z

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

packages/contracts-bedrock/contracts/L1/OptimismPortal.sol

maurelian · 2022-07-15T16:45:51Z

@mergify refresh

mergify · 2022-07-15T16:46:12Z

refresh

✅ Pull request refreshed

maurelian · 2022-07-15T17:26:39Z

@mergify refresh

mergify · 2022-07-15T17:27:12Z

refresh

✅ Pull request refreshed

feat(bedrock): Handle error on empty output in Portal feat(ctb): use isOutputFinalized in finalizeWithdrawalTx

Revert "feat(ctb): use isOutputFinalized in finalizeWithdrawalTx" This reverts commit 4d4e89d. ctb: getL2Output returns the next output for a block ctb: Fix error message style

mergify · 2022-07-17T18:45:42Z

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

mergify · 2022-07-17T18:45:49Z

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

github-actions bot added 2-reviewers A-pkg-contracts-bedrock Area: packages/contracts-bedrock labels Jul 11, 2022

mergify bot added the conflict label Jul 11, 2022

maurelian force-pushed the m/revert-on-get-zerol2output branch 3 times, most recently from c64b175 to 5a7873b Compare July 12, 2022 00:45

mergify bot removed the conflict label Jul 12, 2022

maurelian requested review from smartcontracts and tynes July 12, 2022 02:20

maurelian force-pushed the m/revert-on-get-zerol2output branch from 5a7873b to 90478ca Compare July 12, 2022 14:18

mergify bot added the conflict label Jul 12, 2022

smartcontracts reviewed Jul 12, 2022

View reviewed changes

packages/contracts-bedrock/contracts/L1/OptimismPortal.sol Outdated Show resolved Hide resolved

maurelian force-pushed the m/revert-on-get-zerol2output branch from 90478ca to dc46973 Compare July 13, 2022 19:58

mergify bot removed the conflict label Jul 13, 2022

maurelian force-pushed the m/revert-on-get-zerol2output branch from dc46973 to a57ead4 Compare July 13, 2022 20:06

tynes approved these changes Jul 13, 2022

View reviewed changes

mergify bot added the conflict label Jul 13, 2022

maurelian force-pushed the m/revert-on-get-zerol2output branch from a57ead4 to 235dba7 Compare July 14, 2022 19:57

mergify bot removed the conflict label Jul 14, 2022

mergify bot added the conflict label Jul 14, 2022

maurelian force-pushed the m/revert-on-get-zerol2output branch from 9fdcbe9 to 4d4e89d Compare July 14, 2022 23:13

mergify bot removed the conflict label Jul 14, 2022

maurelian requested a review from smartcontracts July 14, 2022 23:22

mergify bot added the conflict label Jul 15, 2022

smartcontracts reviewed Jul 15, 2022

View reviewed changes

packages/contracts-bedrock/contracts/L1/OptimismPortal.sol Outdated Show resolved Hide resolved

smartcontracts reviewed Jul 15, 2022

View reviewed changes

packages/contracts-bedrock/contracts/L1/OptimismPortal.sol Outdated Show resolved Hide resolved

smartcontracts reviewed Jul 15, 2022

View reviewed changes

packages/contracts-bedrock/contracts/L1/OptimismPortal.sol Show resolved Hide resolved

maurelian commented Jul 15, 2022

View reviewed changes

packages/contracts-bedrock/contracts/L1/OptimismPortal.sol Outdated Show resolved Hide resolved

maurelian force-pushed the m/revert-on-get-zerol2output branch 2 times, most recently from 82017d8 to cbfb4e8 Compare July 15, 2022 16:23

mergify bot removed the conflict label Jul 15, 2022

smartcontracts approved these changes Jul 15, 2022

View reviewed changes

maurelian commented Jul 15, 2022

View reviewed changes

packages/contracts-bedrock/contracts/L1/OptimismPortal.sol Show resolved Hide resolved

maurelian force-pushed the m/revert-on-get-zerol2output branch from cbfb4e8 to efb96fd Compare July 15, 2022 17:07

maurelian added 2 commits July 17, 2022 14:18

feat(bedrock): getL2Output reverts if not found

a8ed236

feat(bedrock): Handle error on empty output in Portal feat(ctb): use isOutputFinalized in finalizeWithdrawalTx

ctb: Remove extraneous abi encoding

ad77328

Revert "feat(ctb): use isOutputFinalized in finalizeWithdrawalTx" This reverts commit 4d4e89d. ctb: getL2Output returns the next output for a block ctb: Fix error message style

maurelian force-pushed the m/revert-on-get-zerol2output branch from a6e9bf4 to ad77328 Compare July 17, 2022 18:18

mergify bot added the on-merge-train label Jul 17, 2022

mergify bot merged commit 7e6eb9b into develop Jul 17, 2022

mergify bot deleted the m/revert-on-get-zerol2output branch July 17, 2022 18:45

mergify bot removed the on-merge-train label Jul 17, 2022

Conversation

maurelian commented Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

mergify bot commented Jul 11, 2022

Uh oh!

maurelian commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergify bot commented Jul 12, 2022

Uh oh!

Uh oh!

mergify bot commented Jul 13, 2022

Uh oh!

maurelian commented Jul 14, 2022

Uh oh!

mergify bot commented Jul 14, 2022

✅ Pull request refreshed

Uh oh!

mergify bot commented Jul 14, 2022

Uh oh!

mergify bot commented Jul 15, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maurelian commented Jul 15, 2022

Uh oh!

mergify bot commented Jul 15, 2022

✅ Pull request refreshed

Uh oh!

maurelian commented Jul 15, 2022

Uh oh!

mergify bot commented Jul 15, 2022

✅ Pull request refreshed

Uh oh!

mergify bot commented Jul 17, 2022

Uh oh!

mergify bot commented Jul 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

maurelian commented Jul 11, 2022 •

edited

Loading

changeset-bot bot commented Jul 11, 2022 •

edited

Loading

maurelian commented Jul 12, 2022 •

edited

Loading