-
Notifications
You must be signed in to change notification settings - Fork 164
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.
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
Extensibility flip #1101
Extensibility flip #1101
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…destroy and remove
I am not sure of benefit vs cost here, considering what we allow is very limited. Technically, we seem to be wrapping object via extension ( please correct me if I am wrong ) |
Are you saying you don't think this is worth doing? |
@dsainati1 I mean this opens up too many edge cases and complicated problems. Not sure if they can be covered easily.
for example if you make an extension, then later some time composite adds a method named ‘foo’ , extension is broken (even extension owner didn’t change anything) Another problems can be order of extensions, removing just one extension, requiring extension to use in another extension etc. All those brings many edge cases, then we will hit the case of some resources don’t want extensions to extend them (e.g. TopShot) which don’t allow derivative works. I love the concept of extensions, but seems like is really complicated in the current state to implement them. |
This is a fair concern, but also one that exists already in the system. If your composite conforms to an interface that you import from another contract, if the interface owner adds a method that can break your composite even though you didn't do anything. This is simply a fact of life when writing code that depends on someone else's code, I think. The right solution here would be to add a feature like contract versioning that would solve this problem generally, rather than limiting the language features we provide to users.
Yea, I think to make this tractable extensions will have to be order sensitive; you can only "push" an extension to the top of the "stack" or "pop" the most recently added one off. This simplifies behavior and also sets us up easily to later add the ability to have extensions require others or to allow extended types to be further extended.
I was under the impression adding signatures to TopShot moments was one of the primary use cases for this feature. Am I wrong here? Extended versions of types would not be the same type as the original, so an extension of a TopShot moment would be no derivative of a TopShot moment than a contract that functionally duplicates the logic. I certainly agree that this is a complex feature and will need careful consideration during design and implementation, but that isn't a reason not to do it; this would significantly improve the life of contract developers and enable an enormous number of new contracts and entire new paradigms. |
Btw my initial comment was not to dismiss, I am fan of this idea from the beginning, just somehow I feel it needs to be more powerful in my opinion, if we limit the extensions capabilities too much, it will be simple wrapper.
I mean as an example I saw that a lot, but they are too protective on the IP, so I am not so sure.
I totally agree.
I think here is situation a bit reversed, in my imagination, here we have NFT has some extensions and NFT is hosting them somehow. So maybe isolating them from each other and NFT can be nice. I mean something like nested resources:
x can be accessed like I think most powerful usage of extensions will be to extend the native functionality ( by hooking some functions ) So somehow extension should be able to override the existing methods. |
This is an interesting idea, which would completely change the fundamental behavior of extensions as outlined in this particular proposal. At the moment the extension are designed such that an extended version of a type is still a valid instance of the original type; if you add a hat to a CryptoKitty, your Kitty still functions as a regular Kitty for any and all applications that use Kitties that don't know about hats. Similarly, if you attach a signature to your TopShot moments, your moments still function as moments for all contracts that interact with regular moments, but also can be used in places where a signature is expected as well. Were we to allow extensions to override existing methods, this property would no longer hold; an extended Kitty could no longer be trusted to function anything like a Kitty at all. I'm interested in the idea, but I'm curious what powerful usage this would enable that is worth sacrificing this property. |
Yeah this works with isolation of the extensions mostly, standalone it is problematic. Let's say you have Kitty. And someone developed some KittyHat Extension. Technically you will have something like this : var kitty <- Kitties.GiveMeKitty()
var hat <- KittyHats.GiveMeHat()
kitty.extend(<-hat)
// kitty will be type of CryptoKitty
// kitty.kittyHat will be type of CryptoKitty+KittyHats Here kitty.kittyHat for example can have different MetadataView. ( Putting a hat on kitty item possibly :) ) Or if we have Main issue is we relied on functions too much lately ( with metadata, new NFT standard etc ) extending without extending the functions will be little problematic. I am more thinking like |
ah I was answering to @siddthesquid's example. ( wrapper kind of ) In my case, I don't think extension should be a specific type, it should be managed with hasExtension instead of isInstance |
@dsainati1 I completely overlooked that the extending resources have
@bluesign It would have been a subtype of whatever it was wrapping. So I was thinking it could also just be Base.Public (and not even Base.Thing). Honestly, a lot of my logic goes out the window when trying to think about how updating contracts affects all this as well, so I haven't thought about those concerns at all One thing I'm curious of is that if multiple "Extension developers" make an NFT extension with overlapping functions, would a resource be allowed to extend both of those? Also, if a transaction wants to use the extended functions of an extended resource, does the "Extension" contract have be imported explicitly? Btw I like this idea - I'm currently trying to do a makeshift version of the dictionary approach for my current contract (i haven't finished so don't know if it'll work, but this would have been easier with extensions) |
Default interface conditions are not quite the same thing as full method overriding. Interfaces are "abstract" in the sense that you cannot instantiate them; if you have some interface However, if you have some composite |
@dsainati1 I am considering you reference:
But this is nothing bad, even we encourage this usage no ? We want things to be composable. But this "makes it difficult to determine which code is invoked." ( which is the reason of we don't allow "overloading or dispatch". Btw I don't defend extensions to override methods, I think extensions should be separate type, where methods fallback on to parent. If I have kittyItem, kittyItem+hat (extension) will not be subtype of kittyItem. but if you take extension reference: |
Yes; if two different developers wanted to make a Hat extension for CryptoKitties, you could have a kitty that has both hats on it at the same time. You would just not be able to reference both extensions at the same time if they declare incompatible/overlapping fields or functions.
Yes, in order for these functions to be available the type of the extension would need to be known, meaning the Extension contract would need to be imported, I believe.
The difference is whether the behavior is expected or surprising. Interfaces being implemented is expected by users, while composites being overridden is not expected, so if it were possible it would be surprising to users and hence potentially dangerous. That said it seems like we are in agreement that extensions would not be able to implicitly override base type methods; it seems like you are suggesting that you would need to explicitly call the method on the extension itself in order to get the fallback behavior, which seems fine. |
“implemented is expected by users” is tricky concept though. If we say “user” is who is reading the code, in practice there is no difference. I dont think people capable of reading Cadence is big enough to have a concern of this. |
To summarize the discussion here for those following along, currently the largest point of debate is over whether or not extending a resource should change its type. The two models we have are:
|
Very nice summary @dsainati1, I have few minor things to add for comparison. Static modelIn this model: We cannot extend (modify) methods, but we can add new ones. Actually this is the initial point where my concern was started.
There is a very big chance of conflicting extensions, in my opinion if 2 extensions implement an interface the base type doesn't implement, they would not be compatible. pub struct interface I {
pub fun foo(): Int
pub fun bar(): String
}
resource R {
fun foo() {}
}
extension E1 for R: I {
fun bar() {}
}
extension E2 for R: I {
fun bar() {}
} Also there is a breaking situation when R decides to implement I in the future. I think we need to clear up how resources will behave in that situation. Preventing to apply extension is ok but what will happen to resources already out there with extension applied? if Btw I love the static type guarantees and Also some positive feedback:
pub resource Collection{
}
access(contract) extension CollectionNFTPublic for Collection: ExampleNFTCollectionPublic, NonFungibleToken.CollectionPublic{
}
access(contract) extension CollectionNFTProvider for Collection: NonFungibleToken.Provider{
}
access(contract) extension CollectionNFTReceiver for Collection: NonFungibleToken.Receiver{
}
access(contract) extension CollectionMetadata for Collection: MetadataViews.ResolverCollection{
} This gives ability to NFT creator to upgrade NFT or CollectionMetadata with micro-transactions. Upgrade your NFT to support VR for X FLOW, or remove royalties for X FLOW etc. |
Updating a resource with a new method is fine, as code is not saved; existing extended resources will just become invalid statically, and extending
Basically what this means is that internally the |
This is good, but how we will manage to handle this I am confused ( my lack of knowledge on internals of Cadence probably ) But Imagine we have static type: Kitty+Hat, if I put this to one resource:
How to handle this situation ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. my only concern was that people could infect e.g. Vault with a backdoor but that requires privileged access which extensions lack
There was a problem hiding this comment.
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 being able to statically enforce the type that is extending / on which type the extension can be added.
Maybe add a section to explain a bit about how casting/assignability works (or is it already there? didn't come across any).
for e.g: Say I do let v = extend S() with E()
, then:
- Can I assign
v
to aS
typed var, or do I have to explicitly upcast? - After assigning/upcasting to
S
, can I down cast it back toE
?
that if `E1` and `E2` declare any fields or methods with the same name, this extension will fail statically, as this would result | ||
in conflicts between the two extended types. | ||
|
||
At runtime, fields and methods are namespaced, so if `E1` and `E2` both declare a field named `foo`, so while a type `S with E1 and E2` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps separate out this section which explains how the name collisions are handled (just adding a topic should be sufficient). This is a vital part of the FLIP, and deserves a separate section.
btw, this is a nice solution to prevent name collisions 👌
|
||
```cadence | ||
let r <- create R() | ||
let e <- create E() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer not allowing to create extensions in contexts other than when adding to a resource/struct.
i.e: forbid let e <- create E()
or let e = E()
but only allow:
extend <- r with E()
for resources.- Or even better
extend r with E()
. Herer
doesn't need a move op; treat it the same way as being in a member-access. The resulting value after adding the extension is a resource, and it would require a move. e.g:let x <- extend r with E()
- Or even better
extend s with E()
for structs
I'm emphasizing not to treat E()
as a value constructor, but rather as an extension init, which can be only used along with with
keyword. This would:
- Eliminate the feel of
E
being just a wrapper. - Avoid having to introduce a new runtime type for 'extensions'. i.e: type of the result of
E()
. Plus, don't have to worry about whether to use assignment vs move for the extensions (e.g: why move operator for extension? is it a resource value?) - Also helps to simplify the syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allowing extensions to be created in any context, independently of the value they are being added to, is designed to support a specific use case: having extensions themselves be resources that can be traded like any other NFT. Consider, for example, if I have a Hat item that i want to add to my Kitty. The Hat is an extension for Kitties that can be bought and sold independently of the Kitty itself, and can be removed from one Kitty and added to another. It makes sense for this use case that the Hat can be created and initialized independently of any Kitty that it may be attached to in the future.
We had a public discussion about this on Tuesday (meeting notes for this can be found here: https://github.com/onflow/cadence/blob/master/meetings/2022-09-20-Extensions.md). Thanks to everyone who attended! Based on the discussion in that meeting, I am working on an alternative FLIP to this one proposing an "attachments" feature that is designed to solve the same problem, but taking into account the meeting feedback. This alternative would function like an additional field added to resources that would contain a list of all the "attachments" present on that resource, which each have their own functionality and fields, as well as a reference back to the resource that contains them. The new proposal will contain much of the DNA of this old one, but lacking some features and including some others to better address the identified use cases. However, I was curious about which parts of this proposal we'd like to see in the new one. It seems to me, based on our discussion, that the primary flaw of this one is that adding an extension to a resource creates a new resource entirely, with the methods and fields of both. This has the potential to create issues with name collisions between the original type and the extension, especially as contracts are changed and updated. In the new proposal, fields and methods of attachments will be accessed explicitly through the attachment, rather than implicitly on the composite object. This will allow attachments to name members without concern for what the base or other attachments may have called their members. It also seems that there is little desire for the ability to remove and re-attach extensions (indeed, as @dete pointed out there is no actual need for this, as "extensions with value" can be achieved by having the extension be a manager for resources rather than a resource in-and-of-itself). This feature will not be present in the new proposal. As for the parts of this proposal that are worth keeping, in my opinion the two primary benefits of this proposal are the dedicated syntax, which makes it very clear and precise what function is being performed by the creation/removal/declaration of an extension, and the static typing. In particular, the attachment model still seems to be compatible with the The benefits of statically typing this are primarily for safety; if there is no static type that expresses the attachments present on a resource, then users have no way to write code that restricts statically what attachments they want resources to have. So, for example, if you receive a resource, if you can't write any static requirements about that resource's attachments, so in order to be sure that it doesn't have any attachments you don't want, users would need to write defensive code every time they receive resources from external code that strips off any undesired attachments from the incoming resources. Similarly, if a user wants to write a function that uses a resource
which is a lot of defensive boilerplate if users need to do this at the top of every function intended to interact with extensions. I am curious to get thoughts from those who weighed in on this proposal about whether this seems like an accurate summary of which features of this one we'd like to see on the next. |
I want to elaborate a little further on the difficulty I mentioned in the language design meeting with the Metadata use case. The core difficulty here is designing an API that allows a resource to iterate over its extensions/attachments without requiring it to know anything about those extensions/attachments in advance. In particular, we'd like for an NFT's implementation of the metadata standard to include any metadata on those attachments. For a simplified example, let's imagine that implementing the metadata standard just involved having a This would require a reference to all these extensions/attachments, either an array type There are a couple options here, as I see it:
It is unclear to me what the best path forward is here. All of these have pros and cons. |
onflow/flips#11 is a new FLIP proposing an alternative to this one: In the interest in keeping discussion on both these FLIPs in a single place, please direct future discussion on these to https://forum.onflow.org/t/flip-cadence-extensions-attachments/3645 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I think the proposal covers all edge cases
the extension is destroyed. Like `init`, because `destroy` may be run when the extension is not attached to a base type, it may not reference | ||
any fields or methods of its base type, and should simply destroy any resources declared on the extension itself. | ||
|
||
Extensions may also declare two special methods: `attach() { ... }` and `remove() { ... }` that are not considered conflicting when attaching |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these/should these be special functions like init
and destroy
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was the intention, yes.
The extension type itself also can be referenced with just `E`, but no operations can be performed on a value of type `E` other than to | ||
move it around or to attach it to a value using `extend` syntax (see below). That is to say, a method or field defined in the declaration of `E` | ||
exists on values of type `@R with @E`, not on values of type `E`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that function calls on just E
will be statically rejected? In the case where dynamic dispatch is involved, e.g. an instance of E
is assigned to a variable of type {I}
, we will also need to dynamically reject the direct function calls.
Maybe add that to the proposal, if it is the planned behaviour. If not, how else could we handle this?
This limitation will also be a bit restrictive. For example, given that an extension could be an NFT, it would be impossible to get metadata of it in the detached state.
Maybe extensions can just be normal type declarations, which allows them to be used in the detached state, and then an "extension declaration" can be added, which defines how the type interacts with the extended type. For example:
struct S {}
struct E {
// usable in detached state
fun foo() { ... }
}
extension E for S {
// only usable in attached state
fun bar() { ... }
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an instance of E is assigned to a variable of type {I}
At the moment this isn't a concern because extensions cannot implement interfaces in their detached state, so there would never be any dynamic dispatch to worry about.
However, this is also unlikely to be a relevant concern for much longer; based on the discussion in the last meeting about extensibility, I think the sentiment is that extensions should not themselves be resources with value, and should not be usable independently of the thing they are attached to. I.e. removing an extension should also destroy it. Instead the extension would be a "manager" for the valuable resource, perhaps through the indirection of a reference or capability.
This has been open for a while with no further comments, and based on the discussion here (https://forum.onflow.org/t/flip-cadence-extensions-attachments/3645), it seems fairly settled that Attachments is the preferred approach for this problem. If nobody disagrees, I am going to close and reject this FLIP in a day or two. |
I have updated this FLIP to be Rejected, as we have elected to go with the attachments proposal instead. Thank you everyone for your input on this! |
Adds FLIP proposing adding Extensions to Cadence. See discussion in https://forum.onflow.org/t/extensibility/622 and onflow/cadence#357
For contributor use:
master
branchFiles changed
in the Github PR explorer