Skip to content

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