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 interfaces, clarify type intentions #2673

Merged
merged 5 commits into from
Jun 3, 2020

Conversation

rekmarks
Copy link
Contributor

@rekmarks rekmarks commented May 27, 2020

Link to file: https://github.com/rekmarks/EIPs/blob/1193-update-interfaces/EIPS/eip-1193.md

Summary of Changes

  • Remove index signatures from interfaces
    • i.e. no more [key: string]: unknown; the ability to add properties is implied
  • RequestArguments
    • params?: unknown -> params?: unknown[] | object
    • Explicitly define relationship between RequestArguments fields and RPC method names and parameters
      • You could say that relationship was heavily implied, but 🤷
  • Remove redundant MAY statements.
  • Various minor fixups and clarifications
    • Clarify author intentions in certain places

A Note on request

After review, here's where I currently stand on the signature of request.

  • request takes a single, options bag argument, to make it as extensible as possible. We are extremely unlikely to have to revisit it ever again. This is a wonderful thing.
    • If devs find the options bag unergonomic, they can define their own wrapper functions. We, however, should not bloat the spec with syntactic sugar.
  • Provider implementers should type RequestArguments and its params property however it makes practical sense to do so. Presumably, they'll type it such that it supports whatever RPC methods the implementation supports.
    • That said, I have not found an example of an RPC protocol that does not take its parameters by name (object) or position (array). As such, we set the type to params?: unknown[] | object.
    • This is not normative for future RPC specifications. This only normative for how those methods are called via this API. If RPC standards emerge that map poorly to this concept, that's IMO an acceptable use case for errata.
  • I recommend that implementers implement request in this way:
    • Do basic input validation (i.e. typeof args?.method === 'string').
    • Try to find a method handler.
    • If no handler, return error. If handler, hand off to handler; return result or error from handler.
      • Do detailed type checking of params in handler.

Ref #2319

cc: @alcuadrado @ryanio @MicahZoltu

@rekmarks rekmarks mentioned this pull request May 27, 2020
Copy link
Contributor

@ryanio ryanio left a comment

Choose a reason for hiding this comment

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

Lgtm, thanks for putting this together from the feedback!

@rekmarks
Copy link
Contributor Author

rekmarks commented May 27, 2020

@ryanio after further discussion with @MicahZoltu and others, I expanded these changes, and changed the type of RequestArguments.params.

cc: @alcuadrado

EIPS/eip-1193.md Outdated Show resolved Hide resolved
EIPS/eip-1193.md Outdated Show resolved Hide resolved
EIPS/eip-1193.md Outdated Show resolved Hide resolved
EIPS/eip-1193.md Outdated Show resolved Hide resolved
EIPS/eip-1193.md Outdated Show resolved Hide resolved
Copy link
Contributor

@MicahZoltu MicahZoltu left a comment

Choose a reason for hiding this comment

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

I'm a fan of short and concise standards that clearly indicate the set of things necessary for communication between two systems implementing the standard. As indicated in my comments, I feel like a number of the additions (along with some of the text that was present prior to this PR) needlessly hand hold developers and are not necessary for the standard to serve its purpose in providing a mechanism for two systems to communicate with each other.

TypeScript is structurally typed (duck typed), and JavaScript is untyped so there is no need for any mention of what else may or may not be on an object and we can instead focus this specification on what MUST be on the object. The time when MAY is useful is when something is optional but if present then it MUST meet some constraints. That is, I think an MAY statement that doesn't also contain a MUST (or in some cases a MUST NOT) should be removed from the spec.

I would also generally like to see this standard cleaned up a of much of the opinion and style comments, and also get rid of some of the redundancy that is currently present. While we have been having a lot of discussion and disagreement on what the standard should be, I think whatever that is can be described in very few words and we should strive to be pithy (unlike this comment).

@rekmarks rekmarks marked this pull request as ready for review May 28, 2020 03:42
Copy link
Contributor

@ryanio ryanio left a comment

Choose a reason for hiding this comment

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

Looks good, nice summary and changes, thx! 👍

@alcuadrado
Copy link
Member

These changes look good. Thanks @rekmarks.

@rekmarks rekmarks force-pushed the 1193-update-interfaces branch from a119f2d to 875aaf0 Compare May 30, 2020 22:07
@rekmarks
Copy link
Contributor Author

FYI: I'm not making any changes to this PR anymore, I'm just force-pushing to cause CI to rerun.

@frozeman
Copy link
Contributor

frozeman commented May 31, 2020

Thanks for that @rekmarks!
I would though like to see at least one example in written out JS as I know how much easier it will make it to implementers, and how it will prevent misunderstandings. 99% developers don’t read the spec they simply copy the example and change it up.

@rekmarks
Copy link
Contributor Author

rekmarks commented May 31, 2020

MetaMask has a working implementation here: https://github.com/MetaMask/inpage-provider/blob/master/src/MetamaskInpageProvider.js

Edit: Our implementation includes a lot of cruft related the old API and some of our internals. I'm considering extracting all of the method and event implementations into a sample implementation gist that could be attached to 1193 itself.

@frozeman
Copy link
Contributor

frozeman commented May 31, 2020

I mean a one liner:

ethereum.request({method: 'eth_exampleRequest', params: ['param1']});

Not a full implementation. We shouldn’t need to go to one companies implantation for reference, as this could be biased. (Don’t get me wrong, I like MM)

@rekmarks
Copy link
Contributor Author

rekmarks commented May 31, 2020

Ah, got it! Haha, our implementation is definitely biased.

There are a number of examples like that in the Examples section already, see: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1193.md#examples

@frozeman
Copy link
Contributor

frozeman commented May 31, 2020 via email

@rekmarks rekmarks force-pushed the 1193-update-interfaces branch from 875aaf0 to 32055a0 Compare June 3, 2020 02:16
@rekmarks rekmarks force-pushed the 1193-update-interfaces branch from 32055a0 to ee2bcba Compare June 3, 2020 15:35
@eip-automerger eip-automerger merged commit b5c0776 into ethereum:master Jun 3, 2020
@rekmarks rekmarks deleted the 1193-update-interfaces branch June 3, 2020 15:39
pizzarob pushed a commit to pizzarob/EIPs that referenced this pull request Jun 12, 2020
Hi, I'm a bot! This change was automatically merged because:

 - It only modifies existing Draft or Last Call EIP(s)
 - The PR was approved or written by at least one author of each modified EIP
 - The build is passing
tkstanczak pushed a commit to tkstanczak/EIPs that referenced this pull request Nov 7, 2020
Hi, I'm a bot! This change was automatically merged because:

 - It only modifies existing Draft or Last Call EIP(s)
 - The PR was approved or written by at least one author of each modified EIP
 - The build is passing
Arachnid pushed a commit to Arachnid/EIPs that referenced this pull request Mar 6, 2021
Hi, I'm a bot! This change was automatically merged because:

 - It only modifies existing Draft or Last Call EIP(s)
 - The PR was approved or written by at least one author of each modified EIP
 - The build is passing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants