-
Notifications
You must be signed in to change notification settings - Fork 830
Add ERC: Universal Orchestrator RPC #778
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
Spelling and grammar check
|
✅ All reviewers have approved. |
Linting issues fixed
Adding temporary EIP to header
Adding links to standard references
Changing links to relative
Fixing relative links to ERCs
|
The commit 64dad91 (as a parent of 47519ee) contains errors. |
Changing link references to EIP
ERCS/erc-7845.md
Outdated
| @@ -0,0 +1,644 @@ | |||
| --- | |||
| eip: 7845 | |||
| title: Minimal Orchestrator RPC | |||
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.
While there's no hard rule against it, I would recommend not using "minimal" in your title/description. The definition of minimal is extremely subjective and the term doesn't really provide useful information to readers. It would be better to highlight characteristics of your proposal that would differentiate from similar hypothetical proposals.
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.
Understood - the name has been changed to Universal Orchestrator RPC. Just a note, i'm currently doing research to figure out if the term Orchestrator is still current.
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.
We have a slight preference for SVG images, if you can provide them. If not, PNGs are fine too.
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.
Unfortunately the tool i used, swimlanes.io - only exports in PDF or PNG. My apologies.
ERCS/erc-7845.md
Outdated
|
|
||
| ## Abstract | ||
|
|
||
| > "Hey Google, swap my SHIB for Pillar" |
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.
We like to avoid mentioning specific products in proposals. It unfairly advertises projects that exist today over ones that come after going final.
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.
Agreed - this has been changed. Thanks.
ERCS/erc-7845.md
Outdated
|
|
||
| The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in RFC 2119 and RFC 8174. | ||
|
|
||
| The Orchestrator's external interface(s) that expose functionality to an end-user application or another system `MUST` use JavaScript Object Notation (JSON). |
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.
Reserve backticks for inline code. The RFC 2119 key words should be unformatted uppercase.
ERCS/erc-7845.md
Outdated
|
|
||
| The Orchestrator's external interface(s) that expose functionality to an end-user application or another system `MUST` use JavaScript Object Notation (JSON). | ||
|
|
||
| The following data definitions are available and `MUST` **prefer chain abstraction (ChA¹)**, unless stated. |
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.
It is unclear what "prefer chain abstraction" means.
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 have added an explanation for clarity.
ERCS/erc-7845.md
Outdated
|
|
||
| The following data definitions are available and `MUST` **prefer chain abstraction (ChA¹)**, unless stated. | ||
|
|
||
| The following sequence diagram shows the flow that this ERC aims to standardise: |
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.
You should write as if the proposal was already Final. Maybe something like:
| The following sequence diagram shows the flow that this ERC aims to standardise: | |
| The following sequence diagram shows the flow of events in this proposal: |
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.
Ahh understood - thanks, i'll check the rest of the document for this also.
|
|
||
| The request definition is what a wallet sends to an Orchestrator for solutions to one or more problems. The request follows the specification from Ethereum JSON-RPC. | ||
|
|
||
| #### RPC |
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.
This section seems to be re-explaining JSON RPC. I'm not sure how necessary it is.
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.
Whilst i understand what you're suggesting - for the sake of being clear and completion, i would prefer to leave this here to make it crystal clear that this is an Ethereum RPC request and that the params property is a Problem[].
ERCS/erc-7845.md
Outdated
| - towards address 0x50840CE036eEf2005d3c4d6f6Eb65f8116a01629 | ||
| - with symbol USDC, amount 5 | ||
|
|
||
| :::info |
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'm not sure this'll render the way you want. Double check it after merging.
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.
Sorry you're right i was using a reference from elsewhere. I have now changed it to the alert standard here: https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax#alerts
eip-review-bot
left a comment
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.
All Reviewers Have Approved; Performing Automatic Merge...
Abstract
The Universal Orchestrator RPC aims to standardise the universal shape and requirements of a request for a solution from an arbitrary system managing an Ethereum wallet to, ultimately, an Orchestrator.
An arbitrary system could be a website, device, app, server program etc - anything that manages an Ethereum wallet, speaks Ethereum JSON-RPC and is looking to request solutions from an Orchestrator.
All solutions from an Orchestrator are ChA¹ (Chain Abstraction-first) by default.
Discussions: https://ethereum-magicians.org/t/erc-7845-universal-orchestrator-rpc/21885