More types: block, transactions, receipt, and log. #20
Conversation
| @@ -366,14 +422,16 @@ mod tests { | |||
| Eth:block:block_by_hash, BlockId::Hash(0x123.into()), true | |||
| => | |||
| "eth_getBlockByHash", vec![r#""0x0000000000000000000000000000000000000000000000000000000000000123""#, r#"true"#]; | |||
| Value::Null => () | |||
| ::serde_json::from_str(EXAMPLE_BLOCK).unwrap() | |||
| => ::serde_json::from_str::<Block>(EXAMPLE_BLOCK).unwrap() | |||
There was a problem hiding this comment.
side note -- testing deserialization isn't really valuable for these tests. we're more interested in whether the right call gets dispatched.
There was a problem hiding this comment.
Right, for simple structures it indeed might not make much sense, but still choosing right params to deserialize from is important and it's part of the logic, and some methods will do different things depending on the output.
Maybe instead of deserializing structs from strings we should just put Default::default()?
There was a problem hiding this comment.
but then we'd also have to serialize the default struct to value -- not a huge win, but you might be right
| pub gas_price: U256, | ||
| /// Gas amount | ||
| pub gas: U256, | ||
| /// Inpur data |
| /// Block number. Null when pending. | ||
| #[serde(rename="blockNumber")] | ||
| pub block_number: Option<U256>, | ||
| /// Transaction Index. Null when pending. |
| pub total_difficulty: U256, | ||
| /// Seal fields | ||
| #[serde(rename="sealFields")] | ||
| pub seal_fields: Vec<Bytes>, |
There was a problem hiding this comment.
Should it be Option<Vec<Bytes>> or will it default to empty Vec if the field is not there?
There was a problem hiding this comment.
I think as things are right now it will fail to deserialize on the field missing. I suppose it could be optional, but then we'd also have to support non-generic nonce and mixHash fields.
| @@ -424,7 +482,8 @@ mod tests { | |||
| Eth:transaction:tx_by_hash, TransactionId::Hash(0x123.into()) | |||
| => | |||
| "eth_getTransactionByHash", vec![r#""0x0000000000000000000000000000000000000000000000000000000000000123""#]; | |||
| Value::Array(vec![]) => Some(()) | |||
| ::serde_json::from_str(EXAMPLE_TX).unwrap() | |||
There was a problem hiding this comment.
maybe use serde_json to avoid those ::?
| @@ -366,14 +422,16 @@ mod tests { | |||
| Eth:block:block_by_hash, BlockId::Hash(0x123.into()), true | |||
| => | |||
| "eth_getBlockByHash", vec![r#""0x0000000000000000000000000000000000000000000000000000000000000123""#, r#"true"#]; | |||
| Value::Null => () | |||
| ::serde_json::from_str(EXAMPLE_BLOCK).unwrap() | |||
| => ::serde_json::from_str::<Block>(EXAMPLE_BLOCK).unwrap() | |||
There was a problem hiding this comment.
Right, for simple structures it indeed might not make much sense, but still choosing right params to deserialize from is important and it's part of the logic, and some methods will do different things depending on the output.
Maybe instead of deserializing structs from strings we should just put Default::default()?
|
For some reason I can't respond directly to your comment which said this, but the issue with passing |
| where D: Deserializer | ||
| { | ||
| let res = Vec::<Transaction>::deserialize(deserializer).map(BlockTransactions::Full) | ||
| .or_else(|_| Vec::<H256>::deserialize(deserializer).map(BlockTransactions::Hashes)); |
There was a problem hiding this comment.
so this line seems not to work as expected -- it only works if the type of transaction you're trying to get comes first. My guess is that the calls of the first attempt's visit_* leave the deserializer in an invalid state to deserialize the other type. We might just want to make Block generic over a transaction type and change the eth API to have two calls:
block(&self, BlockId) -> Block<H256> and
block_with_txs(&self, BlockId) -> Block<Transaction>
(uncle functions will return Block<H256>)
There was a problem hiding this comment.
I like splitting the API better.
No description provided.