Remove public let fields

[SupunS]

Work towards onflow/cadence#2273

An alternative solution for improving external mutability restrictions.

[dsainati1]

One potential point to note; the existing mutability restrictions also apply to var fields; a pub var array cannot be mutated outside of the enclosing composite. The only way to do that is to declare a pub(set) var field. This may cause a slight inconsistency or point of confusion if our solution to this is to only support priv let fields, as this does not solve the "problem" where you can mutate regular pub var fields through references or functions. However we may simply decide this isn't actually an issue and move on.

[SupunS]

That's a good point! The two options I think we can take are:

• Like you've said, we can assume the problem/confusion is only with let fields, and assume users are more cautious of using var fields with access controls
• Or we can extend this to all var fields as well, by merging pub and pub(set) into one and say anything that is public is also settable. i.e: Removing pub and keeping only pub(set) (and maybe just call it pub for syntax convenience), which is essentially what Dete proposed I think.

[dsainati1]

I like the second option; pub(set) seems like a very weird distinction to make.

[SupunS]

Adding a Summary of the last meeting we had on 26/01/2023:

Questions:

• Is this a problem to be solved?
  • The outer access modifier should override the inner modifier (mutability)
  • Mutating via a reference is definitely a problem
• Would delegating functions (getters/setters) may just move the problem to a different place rather than solving it?
  • Try migrating a few existing contracts and evaluate
• Do we want to extend this to pub fields?
  • pub var is also in a way pub let for outsiders (and pub(set) var within the composite type itself). So what is applied for pub let should also apply to pub var - even if that means removal

Solutions:

During the meeting, it seemed there was clear preference towards the "removal/limiting of public let fields" compared to Adding static checks preventing mutations (i.e: the original proposal). The reason being the latter is more complex, resulting in a steeper learning curve for developers (especially newcomers), and reduced composability due to the added multiple restrictions.

Also, there were several variants of this solution was also proposed:

• Removing public let fields (this FLIP)

• Restrict let for only non-container/built-in types

• Restrict mixing var fields with let fields in the same hierarchy. e.g:

  pub struct Foo {
      pub let bar: Bar  // not valid: because `Bar` has settable fields
  }
  
  struct Bar {
      pub(set) var id: Int
  }

• Immutable boxing, similar to optional

  • Use a type instead of a keyword
  • only allow view functions

• Combination of:

  • Restrict creating references outside of their scope
  • Use view functions instead of immutable functions
  • Still doesn’t solve the [problem_1](https://www.notion.so/External-Mutability-49baf90d70094ab0a692c0bb0ebb6706).

Action Items:

• Try several solutions and see the impact on the existing contracts (@SupunS )

[SupunS]

So I've been applying some of the suggested solutions, mainly the first two (because other solutions only solve the problem partially):
(1) Removing public let fields, and (2) Restrict let for only non-container/built-in types. Below are some of the observations.

• Public container typed fields are heavily used to represent ‘data’. i.e: values returned by a contract.
  e.g: Some known contracts:
  • MetadataViews
  • Core contracts: FlowIDTableStaking, etc.
  • On the other hand, some contracts like TopShot have few such fields, but not a lot compared to the previous two.
• Exposing functionalities of these fields via delegation functions adds a huge amount of boilerplate code.
  • For e.g: MetadataViews.NFTView has six public container fields, and exposing all the nested fields and functions via delegation would be an overkill.
• Also, as a result, many users/developers would end up returning a reference to these inner fields, so that they can avoid the boilerplates. But by doing so, they give full mutable access to the fields via a reference. i.e: The problem is only moved to a different location, but hasn’t solved anything.

To me, it feels like if we go ahead with any of these solutions, we would end up making life harder for the majority (making the simple/common case difficult), and also may end up just moving the problem to a different place (e.g: returning a reference), or hiding/burying the problem inside the boilerplate code making it even far less obvious to tackle. Maybe we should just properly document the behavior and let it be (current documentation isn't quite accurate)?

I'm curious about what everyone else thinks about this.

[dsainati1]

To me, it feels like if we go ahead with any of these solutions, we would end up making life harder for the majority (making the simple case difficult), and also may end up just moving the problem to a different place (e.g: returning a reference), or hiding/burying the problem inside the boilerplate code making it even far less obvious to tackle. Maybe we should just properly document the behavior and let it be (current documentation isn't quite accurate)?

I've said this in internal meetings, but I should put this down in a public place as well: I agree with basically all of what you've said here. I think a partial solution here is really only going to push the problem elsewhere or paper over it without really solving the core of the issue. IMO we should either implement the full solution (with mutability tracking, read-only references, etc) or just fix the bug with reference-field mutation and document the existing behavior and move on.

[bluesign]

Public container typed fields are heavily used to represent ‘data’. i.e: values returned by a contract.
e.g: Some known contracts:

• MetadataViews
• Core contracts: FlowIDTableStaking, etc.
• On the other hand, some contracts like TopShot have few such fields, but not a lot compared to the previous two.

All those actually can be var instead of let. (when struct is used as function result, it shouldn't matter) Restrict let for only non-container/built-in types should solve this, am I wrong?

Actually we would retire pub(set) with this I guess, to make it more sense, so pub var would be mutable, pub let would be immutable. ( For new learner it seems like a easier distinction )

Returning reference has danger only for resource fields, but I think it is acceptable. You cannot return nested resource anyway.

[SupunS]

All those actually can be var instead of let. (when struct is used as function result, it shouldn't matter) Restrict let for only non-container/built-in types should solve this, am I wrong?

I agree it shouldn't be much of a problem for values returned from functions. However, I haven't been involved enough in writing smart contracts to have a strong say towards/against that. So I would trust you and everyone else who has more exposure to smart contracts use-cases than me (especially authors of these contracts) to have a better insight.

Actually we would retire pub(set) with this I guess, to make it more sense, so pub var would be mutable, pub let would be immutable. ( For new learner it seems like a easier distinction )

Yes, true.

[SupunS]

Returning reference has danger only for resource fields, but I think it is acceptable. You cannot return nested resource anyway.

Returning references is possible even now, so there won't be a change in that. My only concern is if people start returning references as a way to avoid boiler-plate code, then that would undo all the effort we put to avoid accidental mutations.

[bluesign]

Returning references is possible even now, so there won't be a change in that. My only concern is if people start returning references as a way to avoid boiler-plate code, then that would undo all the effort we put to avoid accidental mutations

can you give an example? I don't see a possibility like this at first look but I am missing some case probably.

is it something like:

pub contract NodeManager{

    priv var node: @NodeRecord

    pub resource NodeRecord {
        pub var tokensStaked: @FlowToken.Vault
        pub var tokensCommitted: @FlowToken.Vault
    }

    pub fun getNodeRecord(): &NodeRecord{
        return &self.node as & NodeRecord
    }
}

[SupunS]

This is a hypothetical example: Assuming all fields in FlowIDTableStaking.NodeRecord need to stay as priv, an example for the problem is: https://github.com/onflow/flow-core-contracts/blob/e05d6949bcf8006c53e3f2fd857f9b3adf3f60f1/contracts/FlowIDTableStaking.cdc#L257-L276

(EDIT: basically yes, same as what you wrote above)

[bluesign]

in this example , entitlements will save us ( as mutations will be via function ), but in my example case problem remains. But it feels like it is a bit bad design anyway.

[diegofornalha]

@SupunS Today I participated in the meeting, and I would like to know if the understanding is correct so far.

The discussion involved the proposal to remove support for public let fields and only allow private let fields (priv). The issue of mutations in pub var fields and the potential confusion between pub and pub(set) was also addressed.

Options considered included addressing the problem with let fields only, extending the solution to var fields, or combining pub and pub(set) into a single access modifier. In addition, concerns were raised about how the solution would affect existing contracts and the possibility of merely moving the problem instead of solving it.

During the meeting, there was a clear preference for removing/limiting public let fields rather than adding static checks to prevent mutations. However, when applying some of the suggested solutions, it was noted that many developers and users would end up returning references to internal fields to avoid writing additional code, which could provide full mutable access to the fields through a reference.

The discussion led to the conclusion that a partial solution might only shift the problem or mask it without actually addressing the core issue. Some participants suggested properly documenting the current behavior and moving forward, while others discussed the possibility of implementing a comprehensive solution with mutability tracking and read-only references.

[SupunS]

Thanks, @diegofornalha for joining the discussion! What you posted is a good summary of the overall discussion so far about this FLIP and the alternative FLIP #58.

In today's meeting, we mostly focused on the impact on existing contracts by the proposed changes (i.e pretty much continued this discussion), and also a possible third option of using Entitlements to restrict external mutability.

[SupunS]

Summary of the meeting on 20/04/2023

Restricting let fields to primitive types

• let fields with non-primitive types are commonly used
• Having to write delegators can be a pain (like in solidity)

Using entitlements to restrict external mutations

This proposal resolves around two core changes:

• Make member-access (field, index access) to return references always.
• Use entitlements to restrict access.

Example:

struct Foo {
    auth(M) var bar: Bar

    fun foo() {
       // Internal access
       self.bar.id       // succeeds
       self.bar.id = 4   // succeeds
    }
}

struct Bar {
    pub(set(AssignID)) var id: UInt64
    // strawman syntax: "allow setting if AssignID entitlement is given"
}

let foo = Foo()

// External access to reference (👍)
let fooRef = &foo as &Foo  // returns un-authorized/entitled `&Bar`
let barRef = fooRef.bar    // returns un-authorized/entitled `&Foo` (due to mapping)
barRef.id                  // succeeds, only needs `&Bar`
barRef.id = 5              // fails, would need `auth(AssignID) &Bar`

let fooEntitledRef = &foo as auth(AssignID) &Foo  // returns `auth(AssignID) &Foo`
let entitledBarRef = fooEntitledRef.bar  // returns `auth(AssignID) &Bar` (due to mapping)
fooEntitledRef.id       // succeeds, only needs `&Bar`
fooEntitledRef.id = 5   // succeeds, needs/has `auth(AssignID) &Bar`

// External access through owned (👎)
let barRef = foo.bar  // returns un-authorized/entitled `auth(owned) &Bar`
barRef.id             // succeeds, only needs `&Bar`
barRef.id = 5         // succeeds, needs `auth(AssignID) &Bar`

    // What about changes to bar field? 
    // e.g. what if bar field is resource, want to move

Notes:

• References are probably needed only for container (composite, array, dictionary) typed fields. Not for primitives, because references to primitives become unuseful.
• Member-access on LHS of an assignment would not be possible.
  • e.g: fooRef.bar = Bar(), would not be possible.
  • Author of Foo need to expose a delegator function for this.
• Should probably generalize field access to any nested access (e.g: for array/dictionary get, etc.)
  • Need support for iteration over references

Concerns:

• Would be hard to get a copy. May lead to implicit shared state. e.g: two branches updating the same nested field, assuming it's a copy.
• Would this make it even more complex to learn?

[turbolent]

We had breakout sessions, so far the sentiment is negative, and there has been no further feedback.

FLIPs #89 and #86 have been opened with proposals for alternative solutions that might better solve the same problems that this FLIP tried to address.