OptimismPortal2 set initial `_balance` through StorageSetter pattern by ezdac · Pull Request #254 · celo-org/optimism

ezdac · 2024-10-08T23:21:52Z

Fixes #239

The custom gas-token feature adaptation for the fault-proof system using the OptimismPortal2 contract has been merged recently upstream.

We are using the custom-gas-token feature and additionally require a modification of the OptimismPortal's _balance value to be set to the entire allocation of Celo on the L2 - meaning that all L1 token is initially locked in the bridge and only usable on the L2.

Those changes are now adapted also to the OptimismPortal2, which was a requirement to make our custom-gas-token pre-locked balance feature work in conjunction with fault-proofs.

packages/contracts-bedrock/scripts/deploy/ChainAssertions.sol

piersy · 2024-10-10T09:36:03Z

op-e2e/celo/src/chain.js

@@ -1,8 +1,8 @@
-import { chainConfig } from 'viem/op-stack'
-import { defineChain } from 'viem'
+import { chainConfig } from "viem/op-stack";


Why the switch from single to double quotes here? The other js files in this dir seem to all be using single quotes

You're right - this was mainly due to my editor doing automatic formats on save.
I now added a .prettierrc.toml config and formatted all files consistently.

Looks like the tab width of 4 is not working out here, there are now about 7k lines of changed code, I think 2 is probably the right value, or whatever doesn't reformat any of the codebase.

Also, let's move out the re-formatting to a separate PR if needed.

piersy · 2024-10-10T09:51:43Z

packages/contracts-bedrock/scripts/deploy/Deploy.s.sol

+        uint256 initialBalance = 0;
+        customGasTokenAddress = cfg.customGasTokenAddress();
+        IERC20 token = IERC20(customGasTokenAddress);
+        initialBalance = token.balanceOf(optimismPortalProxy);


Where is the balance of the optimismPortalProxy set?

This is set within the Deploy.s.sol script:

Our Celo L1 Token contract takes the Portal address as an initializer argument:

optimism/packages/contracts-bedrock/scripts/deploy/Deploy.s.sol

Lines 1693 to 1697 in 981f87f

_upgradeAndCallViaSafe({

_proxy: payable(customGasTokenProxyAddress),

_implementation: customGasTokenAddress,

_innerCallData: abi.encodeCall(CeloTokenL1.initialize, (portalProxyAddress))

});

And then mints the whole market cap to the provided addresses balance:

optimism/packages/contracts-bedrock/src/celo/CeloTokenL1.sol

Lines 8 to 14 in 981f87f

uint256 constant TOTAL_MARKET_CAP = 1000000000e18; // 1 billion CELO

contract CeloTokenL1 is ERC20Upgradeable {

function initialize(address portalProxyAddress) public initializer {

__ERC20_init(NAME, SYMBOL);

_mint(portalProxyAddress, TOTAL_MARKET_CAP);

}

carterqw2 · 2024-10-10T09:52:01Z

op-e2e/celo/src/withdraw.js


-  const l2GasPayment =
-    receipt.gasUsed * receipt.effectiveGasPrice + receipt.l1Fee
+  const l2GasPayment = receipt.gasUsed * receipt.effectiveGasPrice;


I thought we want to keep the l1Fee, just have it 0 everywhere, maybe doesn't make sense to remove it here.

True, but it seems this is not yet reflected in the code. This means that receipt values do not align with deducted values.
This should only happen when the blobbasefeeScalar and basefeeScalar are not set to 0.

I opened an issue for it, I don't know if we were aware of this behavior already.

A hot-fix to make the test pass would be to set the blobbasefeeScalar and basefeeScalar values to 0 in the devnet deploy-config - currently the default values are used.

@karlb reverted the change (and merged only today), so we shouldn't be shortcutting anymore.

The change is in op-geth and we didn't update the dependency yet. I'll probably do it as part of the optimism repo rebase.

socket-security · 2024-10-10T22:14:15Z

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/prettier@3.3.3	environment, filesystem, unsafe	`0`	7.7 MB	prettier-bot
npm/viem@2.21.19	network Transitive: environment	`+10`	18.1 MB	awkweb, jmoxey

🚮 Removed packages: npm/viem@2.17.4

View full report↗︎

ezdac · 2024-10-10T22:23:28Z

So I added the l1Fee to the gas calculation in the test again. And I set the blobbasefeeScalar and basefeeScalar parameters to zero in the deploy config. (see #254 (comment) for context)

However the basefeeScalar zero value does not end up in the L1InfoTransaction that op-geth receives, and the receipts still have a non-zero value set for the L1BasefeeScalar.

I checked the SystemConfig contract, here both values are set to zero. However, e.g. one L1InfoTransaction on L2 I checked showed the following data, and the slice of data where the basefeeScalar value should be encoded is set to 1000000:

https://go.dev/play/p/8PN2lhQ-QGE

This is the same value that I could observe in the transaction receipts.

For now I don't know what's wrong here, but this is likely something that happens in the payload building in the L2 node.
Probably the SystemConfig update events are not read properly while building the payload attributes or the data is not encoded properly for the Ecotone encoding scheme.

I suggest we postpone fixing the tests for now and review this PR mainly for the changes to the OptimismPortal and the deploy script.

@pahor167 , could you have a look at this part of the PR?

packages/contracts-bedrock/scripts/deploy/Deploy.s.sol

karlb · 2024-10-14T11:10:54Z

op-e2e/celo/package.json

Does your editor change the indentation or did npm change in this respect? If it's your editor, please avoid the additional diff (for this and the other json files).

Ah, it is probably prettier. We should avoid having npm and prettier fight over the formatting.

I think npm doesn't have auto-formatting included. Before, the code probably has been formatted by an editor's global settings of prettier or the LanguageServer.
The problem with the diff was the tab-width setting in prettier - I changed it to 2, which seem to have been the setting the code has been formatted with previously. This got rid of most of the diff.

I added the npm run format script that calls prettier in write mode with the checked in prettier config file.
Like that we can have an opinionated formatter for the e2e tests from now on and avoid larger formatting diffs in the future. Maybe we can include this in a pre-commit config or CI run as well.

Just for comparison the formatting instructions in our op-geth e2e tests: https://github.com/celo-org/op-geth/blob/celo8/e2e_test/js-tests/Makefile

We could adjust them to be the same (same formatter, same config and called int the same way), but we can do that later.

Fixes #239 The custom gas-token feature adaptation for the fault-proof system using the `OptimismPortal2` contract has been merged recently upstream. We are using the custom-gas-token feature and additionally require a modification of the OptimismPortal's `_balance` value to be set to the entire allocation of Celo on the L2 - meaning that all L1 token is initially locked in the bridge and only usable on the L2. Those changes are now adapted also to the `OptimismPortal2`, which was a requirement to make our custom-gas-token pre-locked balance feature work in conjunction with fault-proofs.

Co-authored-by: Valentin Rodygin <20768968+carterqw2@users.noreply.github.com>

karlb · 2024-10-15T14:34:29Z

packages/contracts-bedrock/scripts/deploy/Deploy.s.sol

-            commands[2] = string.concat("[[ -f ", filePath, " ]] && echo \"present\"");
+            commands[2] = string.concat("[[ -f ", filePath, ' ]] && echo "present"');


This looks like an unnecessary change. Did you do it intentionally or did an autoformatter do this?

…254) * OptimismPortal2 set initial `_balance` through StorageSetter pattern Fixes #239 The custom gas-token feature adaptation for the fault-proof system using the `OptimismPortal2` contract has been merged recently upstream. We are using the custom-gas-token feature and additionally require a modification of the OptimismPortal's `_balance` value to be set to the entire allocation of Celo on the L2 - meaning that all L1 token is initially locked in the bridge and only usable on the L2. Those changes are now adapted also to the `OptimismPortal2`, which was a requirement to make our custom-gas-token pre-locked balance feature work in conjunction with fault-proofs. * Adapt withdraw e2e-tests to work with fault-proofs * Use prettier for formatting e2e tests * Fix typo Co-authored-by: Valentin Rodygin <20768968+carterqw2@users.noreply.github.com> * Set L1-fee scalars to zero in devnet --------- Co-authored-by: Valentin Rodygin <20768968+carterqw2@users.noreply.github.com>

carterqw2 reviewed Oct 9, 2024

View reviewed changes

packages/contracts-bedrock/scripts/deploy/ChainAssertions.sol Outdated Show resolved Hide resolved

piersy reviewed Oct 10, 2024

View reviewed changes

carterqw2 reviewed Oct 10, 2024

View reviewed changes

ezdac force-pushed the ezdac/optimismPortal2BalanceUpdate branch 2 times, most recently from 981f87f to e68c0e3 Compare October 10, 2024 20:45

ezdac requested a review from pahor167 October 10, 2024 22:24

pahor167 reviewed Oct 11, 2024

View reviewed changes

packages/contracts-bedrock/scripts/deploy/Deploy.s.sol Show resolved Hide resolved

karlb reviewed Oct 14, 2024

View reviewed changes

ezdac force-pushed the ezdac/optimismPortal2BalanceUpdate branch from 8762b47 to 72e56e4 Compare October 14, 2024 14:02

ezdac and others added 5 commits October 14, 2024 10:20

Adapt withdraw e2e-tests to work with fault-proofs

0a6f636

Use prettier for formatting e2e tests

a89cbf4

Fix typo

d00f780

Co-authored-by: Valentin Rodygin <20768968+carterqw2@users.noreply.github.com>

Set L1-fee scalars to zero in devnet

7009efb

ezdac force-pushed the ezdac/optimismPortal2BalanceUpdate branch from 72e56e4 to 7009efb Compare October 14, 2024 14:20

ezdac requested a review from pahor167 October 14, 2024 16:11

pahor167 approved these changes Oct 14, 2024

View reviewed changes

ezdac mentioned this pull request Oct 14, 2024

baseFeeScalar zero value does not translate to L1InfoTransaction on L2 #258

Closed

ezdac merged commit 3ea562b into celo9 Oct 14, 2024

ezdac deleted the ezdac/optimismPortal2BalanceUpdate branch October 14, 2024 20:05

karlb reviewed Oct 15, 2024

View reviewed changes

	_upgradeAndCallViaSafe({
	_proxy: payable(customGasTokenProxyAddress),
	_implementation: customGasTokenAddress,
	_innerCallData: abi.encodeCall(CeloTokenL1.initialize, (portalProxyAddress))
	});

	uint256 constant TOTAL_MARKET_CAP = 1000000000e18; // 1 billion CELO

	contract CeloTokenL1 is ERC20Upgradeable {
	function initialize(address portalProxyAddress) public initializer {
	__ERC20_init(NAME, SYMBOL);
	_mint(portalProxyAddress, TOTAL_MARKET_CAP);
	}

		commands[2] = string.concat("[[ -f ", filePath, " ]] && echo \"present\"");
		commands[2] = string.concat("[[ -f ", filePath, ' ]] && echo "present"');

Conversation

ezdac commented Oct 8, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezdac Oct 10, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

ezdac Oct 10, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

socket-security bot commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ezdac commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

karlb Oct 14, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ezdac Oct 10, 2024 •

edited

Loading

ezdac Oct 10, 2024 •

edited

Loading

socket-security bot commented Oct 10, 2024 •

edited

Loading

ezdac commented Oct 10, 2024 •

edited

Loading

karlb Oct 14, 2024 •

edited

Loading