FLIP 262: Require matching access modifiers for interface implementation members

[turbolent]

FLIP #262

[SupunS]

+1. I'm very much in favour of this proposal

[austinkline]

My main concern is what this means for some things like HybridCustody which use access modifiers to allow hooks between different interfaces. For instance, when a parent account adds a child account capability, an access(contract) function is invoked to mark that the parent has redeemed it on the ChildAccount resource.

If we cannot use this form of access control, we have no way to only permit this type of callback within the internal mechanisms of a contract while also allowing said resources/interfaces to be composable, we'd have to instead use concrete types which means we're stuck on a specific implementation going forward

some example definitions:

• https://github.com/onflow/hybrid-custody/blob/main/contracts/HybridCustody.cdc#L99
• https://github.com/onflow/hybrid-custody/blob/main/contracts/HybridCustody.cdc#L184
• https://github.com/onflow/hybrid-custody/blob/main/contracts/HybridCustody.cdc#L229

and a sample use:
https://github.com/onflow/hybrid-custody/blob/main/contracts/HybridCustody.cdc#L310

Additionally, this would mean that the Burner contract defined to allow pre-burn methods to be defined (when sent to the Burner for destruction) would not be safe because anyone would be able to call the burn method and then store their resource again

[joshuahannan]

Yeah, after thinking about it some more, I don't know if I am in favor of this any more. I think if this would have come up a while ago, I would be in favor because we would have had plenty of time to figure out how to work around the restrictions, but like Austin said, this does limit the functionality that is possible in some contracts and in the Burner case, I don't think there is a way to get around it. With how close we are to Crescendo and that people have already started staging contracts on testnet, I don't know if the benefit of this upgrade is worth all the extra hassle that getting it added and adopted will entail, but I could be convinced if others are in favor of it or if anyone thinks of a way around the Burner issue!

[cody-evaluate]

this change would break this FLIP i believe because it relies on ownedNFTs being part of the interface (which is access(contract) to allow access to large collections with an iterator (forEachID) in a standard way. This FLIP also allows for default implementations for getLength() which will prevent some cases where a developer might implement it as ownedNFTs.keys.length, reading the entire array into memory and causing compute limit issues.

Since this FLIP would already be a breaking change then an acceptable replacement could be making forEachID required in the new standard since a default implementation will no longer be possible with ownedNFTs being removed from the interface.

Unless there is a direct replacement for this functionality, I am against this change.

@joshuahannan

onflow/flow-nft#211

[joshuahannan]

@cody-evaluate The only reason we made ownedNFTs access(contract) in that change was so that it wouldn't be a breaking change. This FLIP is a breaking change, so we can just change ownedNFTs to access(all) and the same functionality would be available to you

[cody-evaluate]

access(all)

couldnt

@cody-evaluate The only reason we made ownedNFTs access(contract) in that change was so that it wouldn't be a breaking change. This FLIP is a breaking change, so we can just change ownedNFTs to access(all) and the same functionality would be available to you

isnt there any security risks with that? i know withdraw() is gated with an entitlement but this would give you direct access to the store of NFTs because the developer would be forced to set ownedNFTs it to access(all)

@joshuahannan

[bjartek]

I am against this change. Yes it might be confusing but that can be documented. On the orher hand it removes some patterns that are useful.

[bluesign]

I am against too, I think this limits the usefulness of interface default implementations. Also patterns like Burner or Callbacks are really hard to replicate.

[joshuahannan]

@cody-evaluate

isnt there any security risks with that? i know withdraw() is gated with an entitlement but this would give you direct access to the store of NFTs because the developer would be forced to set ownedNFTs it to access(all)

It isn't a problem. I mistakenly thought it was originally which is why I was recommending access(contract), but I was wrong.

[cody-evaluate]

@cody-evaluate

isnt there any security risks with that? i know withdraw() is gated with an entitlement but this would give you direct access to the store of NFTs because the developer would be forced to set ownedNFTs it to access(all)

It isn't a problem. I mistakenly thought it was originally which is why I was recommending access(contract), but I was wrong.

assuming ownedNFTs was defined with access(all), like this:

access(all) resource interface Collection: Provider, Receiver, CollectionPublic, ViewResolver.ResolverCollection {

	access(all) var ownedNFTs: @{UInt64: {NonFungibleToken.NFT}}

	...
}

why would this not be possible?

transaction(ownerAddress: Address) {

	let myCollection: &{NonFungibleToken.Collection}
	let theirCollection: &{NonFungibleToken.Collection}

	prepare(signer: auth(Storage, Capabilities) &Account) {

		let account = getAccount(ownerAddress)

		self.myCollection = signer.capabilities.borrow<&{NonFungibleToken.Collection}>(/public/MomentCollection) ?? panic("no collection")
		self.theirCollection = account.capabilities.borrow<&{NonFungibleToken.Collection}>(/public/MomentCollection) ?? panic("no collection")
	}

	execute {

		self.theirCollection.ownedNFTs.forEachKey(fun (nftId: UInt64): Bool {

			let token <- self.theirCollection.ownedNFTs.remove(key: nftId) ?? panic("cannot withdraw")

			self.myCollection.deposit(token: <-token)

			return true
		})
	}
}

isnt that the reason the migration guide has this disclaimer?
https://cadence-lang.org/docs/cadence-migration-guide/nft-guide#change-the-access-specification-of-ownednfts-to-accesscontract

with this change it would force developers to allow this to happen because the interface would dictate access(all)

you would have to drop ownedNFTs from the interface completely and just make forEachID required and just hope developers implement it properly (same for getLength() since you cant have a default implementation anymore)

developers would still need to make sure ownedNFTs is access(contract) regardless otherwise you could force cast to the concrete type.

@joshuahannan

[joshuahannan]

@cody-evaluate

isnt that the reason the migration guide has this disclaimer?
https://cadence-lang.org/docs/cadence-migration-guide/nft-guide#change-the-access-specification-of-ownednfts-to-accesscontract

Thanks for pointing that out. I'm going to update that right now. That isn't true. anyone who wants to modify the ownedNFTs field has to have a Mutate entitlement to be able to do that, so what you suggest isn't possible

[cody-evaluate]

@cody-evaluate

isnt that the reason the migration guide has this disclaimer?
https://cadence-lang.org/docs/cadence-migration-guide/nft-guide#change-the-access-specification-of-ownednfts-to-accesscontract

Thanks for pointing that out. I'm going to update that right now. That isn't true. anyone who wants to modify the ownedNFTs field has to have a Mutate entitlement to be able to do that, so what you suggest isn't possible

oh then yeah that makes sense, wasnt even aware of the Mutate entitlement. 🫡

if ownedNFTs is access(all) then you dont really need forEachID in those cases but its still good to have so developers not using ownedNFTs have a way to expose their IDs in a standard way

@joshuahannan

[btspoony]

I am against it as well, It imposes a lot of limitations on the Interface.
Of course, in some cases, entitlement can be used instead.
But to be frank, the use of entitlement is not as intuitive as the original access (contract) access (account) in most cases.

[sisyphusSmiling]

Chiming in here to say I'm in agreement with others. While I agree the implications of inheriting access(contract) and/or access(account) methods are more nuanced than most other access modifiers, this is a useful pattern and removing it feels unnecessarily strict. At this point, the change would require more communal effort to implement than any perceived benefit IMHO.

[turbolent]

Thank you everyone for the great feedback, it's much appreciated!

@austinkline

If we cannot use this form of access control, we have no way to only permit this type of callback within the internal mechanisms of a contract [...]

I don't know much about the Hybrid Custody contract, so I'm trying to understand what is written in the contract, and the original intent.

What do you mean with the above? OwnedAccountPublic for example defines access(contract) fun setRedeemed. Who is supposed to be allowed to call this function?

Is it OK that code in another contract that contains an implementation of this interface can call the function? This would be the case if the implementation uses access(contract), like in the interface.

Is it OK that potentially any code can call this function? This would be the case if the implementation of the interface uses access(all).

[...] the Burner contract [...] would not be safe because anyone would be able to call the burn method and then store their resource again.

Do you mean if the proposal would be accepted, which means access(contract) could not be used in the Burnable interface?

Note that this already possible, in the current version of Cadence, even if this proposal does not get accepted:
Any implementation of Burnable needs to make their burnCallback at least access(contract) – but they may choose to use a more-permissive, and e.g. choose access(all).
Even if they choose access(contract), any code defined in the contract of the implementation may then call it.

If that is a problem, then the proposal does not make the situation not worse. It is true that it does not make the use-case work, but at least it removes the potential footgun caused by misunderstanding what a non-public/entitled access modifier means in an interface.

I'm fairly confident that we can still figure out a proper solution for contracts like Burner and achieve the intended access control, even with this proposal implemented.

[turbolent]

To help clarify the current behaviour, here is an example that demonstrates the usage of non-public/entitled access modifiers (access(contract) and access(account) in an interface defined in one contract/account, and multiple different implementations of the interface in another contract.

Let's assume contract C1 is deployed to account 0x1, and defines an interface S1.
It defines two functions, contractTest which is required to be at least access(contract), and accountTest which is required to be at least access(account):

access(all) 
contract C1 {

	access(all)
    struct interface SI {

		access(contract)
        fun contractTest()

        access(account)
        fun accountTest()
	}

    access(all)
    fun test(_ si: {SI}) {
        si.contractTest()
        si.accountTest()
    }
}

Note how C1.test can take any implementation of SI, and call both functions on it.

Let's also assume contract C2 is deployed to a different account, 0x2.
This contract defines two implementations that conforms to/implements the interface of the first contract, C1.SI.

As required by the interface, both implementations define the two functions, contractTest and contractTest.

While the first implementation S1 uses exactly the same access modifiers for the function as in the interface, the second implementation S2 chooses the more permissive access modifier access(all) (!).

import C1 from 0x1

access(all) 
contract C2 {

	access(all)
    struct S1: C1.SI {

		access(contract)
        fun contractTest() {}

        access(account)
        fun accountTest() {}
	}

    access(all)
    struct S2: C1.SI {
		access(all)
        fun contractTest() {}

        access(all)
        fun accountTest() {}
	}

    access(all)
    fun test() {
        S1().contractTest()
        S1().accountTest()
        S2().contractTest()
        S2().accountTest()
    }
}

Note how C2.test is able to call all functions of all implementations!
Note also how any code is able to call all functions of the implementation S2!

To be clear, this is the current behaviour, not what is being proposed in this proposal, i.e. this code is valid in the current version of Cadence (v0.42).

There isn't necessarily anything wrong here, and maybe the outcome of this proposal and discussion is just that this behaviour needs to be documented better.

However, answering questions around this, it seemed to me that some developers assumed that the above code would be invalid, or for example that only C1 could call the functions defined in SI. Both aren't the case!

If developers can make wrong assumptions about access control behaviour, that is potentially dangerous.
There's quite some potential here to make the wrong assumptions – I even had to look this up myself, because I was not sure!

Maybe I also just assumed misunderstanding, and this behaviour is actually understood well and totally acceptable.

Please let me know if you have any questions or concerns!

I'll likely add this to the FLIP itself.

[austinkline]

I don't know much about the Hybrid Custody contract, so I'm trying to understand what is written in the contract, and the original intent.

What do you mean with the above? OwnedAccountPublic for example defines access(contract) fun setRedeemed. Who is supposed to be allowed to call this function?

Is it OK that code in another contract that contains an implementation of this interface can call the function? This would be the case if the implementation uses access(contract), like in the interface.

Technically yes, but then you can combat that by understanding what type/implementation of a resource is doing what, and decide to ignore them (or only look for those that you know). If you were to do entitlements, I could just mark owners as I see fit without going to the hassle of implementing things. Risk vector is the same, but much easier to do with entitlements

The real reason we have these callbacks is for the happy path of child account/parent account interactions to automatically keep state in sync for you. Perhaps there is a way around that, but the main thing I am trying to demonstrate here is the value in allowing interfaces to communicate while not needing a concrete implementation. I really just don't see a way to do that with entitlements without opening up the access of a method significantly

Do you mean if the proposal would be accepted, which means access(contract) could not be used in the Burnable interface?

Note that this already possible, in the current version of Cadence, even if this proposal does not get accepted:
Any implementation of Burnable needs to make their burnCallback at least access(contract) – but they may choose to use a more-permissive, and e.g. choose access(all).
Even if they choose access(contract), any code defined in the contract of the implementation may then call it.

If that is a problem, then the proposal does not make the situation not worse. It is true that it does not make the use-case work, but at least it removes the potential footgun caused by misunderstanding what a non-public/entitled access modifier means in an interface.

At a high level, the Burner's job is to permit logic to be run before destruction of something. It might be true that a contract can call it themselves if it wants to, but at that point you are just harming yourself by calling it. It is generally my view that all code within a contract trusts itself. Maybe that gets a little stranger with things like Hybrid Custody which talk to interfaces versus concrete implementations, but that isn't very relevant for the Burner. If a contract wants to go around its own safeguards and do something it is of course welcome to do so, but that is the same as any other bug you might have in your smart contract.

I would argue this proposal does make the situation worse because anyone would be able to call that burn method now, not just the contract itself. Let's say there is a token that uses the burn callback method to change its total supply. I could simply take my vault out of storage and call that callback method as a holder of the token over and over to get whatever effect I want. That's quite different than the contract itself acting outside the boundaries of the typical burn lifecycle

---

At the end of the day, the argument I have here is not so much tied to individual implementations but to get across what they are making use of to achieve their objectives. Restricting the ability to call a method to only be from the interface definition and the actual implementation is very powerful. Entitlements just do not have a way to express that unfortunately

[bjartek]

Has it been considered that an implementing contract cannot change the modifier if functions in an interface?

Not sure what consequences that would bring though.

[bluesign]

Burner is not the issue here, it can be solved ( as it is passing resource ) It can delegate burning part to the contract. There are many options, can pass resource, can pass resource in envelope resource etc.

Callbacks are a bit tricky, only workaround is passing envelope ( a resource ) and to be honest it is a bit ugly. Create resource, send it to callback as parameter, and callback checks resource , if it is valid then destroys voucher resource and continues as valid callback.

for sure this looks like anti-pattern, but also this change can be helpful in the future for breaking interface-contract 100% trust relationship, so I am a bit in the middle.

[turbolent]

@bjartek

Has it been considered that an implementing contract cannot change the modifier if functions in an interface?

Do you mean the second part of the proposal, https://github.com/onflow/flips/pull/263/files#diff-7886c5900aee294558f3b08638f4322958c013fbe9a8cb429f8d8827f29752d6R51 ?

Yeah, we could definitely also consider a subset of the proposal, i.e. remove the first part of the proposal, forbidding the use of for non-public/entitled access modifiers, and only keep the second part of the proposal, requiring the access modifier in an implementation to be the same as in the interface. 👍

[dete]

Here's an attempt for me to frame the problem in a way that makes it easier for me to think about. Perhaps it'll be useful for others, and make discussion easier.

There are two axes for access control in Cadence 1.0. The obvious one is Entitlements, and – by design – Entitlements are restrictions on access control that are managed by the owner of the object. The owner effectively has all Entitlements, and when they provide a reference or Capability to the object to someone else, they can provide a subset of those Entitlements to limit access. (Note that "owner" can mean "a user that has the object in their storage", but also "code that has the object in an @ variable".)

However, there is another axis of access control, which is less obvious, and that is access(self), access(contract), and access(account). These restrictions are managed by the implementor of the object. This is critical, because even owners of objects need to have limits on what they can do, because those limitations define what the object is at a basic level. (Even an owner shouldn't directly modify the balance of a Vault, or the genes of their CryptoKitty; the implementor gets to decide those things alone.)

These "implementor restrictions" exist to make implementation easier. They allow implementors to structure their protected code into a convenient number of functions, objects, or even contracts within a single account, to match the design they find most effective, while ensuring that even the owner of those objects (who have access to all Entitlements) don't mess with the fundamental behaviour of the object as designed.

The problem that Bastian identified in this FLIP is that it's really not clear what functions, objects, and contracts we mean when we use one of these implementor restrictions on an interface. Is it the context where the interface was defined? Where the interface was implemented? Or both? And what if the implementor restrictions are expanded?

Here's an example I found useful to think through the problem:

Imagine a game with a "Player" resource type, and an "Equipment" interface. There will be lots of Equipment implementations, and I even want third-parties to be able to implement new Equipment types. Additionally, I want the Equipment to be able to update the Player object in limited ways (mediated by the {EquipmentUpdates} Entitlement).

When the user first equips their Player object with new Equipment, it's useful for the Equipment to react, possibly updating the Player object. So I want a function like this in the Equipment interface:

access(???) fun attached(toPlayer: &{Player{EquipmentUpdates}})

An "Armor" Equipment, for example, could use that callback to increase the Player's defence stat.

I want attached() to be called if and only if the Equipment is actually attached to the Player, otherwise it breaks the game (we can't let someone call attached() a million times with the same Armor object, pushing their defence into the stratosphere).

Entitlements don't save me; a user that owns a Player and one or more Equipments can call the attached() method as many times as they want because whatever Entitlement I use, the user/owner has access to it.

So, let's imagine we only allow access(all), as proposed by this FLIP. (A FLIP which made perfect sense to me before this discussion, BTW!) Can we solve this problem?

In the game example from above, I see two workarounds, neither of which is very satisfying:

1. A precondition on attached() (and other functions like it) that ensures that Player has the specific Equipment attached before the function can execute. It works in this case, and might work in other cases (I'm not sure if the HybridCustody contracts have state they could check for this, for example.) But the worst part about it is that it's a dynamic check, not a static one. It's always preferable to prevent mistakes before they happen...
2. The Player object could wrap the reference to itself in an object that has no public constructors. So, attached() wouldn't take an argument of type &{Player{EquipmentUpdates}}, it would take in a Resource (or reference to a Resource) called PlayerWrapper (or similar). No one but the defining contract would be able to call this function, because no one but the defining contract would be able to create a PlayerWrapper instance. This is obviously awkward, but it's also opaque. Imagine someone trying to learn Cadence encountering this pattern! It's not clear what it's doing and why you'd bother.

I don't love these workarounds, but they seem sound to me (in the sense that they work, not that they reflect good design principles!).

Leaving things as they stand also seems problematic. So I think we need something in the middle.

My current thinking is as follows:

1. Allowing access(contract) on Interfaces seems to have real value, and the workarounds required to provide the same restrictions without it seem awkward and confusing. I would propose that access(contract) on an Interface means "the contract where the interface is defined can call this method on all implementations of the interface, and the contract where the implementation exists can call this method on all instances of that particular implementation." (i.e. It would be an error for code in the contract that defines the Armor class to call attached() on a value with the Equipment type, but it could call attached() on a value with the Armor type.)
2. access(self) also seems potentially useful for interfaces, and could make default implementations even more powerful and interesting. I think it's straightforward to adapt the semantics I suggest above for access(contract) for this case, but I'm much less convinced this is necessary.
3. Allowing implementors to expand the access modifiers on Interface methods seems problematic to me, but maybe I haven't thought about this enough. At the very least, a wrapper function seems like an easy workaround if we don't support expansion, and this workaround seems more intuitive to new Cadence devs than the problems that could arise from allowing access expansion. My preference is requiring the implementation access modifiers to exactly match the Interface, but I don't feel like I have enough context to be certain.
4. access(account) always feels dangerous to me, but I can see situations where it makes sense. In the case of an Interface method, tho, it feels positively outlandish. I'm inclined to say we should disallow access(account) in interfaces. Again, I'm not that confident here.

Obviously the question of what we should do depends a lot on implementation effort (both by the Cadence team, and any contract devs who have to react to those Cadence changes).

[turbolent]

Thanks for your feedback @dete!

1. Allowing access(contract) on Interfaces seems to have real value, [...]. I would propose that access(contract) on an Interface means "the contract where the interface is defined can call this method on all implementations of the interface, and the contract where the implementation exists can call this method on all instances of that particular implementation."

This is already the current behaviour.

Allowing implementors to expand the access modifiers on Interface methods seems problematic to me, [...]
My preference is requiring the implementation access modifiers to exactly match the Interface, but I don't feel like I have enough context to be certain.

That is basically the second part of the proposal.

I'm inclined to say we should disallow access(account) in interfaces.

We could adjust the proposal to only forbid access(self) and access(account) for now, and keep allowing access(contract).

[turbolent]

We had a short chat about this proposal in the Cadence 1.0 office hours today, and mainly discussed revising the proposal to just the second part, requiring access modifiers in implementations to match those of the interface.

Sentiment was positive, so I'm going to make the appropriate changes to the title and contents of the FLIP and we can further discuss and maybe vote on the proposal in the working group call tomorrow.

[turbolent]

Please take another look @SupunS @austinkline @joshuahannan @bjartek @bluesign @cody-evaluate @btspoony @dete @sisyphusSmiling

[btspoony]

Now it is more reasonable and will not cause too much impact!

[joshuahannan]

This sounds good to me

[sisyphusSmiling]

Great conversation thread! I'm in agreement with the updated FLIP.

[bjartek]

LGTM

[austinkline]

With the FLIP scope now changed to only be about enforcing access modifiers matching across interface and implementations, I am in favor of this as well

[turbolent]

In today's working group call we had another round of discussion about this FLIP.

We discussed the two questions raised by @dete:

• Should access(account) be forbidden?

  • Determine impact by scanning Mainnet contracts
  • Probably too late to make breaking change like that, \decision would have been easier a while ago

• Should acccess(self) be allowed?

  • Useful for default functions
  • Like in Java
  • Already forbidden in current version
  • Can be added later, after Cadence 1.0 is released, not a breaking change

Unless there are any further concerns, the FLIP is planned to be accepted by Tuesday next week.

[bluesign]

Should access(account) be forbidden?

it is at least as common as access(contract) in interfaces

[turbolent]

In the Cadence repo we have a tool to parse Cadence code. It supports parsing all contracts in a CSV file, and allows querying the AST with JQ.

I did a quick analysis of a recent dump of all Mainnet contracts (find all interface declarations, find all members in them, and show the identifiers of the ones where the access is access(contract)):

parse -readCSV -jqAST '.. | objects | select(.Type == "InterfaceDeclaration") | .Members | .Declarations | select (. != null) | add | select(.Access == "AccessContract") | .Identifier.Identifier'

The usage of access(account) in interfaces is actually higher than that of access(contract).