Conversation
Raid5594
requested review from
StanislavBreadless and
vladbochok
as code owners
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cd2430c09
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
l1-contracts/contracts/common/l2-helpers/L2ContractAddresses.sol
Outdated
Show resolved
Hide resolved
StanislavBreadless
left a comment
StanislavBreadless
left a comment
There was a problem hiding this comment.
As discussed in DM, the BaseTokenHolder is not used rn inside the l1-contracts folder, while on the contrary every time we try to use L2BaseToken there we should use BaseTokenHolder (the only situation where it is still okay to use L2BaseToken is inside bootloader as it is basically the same "manual" manipulation as we'll do inside zksync os)
l1-contracts/contracts/dev-contracts/test/DummyBaseTokenSystemContract.sol
Show resolved
Hide resolved
| /// @dev then transfers all tokens to BaseTokenHolder. | ||
| /// @dev Can only be called by the ComplexUpgrader contract. | ||
| function initializeBaseTokenHolderBalance() external { | ||
| if (msg.sender != L2_COMPLEX_UPGRADER_ADDR) { |
There was a problem hiding this comment.
nit: this if statement can be moved to a single modifier to reuse the logic
| function setZkosPreV31TotalSupply( | ||
| uint256 _totalSupply | ||
| ) external onlyAdmin onlyL1 returns (bytes32 canonicalTxHash) { | ||
| if (!s.zksyncOS) { |
There was a problem hiding this comment.
this method should not be calleable for new zksync os chains, their totalSupply effectively starts from zero.
also, IMO this method should not be calleable more than once as it is a permanent backdoor.
I added the corresponding mocks in this PR #2041, but we'll have to adapt the PR to use those.
| L2_BASE_TOKEN_SYSTEM_CONTRACT_ADDR, | ||
| abi.encodeCall(IL2BaseTokenZKOS.setZkosPreV31TotalSupply, (_totalSupply)) | ||
| ); | ||
| emit SetZkosPreV31TotalSupply(_totalSupply); |
There was a problem hiding this comment.
nit: better to call it ZKsyncOS. This is the right naming, though I do know we dont use it everywhere, but let's introduce the right one one step at a time
0xValera
left a comment
0xValera
left a comment
There was a problem hiding this comment.
Not much issues found on my side, however I would like to quickly rereview once suggestions are applied, I've seen that there is a lot of outstanding comments (maybe most of them are resolved already, haven't checked conversations content).
One more thing to take into account is testing. I see that the new code is pretty covered already, but
- L2BaseTokenEra is at 0% coverage, please add testing for it
- I would suggest running claude with prompt asking to scan the PR, diff, and testing added, and to ensure that (at least) all happy path are covered, with sufficient asserts/checking of effects of interactions, not just calling functions.
I would then rereview tests separately (doesn't take that much time), if these suggestions will be applied.
| * ## Selfdestruct Caveat | ||
| * | ||
| * The implicit meaning of this contract's balance is "funds that the chain can still mint". | ||
| * On Era, totalSupply is computed as INITIAL_BASE_TOKEN_HOLDER_BALANCE - eraAccountBalance[BaseTokenHolder]. | ||
| * On ZK OS, totalSupply is computed as _zkosPreV31TotalSupply + (INITIAL - holder.balance). | ||
| * If funds were sent to this contract via selfdestruct (bypassing access controls), the holder balance | ||
| * would increase, causing totalSupply() to undercount. However, selfdestruct is not supported on Era. | ||
| * On ZK OS, selfdestruct would increase native balance, effectively returning tokens to the reserve. |
There was a problem hiding this comment.
On ZK OS, selfdestruct would increase native balance, effectively returning tokens to the reserve.
This sentence confuses me. on ZKsync OS selfdestruct works, and thus it'll be possible for totalSupply to undercount, right?
What exactly is meant by "increasing native balance", and "returning tokens to the reserve"?
| /// @param _account The address which to mint the funds to. | ||
| /// @param _amount The amount of ETH in wei to be minted. | ||
| function mint(address _account, uint256 _amount) external override onlyBootloader { | ||
| L2_ASSET_TRACKER.handleFinalizeBaseTokenBridgingOnL2(_amount); |
There was a problem hiding this comment.
I would follow CEI here just in case, and move call to L2AT after changes on L85-86
| /// @notice Sets the pre-V31 total supply for ZKOS chains. | ||
| /// @dev Can only be called via a service transaction (triggered by the chain admin on L1). | ||
| /// @param _totalSupply The total supply that existed before the V31 upgrade. | ||
| function setZkosPreV31TotalSupply(uint256 _totalSupply) external { |
There was a problem hiding this comment.
I suggest adding event emission here (like the one in Admin.sol when calling this, we can think of it as follows:
- event in Admin.sol is confirmation of intention on SL
- event in L2BaseTokenZKOS is confirmation of action being done on L2
| /// @dev L2BaseToken: returns burned tokens during withdrawals | ||
| modifier onlyBridgingCaller() { | ||
| if ( | ||
| msg.sender != address(L2_INTEROP_HANDLER) && |
There was a problem hiding this comment.
Can we use L2_INTEROP_HANDLER_ADDR? For consistency.
No strong opinion here
| */ | ||
| abstract contract L2BaseTokenBase is IL2BaseTokenBase { | ||
| /// @notice The L1Messenger contract for sending messages to L1 | ||
| IL2ToL1Messenger internal constant L1_MESSENGER = IL2ToL1Messenger(L2_TO_L1_MESSENGER_SYSTEM_CONTRACT_ADDR); |
There was a problem hiding this comment.
Would it be possible to reuse the constant from l2contractinterfaces.sol?
|
|
||
| /// @notice The pre-V31 total supply for ZKOS chains, set by chain admin via service transaction. | ||
| /// @dev On ZKOS chains, pre-V31 total supply was never tracked on-chain. This value is set after the V31 upgrade so that totalSupply() can be computed correctly. | ||
| /// @dev Only used by the ZKOS implementation. Declared in the base contract so that L2BaseTokenEra and L2BaseTokenZKOS share the same storage layout. |
There was a problem hiding this comment.
they dont have to share the same storage layout. The reason why we have gap in this contract is so that if there is a variable that needs to be used in both places, we could insert it here, but if there are variables tied to only specific implementation, we should use it only there.
eraAccountBalance and __DEPRECATED_totalSupply are the exceptions only because they existed prior to v31 upgrade
| } | ||
|
|
||
| /// @inheritdoc IAdmin | ||
| function setZkosPreV31TotalSupply( |
There was a problem hiding this comment.
could you please mark baseTokenHasTotalSupply as true in this function? add a comment that we can just change this value, since we assume that the service transaction wont fail
| function setZkosPreV31TotalSupply( | ||
| uint256 _totalSupply | ||
| ) external onlyAdmin onlyL1 returns (bytes32 canonicalTxHash) { | ||
| if (!s.zksyncOS) { |
There was a problem hiding this comment.
please do not allow it to call the second time use baseTokenHasTotalSupply
There was a problem hiding this comment.
As s.baseTokenHasTotalSupply is true for new chains, so it prevents it from being successfully called for new ZKsync OS chains
| /// @dev The delta (INITIAL - holder.balance) tracks tokens minted after V31 via the BaseTokenHolder pattern. | ||
| /// @dev WARNING: totalSupply() will return an incorrect value until the chain admin sets the pre-V31 total supply via setZkosPreV31TotalSupply(). This is done as a separate post-upgrade step. | ||
| function totalSupply() external view returns (uint256) { | ||
| return _zkosPreV31TotalSupply + (INITIAL_BASE_TOKEN_HOLDER_BALANCE - L2_BASE_TOKEN_HOLDER_ADDR.balance); |
There was a problem hiding this comment.
the following might happen:
- After the upgrade,
_zkosPreV31TotalSupply = 0. - But we know for a fact that on the chain some users have balances.
Someone might send 1 wei to L2_BASE_TOKEN_HOLDER_ADDR via selfdestruct.
Now: _zkosPreV31TotalSupply + (INITIAL_BASE_TOKEN_HOLDER_BALANCE - L2_BASE_TOKEN_HOLDER_ADDR.balance) would underflow leading to revert.
I think it is better to just totally revert unless the value has been set up. We have L2AssetTracker.needBaseTokenTotalSupplyBackfill variable for that, you can either reuse this variable or move it to this contract.
But please double check that it would not brick the chain.
Raid5594
force-pushed
the
ra/base-token-holder
branch
2 times, most recently
from
6efe37b to
378b3bf
Compare
Raid5594
force-pushed
the
ra/base-token-holder
branch
from
378b3bf to
e15a710
Compare
l1-contracts/contracts/common/l2-helpers/L2ContractInterfaces.sol
Outdated
Show resolved
Hide resolved
| /// @dev Executes a bundle with value > 0 through InteropHandler, which calls | ||
| /// BaseTokenHolder.give(). Asserts that handleFinalizeBaseTokenBridgingOnL2 is called | ||
| /// on L2AssetTracker to track the inbound base token bridging. | ||
| function test_give_inboundFlow_notifiesAssetTracker() public { |
There was a problem hiding this comment.
@0xValera tbh this test checks almost nothing: just that the function gets called. It does not check whether it succeeds, how the storage is updated, etc.
Probably it is easier to amend it as part of the bigger refactoring of our unit tests
...ry/l1/integration/l2-tests-abstract/L2NativeTokenVaultBridgeBurnRegressionTestAbstract.t.sol
Outdated
Show resolved
Hide resolved
| function test_give_notifiesAssetTracker() public { | ||
| uint256 amount = 1 ether; | ||
|
|
||
| vm.mockCall( |
There was a problem hiding this comment.
Same here @0xValera too many mocks, but I think it is easier to rewrite everything in one go
There was a problem hiding this comment.
this is part of unit tests, or shouldn't we have mocks there?
| /// @dev Formula: eraAccountBalance[holder] = INITIAL_BASE_TOKEN_HOLDER_BALANCE - __DEPRECATED_totalSupply + eraAccountBalance[holder] | ||
| /// @dev Can only be called by the ComplexUpgrader contract. | ||
| function initializeBaseTokenHolderBalance() external override onlyComplexUpgrader { | ||
| if (baseTokenHolderInitialized) { |
There was a problem hiding this comment.
baseTokenHolderInitialized or baseTokenHolderBalanceInitialized for precision?
There was a problem hiding this comment.
baseTokenHolderBalanceInitialized
this one is better I think
|
|
||
| // We expect that for all registered tokens, the zero `totalPreV31TotalSupply` should be saved. | ||
|
|
||
| // For genesis chains, the base token is registered during _finalizeDeployments() via |
There was a problem hiding this comment.
nit: could you please add require isAssetRegistered? this require should never get triggered, but it better to have invariants explicitly enforced
| /// are truly new), this function is for upgrading existing chains where the base | ||
| /// token already exists on-chain but the asset tracker is deployed during the | ||
| /// current upgrade. The base token originates on L1 (non-native to this chain). | ||
| /// Returns without changes if the base token is already registered. |
There was a problem hiding this comment.
nit: Since it is called first thing during the upgrade, it probably makes sense to just assert this as an invariant
| /// @notice Emitted when base tokens are given out (minted) from the holder to a recipient. | ||
| /// @param to The address that received the base tokens. | ||
| /// @param amount The amount of base tokens given out. | ||
| event BaseTokenMinted(address indexed to, uint256 amount); |
There was a problem hiding this comment.
Maybe makes sense to rename to BaseTokenMintedInterop (or something like that). I am just a bit afraid of people overrelying on this event and e.g. expecting that the total sum of emitted from BaseTokenBurnt is <= the total sum from BaseTokenMinted.
I.e. I think this event is helpful, but we should clearly signify that it is not for all mints. We should also add the corresponding comment to the BaseTokenBurnt
| /// @dev This function is specifically for the chain's native base token used for gas payments. | ||
| /// @param _amount The amount of base tokens being bridged into this chain. | ||
| function handleFinalizeBaseTokenBridgingOnL2(uint256 _amount) external onlyL2BaseTokenSystemContract { | ||
| function handleFinalizeBaseTokenBridgingOnL2(uint256 _amount) external onlyBaseTokenHolderOrL2BaseToken { |
There was a problem hiding this comment.
This mistake was not introduced in this PR, but this function is called both for L1-deposit and "interop" mints, but it always passes _fromChainId: L1_CHAIN_ID which is wrong, please amend the function to accept fromChainId
|
|
||
| Address.sendValue(payable(_to), _amount); | ||
| L2_ASSET_TRACKER.handleFinalizeBaseTokenBridgingOnL2(_amount); | ||
| emit BaseTokenMinted(_to, _amount); |
There was a problem hiding this comment.
Need to coordinate with the explorer team how they will use these events at all.
Starting from this PR we will have two events for minting base token:
- one inside the l2basetokenera contract
- one inside this contract.
they would have to track the sum of those or smth (if they care about it at all).
I would personally not make it a blocker for merging, this discussion will probably be async and lengthy, but we can always adapt a bit later as part of the fix review
|
|
||
| /// @dev Storage slot for totalPreV31TotalSupply mapping from | ||
| /// `forge inspect L2AssetTracker storage-layout`. | ||
| uint256 private constant TOTAL_PRE_V31_TOTAL_SUPPLY_SLOT = 157; |
There was a problem hiding this comment.
nit: maybe it is possible to use stdstore for variables related to it, like we do above?
| /// InteropL2Info struct layout: | ||
| /// offset 0: totalWithdrawalsToL1 | ||
| /// offset 1: totalSuccessfulDepositsFromL1 | ||
| uint256 internal constant INTEROP_INFO_MAPPING_SLOT = 156; |
There was a problem hiding this comment.
maybe makes sense to make the field public?
|
Coverage after merging ra/base-token-holder into draft-v31 will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
5 participants
What ❔
Adds Base Token Holder contract to L2 in order to manage minting / burning.
Why ❔
Better support for existing tooling. Former approach resulted in harder debugging of transactions
Checklist