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

Update EIP-5792: Fine-tune the "capability" rhetoric of EIP-5792 (plus non-normative wordsmithing as a bonus) #8396

Merged
merged 22 commits into from
Apr 17, 2024

Conversation

bumblefudge
Copy link
Contributor

This PR:

  • moves the wallet_getCapabilities RPC method to the END of the EIP, and adds a little guidance (some normative, some non-) for implementers to use it safely
    • in particular it mentions provider caching and injecting of capability objects, which are unspecified behaviors already present in the market and which can add additional vectors to be considered for both security and privacy
    • it also explains why "capability" semantics are used and adds a SHOULD warning people not to misinterpret unset variables or trust a provider too much
  • has tons of unrelated grammar tweaks and clarity tweaks which I don't feel too strongly about. Suggestions from any authors merged if they're distracting from the more substantive changes!

@github-actions github-actions bot added c-update Modifies an existing proposal s-draft This EIP is a Draft t-interface labels Apr 9, 2024
@eth-bot
Copy link
Collaborator

eth-bot commented Apr 9, 2024

✅ All reviewers have approved.

@eth-bot eth-bot added the a-review Waiting on author to review label Apr 9, 2024
@eth-bot eth-bot changed the title Fine-tune the "capability" rhetoric of EIP-5792 (plus non-normative wordsmithing as a bonus) Update EIP-5792: Fine-tune the "capability" rhetoric of EIP-5792 (plus non-normative wordsmithing as a bonus) Apr 9, 2024
@bumblefudge
Copy link
Contributor Author

marking as draft to prevent any auto-merging-- but ready for review!

Copy link

@arein arein left a comment

Choose a reason for hiding this comment

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

Lgtm

EIPS/eip-5792.md Show resolved Hide resolved
EIPS/eip-5792.md Outdated
Comment on lines 83 to 39
The capabilities field is how an app can communicate with a wallet about capabilities a wallet supports. For example, this is where an app can specify a paymaster service URL from which an [ERC-4337](./eip-4337.md) wallet can request a `paymasterAndData` input for a user operation.
The optional `capabilities` property enable an app to grant to a wallet its own relevant capabilities as objects. For example, this is where an app can specify a paymaster service URL from which an [ERC-4337](./eip-4337.md) wallet can request a `paymasterAndData` input for a given user operation.
Copy link
Contributor

Choose a reason for hiding this comment

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

can we remove this change? think the original was more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please re-review after newest commit. here too an OCap diehard might want "grant" rather than "communicate"?

EIPS/eip-5792.md Outdated
Comment on lines 255 to 261
For each chain on which a wallet can submit multiple calls atomically, the wallet SHOULD include an `atomicBatch` capability with a `supported` field equal to `true`.
For each chain on which a wallet can submit multiple calls atomically, the wallet SHOULD include an `atomicBatch` capability with a `supported` field equal to `true`. For each chain where it cannot, it SHOULD return the same field as `false`. Unspecified support for a given, named chain SHOULD be interpreted as not exposing this information to the application, and as such, applications SHOULD NOT interpret an absent value as equivalent to `false`.
Copy link
Contributor

Choose a reason for hiding this comment

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

i disagree with this. can we change to ...For each chain where it cannot, it MAY return the same field as 'false'. Unspecified support for a given, named chain SHOULD be interpreted as equivalent to 'false'.

i dont think the wallet should have to be so explicit about this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I think this was worded badly. Please rereview after latest commit.

(Sidenote, if we are really going full OCap, would authorized make more sense than supported as the name for that boolean?)

EIPS/eip-5792.md Outdated Show resolved Hide resolved
EIPS/eip-5792.md Outdated Show resolved Hide resolved
EIPS/eip-5792.md Outdated Show resolved Hide resolved
EIPS/eip-5792.md Outdated Show resolved Hide resolved
EIPS/eip-5792.md Outdated Show resolved Hide resolved
EIPS/eip-5792.md Outdated Show resolved Hide resolved
@bumblefudge
Copy link
Contributor Author

tysm!

Copy link
Contributor

@lukasrosario lukasrosario left a comment

Choose a reason for hiding this comment

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

🙏

@bumblefudge bumblefudge marked this pull request as ready for review April 17, 2024 13:48
@bumblefudge bumblefudge requested a review from eth-bot as a code owner April 17, 2024 13:48
@eth-bot eth-bot enabled auto-merge (squash) April 17, 2024 14:15
Copy link
Collaborator

@eth-bot eth-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...

@eth-bot eth-bot merged commit 70cd5c8 into ethereum:master Apr 17, 2024
17 checks passed
@Jaca777
Copy link

Jaca777 commented Apr 22, 2024

bumblefudge:bumblefudge/wordsmithing-eip-5792

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-review Waiting on author to review c-update Modifies an existing proposal s-draft This EIP is a Draft t-interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants