-
Notifications
You must be signed in to change notification settings - Fork 98
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: derive serialize for FeeTokenAddress and ChainInfo #1896
base: main
Are you sure you want to change the base?
chore: derive serialize for FeeTokenAddress and ChainInfo #1896
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1896 +/- ##
==========================================
+ Coverage 77.80% 77.88% +0.08%
==========================================
Files 61 61
Lines 8749 8781 +32
Branches 8749 8781 +32
==========================================
+ Hits 6807 6839 +32
Misses 1502 1502
Partials 440 440 ☔ View full report in Codecov by Sentry. |
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 9 of 9 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ArniStarkware)
crates/blockifier/src/context.rs
line 83 at r2 (raw file):
#[derive(Clone, Debug, Deserialize, PartialEq, Serialize, Validate)] pub struct ChainInfo {
consider moving this struct in the future to starknet-api
Code quote:
pub struct ChainInfo
crates/blockifier/src/context_test.rs
line 27 at r2 (raw file):
fn test_valid_config_body< T: for<'a> Deserialize<'a> + SerializeConfig + Validate + PartialEq + Debug,
is Deserialize required here? I only see the usage of serialize.
Code quote:
Deserialize
d34cee3
to
88d3ef4
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.
Reviewable status: 8 of 9 files reviewed, 1 unresolved discussion (waiting on @Yael-Starkware)
crates/blockifier/src/context.rs
line 83 at r2 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
consider moving this struct in the future to starknet-api
Added a TODO.
crates/blockifier/src/context_test.rs
line 27 at r2 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
is Deserialize required here? I only see the usage of serialize.
get_config_from_file
requires Deserialize
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: 8 of 9 files reviewed, all discussions resolved (waiting on @ArniStarkware)
This change is![Reviewable](https://camo.githubusercontent.com/23b05f5fb48215c989e92cc44cf6512512d083132bd3daf689867c8d9d386888/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)