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

[Doc] Cleanup Bus-Mapping, ExecutionSteps, and ExecutionTrace #1775

Merged
merged 9 commits into from
Feb 26, 2024

Conversation

ChihChengLiang
Copy link
Collaborator

Description

  • Bus-Mapping has two library docs, one is inside the lib.rs, another locates in README.md, untested. We remove the one in lib.rs and make the README one checked.
  • ExecutionSteps and ExecutionTrace were

We hope the update can reduce confusion.

Issue Link

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Refactor (no updates to logic)

How Has This Been Tested?

cargo test --doc

@github-actions github-actions bot added crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member crate-eth-types Issues related to the eth-types workspace member labels Feb 21, 2024
@ChihChengLiang
Copy link
Collaborator Author

@ChihChengLiang ChihChengLiang force-pushed the doc-fix branch 2 times, most recently from 40ace16 to eef9899 Compare February 22, 2024 08:19
@KimiWu123 KimiWu123 self-requested a review February 22, 2024 08:34
@hero78119 hero78119 self-requested a review February 22, 2024 08:46
Copy link
Contributor

@KimiWu123 KimiWu123 left a comment

Choose a reason for hiding this comment

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

LGTM! Left a question and a nitpick.

eth-types/src/lib.rs Show resolved Hide resolved
- Iterate over the `ExecutionTrace` itself over
each `ExecutionStep`'s is formed by and check which Stack/Memory&Storage operations are linked to each step.
- Iterate over the [`GethExecTrace`](eth_types::GethExecTrace) itself over
each [`ExecStep`](crate::circuit_input_builder::ExecStep)'s is formed by and check which Stack/Memory&Storage operations are linked to each step.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: not changing line here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I fixed all the line breaks. I also added a GPT round to improve the readability.

Copy link
Member

@hero78119 hero78119 left a comment

Choose a reason for hiding this comment

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

Overall LGTM!

I am checking whether we should add -D warnings along with cargo test --doc to assure even warning can be captured.

@hero78119
Copy link
Member

hero78119 commented Feb 26, 2024

Overall LGTM!

I am checking whether we should add -D warnings along with cargo test --doc to assure even warning can be captured.

Since this PR targeting on docs clean up, I suggest we can further restrict warnings on docs test

Example:
https://github.com/privacy-scaling-explorations/zkevm-circuits/compare/doc-fix...hero78119:doc_deny_on_warning?expand=1

Verify to capture new errors via cargo test --workspace bus-mapping --doc --exclude integration-tests --exclude circuit-benchmarks

with below error

error: unused import: `Error`
 --> bus-mapping/src/lib.rs:30:19
  |
5 | use bus_mapping::{Error, mock::BlockData};
  |                   ^^^^^
  |
note: the lint level is defined here
 --> bus-mapping/src/lib.rs:26:9
  |
1 | #![deny(warnings)]
  |         ^^^^^^^^
  = note: `#[deny(unused_imports)]` implied by `#[deny(warnings)]`

error: unused imports: `CodeDB`, `StateDB`, `self`
 --> bus-mapping/src/lib.rs:31:29
  |
6 | use bus_mapping::state_db::{self, StateDB, CodeDB};
  |                             ^^^^  ^^^^^^^  ^^^^^^

error: unused imports: `Address`, `Hash`, `U64`, `Word`, `address`
 --> bus-mapping/src/lib.rs:33:11
  |
8 |     self, address, Address, Word, Hash, U64, GethExecTrace, GethExecStep, geth_types::GethData, bytecode
  |           ^^^^^^^  ^^^^^^^  ^^^^  ^^^^  ^^^

error: the item `eth_types` is imported redundantly
 --> bus-mapping/src/lib.rs:33:5
  |
8 |     self, address, Address, Word, Hash, U64, GethExecTrace, GethExecStep, geth_types::GethData, bytecode
  |     ^^^^ the item `eth_types` is already defined here

error: unused imports: `Block`, `CircuitInputBuilder`
  --> bus-mapping/src/lib.rs:36:42
   |
11 | use bus_mapping::circuit_input_builder::{Block, CircuitInputBuilder};
   |                                          ^^^^^  ^^^^^^^^^^^^^^^^^^^

error: unused variable: `geth_trace`
  --> bus-mapping/src/lib.rs:112:5
   |
87 | let geth_trace = GethExecTrace {
   |     ^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_geth_trace`
   |
   = note: `#[deny(unused_variables)]` implied by `#[deny(warnings)]`

error: unused variable: `stack_ops`
  --> bus-mapping/src/lib.rs:121:5
   |
96 | let stack_ops = builder.block.container.sorted_stack();
   |     ^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_stack_ops`

error: unused `std::slice::Iter` that must be used
  --> bus-mapping/src/lib.rs:123:1
   |
98 | builder.block.txs()[0].steps().iter();
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

it's a bit misleading for actually these error are from README.md :)

Copy link
Member

@hero78119 hero78119 left a comment

Choose a reason for hiding this comment

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

LGTM! appreciate for taking care of this

@ChihChengLiang ChihChengLiang added this pull request to the merge queue Feb 26, 2024
Merged via the queue into main with commit 8e2e74a Feb 26, 2024
15 checks passed
@ChihChengLiang ChihChengLiang deleted the doc-fix branch February 26, 2024 11:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
crate-bus-mapping Issues related to the bus-mapping workspace member crate-eth-types Issues related to the eth-types workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants