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

Conversation

AronisAt79
Copy link
Contributor

@AronisAt79 AronisAt79 commented Sep 19, 2023

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

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
Copy link
Collaborator

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

@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?

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
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

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
@ChihChengLiang ChihChengLiang mentioned this pull request Jan 22, 2024
1 task
github-merge-queue bot pushed a commit that referenced this pull request Jan 23, 2024
### 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.
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
Development

Successfully merging this pull request may close these issues.

Port integration tests from zkevm-chains -- create necessary Txs/blocks for benchmarking
4 participants