rpc: several fixes and support for optional arguments #2359

bas-vk · 2016-03-17T10:03:49Z

This PR includes:

support for optional arguments in the RPC layer miner.stop threadCount actually required #2312.
let eth_call accept null values for some of the arguments geth 1.4-unstable RPC returns error when receiving null properties #2319
uses a different mechanism for sending RPC request which does not do an export of otto values to go values which could crash the otto vm with null values in arrays (otto bug) panic on eth.getBlock() #2230
be less restrictive on the request id Json-Rpc request Id only supports numbers in the latest version as opposed to string, numbers and null #2270
fixes several failing RPC tests
removal of unused code
upgrade of web3 to version 0.16.0 console: warn user if account is locked #2403

robotally · 2016-03-17T10:03:50Z

Vote	Count	Reviewers
👍	1	@karalabe
👎	0

Reaction	Users
:28:	@obscuren
:34:	@obscuren

Updated: Tue Apr 12 12:03:46 UTC 2016

codecov-io · 2016-03-17T10:10:10Z

Current coverage is `50.88%`

Merging #2359 into develop will decrease coverage by -0.03% as of 0c5d283

Powered by Codecov. Updated on successful CI builds.

fjl · 2016-03-17T11:13:25Z

rpc/json_test.go

+		{`[null,"2"]`, []reflect.Type{reflect.TypeOf(new(int)), reflect.TypeOf(""), reflect.TypeOf(new(int))},
+			[]reflect.Value{reflect.ValueOf(new(int)), reflect.ValueOf("2"), reflect.ValueOf(new(int))}},
+		{`[null,"2",null]`, []reflect.Type{reflect.TypeOf(new(int)), reflect.TypeOf(""), reflect.TypeOf(new(int))},
+			[]reflect.Value{reflect.ValueOf(new(int)), reflect.ValueOf("2"), reflect.ValueOf(new(int))}},


These tests would be more readable if you defined the types upfront, e.g.

var ( stringT = reflect.TypeOf("") intT = reflect.TypeOf(0) intPtrT = reflect.TypeOf(new(int)) ) .... []reflect.Type{stringT, intT, intPtrT}

bas-vk · 2016-03-18T07:52:20Z

@fjl, added the extra types in the tests to improve readability.

obscuren · 2016-03-18T10:07:22Z

A general observation on this PR is that it contains almost no inline comments. Please specify some more comments on the code and what it does, this makes in easier to review and makes it easier for outsiders to understand the code. This is especially true for this part of the code since it's the most "visible" part of the client. Making it easier to understand for outsiders will only further encourage them to fix something if it's broken and send RPs.

In general I believe we should strive for better inline documentation and descriptive code as this project gets more visibility.

bas-vk · 2016-04-04T10:20:14Z

rebased

karalabe · 2016-04-05T08:10:35Z

cmd/utils/jeth.go

-			passwd, _ = passwdExport.(string)
+
+	// verify that if a second non null argument was given it was a string
+	if call.Argument(1).IsDefined() && !call.Argument(1).IsNull() {


This could simply be else. I think it's more readable than having to understand the conditions again. Also probably more future proof rather than having to maintain te exact negation of the previous if condition.

karalabe · 2016-04-11T13:04:24Z

LGTM 👍

obscuren · 2016-04-12T08:38:55Z

Contains a windows build failure:

..\go\src\github.com\ethereum\go-ethereum\rpc\ipc_windows.go:28: undefined: winio in winio.ListenPipe
..\go\src\github.com\ethereum\go-ethereum\rpc\ipc_windows.go:28: undefined: winio in winio.PipeConfig
..\go\src\github.com\ethereum\go-ethereum\rpc\ipc_windows.go:34: undefined: winio in winio.DialPipe

karalabe · 2016-04-12T08:42:00Z

@obscuren @bas-vk Probably needs a rebase. @fjl added the winio thing which was merged last week. Probably it's some strange merge issue or whatnot. Lets rebase and see if it solves it.

rpc: be less restrictive on the request id rpc: improved documentation console: upgrade web3.js to version 0.16.0 rpc: cache http connections rpc: rename wsDomains parameter to wsOrigins

bas-vk · 2016-04-12T09:09:08Z

This seem to be a bug in goimports. Goimports removes "github.com/microsoft/go-winio" from the import list which was run after the rebase (on linux). Solution is to import winio "github.com/microsoft/go-winio". Goimports is then able to figure out that this package is used and won't remove it from the import list.

obscuren · 2016-04-12T12:03:45Z

@fjl please update your PR instead.

obscuren added the in progress label Mar 17, 2016

bas-vk added review and removed in progress labels Mar 17, 2016

fjl reviewed Mar 17, 2016
View reviewed changes

bas-vk force-pushed the rpc-optional-args branch from e9b5788 to 453c828 Compare March 18, 2016 07:51

bas-vk mentioned this pull request Mar 18, 2016

rpc: be less restrictive on the request id #2282

Closed

bas-vk force-pushed the rpc-optional-args branch from 97c5c50 to 7f4414a Compare March 18, 2016 09:53

bas-vk force-pushed the rpc-optional-args branch from a80cb06 to cddf6d0 Compare March 24, 2016 10:20

bas-vk mentioned this pull request Apr 1, 2016

console: warn user if account is locked #2403

Closed

karalabe added this to the 1.4.0 milestone Apr 1, 2016

bas-vk force-pushed the rpc-optional-args branch from cddf6d0 to 70f864f Compare April 4, 2016 10:19

fjl mentioned this pull request Apr 4, 2016

cmd/geth, cmd/utils, node: rename ws domains to origins #2418

Closed

bas-vk force-pushed the rpc-optional-args branch from 788800d to f95c6cb Compare April 4, 2016 14:51

karalabe reviewed Apr 5, 2016
View reviewed changes

bas-vk force-pushed the rpc-optional-args branch from f95c6cb to 91ce2b5 Compare April 6, 2016 06:03

bas-vk mentioned this pull request Apr 11, 2016

personal_unlockAccount does not work on develop #2433

Closed

karalabe added the merge imminent label Apr 11, 2016

obscuren removed the merge imminent label Apr 12, 2016

rpc: various fixes/enhancements

aa9fff3

rpc: be less restrictive on the request id rpc: improved documentation console: upgrade web3.js to version 0.16.0 rpc: cache http connections rpc: rename wsDomains parameter to wsOrigins

bas-vk force-pushed the rpc-optional-args branch from 91ce2b5 to aa9fff3 Compare April 12, 2016 09:03

obscuren merged commit 8627680 into ethereum:develop Apr 12, 2016

obscuren removed the review label Apr 12, 2016

bas-vk deleted the rpc-optional-args branch April 14, 2016 15:29

maoueh pushed a commit to streamingfast/go-ethereum that referenced this pull request Apr 17, 2024

triedb: do not open state freezer under notries (ethereum#2359)

75a03f4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc: several fixes and support for optional arguments #2359

rpc: several fixes and support for optional arguments #2359

bas-vk commented Mar 17, 2016

robotally commented Mar 17, 2016

codecov-io commented Mar 17, 2016

fjl Mar 17, 2016

bas-vk commented Mar 18, 2016

obscuren commented Mar 18, 2016

bas-vk commented Apr 4, 2016

karalabe Apr 5, 2016

karalabe commented Apr 11, 2016

obscuren commented Apr 12, 2016

karalabe commented Apr 12, 2016

bas-vk commented Apr 12, 2016

obscuren commented Apr 12, 2016

rpc: several fixes and support for optional arguments #2359

rpc: several fixes and support for optional arguments #2359

Conversation

bas-vk commented Mar 17, 2016

robotally commented Mar 17, 2016

codecov-io commented Mar 17, 2016

Current coverage is 50.88%

fjl Mar 17, 2016

Choose a reason for hiding this comment

bas-vk commented Mar 18, 2016

obscuren commented Mar 18, 2016

bas-vk commented Apr 4, 2016

karalabe Apr 5, 2016

Choose a reason for hiding this comment

karalabe commented Apr 11, 2016

obscuren commented Apr 12, 2016

karalabe commented Apr 12, 2016

bas-vk commented Apr 12, 2016

obscuren commented Apr 12, 2016

Current coverage is `50.88%`