Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

Integration tests benchmarks #1615

Merged
merged 63 commits into from
Nov 18, 2023
Merged

Integration tests benchmarks #1615

merged 63 commits into from
Nov 18, 2023

Conversation

AronisAt79
Copy link
Contributor

@AronisAt79 AronisAt79 commented Sep 19, 2023 •
edited
Loading

Description

Move SC compilation step to a build script. Rewrite deployment function based on SC rust bindings generated with
ethers_contract_abigen::Abigen and submit Txs for proof generation benchmarking.

Issue Link

#1557

Type of change

  • New feature (non-breaking change which adds functionality)

Contents

  • build.rs : Compile smart contracts and generate rust bindings
  • gen_blockchain_data:
    - Remove compilation step and rewrite fn deploy. New one uses the bindings generated at build step to deploy SCs and also instantiates SC instances to interact with.
    - Create a dump TxTrace function [needed for testing future implementations of benchmark contracts - needs attribute allow(dead_code)]
    - Submit Txs optimized for maximum calls of MLOAD and SDIV up to 300k gas

How Has This Been Tested?

https://github.com/privacy-scaling-explorations/zkevm-circuits/actions/runs/6264433154

https://github.com/privacy-scaling-explorations/zkevm-circuits/actions/runs/6234076847/job/16920593180
>> Workflow build step is currently failing

@github-actions github-actions bot added the crate-integration-tests Issues related to the integration-tests workspace member label Sep 19, 2023
@AronisAt79
Copy link
Contributor Author

AronisAt79 commented Sep 19, 2023 •
edited
Loading

Seems that introduction of build.rs causes build step to fail.
Logs show that compilation of OpenZeppelinERC20TestToken.sol returns the following error:

"ParserError: Source \"contracts/vendor/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol\" not found: File not found. Searched the following locations: \"\".\n --> contracts/OpenZeppelinERC20TestToken.sol:5:1:\n  |\n5 | import \"./vendor/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol\";\n

I assume the problem is associated with the git submodule pointing to OpenZeppelin ERC20.sol but i have yet not managed to figure out and fix it. Could we maybe introduce a worflow step that adds/init the submodule instead of doing that in run.sh? (if i locally directly execute run.sh without a prior cargo build step, tests proceed)

@ChihChengLiang ChihChengLiang removed the request for review from leolara September 20, 2023 08:28
@ChihChengLiang
Copy link
Collaborator

I unassigned @leolara since Leo recently focused on the DSL effort.

This was linked to issues Sep 21, 2023
Port integration tests from zkevm-chains #1552
Open
Port integration tests from zkevm-chains -- create necessary Txs/blocks for benchmarking #1557
Closed
This was unlinked from issues Sep 21, 2023
Port integration tests from zkevm-chains -- create necessary Txs/blocks for benchmarking #1557
Closed
Port integration tests from zkevm-chains #1552
Open
@ed255 ed255 linked an issue Sep 21, 2023 that may be closed by this pull request
Port integration tests from zkevm-chains -- create necessary Txs/blocks for benchmarking #1557
Closed
@github-actions github-actions bot added the CI Issues related to the Continuous Integration mechanisms of the repository. label Sep 21, 2023
@AronisAt79
Copy link
Contributor Author

@ChihChengLiang @ed255
i moved the git submodule update step from run.sh to the action workflow definition. Now setup and gendata steps are passing. Will now work on the panic shown here https://github.com/privacy-scaling-explorations/zkevm-circuits/actions/runs/6260891594/job/16999835172

@AronisAt79 AronisAt79 marked this pull request as ready for review September 22, 2023 03:54
@ChihChengLiang
Copy link
Collaborator

Hi @ed255, do you still have time to review this?

ed255
ed255 approved these changes Oct 20, 2023
View reviewed changes
Copy link
Member

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

LGTM! Everything looks really good and really clean! Also I found it really well commented an organized! I left a few nitpicks but they are minor, just some comments that popped up in my min while reviewing; feel free to read them and apply changes or not :)

integration-tests/build.rs Show resolved Hide resolved
integration-tests/build.rs Outdated Show resolved Hide resolved
integration-tests/build.rs Show resolved Hide resolved
integration-tests/src/bin/gen_blockchain_data.rs Outdated Show resolved Hide resolved
integration-tests/src/bin/gen_blockchain_data.rs Outdated Show resolved Hide resolved
ChihChengLiang
ChihChengLiang approved these changes Oct 20, 2023
View reviewed changes
Copy link
Collaborator

@ChihChengLiang ChihChengLiang left a comment

Choose a reason for hiding this comment

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

LGTM after Edu's and my comments are addressed.

integration-tests/build.rs Show resolved Hide resolved
integration-tests/build.rs Outdated Show resolved Hide resolved
integration-tests/build.rs Outdated Show resolved Hide resolved
integration-tests/build.rs Show resolved Hide resolved
integration-tests/build.rs Outdated Show resolved Hide resolved
integration-tests/build.rs Outdated Show resolved Hide resolved
@AronisAt79
Copy link
Contributor Author

@ChihChengLiang proposed removals of redundant string on BuildError variants plus enum adaptations and replacement of ok_or_else() with ok_or() according to clippy output addressed with commits: d80a1b4 & 3a0a27f

ChihChengLiang
ChihChengLiang approved these changes Nov 18, 2023
View reviewed changes
Copy link
Collaborator

@ChihChengLiang ChihChengLiang left a comment

Choose a reason for hiding this comment

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

LGTM

@ChihChengLiang ChihChengLiang added this pull request to the merge queue Nov 18, 2023
Merged via the queue into main with commit f07ca3d Nov 18, 2023
14 of 15 checks passed
@ChihChengLiang ChihChengLiang deleted the integration-tests-benchmarks branch November 18, 2023 09:22
@SuccinctPaul SuccinctPaul mentioned this pull request Nov 23, 2023
Merged
@ChihChengLiang ChihChengLiang mentioned this pull request Jan 22, 2024
Merged
1 task
github-merge-queue bot pushed a commit that referenced this pull request Jan 23, 2024
@ChihChengLiang
Dependency cleaning round (#1741)
d972588 
### Description

We removed the warning message shown during the project build.

```
warning: the following packages contain code that will be rejected by a future version of Rust: traitobject v0.1.0
note: to see what the problems were, use the option `--future-incompat-report`, or run `cargo report future-incompatibilities --id 2`
```
The message is caused by an unused dependency introduced during #1615

### Issue Link

None

### Type of change

- [x] Bug fix (non-breaking change which fixes an issue)

### Contents

- Removed some unused dependencies
- Removed some unused imports


### How Has This Been Tested?

Local build on all targets
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ChihChengLiang ChihChengLiang ChihChengLiang approved these changes

@ed255 ed255 ed255 approved these changes

Assignees
No one assigned
Labels
CI Issues related to the Continuous Integration mechanisms of the repository. crate-integration-tests Issues related to the integration-tests workspace member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Port integration tests from zkevm-chains -- create necessary Txs/blocks for benchmarking
4 participants
@AronisAt79 @ChihChengLiang @ed255 @ntampakas