-
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
chore(config): re-enable config pointers with a new whitelist #1775
Conversation
9fb213e
to
d9a8462
Compare
d9a8462
to
0430ae3
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1775 +/- ##
==========================================
- Coverage 40.10% 8.97% -31.13%
==========================================
Files 26 287 +261
Lines 1895 32882 +30987
Branches 1895 32882 +30987
==========================================
+ Hits 760 2952 +2192
- Misses 1100 29708 +28608
- Partials 35 222 +187 ☔ View full report in Codecov by Sentry. |
0430ae3
to
adb4ccb
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 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Itay-Tsabary-Starkware)
crates/papyrus_config/src/dumping.rs
line 58 at r1 (raw file):
/// Detailing pointers in the config map. pub type ConfigPointers = Vec<((ParamPath, SerializedParam), HashSet<ParamPath>)>;
Consider making this more readable by introducing the following:
pub type PointerTarget = (ParamPath, SerializedParam);
pub type Pointers = HashSet<ParamPath>;
pub type ConfigPointers = Vec<(PointerTarget, Pointers)>
Code quote:
pub type ConfigPointers = Vec<((ParamPath, SerializedParam), HashSet<ParamPath>)>;
crates/papyrus_config/src/dumping.rs
line 70 at r1 (raw file):
/// Takes a vector of {target pointer params, SerializedParam, and vector of pointing params}, /// adds the target pointer params with the description and a value, and replaces the value of /// the pointing params to contain only the name of the target they point to.
Doc that it will fail if a param is not pointing to a same-named pointer target unless whitelisted
Code quote:
/// Serialization of a configuration into a JSON file.
/// Takes a vector of {target pointer params, SerializedParam, and vector of pointing params},
/// adds the target pointer params with the description and a value, and replaces the value of
/// the pointing params to contain only the name of the target they point to.
crates/papyrus_config/src/dumping.rs
line 298 at r1 (raw file):
/// Adds each target param to the config map. /// Updates entries in the map to point to these targets, replacing values of entries that match /// the target parameter paths to contain only the name of the target they point to.
same
Code quote:
/// Takes a config map and a vector of target parameters with their serialized representations.
/// Adds each target param to the config map.
/// Updates entries in the map to point to these targets, replacing values of entries that match
/// the target parameter paths to contain only the name of the target they point to.
crates/papyrus_config/src/dumping.rs
line 345 at r1 (raw file):
}; } });
Consider moving into a function
Code quote:
// Iterate over the config, check that all parameters whose name matches a pointer target either
// point at it or are in the whitelist.
config_map.iter().for_each(|(param_path, serialized_param)| {
for ((target_param, _), _) in pointers {
// Check if the param name matches a pointer target, and that it is not in the
// whitelist.
if param_path.ends_with(format!("{FIELD_SEPARATOR}{target_param}").as_str())
&& !non_pointer_params.contains(param_path)
{
// Check that the param points to the target param.
assert!(
serialized_param.content
== SerializedContent::PointerTarget(target_param.to_owned()),
"The target param {} should point to {}, or to be whitelisted.",
param_path,
target_param
);
};
}
});
commit-id:e4597e85
adb4ccb
to
83794de
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: all files reviewed, 3 unresolved discussions (waiting on @yair-starkware)
crates/papyrus_config/src/dumping.rs
line 58 at r1 (raw file):
Previously, yair-starkware (Yair) wrote…
Consider making this more readable by introducing the following:
pub type PointerTarget = (ParamPath, SerializedParam); pub type Pointers = HashSet<ParamPath>; pub type ConfigPointers = Vec<(PointerTarget, Pointers)>
Added a todo, as I will refactor the entire code base.
crates/papyrus_config/src/dumping.rs
line 70 at r1 (raw file):
Previously, yair-starkware (Yair) wrote…
Doc that it will fail if a param is not pointing to a same-named pointer target unless whitelisted
Done.
crates/papyrus_config/src/dumping.rs
line 298 at r1 (raw file):
Previously, yair-starkware (Yair) wrote…
same
Done.
crates/papyrus_config/src/dumping.rs
line 345 at r1 (raw file):
Previously, yair-starkware (Yair) wrote…
Consider moving into a function
Done.
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 @Itay-Tsabary-Starkware)
✓ Commit merged in pull request #1776 |
Stack: