-
Notifications
You must be signed in to change notification settings - Fork 354
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
Add upgrade_and_call #1148
Add upgrade_and_call #1148
Conversation
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.
Looking good Andrew. Left some comments.
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.
Good job, Andrew!
I've left a couple of comments
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1148 +/- ##
==========================================
+ Coverage 91.51% 91.53% +0.02%
==========================================
Files 46 46
Lines 1178 1182 +4
==========================================
+ Hits 1078 1082 +4
Misses 100 100
Continue to review 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.
Left a couple of minor suggestions. Apart from that, I think we are ready to go
} | ||
|
||
#[test] | ||
#[ignore] // REASON: |
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.
Let's not forget to add a reason or un-ignore the test
Co-authored-by: immrsd <103599616+immrsd@users.noreply.github.com>
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.
LGTM! Let's just add the reason to the ignored tests, and I think we can merge this and fix the test later when we get more information about the test env issue since we already tested it on testnet.
Fixes #694.
This PR proposes to add
upgrade_and_call
to UpgradeableComponent's InternalImpl. This PR also proposes to addIUpgradeAndCall
interface which includes bothupgrade
andupgrade_and_call
.PR Checklist
upgrade_and_call
to new selector (set_value2
) in upgraded context: https://sepolia.starkscan.co/call/0x03ecdb12d63c133ded1bb2b85a9650aaf2c90e6dd6cb460aa8cf651fb3b73894_1_1