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

rust generator-client fails on multi-geth openrpc.json #146

Closed
shanejonas opened this issue Apr 12, 2019 · 9 comments
Closed

rust generator-client fails on multi-geth openrpc.json #146

shanejonas opened this issue Apr 12, 2019 · 9 comments
Assignees
Labels
bug Something isn't working

Comments

@shanejonas
Copy link
Member

Describe the bug
When I try to generate a client with mutli-geth's openrpc.json I get errors

To Reproduce
Steps to reproduce the behavior:

  1. open-rpc-generator-client -s https://raw.githubusercontent.com/etclabscore/multi-geth/feat/openrpc/openrpc.json MultiGethRpc
  2. See error output below

Expected behavior
I expect no errors and get a client output in the specified directory

Additional context

➜  MultiGethRpc git:(master) ✗ npm run generateclient

> test-eth-rpc-change@1.0.0 generateclient /Users/shanejonas/scratch/test-eth-rpc
> open-rpc-generator-client MultiGethRpc

Error:     Updating crates.io index
   Compiling proc-macro2 v0.4.27
   Compiling unicode-xid v0.1.0
   Compiling syn v0.15.30
   Compiling autocfg v0.1.2
   Compiling libc v0.2.51
   Compiling serde v1.0.90
   Compiling ryu v0.2.7
   Compiling cfg-if v0.1.7
   Compiling itoa v0.4.3
   Compiling rustc-demangle v0.1.13
   Compiling futures v0.1.26
   Compiling log v0.4.6
   Compiling backtrace v0.3.15
   Compiling log v0.3.9
   Compiling quote v0.6.12
   Compiling error-chain v0.12.0
   Compiling serde_derive v1.0.90
   Compiling serde_json v1.0.39
   Compiling jsonrpc-core v8.0.1
   Compiling jsonrpc-client-core v0.5.0
   Compiling template-client v1.0.0 (/Users/shanejonas/scratch/test-eth-rpc/MultiGethRpc/rs)
error[E0252]: the name `HashMap` is defined multiple times
  --> src/lib.rs:79:5
   |
27 | use std::collections::HashMap;
   |     ------------------------- previous import of the type `HashMap` here
...
79 | use std::collections::HashMap;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^ `HashMap` reimported here
   |
   = note: `HashMap` must be defined only once in the type namespace of this module
help: you can use `as` to change the binding name of the import
   |
79 | use std::collections::HashMap as OtherHashMap;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0252]: the name `HashMap` is defined multiple times
  --> src/lib.rs:96:5
   |
27 | use std::collections::HashMap;
   |     ------------------------- previous import of the type `HashMap` here
...
96 | use std::collections::HashMap;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^ `HashMap` reimported here
   |
   = note: `HashMap` must be defined only once in the type namespace of this module
help: you can use `as` to change the binding name of the import
   |
96 | use std::collections::HashMap as OtherHashMap;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0428]: the name `Address` is defined multiple times
   --> src/lib.rs:198:1
    |
114 | pub type Address = String;
    | -------------------------- previous definition of the type `Address` here
...
198 | pub enum Address {
    | ^^^^^^^^^^^^^^^^ `Address` redefined here
    |
    = note: `Address` must be defined only once in the type namespace of this module

error[E0428]: the name `TransactionElement` is defined multiple times
   --> src/lib.rs:234:1
    |
220 | pub enum TransactionElement {
    | --------------------------- previous definition of the type `TransactionElement` here
...
234 | pub enum TransactionElement {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ `TransactionElement` redefined here
    |
    = note: `TransactionElement` must be defined only once in the type namespace of this module

error[E0428]: the name `_IMPL_DESERIALIZE_FOR_TransactionElement` is defined multiple times
   --> src/lib.rs:232:21
    |
218 | #[derive(Serialize, Deserialize)]
    |                     ----------- previous definition of the value `_IMPL_DESERIALIZE_FOR_TransactionElement` here
...
232 | #[derive(Serialize, Deserialize)]
    |                     ^^^^^^^^^^^ `_IMPL_DESERIALIZE_FOR_TransactionElement` redefined here
    |
    = note: `_IMPL_DESERIALIZE_FOR_TransactionElement` must be defined only once in the value namespace of this module

error[E0428]: the name `_IMPL_SERIALIZE_FOR_TransactionElement` is defined multiple times
   --> src/lib.rs:232:10
    |
218 | #[derive(Serialize, Deserialize)]
    |          --------- previous definition of the value `_IMPL_SERIALIZE_FOR_TransactionElement` here
...
232 | #[derive(Serialize, Deserialize)]
    |          ^^^^^^^^^ `_IMPL_SERIALIZE_FOR_TransactionElement` redefined here
    |
    = note: `_IMPL_SERIALIZE_FOR_TransactionElement` must be defined only once in the value namespace of this module

error: no rules expected the token `>`
   --> src/lib.rs:857:78
    |
857 |     pub fn eth_getFilterChanges(&mut self, filterId: FilterId) -> RpcRequest<>;
    |                                                                              ^ no rules expected this token in macro call

warning: unused import: `std::collections::HashMap`
  --> src/lib.rs:79:5
   |
79 | use std::collections::HashMap;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: #[warn(unused_imports)] on by default

warning: unused import: `std::collections::HashMap`
  --> src/lib.rs:96:5
   |
96 | use std::collections::HashMap;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0119]: conflicting implementations of trait `serde::Deserialize<'_>` for type `std::string::String`:
   --> src/lib.rs:196:21
    |
196 | #[derive(Serialize, Deserialize)]
    |                     ^^^^^^^^^^^
    |
    = note: conflicting implementation in crate `serde`:
            - impl<'de> serde::Deserialize<'de> for std::string::String;

error[E0119]: conflicting implementations of trait `serde::Deserialize<'_>` for type `TransactionElement`:
   --> src/lib.rs:232:21
    |
218 | #[derive(Serialize, Deserialize)]
    |                     ----------- first implementation here
...
232 | #[derive(Serialize, Deserialize)]
    |                     ^^^^^^^^^^^ conflicting implementation for `TransactionElement`

error[E0119]: conflicting implementations of trait `serde::Serialize` for type `std::string::String`:
   --> src/lib.rs:196:10
    |
196 | #[derive(Serialize, Deserialize)]
    |          ^^^^^^^^^
    |
    = note: conflicting implementation in crate `serde`:
            - impl serde::Serialize for std::string::String;

error[E0119]: conflicting implementations of trait `serde::Serialize` for type `TransactionElement`:
   --> src/lib.rs:232:10
    |
218 | #[derive(Serialize, Deserialize)]
    |          --------- first implementation here
...
232 | #[derive(Serialize, Deserialize)]
    |          ^^^^^^^^^ conflicting implementation for `TransactionElement`

error[E0117]: only traits defined in the current crate can be implemented for arbitrary types
   --> src/lib.rs:196:21
    |
196 | #[derive(Serialize, Deserialize)]
    |                     ^^^^^^^^^^^ impl doesn't use types inside crate
    |
    = note: the impl does not reference only types defined in this crate
    = note: define and implement a trait or new type instead

error[E0117]: only traits defined in the current crate can be implemented for arbitrary types
   --> src/lib.rs:196:10
    |
196 | #[derive(Serialize, Deserialize)]
    |          ^^^^^^^^^ impl doesn't use types inside crate
    |
    = note: the impl does not reference only types defined in this crate
    = note: define and implement a trait or new type instead

error: aborting due to 13 previous errors

Some errors occurred: E0117, E0119, E0252, E0428.
For more information about an error, try `rustc --explain E0117`.
error: Could not compile `template-client`.

To learn more, run the command again with --verbose.

    at /Users/shanejonas/scratch/test-eth-rpc/node_modules/@open-rpc/generator-client/build/src/bootstrapGeneratedPackage.js:19:20
    at ChildProcess.exithandler (child_process.js:283:5)
    at emitTwo (events.js:126:13)
    at ChildProcess.emit (events.js:214:7)
    at maybeClose (internal/child_process.js:915:16)
    at Socket.stream.socket.on (internal/child_process.js:336:11)
    at emitOne (events.js:116:13)
    at Socket.emit (events.js:211:7)
    at Pipe._handle.close [as _onclose] (net.js:561:12)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! test-eth-rpc-change@1.0.0 generateclient: `open-rpc-generator-client MultiGethRpc`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the test-eth-rpc-change@1.0.0 generateclient script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/shanejonas/.npm/_logs/2019-04-12T05_35_23_030Z-debug.log
@BelfordZ
Copy link
Member

the issues involved here:

  1. Generated code's return value is sometimes missing its generic type.
  2. Tries to add derive/serialize to foreign types
  3. multiple duplicate use statements

@BelfordZ BelfordZ added the bug Something isn't working label Apr 23, 2019
@mersinvald
Copy link

Looked into the generated code, so here are some more specifics:

Multiple duplicate use statements

use $path gets generated in the header for every use of the $path in the type definitions, i.e for every sub-schema.

Multiple definitions of the same type:
pub enum TransactionElement {
    String(String),
    TransactionClass(TransactionClass),
}
/* ... */
pub enum TransactionElement {
    String(String),
    TransactionClass(TransactionClass),
}

Seems like the same issue as above, but with some types being regenerated if used twice in different methods?
Possible solution 1: deduplicate typings if they're the same
Possible solution 2: threat the whole schema as the whole when generating typings
Possible solution 3 (hacky): prefix the types with the method they relate to, or enclose them in modules like

pub mod method_name {
     /* method types */
}

Though the solution 3 is very dirty in terms of resulting API, as types related to different methods aren't compatible out of the box.

Tries to add derive/serialize to foreign types

Not present after fixing other errors

Generated code's return value is sometimes missing its generic type.

For the following result schemas the generator fails to derive the correct return type:

    "result": {
        "name": "",
        "schema": {
          "type": "array",
          "items": {
            "$ref": "#/components/schemas/Log"
          }
        }
      }
    "result": {
        "name": "",
        "schema": {
          "$ref": "#/components/schemas/BlockRLP"
        }
      }
    "result": {
        "name": "",
        "schema": {
          "description": "",
          "$ref": "#/components/schemas/Bytes"
        }
      }

The common issue here is the "name": "" which is causing troubles, as far as `I can see.
Possible solution: fallback to the schema name if name's not present?

Missing types

EndHash type isn't present in type definitions, and it's a pretty problematic type as in different methods it has different characteristics, even though it is named identically.

Possible solutions: prefix the method name to the ambiguous types, no idea why there's no definition at all though, maybe generator couldn't decide which one of the multiple variants of EndHash to generate.

Tests mis-generation for types with common prefix

There's the following list of functions:

pub fn debug_traceBlock(&mut self, blockRLP: BlockRlp) -> RpcRequest<StackTrace>;
pub fn debug_traceBlockByHash(&mut self, blockHash: BlockHash) -> RpcRequest<Trace>;
pub fn debug_traceBlockByNumber(&mut self, blockNumber: BlockNumber) -> RpcRequest<Trace>;
pub fn debug_traceBlockFromFile(&mut self, filename: Filename) -> RpcRequest<Bytes>;

The generated test for debug_traceBlock tries to pass 4 arguments (BlockRlp, BlockHash, BlockNumber, Filename) as the arguments.
Seems like arguments list for foo contains all arguments for any foo* combined.

@mersinvald
Copy link

mersinvald commented Apr 23, 2019

Actually about the Generated code's return value is sometimes missing its generic type.

I'd propose not using the name at all, if the schema is defined as just a single ref statement.
It cripples the API a lot: two identical structs with just different names wouldn't be interchangeable in Rust, so say there are 2 methods:

fn one() -> SameType1
fn two(arg: SameType2)

The SameType1 and SameType2 have common schema and different names, therefore user wouldn't be able to do two(one()) call without manually converting the same type into another same type:

let st1 = one();
let st2 = SameType2 {
    a: st1.a,
    b: st1.b,
    c: st1.c,
    ...
};
two(st2);

@shanejonas
Copy link
Member Author

@mersinvald names shouldn't be empty, I'll fix that up in the openrpc.json for multi-geth

@shanejonas
Copy link
Member Author

shanejonas commented Apr 26, 2019

@mersinvald the openrpc.json is all fixed up and filled out, latest is here: https://raw.githubusercontent.com/etclabscore/multi-geth/feat/openrpc-discover/rpc/openrpc.json

@BelfordZ
Copy link
Member

related to #169

@mersinvald
Copy link

mersinvald commented May 13, 2019

@shanejonas
Copy link
Member Author

this openrpc file lives here now

https://github.com/etclabscore/ethereum-json-rpc-specification

@mersinvald

@BelfordZ
Copy link
Member

fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants