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

[austinkline]

FLIP for enabling the addition of new fields in existing resource and struct definitions

For contributor use:

• [ ] Targeted PR against master branch
• [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• [ ] Updated relevant documentation
• [ ] Re-reviewed Files changed in the Github PR explorer
• [ ] Added appropriate labels

[vercel[bot]]

@austinkline is attempting to deploy a commit to the Flow Team on Vercel.

A member of the Team first needs to authorize it.

[dsainati1]

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!

[austinkline]

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 :+1:

[austinkline]

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.

To make sure I can cover what is missing as explicitly as possible, it sounds like two main pieces:

1. How would default initialized values work? Sounds like this approach could go against some core design decisions in cadence so this might be better off getting scoped out.
2. How would cadence handle these new fields on existing instances of composite types?

[austinkline]

@turbolent replying inline here but wanted to make sure your points are all addressed to be the best of my ability:

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.

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.

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.

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

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.

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 amount field is accessed. That value would still remain unless explicitly overridden in the scope of a transaction

Alternatively, if we allowed default values, instead of nil we would assign the colliding field to that default value.

struct Foo {
  amount: UInt64 = 100
}

The above would mean that all existing instances of Foo which have the old type for amount would read as 100

[bluesign]

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!

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 )

[bluesign]

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.

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.

[austinkline]

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.

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

[bluesign]

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.

[turbolent]

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