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

Refactor existing native composite types/values to use CompositeType and CompositeValue #698

Merged
merged 12 commits into from
Apr 6, 2021

Conversation

SupunS
Copy link
Member

@SupunS SupunS commented Mar 16, 2021

Closes #650
Closes #661

Description

Some native composite values and types (e.g. AuthAccount, AuthAccount.Contracts, PublicAccount, etc.) are defined through separate Go types implementing the Value interface (e.g. AuthAccountValue) and Type interface (e.g. AuthAccountType).

This PR refactor those existing values and types to use CompositeValue and CompositeType respectively.


For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@SupunS SupunS force-pushed the supun/refactor-native-types-0 branch from dbd6b00 to 6739995 Compare March 16, 2021 12:05
@SupunS SupunS force-pushed the supun/refactor-native-types-0 branch from d725d22 to 90d63df Compare March 17, 2021 06:10
@SupunS SupunS marked this pull request as ready for review March 17, 2021 06:11
@SupunS SupunS requested a review from turbolent as a code owner March 17, 2021 06:11
@SupunS SupunS self-assigned this Mar 18, 2021
@turbolent
Copy link
Member

Thank you for cleaning this up @SupunS!

Would it be OK if we would wait and merge this into the next major release? It's a big change and it might be good to only merge minor fixes for now

@SupunS
Copy link
Member Author

SupunS commented Mar 19, 2021

@turbolent yes, that would be a good idea.

I will see if there are other possible types we could refactor, and get them in too. Then maybe we could push everything at once.

@SupunS SupunS force-pushed the supun/refactor-native-types-0 branch from 90d63df to 534a83c Compare March 23, 2021 09:59
@SupunS SupunS mentioned this pull request Mar 26, 2021
6 tasks
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great clean-up! 👏

I just had one concern regarding backwards-compatibility for stored data

runtime/interpreter/primitivestatictype.go Outdated Show resolved Hide resolved
runtime/interpreter/value.go Show resolved Hide resolved
@@ -5278,6 +5278,7 @@ type CompositeValue struct {
Owner *common.Address
destroyed bool
modified bool
stringer func() string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

runtime/interpreter/value.go Show resolved Hide resolved
runtime/runtime.go Outdated Show resolved Hide resolved
}

payerAddress := payerAddressValue.(interpreter.AddressValue).ToAddress()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}()

func init() {
// Set the container type after initializing the `AuthAccountContractsType`, to avoid initializing loop.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@SupunS SupunS force-pushed the supun/refactor-native-types-0 branch from 534a83c to 310976f Compare April 1, 2021 04:52
@SupunS SupunS force-pushed the supun/refactor-native-types-0 branch from eac96cc to 79e464b Compare April 2, 2021 05:58
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

@SupunS SupunS merged commit 8b81bb1 into master Apr 6, 2021
@SupunS SupunS deleted the supun/refactor-native-types-0 branch April 6, 2021 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants