contracts-bedrock: remove crossdomain messenger pausability #4913

tynes · 2023-02-17T02:01:53Z

Description

Removes pausability from the messenger and ensures to maintain the storage layout.
This touches a lot of parts of the system. The deploy scripts should get special attention during review.

The storage layout checking script was refactored a bit so that it shows progress when it runs, because it is pretty slow.

Tests
Unit tests simply needed to be removed for this feature. The tests covering ownability of the cross domain messengers are removed. The storage layout is preserved, you can look at the layout lock file to see this.

Additional Context
This builds on top of #4921 which added pausability to the portal. Now it is expected that the portal can be paused (withdrawals only) and there is no more pausability at the layer of the cross domain messenger. This prevents issues that arise when users try to do withdrawals when the messenger is paused, causing their withdrawals to get stuck.

Follow Up
I'd like to unify the spacer contracts in a follow up PR and then add the initializable contract afterwards. This would mean that we only need a single spacer contract and the old initialize storage slot would become a legacy spacer. The Initializable contract would follow the spacer contract in the inheritance chain, so that a new storage slot would be used for the new initialized slot. This would allow us to delete the try/catch in the dictator for the call to initializing the portal.

changeset-bot · 2023-02-17T02:01:57Z

⚠️ No Changeset found

Latest commit: 608ebc9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

mergify · 2023-02-17T02:02:27Z

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

tynes · 2023-02-17T02:13:54Z

This can remove the ownable upgradable from the crossdomain messenger as well

mergify · 2023-02-17T03:07:46Z

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

mergify · 2023-02-17T18:29:47Z

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

tynes · 2023-02-18T01:38:19Z

Will follow up with notes on how to review this

codecov · 2023-02-18T01:38:28Z

Codecov Report

Merging #4913 (608ebc9) into develop (1b0c603) will decrease coverage by 0.68%.
The diff coverage is 100.00%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4913      +/-   ##
===========================================
- Coverage    40.84%   40.17%   -0.68%     
===========================================
  Files          324      303      -21     
  Lines        19652    19009     -643     
  Branches       770      655     -115     
===========================================
- Hits          8027     7637     -390     
+ Misses       11026    10772     -254     
- Partials       599      600       +1

Flag	Coverage Δ
bedrock-go-tests	`36.12% <100.00%> (-0.04%)`	⬇️
contracts-bedrock-tests	`49.74% <ø> (+0.31%)`	⬆️
contracts-tests	`98.86% <ø> (ø)`
core-utils-tests	`?`
dtl-tests	`47.15% <ø> (ø)`
fault-detector-tests	`33.88% <ø> (ø)`
sdk-tests	`38.74% <ø> (ø)`

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

Impacted Files	Coverage Δ
op-chain-ops/genesis/config.go	`29.26% <ø> (-0.46%)`	⬇️
...ts-bedrock/contracts/L1/L1CrossDomainMessenger.sol	`75.00% <ø> (+15.00%)`	⬆️
...ts-bedrock/contracts/deployment/SystemDictator.sol	`0.00% <ø> (ø)`
...drock/contracts/universal/CrossDomainMessenger.sol	`78.37% <ø> (+4.56%)`	⬆️
op-chain-ops/genesis/layer_one.go	`75.34% <100.00%> (-0.21%)`	⬇️
op-node/sources/batching.go	`82.65% <0.00%> (-3.07%)`	⬇️
packages/core-utils/src/common/misc.ts
packages/core-utils/src/common/test-utils.ts
packages/core-utils/src/optimism/hashing.ts
packages/core-utils/src/index.ts
... and 17 more

mergify · 2023-02-18T01:50:50Z

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

tynes · 2023-02-21T19:33:39Z

This PR is going to need some changes once #4921 is merged

mergify · 2023-02-21T23:50:21Z

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

mergify · 2023-02-23T22:52:00Z

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

packages/contracts-bedrock/.storage-layout

packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol

packages/contracts-bedrock/deploy/021-SystemDictatorSteps-2.ts

mergify · 2023-02-24T18:07:34Z

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

packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol

These storage values were removed from the contracts so we no longer need to configure them in the storage config

tynes · 2023-02-25T04:40:25Z

@Mergifyio refresh

mergify · 2023-02-25T04:40:28Z

refresh

✅ Pull request refreshed

mslipper

Go changes LGTM.

tynes requested a review from a team as a code owner February 17, 2023 02:01

tynes requested a review from smartcontracts February 17, 2023 02:01

mergify bot added the conflict label Feb 17, 2023

tynes force-pushed the feat/ctb-remove-xdm-pause branch from 06a9cc2 to 8e3f3af Compare February 17, 2023 02:09

tynes requested a review from a team as a code owner February 17, 2023 02:09

tynes requested a review from trianglesphere February 17, 2023 02:09

mergify bot removed the conflict label Feb 17, 2023

tynes marked this pull request as draft February 17, 2023 02:14

mergify bot added the conflict label Feb 17, 2023

tynes force-pushed the feat/ctb-remove-xdm-pause branch from 3922ab5 to 9398712 Compare February 17, 2023 17:40

mergify bot removed the conflict label Feb 17, 2023

tynes force-pushed the feat/ctb-remove-xdm-pause branch from a96b37d to 2b35023 Compare February 17, 2023 18:24

mergify bot added the conflict label Feb 17, 2023

tynes force-pushed the feat/ctb-remove-xdm-pause branch from 2b35023 to 09ed8e1 Compare February 17, 2023 18:40

mergify bot removed the conflict label Feb 17, 2023

tynes force-pushed the feat/ctb-remove-xdm-pause branch from 1692625 to 800779d Compare February 18, 2023 00:18

tynes marked this pull request as ready for review February 18, 2023 00:18

tynes mentioned this pull request Feb 18, 2023

feat: portal pausable #4921

Merged

mergify bot added the conflict label Feb 18, 2023

tynes force-pushed the feat/ctb-remove-xdm-pause branch from 2c4c4bd to 49c0a3e Compare February 21, 2023 19:30

mergify bot removed the conflict label Feb 21, 2023

mergify bot added the conflict label Feb 21, 2023

tynes force-pushed the feat/ctb-remove-xdm-pause branch from 49c0a3e to 39d8d7a Compare February 23, 2023 18:15

mergify bot removed the conflict label Feb 23, 2023

tynes force-pushed the feat/ctb-remove-xdm-pause branch 2 times, most recently from bfcee85 to 49a8621 Compare February 23, 2023 18:35

tynes removed the do-not-merge label Feb 23, 2023

mergify bot added the conflict label Feb 23, 2023

tynes force-pushed the feat/ctb-remove-xdm-pause branch from 49a8621 to 356c9d1 Compare February 23, 2023 23:33

mergify bot removed the conflict label Feb 23, 2023

clabby reviewed Feb 24, 2023

View reviewed changes

mergify bot added the conflict label Feb 24, 2023

maurelian reviewed Feb 24, 2023

View reviewed changes

packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol Outdated Show resolved Hide resolved

tynes added 7 commits February 24, 2023 13:03

contracts-bedrock: remove crossdomain messenger pausability

c1422d9

op-chain-ops: remove dead storage values

4c8bb14

These storage values were removed from the contracts so we no longer need to configure them in the storage config

op-chain-ops: update initialize call with no arg

a8a0bec

contracts-bedrock: fix checkl2

de48539

contracts-bedrock: fix deployment

4190281

ctb: refactor layout script

b50bb51

contracts-bedrock: slight cleanup

608ebc9

tynes force-pushed the feat/ctb-remove-xdm-pause branch from 356c9d1 to 608ebc9 Compare February 24, 2023 21:46

mergify bot removed the conflict label Feb 24, 2023

maurelian approved these changes Feb 25, 2023

View reviewed changes

mslipper approved these changes Feb 25, 2023

View reviewed changes

mslipper merged commit 8969e46 into develop Feb 25, 2023

mslipper deleted the feat/ctb-remove-xdm-pause branch February 25, 2023 06:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

contracts-bedrock: remove crossdomain messenger pausability #4913

contracts-bedrock: remove crossdomain messenger pausability #4913

tynes commented Feb 17, 2023 •

edited

Loading

changeset-bot bot commented Feb 17, 2023 •

edited

Loading

mergify bot commented Feb 17, 2023

tynes commented Feb 17, 2023

mergify bot commented Feb 17, 2023

mergify bot commented Feb 17, 2023

tynes commented Feb 18, 2023

codecov bot commented Feb 18, 2023 •

edited

Loading

mergify bot commented Feb 18, 2023

tynes commented Feb 21, 2023

mergify bot commented Feb 21, 2023

mergify bot commented Feb 23, 2023

mergify bot commented Feb 24, 2023

tynes commented Feb 25, 2023

mergify bot commented Feb 25, 2023

mslipper left a comment

contracts-bedrock: remove crossdomain messenger pausability #4913

contracts-bedrock: remove crossdomain messenger pausability #4913

Conversation

tynes commented Feb 17, 2023 • edited Loading

changeset-bot bot commented Feb 17, 2023 • edited Loading

⚠️ No Changeset found

mergify bot commented Feb 17, 2023

tynes commented Feb 17, 2023

mergify bot commented Feb 17, 2023

mergify bot commented Feb 17, 2023

tynes commented Feb 18, 2023

codecov bot commented Feb 18, 2023 • edited Loading

Codecov Report

mergify bot commented Feb 18, 2023

tynes commented Feb 21, 2023

mergify bot commented Feb 21, 2023

mergify bot commented Feb 23, 2023

mergify bot commented Feb 24, 2023

tynes commented Feb 25, 2023

mergify bot commented Feb 25, 2023

✅ Pull request refreshed

mslipper left a comment

Choose a reason for hiding this comment

tynes commented Feb 17, 2023 •

edited

Loading

changeset-bot bot commented Feb 17, 2023 •

edited

Loading

codecov bot commented Feb 18, 2023 •

edited

Loading