Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Sync layer stable #672

Open
wants to merge 685 commits into
base: dev
Choose a base branch
from
Open

feat: Sync layer stable #672

wants to merge 685 commits into from

Conversation

kelemeno
Copy link
Contributor

What ❔

Why ❔

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.

@kelemeno kelemeno changed the base branch from kl/sync-layer-reorg to kl/custom-asset-bridging July 31, 2024 14:08
@kelemeno kelemeno changed the base branch from kl/custom-asset-bridging to dev August 8, 2024 11:30
@kelemeno kelemeno mentioned this pull request Aug 12, 2024
3 tasks
bytes32 _l2DAValidatorOutputHash,
bytes calldata _operatorDAInput,
uint256 _maxBlobsSupported
) external returns (L1DAValidatorOutput memory output);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure you want to make it non-static? This opens many questions on the call trace control (like reentrancy) but I don't think a stateful DA contract is popular

Suggested change
) external returns (L1DAValidatorOutput memory output);
) external view returns (L1DAValidatorOutput memory output);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vladbochok The reason it is non-static is because on gateway we relay the calldata to L1 in this method.

pragma solidity 0.8.24;

struct L1DAValidatorOutput {
/// @dev The hash of the uncompressed state diff.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bytes32[] blobsOpeningCommitments;
}

interface IL1DAValidator {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these contracts stored in a separate directory? Wouldn't it be better to store it in l1-contracts dir?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of compilation reasons. They use the blobHash opcode, which is not supported for hardhat zk compilation. This was the easiest solution that we saw


pragma solidity 0.8.24;

struct L1DAValidatorOutput {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The convention we use for people who find a bug in the contract can find our contact quickly.

Suggested change
struct L1DAValidatorOutput {
/// @author Matter Labs
/// @custom:security-contact [email protected]
struct L1DAValidatorOutput {

da-contracts/contracts/CalldataDA.sol Outdated Show resolved Hide resolved
da-contracts/contracts/ValidiumL1DAValidator.sol Outdated Show resolved Hide resolved

/// @notice Contract that contains the functionality for process the calldata DA.
/// @dev The expected l2DAValidator that should be used with it `RollupL2DAValidator`.
abstract contract CalldataDA {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this abstract contract exist? Wouldn't be better to just have it internal functions in the RollupL1DAValidator?

Copy link
Collaborator

@StanislavBreadless StanislavBreadless Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is functionality reused in RollupL1DAValidator and CalldataDAGateway. In the perfect world we would've put those into the same folder but we can not for compilation reasons

@@ -180,15 +181,19 @@ contract ValidatorTimelock is IExecutor, Ownable2Step {

/// @dev Check that batches were committed at least X time ago and
/// make a call to the hyperchain diamond contract with the same calldata.
function executeBatches(StoredBatchInfo[] calldata _newBatchesData) external onlyValidator(ERA_CHAIN_ID) {
_executeBatchesInner(ERA_CHAIN_ID, _newBatchesData);
function executeBatches(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ValidatorTimelock should be backward compatible with all chain version from now and later, so I propose adding function executeBatches(StoredBatchInfo[] calldata _newBatchesData) so chains that haven't update yet would work as expected.

Better to make the one interface to not redeploy contract in the future at all, but let discuss it

kelemeno and others added 16 commits August 21, 2024 16:10
Signed-off-by: Danil <[email protected]>
Co-authored-by: Neo <[email protected]>
Co-authored-by: tommysr <[email protected]>
Co-authored-by: Rahul Saxena <[email protected]>
Co-authored-by: Artem Makhortov <[email protected]>
Co-authored-by: Bence Haromi <[email protected]>
Co-authored-by: Zach Kolodny <[email protected]>
Co-authored-by: Stanislav Bezkorovainyi <[email protected]>
Co-authored-by: Vlad Bochok <[email protected]>
Co-authored-by: perekopskiy <[email protected]>
Co-authored-by: perekopskiy <[email protected]>
Co-authored-by: Danil <[email protected]>
Co-authored-by: Ivan Schasny <[email protected]>
Co-authored-by: Raid5594 <[email protected]>
Co-authored-by: Raid Ateir <[email protected]>
Signed-off-by: Danil <[email protected]>
Co-authored-by: Neo <[email protected]>
Co-authored-by: tommysr <[email protected]>
Co-authored-by: Rahul Saxena <[email protected]>
Co-authored-by: Artem Makhortov <[email protected]>
Co-authored-by: Bence Haromi <[email protected]>
Co-authored-by: Zach Kolodny <[email protected]>
Co-authored-by: Stanislav Bezkorovainyi <[email protected]>
Co-authored-by: Vlad Bochok <[email protected]>
Co-authored-by: perekopskiy <[email protected]>
Co-authored-by: perekopskiy <[email protected]>
Co-authored-by: Danil <[email protected]>
Co-authored-by: Ivan Schasny <[email protected]>
Co-authored-by: Raid5594 <[email protected]>
Co-authored-by: Raid Ateir <[email protected]>
/// @dev The contract responsible for handling tokens native to a single chain.
IL2NativeTokenVault constant L2_NATIVE_TOKEN_VAULT = IL2NativeTokenVault(address(USER_CONTRACTS_OFFSET + 0x04));

uint256 constant L1_CHAIN_ID = 1;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets not forget to remove

Copy link

github-actions bot commented Oct 8, 2024

Coverage after merging sync-layer-stable into dev will be

85.84%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
../da-contracts/contracts
   RollupL1DAValidator.sol64.94%37.50%83.33%70.91%145, 148, 148, 148, 150, 183–184, 187–188, 27, 27–28, 30, 30–31, 34, 36–37, 41–42, 65, 67, 67, 67–68, 70
contracts/bridge
   BridgeHelper.sol93.33%50%100%100%22
   BridgedStandardERC20.sol74.03%25%92.31%76.79%121–122, 127–128, 140–141, 165–166, 207, 207, 214, 214, 221, 221, 232, 62–63, 94–95
   L1ERC20Bridge.sol93.18%80%100%93.75%188–189, 264
   L1Nullifier.sol77.23%58.82%80%83.22%116–117, 132, 132–133, 140, 140–141, 148, 148–149, 178–179, 238–239, 241–242, 251–252, 260–261, 263, 429, 431–432, 432, 432, 434–435, 435, 435, 460–461, 482–483, 522, 617, 652–653, 706, 708, 710, 723, 737, 742
contracts/bridge/asset-router
   AssetRouterBase.sol83.78%40%100%88%138–139, 57–58, 85–86
   L1AssetRouter.sol88.75%72%88.89%92.59%204–205, 241, 250, 252, 255, 350, 57–58, 603, 622, 73–74, 81–82
contracts/bridge/ntv
   L1NativeTokenVault.sol78.91%61.76%93.33%83.54%137, 142, 145–146, 146, 146–148, 148, 148–150, 150, 150–151, 153, 205, 214, 216, 216, 216–217, 219, 254, 254–255
   NativeTokenVault.sol87.26%68%90.48%90.99%202, 204, 222–223, 230–231, 384, 386, 398–399, 433–434, 437–438, 464, 469, 67–68
contracts/bridgehub
   Bridgehub.sol79.12%48.48%89.36%85.90%114, 114–115, 121–122, 129–130, 136–137, 143, 143–144, 173, 188–189, 235–236, 236, 236–237, 244–245, 247–248, 251–252, 262–263, 277–278, 327–328, 330–331, 388–389, 404–405, 435–436, 442, 519–520, 601, 700, 703–704, 708–709, 744–745, 758, 801–802, 804–805, 807–808, 842–843, 846–847, 849–850, 885, 890
   CTMDeploymentTracker.sol79.07%50%90%94.74%115, 119, 34, 41, 64, 91, 94, 96
   MessageRoot.sol91.07%63.64%100%96.97%116–117, 148, 69, 87
contracts/common
   ReentrancyGuard.sol90%66.67%100%92.86%78–79
contracts/common/libraries
   DataEncoding.sol77.14%50%100%80%108, 112, 119, 129, 131, 134, 75, 83
   DynamicIncrementalMerkle.sol74.42%100%80%72.22%67–70, 72–74, 76–78
   FullMerkle.sol100%100%100%100%
   L2ContractHelper.sol55.56%0%66.67%64%100, 100–101, 109, 68–69, 74–75, 78–79, 93, 95, 95–96
   Merkle.sol96.61%90.91%100%97.67%80–81
   MessageHashing.sol100%100%100%100%
   SemVer.sol100%100%100%100%
   SystemContractsCaller.sol0%0%0%0%114, 122–125, 135–138, 138–139, 141, 141–142, 33, 33–34, 37, 45, 47, 49, 51, 53, 66, 66, 66, 69, 72, 75, 78, 89, 91, 93, 96, 98
   UncheckedMath.sol100%100%100%100%
   UnsafeBytes.sol84.21%100%83.33%84.62%35–36
contracts/governance
   AccessControlRestriction.sol100%100%100%100%
   ChainAdmin.sol95.12%80%100%96.15%27–28
   Governance.sol98.15%94.74%100%98.55%45–46
   L2ProxyAdminDeployer.sol0%100%0%0%17–18, 20
   PermanentRestriction.sol89.43%82.61%100%89.41%110, 110–111, 138, 201, 201–202, 205, 207, 207–208, 248–249
   TransitionaryOwner.sol0%100%0%0%17, 22–23
contracts/state-transition
   ChainTypeManager.sol70.99%33.33%68.57%80.58%108, 135–136, 138–139, 141–142, 144–145, 200–201, 252, 276, 295, 302, 309, 317, 324, 332, 339, 355, 357, 419, 438, 438, 438, 441, 441, 441, 443, 456, 461, 486, 74, 87–88
   TestnetVerifier.sol77.78%66.67%100%75%16, 28
   ValidatorTimelock.sol95.08%83.33%100%95.24%200, 82–83
   Verifier.sol89.90%40%96.30%90.93%1674–1675, 287–302, 305–308, 311–318, 321–328, 331–332, 335–336, 339, 383–384, 394–395, 405–406, 416–417, 427–428, 443–444, 453, 453–454, 905–906
contracts/state-transition/chain-deps
   DiamondInit.sol80.43%50%100%88.24%39–40, 42–43, 45–46, 48–49, 73
   DiamondProxy.sol92.31%75%100%100%16, 27
contracts/state-transition/chain-deps/facets
   Admin.sol80.56%50%95.45%91.18%104–105, 115–116, 130, 130–131, 133–134, 157–158, 239, 241, 254–255, 261, 266, 284, 295–296, 301, 313, 315, 315, 315, 321, 321, 321–322, 322, 322–323, 325, 354, 356, 360, 369, 379, 383, 40, 40
   Executor.sol81.61%62.34%96%87.02%120–121, 173, 178, 183, 188, 193, 198, 202–203, 208–210, 212–213, 227–228, 246–247,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants