-
Notifications
You must be signed in to change notification settings - Fork 163
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
FLIP: Capability Controllers #798
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/onflow/flow-docs/J5dNbSkiW9GF1KvNN9tup7dnp3ZT |
I am super excited about this, thanks for this FLIP @janezpodhostnik Few comments and ideas I have.
Technically AuthAccount.capabilities can have different type like
Something like this as a result: Issue:
This will create Use:
For Private:
What do you think ? |
@bluesign thank you for the comments!
That is true, but the difference in this scenario is that if:
I like this, but should probably be an optional. Not sure it is a good idea to force descriptions.
This might be easier to understand! What I'm thinking though, would this make it easier for people to make mistakes (by accidentally giving people the access to &AuthCapability). I have to think through this.
I like your example, but I have a problem with it. The public domain paths sort of serve as a name... so when you |
I think this is a great improvement. To me handling capabilities always felt complex and abstracting that logic with a nice API would be awesome. |
hey @janezpodhostnik Thanks for the answers, sorry I missed the answers when you posted, thanks to @sideninja I just saw. Resource vs Struct discussion, I think we have just opposite perspective, which is very nice, I think both sides have cons/pros. My main perspective here was, giving a resource, creating a false sense of security and uniqueness. Resources have inherited "unique" from their definition, which can give more assurances than it should be. But I am ok with resources, maybe just would need to document.
The problem here is in my opinion: Paths are more error prone. Actually this is more of a business logic problem than technical. Imagine this: if I am a wallet developer, user has 2 FlowVaults, what should wallet do ?
Now for me obvious better solution is the second one, but I feel Flow pushing people to use first one. |
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.
Great start. I left a bunch of comments, mostly grammar, formatting, but a few suggestions.
For the other discussions above, I feel better about them usually being resources, because while it is possible for people to just get around it, it makes it a lot more difficult than if it were a struct. I could potentially support allowing users to specify if they want to create a non-resource capability in the public domain to make it optional.
I also fully support removing private paths, but I am a little undecided on what we should do about the paths vs types point that @bluesign made. paths are definitely error prone, but @janezpodhostnik 's point about not knowing which vault to use is a good one.
Also, an array of CapCons could potentially get expensive if you need to iterate to find one. Any reason it can’t be a dictionary?
Could still iterate like with an array, but could also index.
@bluesign just made me aware of this. Will this not break a lot of code. Today I store Capabilities inside structs very often and this change will make this impossible. Since a Capability is a resource and I cannot store a reference to it. Or am I missing something obivious here? |
It will break a lot of code in theory. We're figuring out a proposal first that we can all agree would be useful, then we'll discuss more about the feasibility of upgrades and migrations to address the breaking changes. We still have a lot of control over upgrades and migrations, so it might not be as terrible as it sounds. Still needs to be discussed a lot though |
Right now there is no event emitted when storage is stored/loaded or when a link/unlink operation happends. It would be very powerful if that is the case. One constant problem that is an edge case you need to handle in cadence is the posibility for blackhat to craft his own transaction and unlink something. If you do not think about this beforehand then your entire system can grind down to a halt. Now having the posibility to listen to those events to see when it happends will atleast make the situation a little better. Also knowing "who has set up a collection of the given type" would also be possible if there was events emitted. |
@bjartek it would be nice, I suggested that before ( onflow/cadence#1059 ) But I think there are some technical limitations preventing that to be implemented. Events per block, events per transaction etc. TPS vs usability trade-offs |
This FLIP introducing new capability functions may be a good spot to add support for run-time types as part of capability functions: onflow/cadence#1617 This is to allow for the following to work: let publicAccount = getAccount(issuerAddress)
let runTimeType: Type = Type<&{NonFungibleToken.CollectionPublic}>()
let countRef = publicAccount.borrowCapability<runTimeType>(/public/hasCount)! // or publicAccount.borrowCapability(/public/hasCount, type: runtimeType)
countRef.count Is this something that would be feasible to include here? |
Borrowing using a run-time type will not be possible, as the type checker needs to be able to statically know what the result of the function is going to be. However, a function to create a capability using a run-time type could be added, as the type does not need to be statically known, just at run-time. |
Co-authored-by: Joshua Hannan <joshua.hannan@dapperlabs.com>
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@janezpodhostnik Could you please provide a summary of the current discussion, i.e. what the current sentiment is, and if and what any outstanding questions are? |
Hi @janezpodhostnik, @turbolent asked me to comment on this FLIP. I’ll share some thoughts, but I’m sure I’m missing some nuances due to relative unfamiliarity with Cadence, so please feel free to correct my misconceptions! First, this is a wonderfully clear document. The example code with private and public sharing of capabilities was especially helpful. Second, I tried to summarize what I saw as the problems and pain points below. Is there anything I’m getting wrong or missing? I'll follow up with another comment. Problems
|
My comment started to get too long, so I'm dividing it up. :) Cutting off access to capabilitiesThere’s some prior literature in capability-based security on cutting off access to capabilities because of misuse:
I’ll attempt to summarize here, but one helpful distinction from the paper is proactive control vs reactive control:
Capabilities support proactive control well, since access and damage can be limited because (when done well) authority is already limited to the least amount needed to carry out the request. But, capabilities in their basic form don’t provide any information about who is to blame for misusing access, which is required for reactive control. To add reactive control to capability-based security, the paper suggests an approach called HORTON. Here’s a simplified explanation of HORTON, using the same scenario as in my previous comment, where an issuer creates a capability for Person A, and Person B asks Person A for access:
I’ll follow up in later comments to connect this to CapCons, but I wanted to post this separately in case it triggered any ideas itself. |
I was assuming some code would need to be run when the target is set, so I opted for a function. But I have no strong opinion about it. It can be a field if that fits better |
…pabilities.forEachController
I am in favor of this in general, however I think that instead of public fields, we should follow styles defined in contract standards (such as FT, NFT, Metadata etc ) and use getters and setters. Doing so would bring some benefits in terms of consistency. |
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'm fine with the rollout plan
I've updated the FLIP by documenting the new migration plan, adding tags to controllers (this was a feature which was requested in breakout sessions), and improved some wording. |
While reviewing the implementation of this proposal, the question of how mutations are handled/supported during iteration using the There are several options for the behaviour:
|
Between the three options presented here, my preference is for the third. The first option is untenable IMO, we elected not to use this behavior for all of our previous iterator implementations (storage, attachments), and it would be problematic here for the same reasons as there. Between the latter two, I think given your point that the snapshotting functionality would be equivalent to calling |
I agree with @dsainati1 here, we already have |
@dsainati1 @bluesign Thank you for the feedback! Agreed with that reasoning, I modified the behaviour of the implementation and will update the FLIP to reflect it |
Given that there has been no further feedback or concerns, this FLIP has been accepted 🎉 Thank you everyone who participated in the FLIP review process for the great discussions and valuable feedback! Your input has helped make this proposal a solid improvement over the existing linking-based capability API. The implementation of this proposal in Cadence has been completed, and is awaiting to get reviewed and merged. Once merged, it is going to be included in an Emulator preview release, at which point additional documentation will be provided. This will allow developers to adopt the new Cap Cons API, migrate from the existing linking API, and provide feedback on the implementation. Finally, the plan is to deploy the feature in an upcoming network upgrade (TBD), in advance of the Stable Cadence release, which will automatically migrate remaining capabilities and links and remove the old linking API. If you have any questions or concerns, please do not hesitate to reach out! |
Description
Cadence encourages a capability-based security model as described on the Flow doc site. Capabilities are themselves a new concept that most Cadence programmers need to understand, but the API for syntax around Capabilities (especially the notion of “links” and “linking”), and the associated concepts of the public and private storage domains, lead to Capabilities being make this concept even more confusing and awkward to use. This proposal suggests that we could get rid of links entirely, and replace them with Capability Controllers (henceforth referred to as CapCons) which could make Capabilities easier to understand, and easier to work with.