-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[Framework] Add dependencies check for code object freezing #14064
Conversation
3850c4f
to
cc9b120
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14064 +/- ##
===========================================
- Coverage 71.5% 58.9% -12.6%
===========================================
Files 2330 824 -1506
Lines 460902 198033 -262869
===========================================
- Hits 329781 116770 -213011
+ Misses 131121 81263 -49858 ☔ View full report in Codecov by Sentry. |
// This would increase the amount of gas used, but this is a rare operation and it's rare to have many packages | ||
// in a single code object. | ||
vector::for_each(registry.packages, |pack| { | ||
check_dependencies(code_object_addr, &pack); |
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.
This check_dependencies
function returns allowed_module_deps
that are normally used in the native bytecode dep verification:
/// is passed on to the native layer to verify that bytecode dependencies are actually what is pretended here. |
Is it fine to ignore the return value?
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.
Yeah here in this freeze function we only care about visibility checks as other checks should have already been performed when this package was first deployed or last updated
LGTM, maybe one more test to verify freezing an upgradeable/mutable package with immutable dependencies should work too. Edit: Added this in! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
* Refactor object deployment CLI commands * Refactor object deployment CLI commands --------- Co-authored-by: JohnChangUK <[email protected]>
…14079) * Refactor object deployment CLI commands * Refactor object deployment CLI commands --------- Co-authored-by: Kevin <[email protected]> Co-authored-by: JohnChangUK <[email protected]>
* Refactor object deployment CLI commands * Refactor object deployment CLI commands --------- Co-authored-by: JohnChangUK <[email protected]>
…bs#14064) * Refactor object deployment CLI commands * Refactor object deployment CLI commands --------- Co-authored-by: JohnChangUK <[email protected]>
* Refactor object deployment CLI commands * Refactor object deployment CLI commands --------- Co-authored-by: JohnChangUK <[email protected]>
* Refactor object deployment CLI commands * Refactor object deployment CLI commands --------- Co-authored-by: JohnChangUK <[email protected]>
Description
Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
E2E tests
Key Areas to Review
Checklist