FLIP 275: Removal of Types in Contract Updates #276
Conversation
|
I guess this proposal does not prevent a |
| } | ||
| ``` | ||
|
|
||
| Placing the pragma at the top level, or nested inside another definition in `C` has no effect, and does not permit the removal of `R`. |
There was a problem hiding this comment.
Could we make this an error/warning?
|
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. 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. |
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. |
Or an interface type definition that is the bedrock for other contracts downstream |
|
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. |
|
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? |
In the design section of the FLIP it says that the current design proposal only allows removing concrete types, not interfaces. |
|
btw if we stay out of Circle situation; and focus on the FLIP:
I think would be nice to have some examples on this.
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 ) |
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 |
This case is quite different fwiw. All existing marketplace contracts (including Flow's own NFTStandards) use |
|
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. 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 ) |
| ```cadence | ||
| access(all) contract C { | ||
|
|
||
| #removedType(R) |
There was a problem hiding this comment.
Would something like this work as well:
resource R {
#removedType
}
This might be a bit cleared because the type is still in the contract code its just flagged as removed.
|
Thank you everyone for your great feedback! Let me provide some additional context, answer your questions, and clarify some misunderstandings. ContextThe 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, onflow/cadence#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
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.
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. 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. 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 #53 (comment) (unfortunately I cannot link to it directly). Reason for rejection
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
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:
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. 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. FinI 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. |
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.
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. |
|
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: onflow/cadence#3410 |
…/flips into sainati/type-removal-flip
|
Thank you for updating the FLIP to the current, post-Crescendo, status @SupunS. The changes look great 👍 |
|
Circling back to this again, now that Cadence 1.0 is out. Thanks everyone for the feedback so far! I updated the FLIP and the description to match the current state of things. I also added some more explanations on things like why the change is needed, and reasons for the existing behaviour, etc., and cleaned up some outdated information. Hope that would clear-out some doubts/confusions if there was any. Please have a look 🙏 (and you beat me to it, Bastian :D ) |
turbolent
changed the title
|
Thank you again everyone for taking time to review and provide feedback. As we discussed in the last working group session, I'm going to consider this FLIP as accepted 🎉 The concerns regarding breaking of downstream dependencies (from type-removals, and any other changes such as removing functions, removing fields, etc.) would be addressed separately as part of the discussion for "making contracts immutable", and a separate FLIP would be proposed in future. |
Closes #275 and onflow/cadence#3210
This is a FLIP for a new pragma that would allow types to be removed in contract updates.
As also mentioned in the FLIP itself, a prototype implementation was added in onflow/cadence#3376 and onflow/cadence#3380. This is currently disabled by default, and has been added only as a proof of concept and to be able to test in local environments.