-
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: add lints to rpc #1852
chore: add lints to rpc #1852
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1852 +/- ##
===========================================
+ Coverage 40.10% 57.24% +17.13%
===========================================
Files 26 252 +226
Lines 1895 32216 +30321
Branches 1895 32216 +30321
===========================================
+ Hits 760 18442 +17682
- Misses 1100 12409 +11309
- Partials 35 1365 +1330 ☔ View full report in Codecov by Sentry. |
1e7d462
to
62c4ff8
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 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dan-starkware and @giladchase)
crates/papyrus_rpc/src/v0_8/transaction.rs
line 1240 at r1 (raw file):
let selector = Token::Bytes(entry_point_selector.0.to_bytes_be().to_vec()); let payload_length_as_felt = Felt::from(u64::try_from(payload.len()).expect("usize should fit in usize"));
or reword the expect
(usize should fit in u64)
Suggestion:
u64_from_usize(payload.len())
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: all files reviewed, 1 unresolved discussion (waiting on @dan-starkware and @dorimedini-starkware)
crates/papyrus_rpc/src/v0_8/transaction.rs
line 1240 at r1 (raw file):
Previously, dorimedini-starkware wrote…
or reword the
expect
(usize should fit in u64)
Whoops thanks.
Reworded the expect
, would have loved to use the util but this hits a different issue:
a) this util is from the blockifier, which isn't a dependency here (and adding it just for that is unreasonable)
b) we can extract it from the blockifier (it isn't blockifier-specific), but we don't have a natural place to put this util ATM. There were talks about utils
crate but nothing settled yet (I personally don't like a global utils
crate, it's too loosely goosey and gets messy.
Maybe we can have a stdx
crate (std extensions), which will have things that could have been part of the std. rust-analyzer have this. Anyway, out of scope here probably.
62c4ff8
to
c9abdaa
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 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dan-starkware)
286a745
to
d2ae293
Compare
Lior banned `as` repo-wide, unless absolutely necessary. Nearly all as uses can be replaced with [Try]From which doesn't have implicit coercions like as (we encountered several bugs due to these coercions). Motivation: we are standardizing lints across the repo and CI, instead of each crate having separate sets of lints.
c9abdaa
to
d575cbe
Compare
Lior banned `as` repo-wide, unless absolutely necessary. Nearly all as uses can be replaced with [Try]From which doesn't have implicit coercions like as (we encountered several bugs due to these coercions). Motivation: we are standardizing lints across the repo and CI, instead of each crate having separate sets of lints. Co-Authored-By: Gilad Chase <gilad@starkware.com>
Lior banned
as
repo-wide, unless absolutely necessary.Nearly all as uses can be replaced with [Try]From which doesn't have implicit
coercions like as (we encountered several bugs due to these coercions).
Motivation: we are standardizing lints across the repo and CI, instead of each
crate having separate sets of lints.