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

Hide Circuit implementation of sub-circuits behind test feature #953

Closed
han0110 opened this issue Nov 30, 2022 · 9 comments
Closed

Hide Circuit implementation of sub-circuits behind test feature #953

han0110 opened this issue Nov 30, 2022 · 9 comments
Assignees
Labels
T-tech-debt Type: tech-debt generated or cleaned up

Comments

@han0110
Copy link
Contributor

han0110 commented Nov 30, 2022

After #937 we no longer need the implementation Circuit for sub-circuits in production, it's only required for test and crates circuit-benchmarks to benchmark individual sub-circuit, so we should move current implementation into #[cfg(any(feature = "test", test))] mod test { ... } to avoid further consumers to use them accidentally.

@CPerezz
Copy link
Member

CPerezz commented Nov 30, 2022

I'm totally in favor of that change!

@SuccinctPaul
Copy link
Contributor

Hi, I can do it.

@CPerezz
Copy link
Member

CPerezz commented Dec 21, 2022

NOTE that circuit-benchmarks will need to import zkevm-circuits with test feature!

Assigning to @ChengYueJia then!

@SuccinctPaul
Copy link
Contributor

@CPerezz @han0110
You can have a view #1003

@aguzmant103 aguzmant103 moved this from 🏗 In progress to 👀 In review in zkEVM Community Edition Dec 22, 2022
@aguzmant103 aguzmant103 linked a pull request Dec 22, 2022 that will close this issue
@ed255 ed255 added the T-tech-debt Type: tech-debt generated or cleaned up label Feb 14, 2023
@ed255 ed255 removed this from the tech-debt-1 milestone Feb 14, 2023
@leolara
Copy link
Contributor

leolara commented Mar 9, 2023

@han0110 @ChengYueJia Doesn't SuperCircuit get used outside the tests to run the actual system?

@han0110
Copy link
Contributor Author

han0110 commented Mar 9, 2023

Yes, SuperCircuit used as Circuit in zkevm-chain, so we should keep its impl, sorry I didn't mention that in the issue.

@leolara
Copy link
Contributor

leolara commented Mar 9, 2023

But then, my question is SuperCircuit uses the Circuit implementation of the other SubCircuits:

https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/main/zkevm-circuits/src/super_circuit.rs#L269-L276

so all these, should not be only in the tests, but also in production, right?

@han0110
Copy link
Contributor Author

han0110 commented Mar 9, 2023

But new_from_block is method of SubCircuit, which doesn't require the struct to implement Circuit, or is there anything inside these implementation requires Circuit implementation?

hero78119 pushed a commit that referenced this issue Apr 10, 2023
### Issue Link

* First mentioned in #953 

* Pr link
This is the second pr for this. The older one #1003 is abandoned as
there are too much conflicts.


### Contents
* Gather test related into test mod.
* Gather most of the `impl Circuit` into `dev.mod`
This is for distinct the `test-circuits` and the test features. #1144
@ed255
Copy link
Member

ed255 commented Apr 20, 2023

Resolved via #1300

@ed255 ed255 closed this as completed Apr 20, 2023
@ed255 ed255 moved this from 👀 In review to ✅ Done in zkEVM Community Edition Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-tech-debt Type: tech-debt generated or cleaned up
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants