Skip to content
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

Support rename / retype inside structs #722

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Amxx
Copy link
Contributor

@Amxx Amxx commented Jan 23, 2023

This PR includes an example of rename that we need to support (possibly with a different natspec comment)

@frangio
Copy link
Contributor

frangio commented Jan 24, 2023

Don't we need retyping in addition to renaming? To convert the Timer struct into an integer type.

@frangio
Copy link
Contributor

frangio commented Jan 25, 2023

@Amxx I changed one of the test cases you had written. There is already a built-in way to make that work, by using "retyped" on the state variable.

I also added another test case which we need for the Governor changes if we want to replace the timer structs with integers. This change also can be accepted by the plugin with the same annotation as the other case. However, that got me thinking that I'm not sure it's an actually acceptable change because I'm not sure about the alignment of the uint32 value inside of the struct. Further testing makes me think that the plugin is not properly validating the retype annotation when it comes to structs: InnerRetypeV2Bad should clearly be considered a bad upgrade, but with the retype annotation it's accepted. (Retyped is not meant to override those deeper layout compatibility protections.)

@frangio
Copy link
Contributor

frangio commented Jan 25, 2023

Ok I see what's happening. If there is a retype annotation we explicitly skip the type comparison where the inside of the struct would get compared. We definitely make sure that there are no "outer layout changes", so e.g. subsequent variables can't move from where they are with a retype, but the inside of the struct may have its layout changed. @ericglau Should we also consider checking for "inner layout changes" when the retype refers to a struct?

const typeChange =
!this.isRetypedFromOriginal(original, updated) &&
this.getTypeChange(original.type, updated.type, { allowAppend: false });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants