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

smart-contract V6 review small tweaks #207

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jpnovais
Copy link
Collaborator

This PR

  • improves DataSubmittedV3 names consistency,
  • tries blob/data submission validation more efficient

Since we are making a change & audit, maybe it's a good opportunity to assess the following points

Other Improvement Suggestions

Blob Submission

Current Interface

  function submitBlobs(
    BlobSubmissionData[] calldata _blobSubmissionData,
    bytes32 _parentShnarf,
    bytes32 _finalBlobShnarf
  ) external;

  struct BlobSubmissionData {
    SupportingSubmissionDataV2 submissionData;
    uint256 dataEvaluationClaim;
    bytes kzgCommitment;
    bytes kzgProof;
  }

  struct SupportingSubmissionDataV2 {
    bytes32 finalStateRootHash;
    uint256 firstBlockInData;
    uint256 finalBlockInData;
    bytes32 snarkHash;
  }

The list of SupportingSubmissionDataV2 has redundant fields (first/final)BlockInData, because _blobSubmissionData[i].subissionData.firstBlockInData must be equal to its ancestor _blobSubmissionData[i-1].subissionData.finalBlockInData;
We can keep just finalBlockInData and pass only firstBlockInData for the first bock in submitBlobs(..). SupportingSubmissionDataV2’s fields can be included in the parent struct BlobSubmissionData.

This simplification will:

  • allow to remove SupportingSubmissionDataV2 struct, which is sometimes confused with SubmissionDataV2 struct used for blob submission as call data;
  • save gas on call data by not sending redundant fields;
  • simplify input validation code _validateSubmissionData, potentially saving more gas;

New Interface Suggestion

function submitBlobs(
  BlobSubmissionData[] calldata _blobSubmissionData,
  uint256 firstBlockInData;
  bytes32 _parentShnarf,
  bytes32 _finalBlobShnarf
) external;

struct BlobSubmissionDataV2 { 
  uint256 finalBlockInData;
  bytes32 finalStateRootHash;
  bytes32 snarkHash;
  uint256 dataEvaluationClaim;
  bytes kzgCommitment;
  bytes kzgProof;
}

Block Finalization

_computeLastFinalizedState relies on "timestamp The final timestamp in the finalization.", should it use finalStateRootHash? Sequencer(s) can have different blocks at same timestamp.

@jpnovais jpnovais added the Contracts Smart Contract related label Oct 17, 2024
@jpnovais jpnovais requested review from julien-marchand and a team October 17, 2024 17:50
@codecov-commenter
Copy link

codecov-commenter commented Oct 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.11%. Comparing base (74529e6) to head (c475447).

Additional details and impacted files
@@            Coverage Diff            @@
##               main     #207   +/-   ##
=========================================
  Coverage     70.11%   70.11%           
  Complexity     1042     1042           
=========================================
  Files           286      286           
  Lines         11827    11829    +2     
  Branches       1087     1088    +1     
=========================================
+ Hits           8292     8294    +2     
  Misses         3065     3065           
  Partials        470      470           
Flag Coverage Δ
hardhat 98.69% <100.00%> (+<0.01%) ⬆️
kotlin 67.69% <ø> (ø)
Files with missing lines Coverage Δ
contracts/contracts/LineaRollup.sol 98.73% <100.00%> (+0.01%) ⬆️

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

Successfully merging this pull request may close these issues.

4 participants