FLIP 242: Improve Public Capability Acquisition

[austinkline]

No description provided.

[austinkline]

Related discussion

One of the big missing pieces here when we were originally discussing this problem was about what to set the ID of a capability if it is not valid. Coming out of today's discussion in discord, it looks like setting id to 0 is the way to go, and also helps solve another problem which would be that capabilities which are not successfully migrated would crash/panic if their id was accessed. It sounds like we might have an opportunity to tackle both problems here with this zero-value ID approach

[bluesign]

I am adding alternative approach I suggested on discord to here also:

We can also migrate public capabilities ( pointing to a public path ) even also change behaviour a bit, and add optional PublicPath to capability. Then public capabilities ( capabilities with public path set ) can go over Path->Capability Controller, instead of ID->Capability. This way we can ensure if I change a capability in my public path, all existing ones will point to that too. ( For example changing my FlowToken Receiver to FungibleToken Switchboard )

I think this also solves the optional Capability problem, and helps with migration problems.

[turbolent]

Nice!

Though I felt like having an optional returning type adequate, I see how having the address is useful.

I also understand the sentiment in regards to the effect of the previous change of changing the function from non-optional to optional, which results in additional code being necessary: like before, once the capability was obtained, it still needs to be checked/borrowed, but now also the "state of publication" (was a capability published under the requested path or not) must be handled with additional code (check for nil / unwrap).

Given how frequently the capability API is used, basically in all transactions, ergonomics are important.
I like the simplicity of the change, the required effort is commensurate to the improvements in ergonomics.

[austinkline]

Thanks for the suggestions @turbolent! Accepted them all.

@bluesign I see what you mean, but wonder what kinds of issues might come up with some capabilities being able to be hot-swapped, and some not. Would that difference in behavior be a problem?

[bluesign]

@bluesign I see what you mean, but wonder what kinds of issues might come up with some capabilities being able to be hot-swapped, and some not. Would that difference in behavior be a problem?

I am not sure, but I think currently all is hot swappable anyway. ( you can change the object in storage )

[austinkline]

@bluesign I see what you mean, but wonder what kinds of issues might come up with some capabilities being able to be hot-swapped, and some not. Would that difference in behavior be a problem?

I am not sure, but I think currently all is hot swappable anyway. ( you can change the object in storage )

A great point for sure, hadn't thought about that

[bjartek]

I agree with the changes proposed in this FLIP.

[joshuahannan]

Looks like a good change to me!

[bluesign]

I think this is very good change and very much needed

[sisyphusSmiling]

I think this would be a great update to the 1.0 Capability retrieval interface!

[joshuahannan]

I looked through most of the contracts and transactions in our repos and it looks like most of them won't be affected by this change, so that means it is probably pretty likely that most of the staged contracts on testnet won't be broken if we introduce this either.

This does affect many transactions though because a lot of transactions use the pattern of getting the capability unwrapping it with the nil-coalescing operator, and borrowing a reference. This is fine though since they are just transactions. Only two of the ledger transactions are affected, because most of them use ccount.capabilities.borrow. This won't affect account.capabilities.borrow, right?

So it looks good from my perspective

[turbolent]

@joshuahannan Nice, thanks for looking into usage!

This won't affect account.capabilities.borrow, right?

Correct, this won't affect borrow, only get – borrow has always been optional, and will stay so.

[jacob-tucker]

Just wanted to say I also support this, it is a very nice thing to keep. I always have empty capabilities because having address is quite useful!

[dsainati1]

As a disclaimer: I'm requesting changes here not because I disagree with this proposed change, but rather because as I have started on a draft implementation of it, there are a couple parts that are underspecified and need to be further fleshed out before we can consider this finished.

In particular, the FLIP does not specify the behavior of Account.Capabilities.get when a Capability is published at the specified path, but the type argument provided to get is not a supertype of the published Capability's actual type. I.e. if a Capability is published with type &Vault, but the type argument to get is auth(Withdraw) &Vault, what should Cadence do here? Clearly it must also return an "invalid" capability in this case, but what the runtime type of that capability is is not specified.

Even though that invalid capability will always return the nil value when borrowed, its runtime borrow type is still relevant because until borrow or checked, the invalid capability value behaves "normally", i.e. is still usable as a value in programs. E.g., if we attempt to cast the capability obtained above to one of type auth(Withdraw) &Vault, will the runtime cast succeed or not?

I see two options here. Given a case where a value of type &X is published at a path but attempted to be borrowed with type &Y where Y is not a supertype of X, we can either:

• Create a capability whose static and runtime borrow type is both &Y
• Create a capability with static type &Y but runtime borrow type &Never

Once we have an answer to this question and the FLIP is updated to specify, I will feel comfortable approving this.

[turbolent]

@dsainati1 Good point/question!

In the case where the requested path has a capability published to it, but the requested type does not "match" the capability's type, instead of returning nil, the function returns an invalid capability (ID = 0) with the requested type (like in the current version of Cadence (v0.42)).

I see two options here. Given a case where a value of type &X is published at a path but attempted to be borrowed with type &Y where Y is not a supertype of X, we can either:

• Create a capability whose static and runtime borrow type is both &Y
• Create a capability with static type &Y but runtime borrow type &Never

What is the difference between the two? Does the latter have any benefit?

[dsainati1]

What is the difference between the two?

The main behavioral difference is that in the former case, an invalid capability created from a call to get<&Y> will succeed on a runtime cast to Capability<&Y>, while in the latter case such a cast would fail.

Does the latter have any benefit?

I don't really know to be honest, but I also felt originally that having get return an optional was a UX improvement, so I've accepted that I don't have a perfect grasp on what developers want from this API in the first place.

[dsainati1]

To give maybe a more useful example: if you have some array of type [Capability] that you got through any various means, and you would like to filter this array for any Capability values that are borrowable as NonFungibleTokens, one way you might do this is by iterating over the array and doing an optional cast cap as? Capability<&{NonFungibleToken}>, and storing the successful results in a new array.

Would we like invalid capabilities to pass this cast (and be included in the filtered array) or fail (and be excluded)?

[bluesign]

Would we like invalid capabilities to pass this cast (and be included in the filtered array) or fail (and be excluded)?

I think passing this cast not failing is better option. In any case, there is no guarantee that capability will check/borrow, so filtering has no added benefit here.

Also failing case would be strange:

var cap: Capability<&{NonFungibleToken}> = capabilities.get<&{NonFungibleToken}>(....)
// cap != nil
var c:Capability = cap 
var x = c as? Capability<&{NonFungibleToken}>
// x = nil

[turbolent]

@dsainati1 Please feel free to suggest additions to the FLIP contents to clarify the proposed design

[turbolent]

@austinkline Could you please accept the suggestions? Once updated, PTAL everyone

[turbolent]

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

• @dsainati1 has now all input needed for implementation
• The FLIP is now up-to-date and covers all cases

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

[turbolent]

@austinkline The FLIP has been approved 🎉 Could you please merge the status update change? Once the status got updated, we can merge