-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat: create infra utils crate #2073
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Artifacts upload triggered. View details here |
Benchmark movements: |
c13d674
to
e889dc4
Compare
89fda5f
to
6875169
Compare
Artifacts upload triggered. View details here |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2073 +/- ##
===========================================
+ Coverage 40.10% 77.38% +37.27%
===========================================
Files 26 385 +359
Lines 1895 40464 +38569
Branches 1895 40464 +38569
===========================================
+ Hits 760 31313 +30553
- Misses 1100 6850 +5750
- Partials 35 2301 +2266 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
e889dc4
to
27d636f
Compare
6875169
to
3778061
Compare
Artifacts upload triggered. View details here |
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 3 of 6 files at r1.
Reviewable status: 3 of 6 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware)
crates/starknet_sequencer_node/src/utils.rs
line 26 at r1 (raw file):
let base_dir = env::var("CARGO_MANIFEST_DIR") // Attempt to get the `CARGO_MANIFEST_DIR` environment variable and convert it to `PathBuf`. // Ascend two directories ("../..") to get to the project root.
Is this intentionally part of this pr, or a pr-split issue? I don't mind lgtming this though.
Code quote:
// Ascend two directories ("../..") to get to the project root.
crates/infra_utils/Cargo.toml
line 7 at r1 (raw file):
repository.workspace = true license-file.workspace = true description = "Infrastructure utility functions for the sequencer node."
Not just for the node, could you please remove this part? E.g., papyrus node also needs this.
Code quote:
for the sequencer node.
27d636f
to
5abee58
Compare
3778061
to
33e89ec
Compare
33e89ec
to
278821c
Compare
Artifacts upload triggered. View details here |
Artifacts upload triggered. View details here |
Artifacts upload triggered. View details here |
Artifacts upload triggered. View details here |
Artifacts upload triggered. View details here |
278821c
to
18235cb
Compare
Artifacts upload triggered. View details here |
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: 2 of 6 files reviewed, 1 unresolved discussion (waiting on @Itay-Tsabary-Starkware)
crates/infra_utils/Cargo.toml
line 7 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
Not just for the node, could you please remove this part? E.g., papyrus node also needs this.
Done.
crates/starknet_sequencer_node/src/utils.rs
line 26 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
Is this intentionally part of this pr, or a pr-split issue? I don't mind lgtming this though.
Not intentionally.
Just cosmetics that did not fit anywhere.
18235cb
to
0961746
Compare
Artifacts upload triggered. View details here |
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 1 of 6 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)
No description provided.