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

Allow built-in composite types to be storable #2881

Conversation

turbolent
Copy link
Member

@turbolent turbolent commented Oct 19, 2023

⚠️ Depends on #2878

Description

  • Allow built-in composite types to be storable

  • Allow built-in composite values to be storable.

    As discussed in https://discord.com/channels/613813861610684416/1108479699732152503/1164334273684324402, allowing CompositeValue.Storable to determine if the value's type is storable, by getting the CompositeType for the value, is tricky, as the function is called by atree, and it does not provide access to an Interpreter easily.

    Allow getting root interpreter from interpreter storage, which is ugly and works. If you have any better ideas, please let me know – I don't like this, but couldn't find another easy workable solution.

    For now this is tech-debt: Eventually in Stable Cadence (which cannot be used, we need this feature on master), we'll have fewer CompositeValues with built-in CompositeTypes that are non-storable (e.g. currently AuthAccount), and we might find a better alternative to this approach later. At least for now this unblocks the work on the EVM integration in flow-go.

    Finally, allow built-in composite values to be storable, by checking for built-in types, if the composite value's composite type is storable (some are not, e.g. AuthAccount). Only check built-in types, because non-built-in composite values/types are always considered storable.


  • 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

@turbolent turbolent self-assigned this Oct 19, 2023
@turbolent
Copy link
Member Author

As mentioned above, this adds some tech debt. After discussing implementation approaches for the Flex contract with @ramtinms and @janezpodhostnik, we realized that the Flex contract does not necessarily need to be a built-in, so this PR is no longer needed

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

Successfully merging this pull request may close these issues.

1 participant