feat(system-contracts): remove unecessary methods from L2BaseToken.sol #509
Conversation
thomas-nguy
requested review from
vladbochok and
StanislavBreadless
as code owners
| @@ -3,49 +3,49 @@ | |||
| "contractName": "AccountCodeStorage", | |||
| "bytecodePath": "artifacts-zk/contracts-preprocessed/AccountCodeStorage.sol/AccountCodeStorage.json", | |||
| "sourceCodePath": "contracts-preprocessed/AccountCodeStorage.sol", | |||
| "bytecodeHash": "0x0100005db7d0621192834d201f8bcd14c6ac987b95f422ca8bb28d500748f1fa", | |||
| "bytecodeHash": "0x0100005d2738cecf77ecb3272d04276e7f9585db5afadd4f33eb8e943b5a07cf", | |||
There was a problem hiding this comment.
For some reasons I am having a lot of diffs in bytecodeHash while nothing has been changed.
Is the dev branch updated?
thomas-nguy
changed the title
| @@ -128,21 +128,4 @@ contract L2BaseToken is IBaseToken, ISystemContract { | |||
| return abi.encodePacked(IMailbox.finalizeEthWithdrawal.selector, _to, _amount, _sender, _additionalData); | |||
| } | |||
|
|
|||
| /// @dev This method has not been stabilized and might be | |||
| /// removed later on. | |||
| function name() external pure override returns (string memory) { | |||
There was a problem hiding this comment.
What is the motivation for removing these?
There was a problem hiding this comment.
It is in the description of this PR but in a nutshell
- incompatibility with chain not using ETH as base token
- confuse some explorers
thomas-nguy
force-pushed
the
thomas/remove-unecessary-method-base-token
branch
from
aeb6efa
to
ba92b7f
Compare
|
@vladbochok any additional info you need ? |
|
@vladbochok any advices on how to update the SystemContractsHashes file properly? |
@thomas-nguy you should be able to merge in dev, take any of the changes (this will be overwritten), and then run |
thomas-nguy
force-pushed
the
thomas/remove-unecessary-method-base-token
branch
from
f1256fe
to
749da2d
Compare
|
@koloz193 Thanks, I have rebased and updated the file I am ending having a lot of differences, perhaps the file in the dev branch is outdated? |
thomas-nguy
force-pushed
the
thomas/remove-unecessary-method-base-token
branch
from
749da2d
to
a19bfdb
Compare
|
Hey @thomas-nguy, we should be good to go with this PR. Let's wait for the CI and then I would merge. |
thomas-nguy
force-pushed
the
thomas/remove-unecessary-method-base-token
branch
from
d2de7e9
to
715d8c3
Compare
What ❔
Remove Name, Symbol and Decimal from L2BaseToken.sol
Why ❔
Multiple issues with those methods :
1- For hyperchains that does not use ETH as base token, the values returned by those methods are wrong so it should not be hardcoded. I couldnt find a good way to make them variable without heavy changes on the initialisation process
2- L2BaseToken IS not an ERC20 as it does not implement IERC20 interface (no approval or allowance) but it looks like it which confuse some explorers
One example is on https://era.zksync.network/
The value of ETH is counted twices as it is also recorded as an ERC20
https://era.zksync.network/address/0xd0c511bdbf219e3d5793e47b9ad559c9f69b3404
We should remove all confusion and make "symbol" and "name" agnostics as it is done on Ethereum
Checklist