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

Coverage increase executor #22

Draft
wants to merge 12 commits into
base: dev
Choose a base branch
from
Draft

Coverage increase executor #22

wants to merge 12 commits into from

Conversation

tommysr
Copy link
Collaborator

@tommysr tommysr commented May 29, 2024

What ❔

Increased coverage in Executor test

Why ❔

Checklist

  • Processing logs reverts
  • Committing batches with system upgrade
  • Proving (multiple batches reverts and verifying reverts)
  • Some reverts during committing process
  • Executing batches with/without system upgrades batches

@tommysr tommysr force-pushed the coverage-increase-executor branch from 1c4baf2 to bce8056 Compare June 13, 2024 07:39
@tommysr tommysr requested a review from tejks June 14, 2024 11:10
executor.commitBatches(genesisStoredBatchInfo, wrongNewCommitBatchInfoArray);
}

function hashSlice(bytes memory data, uint256 start, uint256 end) internal pure returns (bytes32) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe move it up to the top of the file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or to the ExecutorTest contract

@@ -256,61 +398,199 @@ contract CommittingTest is ExecutorTest {
executor.commitBatches(genesisStoredBatchInfo, wrongNewCommitBatchInfoArray);
}

function _getCommitInfoWithLogs(bytes[] memory logs) internal returns (IExecutor.CommitBatchInfo[] memory) {
Copy link
Collaborator

@tejks tejks Jun 14, 2024

Choose a reason for hiding this comment

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

Same as #22 (comment)

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

Successfully merging this pull request may close these issues.

2 participants