Skip to content
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

Add ERC: Grant Permissions from Wallets #436

Open
wants to merge 75 commits into
base: master
Choose a base branch
from

Conversation

pedrouid
Copy link
Contributor

@pedrouid pedrouid commented May 24, 2024

Adds JSON-RPC method for granting permissions from a wallet

@eip-review-bot
Copy link
Collaborator

eip-review-bot commented May 24, 2024

File ERCS/erc-7715.md

Requires 1 more reviewers from @axic, @g11tech, @SamWilsn, @xinbenlv

@github-actions github-actions bot removed the w-ci label May 24, 2024
@eip-review-bot eip-review-bot changed the title ERC-xxxx: Request Permissions from Wallets Add ERC: Request Permissions from Wallets May 24, 2024
@github-actions github-actions bot added the w-ci label May 24, 2024
@github-actions github-actions bot added w-ci and removed w-ci labels May 24, 2024
pedrouid and others added 2 commits May 25, 2024 07:37
Co-authored-by: Andrew B Coathup <28278242+abcoathup@users.noreply.github.com>
Co-authored-by: Andrew B Coathup <28278242+abcoathup@users.noreply.github.com>
@github-actions github-actions bot added w-ci and removed w-ci labels May 25, 2024
pedrouid and others added 2 commits May 25, 2024 07:47
@github-actions github-actions bot removed the w-ci label May 25, 2024
@github-actions github-actions bot added the w-ci label May 25, 2024
Comment on lines +568 to +575
// Alice sends the transaction by calling redeemDelegation on Bob's account
const tx = await bob.sendTransaction({
to: delegationManager,
data: bob.interface.encodeFunctionData('redeemDelegation', [
permissionsContext,
action
])
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct?

  1. Why bob.sendTransaction? shouldn't it be alice.sendTransaction?
  2. Why bob has interface of redeemDelegation? bob is Delegator, so it has ERC7710 interface with executeDelegatedAction method. redeemDelegation lives in the ERC7710Manager interface implemented by delegationManager.
    ERC-7710 for reference

ERCS/erc-7715.md Outdated
policies: [
{
type: 'gas-limit',
data: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just did a pretty big dive into this and can't really see concrete definitions with data in the context of policies. It also might make sense to try and define the permissions/policies deeper before getting into examples like these

ERCS/erc-7715.md Outdated
],
required: true,
data: {
address: '0x...',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there shouldn't be address field in native-token-transfer permission.

SamWilsn
SamWilsn previously approved these changes Jul 9, 2024

## Abstract

We define a new JSON-RPC method `wallet_grantPermissions` for DApp to request a Wallet to grant permissions in order to execute transactions on the user’s behalf. This enables two use cases:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wording here is a bit awkward. How about:

Suggested change
We define a new JSON-RPC method `wallet_grantPermissions` for DApp to request a Wallet to grant permissions in order to execute transactions on the user’s behalf. This enables two use cases:
We define a new JSON-RPC method `wallet_grantPermissions`. With it, a DApp can request that a wallet grant permissions in order to execute transactions on the user’s behalf. This enables two use cases:


Currently most DApps implement a flow similar to the following:

```mermaid
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if mermaid diagrams will render properly in Jekyll. If not, we have a slight preference for SVGs.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked, and no, this will not render.

@eip-review-bot eip-review-bot enabled auto-merge (squash) July 9, 2024 14:46
eip-review-bot
eip-review-bot previously approved these changes Jul 9, 2024
Copy link
Collaborator

@eip-review-bot eip-review-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All Reviewers Have Approved; Performing Automatic Merge...

@aminhdev
Copy link

.

auto-merge was automatically disabled July 29, 2024 15:29

Head branch was pushed to by a user without write access

@pedrouid pedrouid dismissed stale reviews from eip-review-bot and SamWilsn via 01f41b6 July 29, 2024 15:29
ERCS/erc-7715.md Outdated
Comment on lines 60 to 67
permission: {
type: string; // enum defined by ERCs
data: Record<string, any>;
};
policies: {
type: string; // enum defined by ERCs
data: Record<string, any>;
}[];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason not to be consistent with permission and policies either both being an Array or both being singletons?

looking at some examples policies vs permissions below, it's also not super clear what the difference is between them. a NativeTokenSpendPolicy seems similar to an Erc20TokenTransferPermission. They both define limits on the spend of some funds.

Copy link

@moldy530 moldy530 Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I was first looking at this, I was thinking it would be used like:

{
  permission: { type: "owner" }, // or maybe "root" or "admin" type?
  policies: [], // no need to for policies because this is a root permission
}

or

{
  permission: { type: "session-key", data: { expiry: fifteenMinutesInMs } },
  policies: [
    ... // spend limits, etc
  ]
}


[ERC-7679](./eip-7679.md) UserOp Builder contract defines `bytes calldata context` parameter in all of its methods. It’s equivalent to the`permissionsContext` returned by the `wallet_grantPermissions` call.

Example of formatting userOp signature using the [ERC-7679](./eip-7679.md) UserOp Builder

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clicking ERC-7579 is giving 404.

Suggested change
Example of formatting userOp signature using the [ERC-7679](./eip-7679.md) UserOp Builder
Example of formatting userOp signature using the [ERC-7679](./erc-7679.md) UserOp Builder


#### ERC-7679 with `Key` type signer

`wallet_grantPermissions` replies with `permissionsContext` and `userOpBuilder` address inside the `signerMeta` field. DApps can use that data with methods provided by [ERC-7679](./eip-7679.md) to build the [ERC-4337](./eip-4337.md) userOp.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clicking ERC-4337 and ERC-7679 is giving 404.

Suggested change
`wallet_grantPermissions` replies with `permissionsContext` and `userOpBuilder` address inside the `signerMeta` field. DApps can use that data with methods provided by [ERC-7679](./eip-7679.md) to build the [ERC-4337](./eip-4337.md) userOp.
`wallet_grantPermissions` replies with `permissionsContext` and `userOpBuilder` address inside the `signerMeta` field. DApps can use that data with methods provided by [ERC-7679](./erc-7679.md) to build the [ERC-4337](./erc-4337.md) userOp.


`wallet_grantPermissions` replies with `permissionsContext` and `userOpBuilder` address inside the `signerMeta` field. DApps can use that data with methods provided by [ERC-7679](./eip-7679.md) to build the [ERC-4337](./eip-4337.md) userOp.

[ERC-7679](./eip-7679.md) UserOp Builder contract defines `bytes calldata context` parameter in all of its methods. It’s equivalent to the`permissionsContext` returned by the `wallet_grantPermissions` call.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clicking ERC-7579 is giving 404.

Suggested change
[ERC-7679](./eip-7679.md) UserOp Builder contract defines `bytes calldata context` parameter in all of its methods. It’s equivalent to the`permissionsContext` returned by the `wallet_grantPermissions` call.
[ERC-7679](./erc-7679.md) UserOp Builder contract defines `bytes calldata context` parameter in all of its methods. It’s equivalent to the`permissionsContext` returned by the `wallet_grantPermissions` call.


The `permissionsContext` field is meant to be an opaque string that's maximally flexible and can encode arbitrary information for different permissions schemes. We specifically had three schemes in mind:

- If a DApp leverages [ERC-7679](./eip-7679.md), it could use `permissionsContext` as the `context` parameter when interacting with the UserOp builder.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- If a DApp leverages [ERC-7679](./eip-7679.md), it could use `permissionsContext` as the `context` parameter when interacting with the UserOp builder.
- If a DApp leverages [ERC-7679](./erc-7679.md), it could use `permissionsContext` as the `context` parameter when interacting with the UserOp builder.

The `permissionsContext` field is meant to be an opaque string that's maximally flexible and can encode arbitrary information for different permissions schemes. We specifically had three schemes in mind:

- If a DApp leverages [ERC-7679](./eip-7679.md), it could use `permissionsContext` as the `context` parameter when interacting with the UserOp builder.
- If a DApp leverages [ERC-7710](./eip-7710.md), it could use `permissionsContext` as the `_data` when interacting with the delegation manager.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- If a DApp leverages [ERC-7710](./eip-7710.md), it could use `permissionsContext` as the `_data` when interacting with the delegation manager.
- If a DApp leverages [ERC-7710](./erc-7710.md), it could use `permissionsContext` as the `_data` when interacting with the delegation manager.


First note that the response contains all of the parameters of the original request and it is not guaranteed that the values received are equivalent to those requested.

`context` is a catch-all to identify a permission for revoking permissions or submitting userOps, and can contain non-identifying data as well. It MAY be the `context` as defined in [ERC-7679](./eip-7679.md) and [ERC-7710](./eip-7710.md). See “Rationale” for details.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`context` is a catch-all to identify a permission for revoking permissions or submitting userOps, and can contain non-identifying data as well. It MAY be the `context` as defined in [ERC-7679](./eip-7679.md) and [ERC-7710](./eip-7710.md). See “Rationale” for details.
`context` is a catch-all to identify a permission for revoking permissions or submitting userOps, and can contain non-identifying data as well. It MAY be the `context` as defined in [ERC-7679](./erc-7679.md) and [ERC-7710](./erc-7710.md). See “Rationale” for details.


`context` is a catch-all to identify a permission for revoking permissions or submitting userOps, and can contain non-identifying data as well. It MAY be the `context` as defined in [ERC-7679](./eip-7679.md) and [ERC-7710](./eip-7710.md). See “Rationale” for details.

`accountMeta` is optional but when present then fields for `factory` and `factoryData` are required as defined in [ERC-4337](./eip-4337.md). They are either both specified, or none. If the account has not yet been deployed, the wallet MUST return `accountMeta`, and the DApp MUST deploy the account by calling the `factory` contract with `factoryData` as the calldata.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`accountMeta` is optional but when present then fields for `factory` and `factoryData` are required as defined in [ERC-4337](./eip-4337.md). They are either both specified, or none. If the account has not yet been deployed, the wallet MUST return `accountMeta`, and the DApp MUST deploy the account by calling the `factory` contract with `factoryData` as the calldata.
`accountMeta` is optional but when present then fields for `factory` and `factoryData` are required as defined in [ERC-4337](./erc-4337.md). They are either both specified, or none. If the account has not yet been deployed, the wallet MUST return `accountMeta`, and the DApp MUST deploy the account by calling the `factory` contract with `factoryData` as the calldata.


`accountMeta` is optional but when present then fields for `factory` and `factoryData` are required as defined in [ERC-4337](./eip-4337.md). They are either both specified, or none. If the account has not yet been deployed, the wallet MUST return `accountMeta`, and the DApp MUST deploy the account by calling the `factory` contract with `factoryData` as the calldata.

`signerMeta` is dependent on the account type. If the signer type is `wallet` then it's not required. If the signer type is `key` or `keys` then `userOpBuilder` is required as defined in [ERC-7679](./eip-7679.md). If the signer type is `account` then `delegationManager` is required as defined in [ERC-7710](./eip-7710.md).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`signerMeta` is dependent on the account type. If the signer type is `wallet` then it's not required. If the signer type is `key` or `keys` then `userOpBuilder` is required as defined in [ERC-7679](./eip-7679.md). If the signer type is `account` then `delegationManager` is required as defined in [ERC-7710](./eip-7710.md).
`signerMeta` is dependent on the account type. If the signer type is `wallet` then it's not required. If the signer type is `key` or `keys` then `userOpBuilder` is required as defined in [ERC-7679](./erc-7679.md). If the signer type is `account` then `delegationManager` is required as defined in [ERC-7710](./erc-7710.md).

@github-actions github-actions bot removed the w-ci label Oct 8, 2024
Copy link

github-actions bot commented Oct 8, 2024

The commit 23fa360 (as a parent of 57c37c6) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot added the w-ci label Oct 8, 2024
@ethereum ethereum locked as off-topic and limited conversation to collaborators Oct 13, 2024
@SamWilsn
Copy link
Collaborator

Note to self: this is still blocked on #433.


I've locked this PR because too much of the discussion is technical in nature. Please discuss anything technical on the discussion thread.


Do not update the links to erc-xxxx.md as suggested above. That'll break our weird rendering system.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.