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

Make CallRequest's to field optional #348

Merged
merged 3 commits into from
May 19, 2020

Conversation

HCastano
Copy link
Contributor

According to the Ethereum Wiki page on JSON-RPC, the eth_call and eth_estimateGas calls use the same parameters, with the exception that all the fields for eth_estimateGas are optional.

Currently, CallRequest's to field was not optional. This PR changes that, making it suitable for use with eth_estimateGas.

I have a question about the RPC test I added with a None value for to. I didn't know what eth_estimateGas would return in that case, so I left the response from the test I copied. It passes, but I'm not sure it should considering that Value::String("0x123".into()) => 0x123 was in reference to the to address. What should this test check instead?

When it is used in the `eth_estimateGas` RPC call, the docs
state that all fields are optional.
Copy link
Owner

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

src/api/eth.rs Outdated
@@ -502,9 +502,21 @@ mod tests {
Value::String("0x123".into()) => 0x123
);

// NOTE: I don't know why this is passing
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

 rpc_test! (
     /// What you want to call (with a test name)
      Eth:estimate_gas:optional_to_addr, CallRequest {
        from: None, to: None,
        gas: None, gas_price: None,
        value: Some(0x1.into()), data: None,
      }, None
      =>
     // what should be received (JSON)
      "eth_estimateGas", vec![r#"{"value":"0x1"}"#];
      /// What should the function return (`Value::String`) and what we expect to receive (JSON)
      Value::String("0x123".into()) => 0x123
    );

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is correct :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants