-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: fix merge issues. #47
Conversation
c0289c0
to
1efe35c
Compare
The author of this PR, liorgold2, is not an activated member of this organization on Codecov. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 103 of 283 files at r1, all commit messages.
Reviewable status: 103 of 283 files reviewed, 9 unresolved discussions (waiting on @liorgold2)
Cargo.toml
line 10 at r1 (raw file):
"crates/gateway", "crates/mempool", "crates/mempool_infra",
Suggestion:
members = [
"crates/blockifier",
"crates/committer",
"crates/committer_cli",
"crates/gateway",
"crates/mempool",
"crates/mempool_infra",
.github/CODEOWNERS
line 0 at r1 (raw file):
this file should be deleted
.github/workflows/blockifier_ci.yml
line 45 at r1 (raw file):
python -m pip install --upgrade pip pip install pytest - run: pytest scripts/merge_paths_test.py
Suggestion:
# TODO(Dori, 1/8/2024): This phase should be it's own job, triggered only on change
# to files matching `scripts/merge_*` glob.
run-python-tests:
runs-on: ubuntu-20.04
steps:
- uses: actions/checkout@v4
- uses: actions/setup-python@v5
with:
python-version: '3.9'
- run: |
python -m pip install --upgrade pip
pip install pytest
- run: pytest scripts/merge_paths_test.py
.github/workflows/blockifier_ci.yml
line 100 at r1 (raw file):
- uses: actions/checkout@v4 - name: Run Machete (detect unused dependencies) uses: bnjbvr/cargo-machete@main
delete this (already in main CI)
.github/workflows/blockifier_compiled_cairo.yml
line 41 at r1 (raw file):
- run: pip install -r crates/blockifier/tests/requirements.txt; cargo test verify_feature_contracts -- --include-ignored
if you don't add this, cargo will attempt to build all crates and likely fail papyrus build due to lack of protoc
installation
Suggestion:
cargo test verify_feature_contracts -p blockifier -- --include-ignored
.github/workflows/blockifier_coverage.yml
line 44 at r1 (raw file):
# token: ${{ secrets.CODECOV_TOKEN }} # verbose: true # fail_ci_if_error: true
Suggestion:
# TODO(Dori, 1/8/2024): Reinstate this phase.
# - name: Generate code coverage
# run: cargo llvm-cov --codecov --output-path codecov.json
# env:
# SEED: 0
# - name: Upload coverage to Codecov
# uses: codecov/codecov-action@v3
# with:
# token: ${{ secrets.CODECOV_TOKEN }}
# verbose: true
# fail_ci_if_error: true
.github/workflows/blockifier_post-merge.yml
line 32 at r1 (raw file):
- run: | pip install -r crates/blockifier/tests/requirements.txt cargo test -- --include-ignored
Suggestion:
cargo test -p blockifier -p native_blockifier -- --include-ignored
crates/committer/Cargo.toml
line 38 at r1 (raw file):
# See [here](https://github.com/bnjbvr/cargo-machete/issues/128). [package.metadata.cargo-machete] ignored = ["strum"]
revert the changes here (didn't Tzahi fix this already...?)
only the version should be set to 0.1.0-rc.0, the rest should be kept as is
Code quote:
[dependencies]
async-recursion.workspace = true
derive_more.workspace = true
ethnum.workspace = true
hex.workspace = true
log.workspace = true
rand.workspace = true
serde.workspace = true
serde_json.workspace = true
starknet-types-core.workspace = true
strum.workspace = true
strum_macros.workspace = true
thiserror.workspace = true
tokio.workspace = true
# Optional dependencies required for tests and the testing feature.
# See [here](https://github.com/bnjbvr/cargo-machete/issues/128).
[package.metadata.cargo-machete]
ignored = ["strum"]
crates/committer/src/block_committer/input.rs
line 0 at r1 (raw file):
how are the changes here related to the monorepo...? this seems like a regression, the commit adding LevelFilter
is from 6 days ago, they should appear in Tzahi's alignment
1efe35c
to
cd31074
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 26 of 283 files at r1, 40 of 62 files at r2, all commit messages.
Reviewable status: 164 of 306 files reviewed, 12 unresolved discussions (waiting on @liorgold2)
a discussion (no related file):
boring changes:
RPC
->Rpc
test_utils
crate is nowpapyrus_test_utils
.github/workflows/committer_ci.yml
line 40 at r2 (raw file):
- run: echo "BENCH_INPUT_FILES_PREFIX=$(cat ./crates/committer_cli/src/tests/flow_test_files_prefix)" >> $GITHUB_ENV - run: gcloud storage cp -r gs://committer-testing-artifacts/$BENCH_INPUT_FILES_PREFIX/* ./crates/committer_cli/benches - run: cargo test --release -- --include-ignored test_regression
Suggestion:
- run: cargo test --release -p committer -- --include-ignored test_regression
.github/workflows/committer_ci.yml
line 65 at r2 (raw file):
# Benchmark the old code. - run: cargo bench
TODO: what is the effect of cargo bench
without specific crate selection?
Code quote:
# List the existing benchmarks.
- run: |
cargo bench -- --list | grep ': benchmark$' | sed -e "s/: benchmark$//" > benchmarks_list.txt
# Benchmark the old code.
- run: cargo bench
crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree_test.rs
line 141 at r2 (raw file):
/// 26 90 /// / / \ /// * 25 65
TODO: fix
Code quote:
*
crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree_test.rs
line 155 at r2 (raw file):
/// E B /// / / \ /// * E B
TODO: fix
Code quote:
* E
crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree_test.rs
line 300 at r2 (raw file):
/// 1 /// / \ /// * *
TODO: fix
Code quote:
* *
crates/committer/src/patricia_merkle_tree/original_skeleton_tree/skeleton_forest_test.rs
line 88 at r2 (raw file):
/// E E B /// / \ / \ /// * B B E
TODO: fix
Code quote:
*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 52 of 283 files at r1, 22 of 62 files at r2.
Reviewable status: 238 of 306 files reviewed, 14 unresolved discussions (waiting on @liorgold2)
docs/blockifier/SECURITY.md
line 1 at r2 (raw file):
# Security policy
TODO: update this + README.md (or delete)
docs/starknet_api/README.md
line 1 at r2 (raw file):
# starknet-api
TODO: delete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 238 of 306 files reviewed, 4 unresolved discussions (waiting on @liorgold2)
.github/workflows/blockifier_ci.yml
line 45 at r1 (raw file):
python -m pip install --upgrade pip pip install pytest - run: pytest scripts/merge_paths_test.py
.github/workflows/blockifier_ci.yml
line 100 at r1 (raw file):
Previously, dorimedini-starkware wrote…
delete this (already in main CI)
.github/workflows/blockifier_compiled_cairo.yml
line 41 at r1 (raw file):
Previously, dorimedini-starkware wrote…
if you don't add this, cargo will attempt to build all crates and likely fail papyrus build due to lack of
protoc
installation
.github/workflows/blockifier_post-merge.yml
line 32 at r1 (raw file):
- run: | pip install -r crates/blockifier/tests/requirements.txt cargo test -- --include-ignored
.github/workflows/committer_ci.yml
line 40 at r2 (raw file):
- run: echo "BENCH_INPUT_FILES_PREFIX=$(cat ./crates/committer_cli/src/tests/flow_test_files_prefix)" >> $GITHUB_ENV - run: gcloud storage cp -r gs://committer-testing-artifacts/$BENCH_INPUT_FILES_PREFIX/* ./crates/committer_cli/benches - run: cargo test --release -- --include-ignored test_regression
already fixed
.github/workflows/committer_ci.yml
line 65 at r2 (raw file):
Previously, dorimedini-starkware wrote…
TODO: what is the effect of
cargo bench
without specific crate selection?
already fixed
crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree_test.rs
line 141 at r2 (raw file):
Previously, dorimedini-starkware wrote…
TODO: fix
crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree_test.rs
line 155 at r2 (raw file):
Previously, dorimedini-starkware wrote…
TODO: fix
crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree_test.rs
line 300 at r2 (raw file):
Previously, dorimedini-starkware wrote…
TODO: fix
crates/committer/src/patricia_merkle_tree/original_skeleton_tree/skeleton_forest_test.rs
line 88 at r2 (raw file):
Previously, dorimedini-starkware wrote…
TODO: fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 238 of 306 files reviewed, 3 unresolved discussions (waiting on @liorgold2)
.github/workflows/blockifier_coverage.yml
line 44 at r1 (raw file):
# token: ${{ secrets.CODECOV_TOKEN }} # verbose: true # fail_ci_if_error: true
PR which should remove the coverage from the blockifier phase
Pull Request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this introduce a breaking change?
Other information
This change is