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

Changing confusing default block parameter constant "2" into boolean #1

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

digitaldonkey
Copy link

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

@SilentCicero
Copy link
Member

ah, okay let me review this further. Sounds reasonable thanks @digitaldonkey sorry, been traveling, haven't had time to review.

@digitaldonkey
Copy link
Author

Would be great if we get in sync again. I based a PHP Library on the schema.

@SilentCicero
Copy link
Member

@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] >,
Copy link

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?

Copy link
Member

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.

Copy link

@sunny-g sunny-g Jun 7, 2017

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"].

@sunny-g
Copy link

sunny-g commented Jun 4, 2017

Can this be merged? I'm building a library in Elixir built off of this schema as well 😄

@SilentCicero
Copy link
Member

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

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.

3 participants