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.

Sign up for GitHub

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: Cadence - Enable new fields on existing resource and struct definitions #1097

Closed
wants to merge 3 commits into from
Closed

FLIP: Cadence - Enable new fields on existing resource and struct definitions #1097

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
af4285d
add 20220817-add-fields-to-existing-definitions.md
austinkline Aug 17, 2022
06ff173
Apply suggestions from code review
austinkline Aug 27, 2022
343c3a1
Update flips/20220817-add-fields-to-existing-definitions.md
turbolent Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 80 additions & 0 deletions flips/20220817-add-fields-to-existing-definitions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
# Allow new fields in deployed Resources and Structs
Copy link
Member

Choose a reason for hiding this comment

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

We might also allow contracts?


| Status | Proposed |
|---------------|:--------------------------------------------------------- |
| **FLIP #** | [NNN](https://github.com/onflow/flow/pull/NNN) |
austinkline marked this conversation as resolved.
Show resolved Hide resolved
| **Author(s)** | Deniz Mert Edincik ([email protected]) |
| | Austin Kline ([email protected]) |
| **Sponsor** | |
| **Updated** | 2022-08-17 |

## Objective
This proposed change will allow existing structs and resources to add new fields to them by
turbolent marked this conversation as resolved.
Show resolved Hide resolved
using optional and default assignment embedded in the object definition itself.

## Motivation

One major challenge in smart contract design in cadence is the inability to add new fields to already deployed
structs and resources. Not having the ability to do this means that contracts either get extended
with data that exists in another contract, entirely new contracts are made that are essentially reprints of the
original, except with the newly added fields, or some other workaround such as storing fields as dictionaries
and building them into at runtime.

Each of these workarounds come with their downsides.
- New contracts lead to complex migrations for applications and sunsetting existing contracts.
- Hosting new data in another contract leads to harder-to-follow code and added complexity.
- Factory patterns increase compute since objects must be built at runtime and also lose the benefits of type-checking
since the underlying structure is not truly typed.

## User Benefit

- It will allow developers to add fields as they need them, making contract development more focused on the needs of the current version
as opposed to undue complexity to take future plans into account.
- It will allow more maintainable contract code with less splintered logic as existing contracts update themselves

## Design Proposal

We propose to add the ability to define new optional fields, and the assignment of default values to fields in
structs and resources. All fields added after the first deployment of a contract must be either optional or have
a default parameter assigned so that initialization of existing instances of these objects can be properly
austinkline marked this conversation as resolved.
Show resolved Hide resolved
initialized.

```cadence
pub struct Message {
pub let content: String
}
```

We could now add a new field to this struct in a few ways

```cadence
pub struct Message {
pub let content: String

// new fields
// an optional new field. Existing instances of Message will have timestamp set to nil when accessed
// unless they are set at a later date
pub let timestamp: UInt64?
// a default initialized field, Existing instances of Message will take the default value.
pub let received: Bool = false
Comment on lines +57 to +59
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

}
```

### Limitations

- This will not allow existing fields to be altered. That is, you cannot take a field and alter its type.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@SupunS SupunS Jul 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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)

- This will not allow new fields to be derived from existing ones.

### Compatibility

This should be backwards compatible
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @ramtinms if implemented this might have implications for potential work on composite type inlining


### User Impact

- Cadence developers will be able to modify their contract more to their needs instead of over-designing with
the first launch of their contract(s).
- If new unforeseen features or fixes require new fields, those additions can be kept in the original contract instead of being silo'd off into their own contract.

## Prior Art

N/A
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Multiple Inheritance Wiki

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)