Skip to content

Naming improvements in Oracle#2990

Merged
mergify[bot] merged 4 commits intodevelopfrom
m/propose-not-append
Jul 14, 2022
Merged

Naming improvements in Oracle#2990
mergify[bot] merged 4 commits intodevelopfrom
m/propose-not-append

Conversation

@maurelian
Copy link
Contributor

@maurelian maurelian commented Jul 12, 2022 •
edited
Loading

Description

The main changes are:

  1. Rename sequencer to proposer
  2. Use propose instead of append
  3. Use outputRoot instead of l2Output when referring to the bytes32 hash of the Output

@changeset-bot
Copy link

changeset-bot bot commented Jul 12, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: e533645

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

@github-actions github-actions bot added 2-reviewers A-pkg-contracts-bedrock Area: packages/contracts-bedrock labels Jul 12, 2022
@mergify
Copy link
Contributor

mergify bot commented Jul 12, 2022

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

@maurelian maurelian force-pushed the m/propose-not-append branch 3 times, most recently from b0f1ab8 to 366eeb0 Compare July 12, 2022 01:58
@maurelian maurelian requested review from protolambda and tynes July 12, 2022 01:58
@tynes
Copy link
Contributor

tynes commented Jul 12, 2022

What do you think about also renaming getL2Output to be more in style?

@tynes
Copy link
Contributor

tynes commented Jul 12, 2022

I like the idea here

@tynes
Copy link
Contributor

tynes commented Jul 12, 2022

Looks like a new gas snapshot is necessary

@maurelian maurelian force-pushed the m/propose-not-append branch from 366eeb0 to 151b501 Compare July 12, 2022 14:36
@maurelian
Copy link
Contributor Author

maurelian commented Jul 12, 2022

What do you think about also renaming getL2Output to be more in style?

What name do you have in mind?
FWIW I did try getOutput as well as proposeOutput, deleteOutput etc. See my comment here about removing l2.

@maurelian maurelian force-pushed the m/propose-not-append branch from 151b501 to 384ff8d Compare July 12, 2022 15:21
smartcontracts
smartcontracts previously approved these changes Jul 12, 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.

Nice, good changes.

@smartcontracts smartcontracts dismissed their stale review July 12, 2022 16:08

Whoops missed the fact that CI is failing

@mergify
Copy link
Contributor

mergify bot commented Jul 12, 2022

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

@mergify mergify bot added the conflict label Jul 12, 2022
@maurelian maurelian force-pushed the m/propose-not-append branch from 384ff8d to 029f163 Compare July 12, 2022 18:53
@mergify mergify bot removed the conflict label Jul 12, 2022
@mergify
Copy link
Contributor

mergify bot commented Jul 13, 2022

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

@mergify mergify bot added the conflict label Jul 13, 2022
@maurelian maurelian force-pushed the m/propose-not-append branch from 029f163 to 8fc9265 Compare July 13, 2022 20:07
@mergify mergify bot removed the conflict label Jul 13, 2022
@maurelian maurelian force-pushed the m/propose-not-append branch from 8fc9265 to 20131bd Compare July 13, 2022 20:29
@mergify
Copy link
Contributor

mergify bot commented Jul 13, 2022

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

@mergify mergify bot added the conflict label Jul 13, 2022
@tynes tynes force-pushed the m/propose-not-append branch from 20131bd to 359be35 Compare July 13, 2022 20:52
@mergify mergify bot removed the conflict label Jul 13, 2022
@tynes
Copy link
Contributor

tynes commented Jul 13, 2022

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Jul 13, 2022

refresh

✅ Pull request refreshed

@maurelian
Copy link
Contributor Author

maurelian commented Jul 13, 2022

@Mergifyio refresh

What does that do @tynes ?

@tynes
Copy link
Contributor

tynes commented Jul 13, 2022

@Mergifyio refresh

What does that do @tynes ?

It makes mergify look at the PR again, helps to get it on the merge train

@mergify
Copy link
Contributor

mergify bot commented Jul 13, 2022

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

@mergify mergify bot added the conflict label Jul 13, 2022
@tynes tynes force-pushed the m/propose-not-append branch from 6cd958d to e3f0d76 Compare July 13, 2022 21:38
@mergify mergify bot removed the conflict label Jul 13, 2022
protolambda
protolambda approved these changes Jul 13, 2022
View reviewed changes
Copy link
Contributor

@protolambda protolambda left a comment

Choose a reason for hiding this comment

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

LGTM. You may need to update the bindings or gas-snapshot later, depending on the merge order with the other PRs in review currently.

@mergify
Copy link
Contributor

mergify bot commented Jul 13, 2022

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

@mergify mergify bot added the conflict label Jul 13, 2022
maurelian added 3 commits July 13, 2022 19:37
@maurelian
style(ctb): Rename append to propose
e613aae 
Please enter the commit message for your changes. Lines starting
@maurelian
style(ctb): Use output root for bytes32
ce32f43 
This is more accurate than saying 'L2 output' which is better
applied to the full unhashed data structure
@maurelian
style(ctb): Rename sequencer to proposer in oracle
8966409
@maurelian maurelian force-pushed the m/propose-not-append branch from e3f0d76 to 8966409 Compare July 13, 2022 23:38
@mergify mergify bot removed the conflict label Jul 13, 2022
@mergify
Copy link
Contributor

mergify bot commented Jul 13, 2022

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

@mergify
Copy link
Contributor

mergify bot commented Jul 13, 2022

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

@mergify mergify bot merged commit fa9823f into develop Jul 14, 2022
@mergify mergify bot deleted the m/propose-not-append branch July 14, 2022 00:09
@mergify mergify bot removed the on-merge-train label Jul 14, 2022
theochap added a commit that referenced this pull request Dec 10, 2025
@sashass1315 @theochap
Remove unnecessary clone of chain_dbs_map in init_l1_watcher (#2990)
d7d4b5d 
- Eliminated chain_dbs_map.clone() before constructing ReorgHandler in
Service::init_l1_watcher().
- The map is created for the spawned task and not used elsewhere, so it
can be moved safely.
- This reduces allocations and minor overhead without changing behavior.

Co-authored-by: theo <80177219+theochap@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tynes tynes tynes approved these changes

@smartcontracts smartcontracts smartcontracts left review comments

+1 more reviewer

@protolambda protolambda protolambda 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.

4 participants

@maurelian @tynes @smartcontracts @protolambda