FLIP: Cadence - Enable new fields on existing resource and struct definitions #1097
Conversation
|
@austinkline is attempting to deploy a commit to the Flow Team on Vercel. A member of the Team first needs to authorize it. |
|
In the future, if you're submitting a Cadence FLIP, please add some people from the Cadence team (e.g. myself, @turbolent, @SupunS and @robert-e-davidson3) as reviewers; otherwise we don't get notifications about these. I wasn't aware this even existed until someone pointed out to me! |
ahhh good to know! Noted for the future 👍 |
There was a problem hiding this comment.
Nice. I like the idea here; I agree this would make life easier for Cadence developers. The proposal could use a little more detail in some places though, specifically with regards to how the proposed changes would interact with existing behavior of the language.
| pub let timestamp: UInt64? | ||
| // a default initialized field, Existing instances of Message will take the default value. | ||
| pub let received: Bool = false |
There was a problem hiding this comment.
The design proposal needs more detail about how optional or default initialized fields would interact with the existing initialization requirements. Do these fields need to be handled in the init function, or is it possible to omit them and have these fields be nil or the default value implicitly on initialization?
There was a problem hiding this comment.
The current design of only allowing field initialization in one place, the initializer, instead of the initializer or in the field declaration, was deliberate: There is only one place to look at when reading the code and it avoids many design questions related to expressions in field declarations:
- When is the expression evaluated? Once, "statically" for all instances? For each instance, before the initializer?
- What is the evaluation order? Can a field refer to another?
Is the default value only used when migrating existing values, or is it also used for new values?
There was a problem hiding this comment.
My thought here was the the init function could recognize what's missing and either accept the default field or set the value to nil. It sounds like a default value might be more tough to achieve specially from what @turbolent is calling out. Happy to scope this FLIP down to specifically nullable additional fields if default initial values aren't feasible.
When is the expression evaluated? Once, "statically" for all instances? For each instance, before the initializer?
If we can accept default values it would have to be before the analyzer, right?
What is the evaluation order? Can a field refer to another?
My thought was that fields cannot refer to one another. If they can, this would get much more complex since then we're talking about migrations and the nuances with multiple new fields all relying on each other (not to mention side effects and function calls if it got that deep). Ideally, when the composite type is accessed, these nullable values would be updated at that time so that we can get around the need for full migrations which aren't feasible to do.
There was a problem hiding this comment.
Could we maybe simplify the proposal, by only keeping the first part of the proposal, adding new optional fields (which get initialized to nil), and removing adding non-optional fields with "default values"?
That way we could get at least some way of adding fields supported fairly quickly and easily.
Later we could propose adding non-optional fields in a follow-up proposal.
|
|
||
| ### Compatibility | ||
|
|
||
| This should be backwards compatible |
There was a problem hiding this comment.
cc @ramtinms if implemented this might have implications for potential work on composite type inlining
|
|
||
| ## Prior Art | ||
|
|
||
| N/A |
There was a problem hiding this comment.
We should add a Questions and Discussion section to talk about related topics to this FLIP. In particular, one that comes to mind is how this is going to interact with the potential addition of extensions to the language proposed here: #1101. As that proposal stands now, we'd need some way of handling the case where a user updates their struct or resource with a new field or method that also exists in an extension. It's also worth considering the extent to which these two proposals overlap in their use cases.
There was a problem hiding this comment.
We should add a Questions and Discussion section to talk about related topics to this FLIP.
Will do 👍
In particular, one that comes to mind is how this is going to interact with the potential addition of extensions to the language proposed here: #1101. As that proposal stands now, we'd need some way of handling the case where a user updates their struct or resource with a new field or method that also exists in an extension.
Would the analyzer know about this conflict when an update is attempted or would it only be detected at runtime? This sounds like the Diamond problem (kind of), so I wonder if we can't pull inspiration from how Go does it, or how interpreted languages like Python do it as a means to discuss in general how overlapping types in general should be handled.
If we went with the way Go does this, we would reject the update outright, stating that the use of certain fields (or methods) are ambiguous, though I'm not sure if we know what we need to when an update is attempted to make that work. Perhaps if you only allow the extension of Composite types inside of the contract they exist in?
If we went with the way Perl and Python do it, we would take whichever definition comes first. That is, if we have struct T and extensions T2 and T3, the order in which the extensions are specified would dictate what wins out. In that model, any overlapping fields which exist both on T and TN would always fallback to T since it came first.
In the end, I think this problem depends on what the "real" type is that we are dealing with. Definitely worth exploring that more here so I'd love to hear your thoughts!
It's also worth considering the extent to which these two proposals overlap in their use cases.
Makes sense, their purpose at their core seem to be the same. Primarily that currently folks have to over-design for their contracts and take on risk for that over-design in order to get flexibility in return so that future features are possible. Curious peoples' thoughts to whether both have their merits, though. My general thought process to this FLIP was also centered around reducing the amount of extra code a dev needs in order to support new fields. Should this FLIP go through, theoretically new fields would be accessible with no extra work (all handled out of sight of the cadence dev)
|
|
||
| ### Limitations | ||
|
|
||
| - This will not allow existing fields to be altered. That is, you cannot take a field and alter its type. |
There was a problem hiding this comment.
Users already have the ability to remove fields from existing composites, so giving them the ability to add new fields can implicitly allow changing a field's type if a user removes an existing field (that had type T1) in one update and then adds it back with a different type T2 in another update. We'd need a specific solution for this case. One obvious answer is to simply fail a contract update if existing instances of a struct or resource have conflicting types with the field being added, but there are other potential solutions. Either way we should make it explicit in the proposal.
There was a problem hiding this comment.
Great callout, I hadn't thought about this. My understanding of what happens when a type definition is taken away is that it is simply hidden (the data itself is not destroyed) but I am assuming that is only for existing definitions of those resources/structs which wouldn't cover doing them in separate instances.
Your proposed option is what I would jump to as well, and then perhaps another alternative is to only allow new nullable fields to struct/resource definitions and then to assign those conflicting types which have the old type definition to nil when accessed since they don't match the new definition.
// old definition
struct Foo {
amount: UInt64
}
//new definition
struct Foo {
amount: Int64?
}If I were to change the definition as mentioned above, existing instances of Foo would have their amounts set to nil because their types do not match. This approach could open up the ability to alter structs/resources even more but I worry about the risk of a malicious dev toggling between type definitions in some capacity to force some kind of odd behavior.
There was a problem hiding this comment.
We could probably propose similar functionality as proposed for type removal in onflow/flips#276: If we propose to allow adding new fields, the removal of fields should be required to "tombstone" a field, so it cannot be re-added.
For example, this could be done through something like a #removedField(fieldName) pragma.
There was a problem hiding this comment.
I like the idea of tomb-stoning. However there is a chance that people have already removed fields from composites without the pragma, and the data for those fields still exist. So adding a new field could still cause type-safety issues. Maybe we could cleanup the composite values as part of the Cadence 1.0 migration, to remove fields that are not part of their corresponding type? (IDK if this is possible / how to do that, though)
There was a problem hiding this comment.
This feature has been a goal ever since the early days of Cadence, thank you for opening a FLIP for it.
The main reason why this feature has not been implemented yet is that it is unclear how it should be implemented. Even though designs/proposals for language features usually do not discuss implementation details, it is an important factor when deciding if the proposal should be accepted.
It would be great if the proposal could describe the behaviour of the necessary migration, in particular: When is the migration performed? Eagerly migrating all existing data at contract update-time is not tractable on-chain. For example, common relational database systems take multiple seconds, minutes, or even hours to migrate tables when adding new columns. The migration should probably occur lazily, when an existing value is accessed.
Another open question is how this feature interacts with the existing feature of removing fields. Currently, this is implemented by only updating the code and not actually removing the data (for the same reasons as mentioned above). Allowing addition would lead to effectively allowing field updates, i.e. changing the type of a field, by first removing the field, then adding it back with a different type.
| pub let timestamp: UInt64? | ||
| // a default initialized field, Existing instances of Message will take the default value. | ||
| pub let received: Bool = false |
There was a problem hiding this comment.
The current design of only allowing field initialization in one place, the initializer, instead of the initializer or in the field declaration, was deliberate: There is only one place to look at when reading the code and it avoids many design questions related to expressions in field declarations:
- When is the expression evaluated? Once, "statically" for all instances? For each instance, before the initializer?
- What is the evaluation order? Can a field refer to another?
Is the default value only used when migrating existing values, or is it also used for new values?
To make sure I can cover what is missing as explicitly as possible, it sounds like two main pieces:
|
|
@turbolent replying inline here but wanted to make sure your points are all addressed to be the best of my ability:
Agreed and makes total sense, I will circle back to this FLIP and add more detail so we have more to go on in terms of discussion.
A few folks I've discussed this topic with have brought up migrations as well. It is why this FLIP doesn't allow reliance on other fields which would make evaluating them much more difficult when they don't "exist" yet under the hood. My take here is also to lazily evaluate as they are accessed for the first time. That could be when we access the entire object, or it could be when the new field itself is accessed for the first time
Yes, this was a good callout which I had neglected to consider but is certainly a core part of the problems in allowing updates like this. My current leaning is that since new fields would need to be nullable (or have a default), and conflicting type would need to be returned as null if it is found to be "incorrect" That is, if I have struct Foo {
amount: Int
}and I then remove the field, and try to add a new one with the same name: struct Foo {
amount: UInt64?
}Then existing instances of Foo would return nil when the Alternatively, if we allowed default values, instead of struct Foo {
amount: UInt64 = 100
}The above would mean that all existing instances of |
| pub let timestamp: UInt64? | ||
| // a default initialized field, Existing instances of Message will take the default value. | ||
| pub let received: Bool = false |
There was a problem hiding this comment.
My thought here was the the init function could recognize what's missing and either accept the default field or set the value to nil. It sounds like a default value might be more tough to achieve specially from what @turbolent is calling out. Happy to scope this FLIP down to specifically nullable additional fields if default initial values aren't feasible.
When is the expression evaluated? Once, "statically" for all instances? For each instance, before the initializer?
If we can accept default values it would have to be before the analyzer, right?
What is the evaluation order? Can a field refer to another?
My thought was that fields cannot refer to one another. If they can, this would get much more complex since then we're talking about migrations and the nuances with multiple new fields all relying on each other (not to mention side effects and function calls if it got that deep). Ideally, when the composite type is accessed, these nullable values would be updated at that time so that we can get around the need for full migrations which aren't feasible to do.
|
|
||
| ### Limitations | ||
|
|
||
| - This will not allow existing fields to be altered. That is, you cannot take a field and alter its type. |
There was a problem hiding this comment.
Great callout, I hadn't thought about this. My understanding of what happens when a type definition is taken away is that it is simply hidden (the data itself is not destroyed) but I am assuming that is only for existing definitions of those resources/structs which wouldn't cover doing them in separate instances.
Your proposed option is what I would jump to as well, and then perhaps another alternative is to only allow new nullable fields to struct/resource definitions and then to assign those conflicting types which have the old type definition to nil when accessed since they don't match the new definition.
// old definition
struct Foo {
amount: UInt64
}
//new definition
struct Foo {
amount: Int64?
}If I were to change the definition as mentioned above, existing instances of Foo would have their amounts set to nil because their types do not match. This approach could open up the ability to alter structs/resources even more but I worry about the risk of a malicious dev toggling between type definitions in some capacity to force some kind of odd behavior.
|
|
||
| ## Prior Art | ||
|
|
||
| N/A |
There was a problem hiding this comment.
We should add a Questions and Discussion section to talk about related topics to this FLIP.
Will do 👍
In particular, one that comes to mind is how this is going to interact with the potential addition of extensions to the language proposed here: #1101. As that proposal stands now, we'd need some way of handling the case where a user updates their struct or resource with a new field or method that also exists in an extension.
Would the analyzer know about this conflict when an update is attempted or would it only be detected at runtime? This sounds like the Diamond problem (kind of), so I wonder if we can't pull inspiration from how Go does it, or how interpreted languages like Python do it as a means to discuss in general how overlapping types in general should be handled.
If we went with the way Go does this, we would reject the update outright, stating that the use of certain fields (or methods) are ambiguous, though I'm not sure if we know what we need to when an update is attempted to make that work. Perhaps if you only allow the extension of Composite types inside of the contract they exist in?
If we went with the way Perl and Python do it, we would take whichever definition comes first. That is, if we have struct T and extensions T2 and T3, the order in which the extensions are specified would dictate what wins out. In that model, any overlapping fields which exist both on T and TN would always fallback to T since it came first.
In the end, I think this problem depends on what the "real" type is that we are dealing with. Definitely worth exploring that more here so I'd love to hear your thoughts!
It's also worth considering the extent to which these two proposals overlap in their use cases.
Makes sense, their purpose at their core seem to be the same. Primarily that currently folks have to over-design for their contracts and take on risk for that over-design in order to get flexibility in return so that future features are possible. Curious peoples' thoughts to whether both have their merits, though. My general thought process to this FLIP was also centered around reducing the amount of extra code a dev needs in order to support new fields. Should this FLIP go through, theoretically new fields would be accessible with no extra work (all handled out of sight of the cadence dev)
Co-authored-by: Daniel Sainati <[email protected]> Co-authored-by: Bastian Müller <[email protected]>
Yeah this is big problem usually, though I think everyone from Dapper should follow @onflow/flow repository at least ( I am following almost all Flow & Dapper repositories, it is taking just 20 mins with my morning coffee, 80% of activity is being flow-go ) |
I think forbidding removal and allowing addition can be net improvement One problem I see from developer perspective, is adding fields as optional. I think it is a bit changing the meaning of Optional. Maybe we can allow adding synthetics fields ? They can lead to lazy migration. But also they have the problem of evaluation order. |
Not sure I follow how this would change the meaning of optionals? Is it really any different to a cadence dev whether the field used to be nil or is nil, now? Or do you mean to flow itself and how it treats them? Is synthetic right here, though? Once a new field is set it would be as real as anything else. Forcing them to be optional just helps ensure we don't need a migration strategy for existing data which I would say is a core requirement to this FLIP |
|
For me, also I think in the design of Cadence, optional variable means more than variable can be nil. ( though I can be wrong, maybe @turbolent can clarify more ) But with my limited knowledge of optionals, actually they are property of the object also. ( some property of an object being optional, is also a property of the object ) so when we add a new property, it is being optional should depend on the object, not our technical limitations. Ofc we can decide this is a valid trade off, and add new fields as optionals. For me migration is not that scary to be honest, if we forbid removal. |
| @@ -0,0 +1,80 @@ | |||
| # Allow new fields in deployed Resources and Structs | |||
There was a problem hiding this comment.
We might also allow contracts?
|
As we're finishing work on Cadence 1.0, we can finally come back to proposals like this. Thank you everyone for your patience! I left some comments and suggestions, and am happy to both sponsor this proposal, as well as apply suggested edits and add remaining details. Does that sound good to you @bluesign @austinkline? Once we addressed the outstanding issues, we can schedule to vote on it in the next WG call |
|
Moved this to the FLIP repo: onflow/flips#295 Let's continue the discussion there and hopefully we can decide on it in the upcoming WG call |
FLIP for enabling the addition of new fields in existing resource and struct definitions
For contributor use: