Skip to content

ctb: Change getL2Output to getL2OutputAfter#4045

Closed
maurelian wants to merge 1 commit intodevelopfrom
jm/getNextL2Output
Closed

ctb: Change getL2Output to getL2OutputAfter#4045
maurelian wants to merge 1 commit intodevelopfrom
jm/getNextL2Output

Conversation

@maurelian
Copy link
Contributor

@maurelian maurelian commented Nov 22, 2022 •
edited
Loading

Description

Renames getL2Output to getL2OutputAfter, which is semantically more descriptive, and could prevent future issues due to misunderstandings.

This change may break external code, and so is posted here for consideration and discussion.

Tests

Tests are updated.

@changeset-bot
Copy link

changeset-bot bot commented Nov 22, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: 5d0d69b

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 Minor
@eth-optimism/sdk Minor
@eth-optimism/actor-tests 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 A-pkg-sdk Area: packages/sdk labels Nov 22, 2022
@mergify mergify bot added the sdk label Nov 22, 2022
@mergify mergify bot requested review from nickbalestra and roninjin10 November 22, 2022 17:08
@maurelian maurelian requested review from K-Ho and removed request for nickbalestra and roninjin10 November 22, 2022 17:50
@mergify mergify bot requested review from nickbalestra and roninjin10 November 22, 2022 17:50
@maurelian maurelian changed the title ctb: Change getL2Output to getNextProposedOutput ctb: Change getL2Output to getNextL2ProposedOutput Nov 22, 2022
@mergify
Copy link
Contributor

mergify bot commented Nov 22, 2022

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

@mergify mergify bot added the conflict label Nov 22, 2022
@mergify mergify bot removed the conflict label Nov 22, 2022
@maurelian maurelian marked this pull request as ready for review November 22, 2022 20:50
@maurelian maurelian requested review from clabby, smartcontracts and tynes and removed request for K-Ho, nickbalestra and roninjin10 November 22, 2022 20:50
@mergify mergify bot requested review from nickbalestra and roninjin10 November 22, 2022 20:51
clabby
clabby suggested changes Nov 22, 2022
View reviewed changes
Copy link
Contributor

@clabby clabby left a comment

Choose a reason for hiding this comment

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

Looks good- will approve after the gas snapshot is updated.

@mergify
Copy link
Contributor

mergify bot commented Nov 22, 2022

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

@mergify
Copy link
Contributor

mergify bot commented Nov 23, 2022

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

@mergify mergify bot added the conflict label Nov 23, 2022
@maurelian maurelian changed the title ctb: Change getL2Output to getNextL2ProposedOutput ctb: Change getL2Output to getL2OutputAfter Nov 23, 2022
@mergify mergify bot removed the conflict label Nov 23, 2022
smartcontracts
smartcontracts reviewed Nov 23, 2022
View reviewed changes
@github-actions github-actions bot removed the A-pkg-sdk Area: packages/sdk label Nov 23, 2022
@mergify
Copy link
Contributor

mergify bot commented Nov 23, 2022

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

@mergify mergify bot added the conflict label Nov 23, 2022
@mergify mergify bot removed the conflict label Nov 23, 2022
@mergify
Copy link
Contributor

mergify bot commented Nov 28, 2022

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

@mergify mergify bot added the conflict label Nov 28, 2022
@maurelian
ctb: Change getL2Output to getL2OuputAfter
5d0d69b 
This change is semantcially clearer since the L2Output returned doesn’t
always directly correspond to the output values of the given L2 block.
@mergify mergify bot removed the conflict label Nov 28, 2022
smartcontracts
smartcontracts requested changes Nov 28, 2022
View reviewed changes
Copy link
Contributor

@smartcontracts smartcontracts left a comment

Choose a reason for hiding this comment

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

Superseded by #4093

@mergify
Copy link
Contributor

mergify bot commented Nov 29, 2022

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

@mergify mergify bot added the conflict label Nov 29, 2022
@tynes
Copy link
Contributor

tynes commented Nov 29, 2022

Going to close this in favor of #4093

@tynes tynes closed this Nov 29, 2022
@mergify mergify bot removed the conflict label Nov 29, 2022
@maurelian maurelian deleted the jm/getNextL2Output branch December 6, 2022 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@smartcontracts smartcontracts smartcontracts requested changes

@tynes tynes Awaiting requested review from tynes

@nickbalestra nickbalestra Awaiting requested review from nickbalestra

@roninjin10 roninjin10 Awaiting requested review from roninjin10

+1 more reviewer

@clabby clabby clabby approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-pkg-contracts-bedrock Area: packages/contracts-bedrock

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@maurelian @codecov-commenter @tynes @clabby @smartcontracts