Replace JrpcConv by custom EthJson flavor#237
Replace JrpcConv by custom EthJson flavor#237nitely wants to merge 8 commits intostatus-im:masterfrom
Conversation
d419160 to
9224003
Compare
tests/helpers/handlers.nim
Outdated
| proc eth_syncing(x: JsonString): SyncingStatus = | ||
| return SyncingStatus(syncing: false) | ||
|
|
||
| proc eth_sendRawTransaction(x: JsonString, data: seq[byte]): Hash32 = |
There was a problem hiding this comment.
hm - this actually looks pretty good, ie having the flavor up in the rpc part.
Not really for this PR, but one thing that has always put me off a bit with json_rpc is the fact that these functions are implicitly async without this being visible in the signature or anywhere really .. it has crossed my mind that it should be made explicit somewhere, ie either server.asyncRpc(EthJson) so that there's "async" in the name, or server.rpc(async = true, Format = EthJson), or variations thereof ..
the point being of course that a lot of these functions are not async, so we would need a way to override the default per-function.
There was a problem hiding this comment.
Well, you can add {.async.} to the signatures, it produces the same code. We could add support to specify the return as Future[T] too.
the point being of course that a lot of these functions are not async, so we would need a way to override the default per-function.
Maybe we should enforce the async pragma. Later we can do a different transformation if the async pragma is not specified (i.e: the proc is synchronous).
There was a problem hiding this comment.
Maybe we should enforce the async pragma.
mmh, but if we do this, we should copy the entire pragma section into the generated async proc. So async: (raises: [MyException]) is applied.
There was a problem hiding this comment.
I implemented support for async and non-async RPC functions status-im/nim-json-rpc#264 and I updated this PR to use raises.
Here - at least for now - though there's maybe room for moving it in the future depending on how we define the role of nim-eth - specially since the encoding has gotten a lot more standardised in recent years there might be a case for putting it in eth.. The other reason we use json_ser is logging which uses the default flavor - something that would be nice for chronicles is if it used its own logging flavor that automatically inherited the default flavor but could be overridden (for example to log more compact representations of things, hide passwords and omit parts). |
arnetheduck
left a comment
There was a problem hiding this comment.
very nice! let's get everything released and merged
Changes:
EthJson(replacesJrpcConv).web3/eth_json_marshal(replacesjson_rpc/jsonmarshal).JrpcConvusage andjson_rpc/jsonmarshalexports.Should
EthJsonlive here or in nim-eth?Note:
rpccontext adds an indentation level; if reviewing in github, turn on "Hide whitespace" config in the tool bar.