-
Notifications
You must be signed in to change notification settings - Fork 15
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
Changing confusing default block parameter constant "2" into boolean #1
base: master
Are you sure you want to change the base?
Conversation
ah, okay let me review this further. Sounds reasonable thanks @digitaldonkey sorry, been traveling, haven't had time to review. |
Would be great if we get in sync again. I based a PHP Library on the schema. |
@digitaldonkey that's great, glad to see it being used. I'll look into switching to your more clear Bool. Sorry just traveling right now. |
@@ -56,7 +56,7 @@ The entire spec is contained in the [schema.json](src/schema.json) file. | |||
|
|||
``` | |||
methods: { | |||
<method name : [ input(s), output(s), minimum required outputs, 'latest' tag default position (if any) ] >, | |||
<method name : [ Array Input(s), Array Output(s), String Output type, Integer Minimum required parameters, Boolean Requires default block parameter] >, |
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 believe the Array Output(s)
is unnecessary, no? The second element is the String Output Type
?
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 might want to look into that. Yes, you may be right.
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 misspoke - the second element should be Array Output(s)
, and so I the String Output Type
is extraneous (e.g. eth_getFilterLogs
returns ["FilterChange"]
.
Can this be merged? I'm building a library in Elixir built off of this schema as well 😄 |
@sunny-g it would mean I need to re-work another library as well (i.e. ethjs-query). If a PR can be made to change that also, then yes. I wont be able to get to this till Wednesday at the earliest. |
I found it very confusing that the boolean "default block parameter required" setting is actually implemented as a constant "2".
https://github.com/ethereum/wiki/wiki/JSON-RPC#the-default-block-parameter