Draft v30 #1572
Draft v30 #1572
Conversation
…/zksync-os-stable-v29
feat: update Plonk verifier
chore: Further ZKsync OS cleanup
| using SafeERC20 for IERC20; | ||
|
|
||
| /// @dev Bridgehub smart contract that is used to operate with L2 via asynchronous L2 <-> L1 communication. | ||
| address public immutable BRIDGE_HUB; |
There was a problem hiding this comment.
Why is BRIDGE_HUB of the address type but nativeTokenVault is not? I think we need to choose one way we do this unless there's a reason not to.
Also, there're a few instances of unnecessary casting of BRIDGE_HUB anyway.
| /// @notice A contract that deploys the upgradeable beacon for the bridged standard ERC20 token. | ||
| /// @dev Besides separation of concerns, we need it as a separate contract to ensure that L2NativeTokenVaultZKOS | ||
| /// does not have to include BridgedStandardERC20 and UpgradeableBeacon and so can fit into the code size limit. | ||
| contract UpgradeableBeaconDeployer { |
There was a problem hiding this comment.
Why is this contract in l1-contracts/contracts/bridge/ntv but not in l1-contracts/contracts/bridge like BridgedStandardERC20 is?
| for (uint256 i = 0; i < publicInputsLength; ++i) { | ||
| initialHash = uint256(keccak256(abi.encodePacked(initialHash, _publicInputs[i]))) >> 32; | ||
| } |
There was a problem hiding this comment.
The common loop should be outside of the if condition to eliminate code duplication
| /// @notice Type of verification for FFLONK verifier. | ||
| uint256 internal constant FFLONK_VERIFICATION_TYPE = 0; | ||
|
|
||
| /// @notice Type of verification for PLONK verifier. | ||
| uint256 internal constant PLONK_VERIFICATION_TYPE = 1; |
There was a problem hiding this comment.
Do we still need these types given that ZKsyncOSDualVerifier is a ZKsync OS-specific contract?
| // @notice This is test only verifier (mock), and must be removed before prod. | ||
| uint256 internal constant ZKSYNC_OS_MOCK_VERIFICATION_TYPE = 3; |
There was a problem hiding this comment.
Should we remove it as the comment states?
| uint256 verifierType = _verifierType & 255; | ||
| uint32 verifierVersion = uint32(verifierType >> 8); |
There was a problem hiding this comment.
Since we do it twice, I suggest to separate this encoding/decoding to an internal function
| function _extractZKSyncOSProof(uint256[] calldata _proof) internal pure returns (uint256[] memory result) { | ||
| uint256 resultLength = _proof.length - 1 - 1; | ||
|
|
||
| // Allocate memory for the new array (_proof.length - 1) since the first element is omitted. | ||
| result = new uint256[](resultLength); | ||
|
|
||
| // Copy elements starting from index 1 (the second element) of the original array. | ||
| assembly { | ||
| calldatacopy(add(result, 0x20), add(_proof.offset, 0x40), mul(resultLength, 0x20)) | ||
| } | ||
| } |
There was a problem hiding this comment.
This function duplicates a lot of code from _extractProof. They should be the same function with a flag argument which tells whether it's ZKsync OS or not.
Co-authored-by: nikitastupin-matterlabs <[email protected]>
Co-authored-by: nikitastupin-matterlabs <[email protected]>
| if (_chainCreationParams.genesisIndexRepeatedStorageChanges == uint64(0)) { | ||
| revert GenesisIndexStorageZero(); | ||
| } |
There was a problem hiding this comment.
Why remove this check?
There was a problem hiding this comment.
good point, it should only be removed in ZKsync OS specific CTM
| /// @param _expirationTimestamp the expiration timestamp for the transaction | ||
| function forwardTransactionOnGateway( | ||
| uint256 _chainId, | ||
| bytes32 _canonicalTxHash, |
There was a problem hiding this comment.
I think this function should be removed from here, its only L2
Signed-off-by: Danil <[email protected]>
15 participants
What ❔
Why ❔
Checklist