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

feat(proof_producer): update request parameters based on new circuits changes #240

Merged
merged 2 commits into from
May 24, 2023

Conversation

davidtaikocha
Copy link
Member

@davidtaikocha davidtaikocha commented May 24, 2023

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

The changes made in this pull request look good. The code is easy to read and understand, and the changes made are clear. I noticed that the code added in this pull request is using a new protocol configuration, which is being passed into the NewZkevmRpcdProducer function. This looks correct and should be tested to make sure that it functions as intended. Additionally, I noticed that the code is using big.Int for some of the parameters, which should be double checked to make sure that it is appropriate for the task. Overall, it looks like a good pull request and should be approved.

@codecov
Copy link

codecov bot commented May 24, 2023

Codecov Report

Merging #240 (2873029) into main (3e12042) will decrease coverage by 0.05%.
The diff coverage is 4.16%.

❗ Current head 2873029 differs from pull request most recent head 678bc39. Consider uploading reports for the commit 678bc39 to get more accurate results

@@            Coverage Diff             @@
##             main     #240      +/-   ##
==========================================
- Coverage   54.43%   54.38%   -0.05%     
==========================================
  Files          36       36              
  Lines        3540     3545       +5     
==========================================
+ Hits         1927     1928       +1     
- Misses       1358     1362       +4     
  Partials      255      255              
Impacted Files Coverage Δ
prover/prover.go 52.87% <0.00%> (-0.15%) ⬇️
prover/proof_producer/zkevm_rpcd_producer.go 31.15% <4.34%> (-0.19%) ⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@davidtaikocha davidtaikocha marked this pull request as ready for review May 24, 2023 04:18
@davidtaikocha davidtaikocha enabled auto-merge (squash) May 24, 2023 04:19
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Overall, this Pull Request looks good. Here are some things I noticed:

  • In NewZkevmRpcdProducer function, the protocolConfig parameter has been updated to accept a pointer of type bindings.TaikoDataConfig. This should be reflected in both the test file and in the prover.go file.
  • The hex values for all the parameters in the RequestProofBodyParam struct have been updated to include the first two characters of the hex string. This should be mentioned in the commit message for clarity.
  • There are some typos in repeating variables, like L2SignalService, which might be worth checking.

@davidtaikocha davidtaikocha requested a review from johntaiko May 24, 2023 04:26
@davidtaikocha davidtaikocha merged commit 31521ef into main May 24, 2023
@davidtaikocha davidtaikocha deleted the new-circuits-update branch May 24, 2023 04:28
vhjiang pushed a commit to taikoverse/taiko-degen-client that referenced this pull request Jul 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants