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

FLIP 282: Import of pre-Cadence 1.0 Programs #283

Merged
merged 2 commits into from
Aug 13, 2024

Conversation

turbolent
Copy link
Member

FLIP #282

@bluesign
Copy link
Collaborator

I strongly support this FLIP.

One side question, I was gonna bring up yesterday on the meeting but time ran out: As we will have pre 1.0 parser in the codebase, does that mean that contracts can still update to 1.0 if they want later?

@turbolent
Copy link
Member Author

As we will have pre 1.0 parser in the codebase, does that mean that contracts can still update to 1.0 if they want later?

At the moment this is not possible, because the state migration for Cadence 1.0/Crescendo is implemented as "all-at once", and does not support individual contracts to be updated later. It might be feasible, but we have not investigated how individual contracts could be updated later. Especially the migration of links to capability controller and the entitlements migration makes this problematic.

We would definitely need the pre-1.0 parser in the codebase for the pre-1.0 to 1.0 contract updatability checking (currently done during the migration).

Copy link
Member

@joshuahannan joshuahannan left a comment

Choose a reason for hiding this comment

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

I also strongly support this.

I think there is an argument to be made for still supporting deposit and withdraw. I need to consider it more, but it seems like if a project is willing to let their contract break with Cadence 1.0, the concern about potentially overwriting any custom withdraw/deposit functionality might be overblown. IMO, losing access to that functionality completely is worse than losing access to some small custom logic in those functions, which probably only applies to <1% of contracts anyway.

Maybe if we are worried about overwriting custom withdraw/deposit functionality, we could do some sort of parsing of those functions to see if they just use the typical implementation and only overwrite them if they match the generic implementation.

cadence/20240726-pre-cadence-1-import.md Outdated Show resolved Hide resolved
Co-authored-by: Joshua Hannan <joshua.hannan@dapperlabs.com>
Copy link
Member

@SupunS SupunS left a comment

Choose a reason for hiding this comment

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

Nice write-up! I like the proposal, and particularly like the idea of this being a run-time recovery, rather than a one-off recovery during the migration. So the recovery can be improved later, and could support more features, and more contracts in future.

@turbolent
Copy link
Member Author

turbolent commented Jul 31, 2024

I've opened onflow/cadence#3505, which implements one of the suggested solutions to how programs could determine if a type (of a value) is defined in a recovered program, e.g. to avoid function calls (which will always fail): Add a new field isRecovered: Bool to the Cadence type Type (run-time types).

For example, when working with a FungibleToken Vault, it is possible to make function calls on the vault conditional on vault.getType().isRecovered.

If this seems like a good idea, I can add it to the proposal. cc @austinkline

@j1010001
Copy link
Member

Thanks everyone for the feedback! This FLIP has not received new comments in over 2 weeks, and had no updates since last breakout session. There has been no concerns and the feedback has been very positive, so we can consider this FLIP as approved! 🎉

@j1010001 j1010001 merged commit 72f531e into main Aug 13, 2024
@j1010001 j1010001 deleted the bastian/282-pre-cadence-1-import.md branch August 13, 2024 20:39
@joshuahannan
Copy link
Member

@turbolent Would this be able to be modified to allow access to the getIDs() function of NFT Collections?

There will still be some projects that accept that their contract is broken but would still like to be able to deploy new contracts for their users to own new NFTs that are the same ones they owned before, so it would be nice to be able to query these collections for their list of IDs. Will that be possible at all or no?

@turbolent
Copy link
Member Author

@joshuahannan From a technical perspective that is definitely possible. In general, the proposal suggests to panic in all functions, as it is impossible to correctly convert pre-1.0 function implementations to 1.0.
However, we currently recover specific cases, i.e. FT and NFT implementations, so can probably implement certain functions, like getIDs() with a "sensible default". For getIDs() it looks like this would be return self.ownedNFTs.keys.

The current implementation of the recovered NFT contract can be found here:
https://github.com/onflow/flow-go/blob/661a4198def2f102f5b0aec8874cc539d5e0af46/fvm/environment/program_recovery.go#L132. We could just add getIDs() there.

Could you please open an issue in flow-go? PRs are of course also very welcome

@joshuahannan
Copy link
Member

Thank you! I'm glad that that should be possible. I created an issue and I can try to work on a PR soon once I get done with my current priorities.

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

Successfully merging this pull request may close these issues.

5 participants