-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[Delegation Toolkit] Add delegation scopes guide #2262
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
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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 did an initial pass of small edits, I have a couple questions:
1. Since using delegation scopes is technically also "restricting a delegation," I think it might be confusing to have the "delegation scopes" and "restricting a delegation" sections separate. Also, with there being only one function call scope and one ownership transfer scope, we could possibly consolidate and simplify. What do you think of the following structure?
- Guides
- Delegation
- Execute on a smart account's behalf
- Restrict a delegation
- Use delegation scopes
- Use caveat enforcers
In "Use delegation scopes," we can either consolidate all the scopes, or add generic guidance with a few examples, and list all the scope details in the Reference section.
2. In the function call scope, how do you specify allowed calldata?
I was thinking, scopes cover basic use cases one might think off, and possibilities of using caveat enforcers to refine the scopes is less. So, we can keep the current structure, and add a new page with label "Refine the scope", "Restrict the scope", or some better terminology that signifies, that it'll only restrict it further instead of overriding. The implementation is that scopes are mandatory, and for instance if I'm using erc-20 transfer scope it internally sets the native transfer to be |
There are parameters in the scope config which can be used. I'll improve the page to add sub sections to add calldata.
We'll add it to reference section regardless of what structure we decide. |
@AyushBherwani1998 I went ahead and simplified the structure to:
Let me know if this makes sense to you and I'll continue to refine the text, add context to relevant sections, and update links. |
Hey, I would recommend sticking with the structure we had previously decided on, mainly for the following reasons:
|
@AyushBherwani1998 Ok, I can edit again tomorrow. I'd also want to confirm that the scopes list will not grow significantly from the current list, otherwise the guides could become overwhelmingly long. Is this structure OK?
Also, according to your previous response: "We'll add it to reference section regardless of what structure we decide" – do you want the scopes list duplicated in the guides and the reference? |
No the scopes won't grow. There might be new scopes under spending limit - nothing planned at the moment.
Overall yes, just want to understand rationale behind changing "Use delegation scope" to "Restrict a delegation", scopes are mandatory, and "Use delegation scopes" makes more sense? The edit you did in this commit was good - ca55b7b. However, I'm fine with Restrict a delegation as well. Just curious to understand how to pick right labels.
Major difference is API reference would have parameters type, and won't have explanation. I'll also working on adding use-cases for the scope to help developer understand usage. This will also go in guide. It's similar to smart accounts. |
We can keep "Use delegation scopes" if that's the preference. I was thinking that someone who doesn't know what a "delegation scope" is would find "Restrict a delegation" more intuitive and action-oriented. I guess "scope" is a descriptive enough term. |
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.
Added more edits to use the preferred structure of including all scopes in the guides: https://metamask-docs-ojp1k35gx-consensys-ddffed67.vercel.app/delegation-toolkit/development/guides/delegation/use-delegation-scopes/ (note that I kept a simplified reference page for the scopes).
Just want to confirm a couple things that still need to be done (can be added in follow-up PRs):
- Add parameter lists in the delegation scopes reference.
- Update the
createDelegation
andcreateOpenDelegation
methods in the API reference to include a requiredscope
property. - Update the Create a delegation step in the Execute on a smart account's behalf guide to include mandatory scopes.
- Update the Delegation concept page to explain delegation scopes. Perhaps scopes can be explained in the Caveat enforcers concept page as well?
Also some questions I have:
- Is this correct? Scopes are always mandatory, and additional caveat enforcers are optional, but still recommended. Scopes use caveat enforcers under the hood, so the two concepts are closely related.
- Why do we have names for scopes instead of referring to them as their
type
(e.g. "ERC-20 periodic scope" instead oferc20PeriodTransfer
scope)?
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.
Some feedback. I'll approve, since I know there may be followup PRs.
delegation-toolkit/guides/delegation/use-delegation-scopes/function-call.md
Outdated
Show resolved
Hide resolved
delegation-toolkit/guides/delegation/use-delegation-scopes/function-call.md
Show resolved
Hide resolved
delegation-toolkit/guides/delegation/use-delegation-scopes/ownership-transfer.md
Show resolved
Hide resolved
delegation-toolkit/guides/delegation/use-delegation-scopes/refine-scope.md
Outdated
Show resolved
Hide resolved
delegation-toolkit/guides/delegation/use-delegation-scopes/spending-limit.md
Show resolved
Hide resolved
delegation-toolkit/guides/delegation/use-delegation-scopes/spending-limit.md
Outdated
Show resolved
Hide resolved
delegation-toolkit/guides/delegation/use-delegation-scopes/spending-limit.md
Outdated
Show resolved
Hide resolved
The ownership transfer scope restricts a delegation to ownership transfer calls only. | ||
For example, Alice has deployed a smart contract, and she delegates to Bob the ability to transfer ownership of that contract. | ||
|
||
Internally, this scope uses the [`ownershipTransfer`](../../../reference/caveats.md#ownershiptransfer) caveat enfrocer. |
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.
Just a question (since I'm not familiar with this, and it may be answered elsewhere). But why use scopes, if you can just use a caveat enforcer instead?
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.
Maybe @AyushBherwani1998 can describe more, but from what I understand scopes are a new, mandatory feature for delegations and are an easier way to configure combinations of caveats for fundamental use cases. Most scopes use multiple caveats, but it looks like a couple scopes are equivalent to a single caveat (erc721transfer
and ownershipTransfer
). Scopes are mandatory, whereas caveats are not, so I'm guessing these are part of the fundamental options that you must choose between when scoping a delegation.
We'll definitely need to add more clarification around delegation scopes in relation to caveat enforcers, in our concepts and guides.
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.
@alexandratran explained it well. I'll explain the reason behind the change. We have lots of caveat enforcers, and as a developer you might be confused by going through the list that what are the possible options. Moreover, earlier caveats
was optional as well which means anyone can create an open ended delegation which results in total control of the account by delegate.
So, to avoid allowing developers to create such delegations, we added scopes which covers the common use cases. And if anyone want's to refine those common use cases they can use additional caveat enforcers.
delegation-toolkit/guides/delegation/use-delegation-scopes/ownership-transfer.md
Outdated
Show resolved
Hide resolved
delegation-toolkit/guides/delegation/use-delegation-scopes/spending-limit.md
Show resolved
Hide resolved
764f6bc
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
Yeah, maybe we can say in the toolkit, we have scopes which one can use.
These are all in my todo list for v0.13 changes. There are few more changes in the interface which are yet to be merged on SDK side. It'll avoid confusion, and every code change can be done in a single PR for v0.13.
It's not recommended to add caveat enforcers. It's for someone how doesn't think scopes cover their use-case, and need additional restriction, they can add caveat enforcers. For example, Alice used the erc20 transfer scope, but needs bob to do transfer in next 2 hours, so alice can further add timestamp enforcer.
It's more SEO friendly, and it's easy on eyes when you browse through right navigation of page content. |
Hey @alexandratran created v0.13-dev changes to keep all the v0.13 changes in place. Since, we are linking tutorials, and couple of guides from development, it'll have broken content for devs browsing the development path. |
Description
Adds delegation scopes guide
Issue(s) fixed
Fixes #2242
Preview
Checklist
Complete this checklist before merging your PR: