search

onflow / flips

Flow Improvement Proposals
24 stars 22 forks source link

Add FLIP for type removal pragma #276

Open dsainati1 opened 1 month ago

dsainati1 commented 1 month ago

Closes https://github.com/onflow/flips/issues/275 and https://github.com/onflow/cadence/issues/3210

This is a FLIP for a new pragma that would allow types to be removed in contract updates.

NB: This FLIP is already implemented and merged into the current preview build of Cadence 1.0. This was necessary in order to unblock the upgrade of the FiatToken contract, which is itself necessary for the Crescendo upgrade as a whole. However, we are still soliciting feedback on its design for two reasons: 1) if a critical issue is identified with this feature, we still have time to change and/or remove the feature as necessary, and 2) given that this feature is not itself a language feature (i.e. it does not effect type checking or execution of Cadence code) it is still possible to change or remove it after the Cadence 1.0 release, so we would still like to iterate and improve on the design with the community even if a primitive version of it is necessary currently.

turbolent commented 1 month ago

I guess this proposal does not prevent a #replacedType pragma being added in the future?

bluesign commented 1 month ago

I see too many edge cases to be honest, maybe at least we can limit this to migration. I always suggested composeability to people, instead of copying to reuse ( not that it worked out much so far ), I feel this has opposite effect.

austinkline commented 1 month ago

My main concern here is that permission to remove types effectively means a contract cannot revoke its keys if it has any dependency because of the risk that it could permanently break.

I'd like to more clearly understand the reason that this is needed beyond just "unblocking the FiatToken contract", what has to change that cannot be without this? Is there another path to doing it?

Ideally, Circle should chime in here, maybe some of us can help them find another way.

bluesign commented 1 month ago

My main concern here is that permission to remove types effectively means a contract cannot revoke its keys if it has any dependency because of the risk that it could permanently break.

It is not only that, imagine having a field that using a struct with type that is removed.

On Circle side, only explanation makes sense is they lost the keys.

austinkline commented 1 month ago

It is not only that, imagine having a field that using a struct with type that is removed.

Or an interface type definition that is the bedrock for other contracts downstream

bluesign commented 1 month ago

copied from Discord to keep discussion in one place:

I think the main issue is you it feels like you are committed to this ( at least in FiatToken case ), unless some security issues comes out of this, I don't feel that it would be rejected.

I am reading it more like; If nothing critical identified, we keep this till migration, then we can always remove if not liked.

austinkline commented 1 month ago

Is this FLIP restricted only to concrete types or interface types as well?

If interface types as well, what is the difference between this FLIP and just allowing contract upgrades to remove conformance types?

dsainati1 commented 1 month ago

Is this FLIP restricted only to concrete types or interface types as well?

In the design section of the FLIP it says that the current design proposal only allows removing concrete types, not interfaces.

bluesign commented 1 month ago

btw if we stay out of Circle situation; and focus on the FLIP:

The Cadence 1.0 upgrade in particular has exacerbated this need, as the changes to access control have rendered a number of types obselete.

I think would be nice to have some examples on this.

Additionally as all contracts on chain need to be updated for Crescendo, the ability to remove dependencies on contracts that are difficult or otherwise challenging to update has become more important.

This part I totally agree but maybe this can be relaxed with one time contract update checking for Crescendo. ( But also it is also tricky )

SupunS commented 1 month ago

I think would be nice to have some examples on this.

I remember running into some cases when updating nft/ft code bases to match C1.0, e.g: https://github.com/onflow/flow-nft/blob/f741cb5352f1889984e60437e0805b8371dcd7fe/contracts/ExampleNFT.cdc#L157C34-L157C41 where it would have been cleaner to be able to remove certain type definitions. In the above case it was an interface, and would have of-course needed some addition relaxations (e.g: being able to remove a conformance of a removed type) to make it really useful for that particular case. Perhaps replacedType(T, R) would have been a better option for that. But just wanted to highlight that there are cases that could benefit from a solution along these lines. Similar case with the NonFungibleToken as well.

austinkline commented 1 month ago

Similar case with the NonFungibleToken as well.

This case is quite different fwiw. All existing marketplace contracts (including Flow's own NFTStandards) use NFT.CollectionPublic capabilities in their types, removal of this isn't an option like the ExampleNFT interface might be

bluesign commented 1 month ago

thanks @SupunS , yeah the problem is it makes sense in the first look, but then it becomes tricky when someone else relies on it somewhere. Thats the reason I pushed forbidding interface removal in the original issue. for example like this:

https://github.com/bluesign/mainnet-contracts/blob/cfcb76228edd5711d9892ce5cb99a178d5caaae5/contracts/0x4eded0de73020ca5/TopShotEscrowV3.cdc#L90

On greater perspective, small suggestion; I think we need to set up some principle rules for language evolution formally. Till 1.0 we had big freedom, but I think there is a need for some guarantees if we want composeability to succeed. If we had a document why we don't allow type removal before ( which would include all this cases came up here and more ) then maybe we would not even consider this. ( or at least we could weight cons pros easier )

turbolent commented 1 month ago

Thank you everyone for your great feedback!

Let me provide some additional context, answer your questions, and clarify some misunderstandings.

Context

The feature proposed in this FLIP has been requested for several years. So far it had not been realized because there was no urgent need for it. Developers asked to be allowed to remove type definitions in contract updates, just like how it is already possible to remove e.g. function definitions.

The purpose and goal of the contract update checking is to prevent contract updates to circumvent the safety mechanisms of the system. The contract update checking does not have as a goal to ensure that contract updates will not also break downstream contracts that import the updated contract. That is a common misunderstanding, please have a look at the Validation Goals section of the documentation for more details and examples of goals and non-goals.

The contract update checking currently prevents the removal of type definitions. However, the removal of type definitions, just like the removal of e.g. function definitions, is safe.

You are of course absolutely right to point out that such removals might break downstream code! I'll try to address this down below in the section "Breaking downstream code".

With the update to Cadence 1.0, requests for the type removal feature increased. Not significantly, but still. Developers used to have type definitions, e.g. interfaces, which got mainly used for access control purposes (restricted types), which are no longer needed in Cadence 1.0 given that entitlements are now used for access control.

Supun therefore created a feature issue at the beginning of April, https://github.com/onflow/cadence/issues/3210, to ideate and look into how such a feature could look like.

In May we realized that such a feature could help a particular developer (Circle) in their process to update to Cadence 1.0, which further increased the priority of the feature. We thus implemented the existing idea and provided it in the preview release to gather feedback on it, based on a real-world problem.

This FLIP is now trying to gather even more feedback and proposes to properly include the feature into the language.

FLIP

we are still soliciting feedback on its design

There might have been some misunderstandings in the team as well. As with every significant language change, we are trying to use the FLIP process to discuss and get feedback on the change, and end up with a decision. This feature / FLIP is no different. We are not still soliciting feedback, we are always trying to solicit feedback.

Most of the time we have a design proposal before we have a prototype implementation, sometimes (like for this proposal), it is the other way around. Having an implementation is useful to evaluate the proposed design and gather feedback from developers. A merged implementation does not mean that a decision has been made!

if a critical issue is identified with this feature, we still have time to change and/or remove the feature as necessary, ... a primitive version of it is necessary currently.

I think the main issue is you it feels like you are committed to this ( at least in FiatToken case ), unless some security issues comes out of this, I don't feel that it would be rejected.

I am reading it more like; If nothing critical identified, we keep this till migration, then we can always remove if not liked.

Again, sorry for the confusion.

This feature is being proposed like any other feature, and the feature will only get included in a proper release if the feature gets approved. The proposal will not only be rejected if a critical issue is identified – it will be approved or rejected based on the normal FLIP process.

We as the core team have discussed this feature internally, as we do for all feature additions and changes, and found enough agreement to advance in the process opened a FLIP for it. As with every FLIP, we are looking for feedback, concerns, issues we didn't see, things we missed, etc. All the note was meant to say is that so far we had not identified any critical issues and did not expect there to be any.

Therefore, yes, any proposer of a FLIP is obviously committed to what they are proposing. Just because the team is proposing the feature does not mean a decision has been made. If the consensus is that the proposal should be rejected, then the feature will get removed.

Similar concerns were raised in a previous FLIP. I had written a similar explanation there, please have a look at the "Implementation" section in https://github.com/onflow/flips/pull/53#issuecomment-1352439069 (unfortunately I cannot link to it directly).

Reason for rejection

If we had a document why we don't allow type removal before

The only reason why type removals are currently not allowed is again to ensure safety, not to prevent downstream code to get broken. Specifically, it prevents a type with the same name, but different definition, to being added back.

For example, the re-added type definition could potentially have a different kind (e.g. resource to struct), different fields (e.g. new field added, field type changed, etc.), which are all invalid contract update changes (which again would subvert safety).

The "tombstoning" approach prevents the re-adding and thus ensures safety.

Breaking downstream code

I see too many edge cases to be honest, maybe at least we can limit this to migration. I always suggested composeability to people, instead of copying to reuse ( not that it worked out much so far ), I feel this has opposite effect.

My main concern here is that permission to remove types effectively means a contract cannot revoke its keys if it has any dependency because of the risk that it could permanently break.

AFAICS, the impact of removing type definitions on downstream code seems to be the main concern so far.

There are two important features / needs of developers:

  1. Evolve a contract. Developers want to be able update contracts e.g. to add additional, change existing, and remove functionality.

    Direct support of contract updatability is a core feature of Cadence / Flow, and helps developers extend and fix their contracts.

  2. Import of on-chain code. Developers want to be able to use and built on other on-chain code. To do so reliably, they need guarantees that changes to imported code work reliably and will not break their programs.

    Direct support for on-chain imports is a core feature of Cadence / Flow, and helps with composability.

The vision is to extend these features more and more. The problem is that they are naturally at odds with each other: While one contract author might want to evolve their contract, another developer that imports the contract wants to make sure there are no changes that could break their code.

Currently there is already fairly good support for updating contracts (see all changes that are possible in the contract updatability documentation), though we still are lacking some features like adding fields. This FLIP proposes an improvement to this feature.

Support for the second need/feature, ensuring that imported code will not break your code, is lacking and is now starting to become a bigger concern. Up until the release of Cadence 1.0, providing functionality to guarantee to a dependent that an import would never cause any breakage was futile, as the language itself did not provide any such guarantees and could still cause breakage.

With Cadence 1.0 and its backward-compatibility guarantee coming soon, it now makes sense to increase support for the second feature. Since the beginning of the design of Cadence in '19 and introduction of the contract updatability feature, we have had ideas how to enable both features at the same time. I hope that soon after releasing Cadence 1.0 we can finally get to continue these ideas and turn them into concrete proposals.

Concretely, we've thought of different "levels" for updatability: At first a contract might be in a "fully-updatable" state, where authors may change anything (according to the contract updatability rules), but dependents have no/little guarantee their code will not break. A more restrictive level would then e.g. only allow adding new functionality (e.g. adding functions), but not e.g. change function signatures – this would give dependents some more guarantees. Finally, a "completely locked" contract would be not changeable at all, and would provide full guarantee for dependents that there will be no breakage. These levels could be first quite granular, and later split up into more fine-grained ones. Again, these are just ideas.

Note that needs for different kinds of contracts are different:

We ideally want to assist all kinds of contracts and contract developers.

Fin

I hope that putting this FLIP into context will help everyone to evaluate it on its own merits and see it in the bigger picture of evolving Cadence.

I also hope it will assure you that the core team is thinking about it in the bigger picture, but as you can see we are trying to deliver on it incrementally, in smaller pieces.

In a way I have a deja-vu with yet another (group of FLIPs): When the team proposed the mutability restriction functionality in smaller FLIPs, those were also seen at micro-level, and the bigger picture was not clearly communicated. We resolved this with a larger vision document (https://github.com/onflow/flips/blob/main/cadence/vision/mutability-restrictions.md), which then made it easer to evaluate the individual FLIPs in that context. Though we are currently busy with the release of Cadence 1.0 and I cannot promise we will be able to deliver something similar soon, I hope that we can at at least "latest" prioritize it first thing after the release.

bluesign commented 1 month ago

Concretely, we've thought of different "levels" for updatability:

This is a great idea actually. I agree lack of macro view creates problems, to be fair, so far we always had micro level FLIPs. It is really hard to see bigger picture like this. I think this FLIP doesn't sound that bad when we read with that prespective.

A merged implementation does not mean that a decision has been made!

I don't know any project ( with a process to similar to FLIP process ) can claim that. But to be fair on this, I am not involved in much of anything else other than Cadence on this level.

turbolent commented 3 weeks ago

To make it clearer that the feature has not been approved, the implementation of it is now behind a feature flag and disabled by default: https://github.com/onflow/cadence/pull/3410