fix: Risk of key collision for monitor custom scripts #258
Conversation
NicoMolinaOZ
requested review from
collins-w,
d-carmo,
shahnami,
tirumerla and
zeljkoX
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #258 +/- ##
=======================================
+ Coverage 95.9% 96.0% +0.1%
=======================================
Files 66 66
Lines 19645 20002 +357
=======================================
+ Hits 18849 19213 +364
+ Misses 796 789 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I think we should move the name uniqueness validation to the more appropriate validate function, instead of inside load_all.
Additionally, we should add a test that ensures the collision is no longer possible (as described in #236)
| fs::write(temp_dir.path().join("monitor1.json"), valid_config_1).unwrap(); | ||
| fs::write(temp_dir.path().join("monitor2.json"), valid_config_2).unwrap(); | ||
|
|
||
| let result: Result<HashMap<String, Monitor>, _> = | ||
| Monitor::load_all(Some(temp_dir.path())).await; | ||
|
|
||
| assert!(result.is_err()); | ||
| if let Err(ConfigError::ValidationError(err)) = result { | ||
| assert!(err.message.contains("Duplicate monitor name found")); | ||
| } | ||
| } |
There was a problem hiding this comment.
Not sure if it's necessary to do fs calls for testing JSON parsing and validation instead of checking in memory skipping disk i/o calls..seems much faster no? Also not a fan of flakiness with temp dir paths
There was a problem hiding this comment.
Yes, it's not the best, but since we are testing the loading of JSON files from disk (that's the real scenario when the monitor starts), we should probably find another way. Thanks for the comment Sai.
NicoMolinaOZ
deleted the
plat-6648-risk-of-key-collision-and-script-data-overwrite-in
branch
Summary
https://linear.app/openzeppelin-development/issue/PLAT-6648/risk-of-key-collision-and-script-data-overwrite-in
Related issue:
#236
Testing Process
Checklist