Revert "contracts-bedrock: remove CGT code (#13921)" #29

blockchaindevsh · 2025-03-17T13:00:28Z

This reverts commit ef03df7, thus re-enables CGT.

Conflicts:
packages/contracts-bedrock/snapshots/.gas-snapshot
packages/contracts-bedrock/snapshots/semver-lock.json
packages/contracts-bedrock/src/L1/L1CrossDomainMessenger.sol
packages/contracts-bedrock/src/L1/L1StandardBridge.sol
packages/contracts-bedrock/src/L1/OptimismPortalInterop.sol
packages/contracts-bedrock/src/L1/SystemConfigInterop.sol

This reverts commit ef03df7. Conflicts: packages/contracts-bedrock/snapshots/.gas-snapshot packages/contracts-bedrock/snapshots/semver-lock.json packages/contracts-bedrock/src/L1/L1CrossDomainMessenger.sol packages/contracts-bedrock/src/L1/L1StandardBridge.sol packages/contracts-bedrock/src/L1/OptimismPortalInterop.sol packages/contracts-bedrock/src/L1/SystemConfigInterop.sol

qizhou · 2025-03-17T21:32:20Z

The title is incorrect: GCT => CGT

blockchaindevsh · 2025-03-17T22:24:05Z

The title is incorrect: GCT => CGT

Fixed, it was generated automatically.

qzhodl · 2025-03-18T10:09:27Z

Do we want to maintain a separate branch to store the contract code we are using (similar to how Celo does it), or should we keep the old contract code in the default branch and merge upstream changes as needed?

blockchaindevsh · 2025-03-18T12:04:53Z

Do we want to maintain a separate branch to store the contract code we are using (similar to how Celo does it), or should we keep the old contract code in the default branch and merge upstream changes as needed?

There's a tag beta_contracts pointing to the version for the contracts we're using.

qzhodl · 2025-03-19T01:59:19Z

Do we want to maintain a separate branch to store the contract code we are using (similar to how Celo does it), or should we keep the old contract code in the default branch and merge upstream changes as needed?

There's a tag beta_contracts pointing to the version for the contracts we're using.

So what is the motivation of this change? Do we want to upgrade these change to the beta testnet?

blockchaindevsh · 2025-03-19T02:53:31Z

Do we want to maintain a separate branch to store the contract code we are using (similar to how Celo does it), or should we keep the old contract code in the default branch and merge upstream changes as needed?

There's a tag beta_contracts pointing to the version for the contracts we're using.

So what is the motivation of this change? Do we want to upgrade these change to the beta testnet?

This change is only for the develop version. We can create a new release version after the develop version is thoroughly tested.

AFAIK there's no project doing overall contracts upgrades besides the super chains right now. Most probably the new release will be used to deploy a new testnet.

qzhodl · 2025-03-19T03:11:27Z

Do we want to maintain a separate branch to store the contract code we are using (similar to how Celo does it), or should we keep the old contract code in the default branch and merge upstream changes as needed?

There's a tag beta_contracts pointing to the version for the contracts we're using.

So what is the motivation of this change? Do we want to upgrade these change to the beta testnet?

This change is only for the develop version. We can create a new release version after the develop version is thoroughly tested.

AFAIK there's no project doing overall contracts upgrades besides the super chains right now. Most probably the new release will be used to deploy a new testnet.

I think if we plan to use the latest contract code for the upcoming mainnet launch, we may need to consider the testnet strategy. We have two options: either upgrade the contract on the beta testnet or deploy a new testnet specifically for validation.

Additionally, have we successfully passed the CircleCI tests for this PR? It would also be beneficial to include details in the PR description about the types of tests conducted to validate these changes.

blockchaindevsh · 2025-03-19T03:39:33Z

I think if we plan to use the latest contract code for the upcoming mainnet launch, we may need to consider the testnet strategy. We have two options: either upgrade the contract on the beta testnet or deploy a new testnet specifically for validation.

Additionally, have we successfully passed the CircleCI tests for this PR? It would also be beneficial to include details in the PR description about the types of tests conducted to validate these changes.

When we merge with upstream, the contracts are automatically merged to the latest develop version.
So the contract changes are expected to be maintained like this:
After the develop version is tested, we cherry-pick the diffs to the corresponding contract release of upstream, to create our corresponding contract release.

The test PRs are yet to come, in order to make this PR not too big. There're 3 upstream PRs in total related with CGT as posted in our group.

blockchaindevsh · 2025-03-19T16:45:45Z

I'm now inclined to doing all reverting in this single PR since it'll be a big one anyway.

WDYT?

qzhodl · 2025-03-20T02:55:13Z

It is a really big PR and it would be more convenient for me (any opinions from other reviewers?) if it is possible to divide it into smaller ones.

blockchaindevsh · 2025-03-20T03:08:47Z

It is a really big PR and it would be more convenient for me (any opinions from other reviewers?) if it is possible to divide it into smaller ones.

The other two PRs are tests for CGT, so if we want this PR to be tested before merging, we have to include them here.
Otherwise we can merge this PR as is, and apply the tests in a follow-up PR.

qzhodl · 2025-03-20T07:09:15Z

packages/contracts-bedrock/src/L2/ETHLiquidity.sol

    /// @notice Semantic version.
-    /// @custom:semver 1.0.0-beta.6
-    string public constant version = "1.0.0-beta.6";
+    /// @custom:semver 1.0.0-beta.5


Why are we downgrading the version number?

It's done automatically when reverting a commit, it should be ok since we're not using the version and it'll be recovered next time we merge with upstream.

qzhodl · 2025-03-20T07:14:25Z

packages/contracts-bedrock/src/L2/ETHLiquidity.sol

    /// @notice Allows an address to lock ETH liquidity into this contract.
    function burn() external payable {
        if (msg.sender != Predeploys.SUPERCHAIN_WETH) revert Unauthorized();
+        if (IL1Block(Predeploys.L1_BLOCK_ATTRIBUTES).isCustomGasToken()) revert NotCustomGasToken();


Is it possible that even if we reintroduce the CGT feature in the latest version, it still won’t function properly due to numerous revert conditions throughout the code? Additionally, since CGT has been removed, future updates may not consider this case, making the added feature incompatible with CGT.

Yes, it's possible, but hopefully will be caught by the other 2 test PRs, especially the e2e test. But yeah it can happen that even the test passes, there're still cases not caught.

We are quite sure that CGT is working for Holocene, but may be left out in subsequent HF. So in the future if we want to enable new HFs, we may need to manually port CGT feature&tests to the new forks.

qzhodl · 2025-03-21T03:23:43Z

packages/contracts-bedrock/src/L2/SuperchainWETH.sol

+
+    /// @inheritdoc WETH98
+    function deposit() public payable override {
+        if (IL1Block(Predeploys.L1_BLOCK_ATTRIBUTES).isCustomGasToken()) revert NotCustomGasToken();


What is the relationship between CGT and SuperchainWETH (is it related to interoperability)?
Does it imply that an L2 with CGT enabled cannot join the Superchain ecosystem?

I think SuperchainWETH has some assumptions here:

It is supposed to be always "wrapped eth" among the super chains.

The native token on L2 is supposed to be ETH.

The assumptions don't hold when CGT is enabled. I remember previously @yazhenwish mentioned super chains can not have code change?

blockchaindevsh requested review from dajuguan, ping-ke, qizhou, qzhodl and syntrust March 17, 2025 13:00

blockchaindevsh changed the title ~~Revert "contracts-bedrock: remove GCT code (#13921)"~~ Revert "contracts-bedrock: remove CGT code (#13921)" Mar 17, 2025

blockchaindevsh mentioned this pull request Mar 18, 2025

EthStorage Devs Meeting #119 Agenda ethstorage/pm#188

Closed

qzhodl reviewed Mar 20, 2025

View reviewed changes

qzhodl reviewed Mar 21, 2025

View reviewed changes

blockchaindevsh mentioned this pull request Mar 21, 2025

QKC/OP Devs Meeting #41 Agenda QuarkChain/pm#14

Closed

qzhodl approved these changes Mar 22, 2025

View reviewed changes

qzhodl mentioned this pull request Mar 24, 2025

EthStorage Devs Meeting #120 Agenda ethstorage/pm#189

Closed

Revert "contracts-bedrock: remove CGT code (#13921)" #29

Are you sure you want to change the base?

Revert "contracts-bedrock: remove CGT code (#13921)" #29

Uh oh!

Conversation

blockchaindevsh commented Mar 17, 2025

Uh oh!

qizhou commented Mar 17, 2025

Uh oh!

blockchaindevsh commented Mar 17, 2025

Uh oh!

qzhodl commented Mar 18, 2025

Uh oh!

blockchaindevsh commented Mar 18, 2025

Uh oh!

qzhodl commented Mar 19, 2025

Uh oh!

blockchaindevsh commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qzhodl commented Mar 19, 2025

Uh oh!

blockchaindevsh commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blockchaindevsh commented Mar 19, 2025

Uh oh!

qzhodl commented Mar 20, 2025

Uh oh!

blockchaindevsh commented Mar 20, 2025

Uh oh!

qzhodl Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

blockchaindevsh Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

qzhodl Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

blockchaindevsh Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qzhodl Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

blockchaindevsh Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

blockchaindevsh commented Mar 19, 2025 •

edited

Loading

blockchaindevsh commented Mar 19, 2025 •

edited

Loading

blockchaindevsh Mar 20, 2025 •

edited

Loading

blockchaindevsh Mar 21, 2025 •

edited

Loading