feat: add RPC methods described in (revised) EIP-7715 #396
Conversation
jeffsmale90
force-pushed
the
feat/eip7715_executionPermissions
branch
from
6eb865b to
4f8bc87
Compare
…ission to support EIP-7715
jeffsmale90
force-pushed
the
feat/eip7715_executionPermissions
branch
from
4f8bc87 to
f339980
Compare
jeffsmale90
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| it('throws if no hook', async () => { | ||
| await expect( | ||
| walletRequestExecutionPermissions(request, response, {}), | ||
| ).rejects.toMatchInlineSnapshot( |
There was a problem hiding this comment.
Nit: Curious, why are we using a snapshot in these tests rather than .toThrow(new Error(...))?
There was a problem hiding this comment.
Yeah, that's a bit silly - I think it was due to the complex error messages. I've replaced it with the more succinct error reason.
…to 5792 and 7715 async middleware to match correct naming.
| ): Promise<void> { | ||
| if (!processRequestExecutionPermissions) { | ||
| throw rpcErrors.resourceNotFound( | ||
| 'wallet_requestExecutionPermissions - no middleware configured', |
There was a problem hiding this comment.
we are throwing methodNotSupported() in the 5792 methods. Thoughts on doing the same here?
There was a problem hiding this comment.
functionally LGTM. I have some concerns regarding spec about if isAdjustmentAllowed should even be included as it ultimately is a proxy for something optional and if something is not strictly required for execution then IMO it should not be included in the request by the dapp
Please ping for re-approval if you feel that my nit above is fair
| res.result = await processRequestExecutionPermissions(params, req); | ||
| } |
There was a problem hiding this comment.
I believe you'll need to pass end in as a param as well for the JSON RPC Engine to handle this gracefully
| res.result = await processRequestExecutionPermissions(params, req); | |
| } | |
| res.result = await processRequestExecutionPermissions(params, req); | |
| return end() | |
| } |
There was a problem hiding this comment.
I guess they aren't doing this in the 5792 methods you used as a model. Need to figure out why that's ok 🤔
Adds methods
wallet_requestExecutionPermissionsandwallet_revokeExecutionPermission, as defined in this revision of the EIP-7715 specification ethereum/ERCs#1098.This supports Readable Permissions project, and is related to the following PRs:
wallet_requestExecutionPermissionsmethod metamask-extension#35193Note: workflows are failing due to existing problems, fixed by #397