RPC - Bugs, missing methods, inconsistencies

[sisou]

This is a collection issue for bugs, missing methods and inconsistencies I found when testing the RPC server:

• Method blockByNumber does not accept a number as the first argument, only stringified numbers
• Methods blockByNumber and blockByHash require a second argument (include transactions: bool) - I think this should be optional and default to false
• Naming inconsistency: blockByNumber/Hash vs getTransactionByHash - make get prefix consistent (either with or without)
• Panic when requesting getAccount with the staking contract address: thread 'tokio-runtime-worker' panicked at 'Failed to serialize return value: Error("the enum variant Account::Staking cannot be serialized", line: 0, column: 0)': /usr/local/cargo/git/checkouts/jsonrpc-9fb05bf7e43ac4a6/1d17aa2/nimiq-jsonrpc-server/src/lib.rs:513
• Missing method to request current consensus state
• Missing method to request current peerCount, peerList, peerState <peerId>
• Missing method to set log level
• Missing method to set constants
• Missing method to retrieve validatorKey, validatorId
• Missing method to retrieve minFeePerByte
• Missing method to unsubscribefrom subscriptions
• mempool methods throw "not implemented" error
• Properties of returned objects mix camelCase and snake_case within the same object (example: Block { blockNumber, extra_data, parentHash, parent_election_hash }
• Accounts from getAccount are returned with the account type as a property instead of a value: {"Basic":{"balance":168157712802}} - the type should rather be the value of a type property
• Interacting with a locked wallet panics instead of returning the error correctly: thread 'tokio-runtime-worker' panicked at 'called Result::unwrap()on anErr value: UnlockedWalletNotFound(Address("8314ae0f9e42e6d872c0bc07b09e2a278b1dfbae"))': rpc-server/src/dispatchers/consensus.rs:124
• getTransactionByHash, getTransactionReceipt not implemented
• All transaction-creation/sending methods require a last validityStartHeight argument. I think this should be optional and default to the current head height
• validityStartHeight argument for transaction creation/sending methods does not accept numbers, only stringified numbers
• Closing the websocket after subscribing to headers does not close the stream on the server and leads to continuous errors: ERROR nimiq_jsonrpc_server | Queue error: channel closed
• Creating a validator reactivation transactions fails (on soeren/rpc branch)

[jgraef]

I'll work on this. Most of it seems easy to fix. (Sorry for accidentally closing it :D)

the blockByNumber only accepting strings is certainly a bug, but it stems from the fact that core-js's blockByNumber either accepts the block number or "latest". I'd suggest splitting this:

• blockByNumber(block_number: u32)
• blockLatest()

I don't like the naming scheme too much. I'd rather call the methods getBlockByNumber and getLatestBlock and so on. Any opinions?

[jgraef]

Yes, the argument include_transactions should be optional, but that's not supported by nimiq-jsonrpc right now. I opened a separate issue for that: nimiq/jsonrpc#1

[brunoffranca]

@jgraef For the getTransactionByHash you need a Blockchain method that doesn't yet exist, but I'm working on that. I'll ping you when that's merged.

[jgraef]

Here's the branch where I'm working on this: https://github.com/nimiq/core-rs-albatross/tree/janosch/rpc

Since this is needed for going further with the testnet, this is high priority, right?

[jgraef]

Unsubscribing to a stream is another missing feature in nimiq-jsonrpc: nimiq/jsonrpc#2

[brunoffranca]

@jgraef Done. It's here 51a184f.

[vlvrd]

The unsubscribe functionality is enabled now: nimiq/jsonrpc@41d26fc

The stream is also closed in the server when the websocket is closed, so that is also fixed but this uncovers a bug in the observer when following blocks since we keep sending blocks to a receiver that no longer exists.

[sisou]

Closing PR was reverted.

[brunoffranca]

Re-merged.