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

Handle invalid api key response #56

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

JonoPrest
Copy link

Hi there 👋🏼

I found an issue when calling contract_abi, where the error returned was a serde error when it should have been an InvalidApiKey error.

This PR

  1. Changes "starts_with" to "contains" for looking through result to handle the result I was getting.
  2. Changes the way ResponseData gets deserialized to correctly deserialize errors
  3. Add a test for the case I was hitting where it was not being deserialized as ResponseData::Error

if let Some(ref result) = result {
if result.starts_with("Max rate limit reached") {
return Err(EtherscanError::RateLimitExceeded);
} else if result.to_lowercase() == "invalid api key" {
}
if result.to_lowercase().contains("invalid api key") {
Copy link
Member

Choose a reason for hiding this comment

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

could you please submit this change separately?

Copy link
Author

@JonoPrest JonoPrest Jul 24, 2024

Choose a reason for hiding this comment

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

Hey @mattsse, I'd be happy to but the reality is that it is a dead case without the change to the deserializer. The test case that I added and the original case where result == "invalid api key" can never be hit since it would deserialize as a ResponseData::Success case rathere than ResponseData::Error

Let me know if you would still like it as a separate change?

Comment on lines 545 to 554
#[test]
fn can_parse_etherscan_mainnet_invalid_api_key() {
let err = json!({
"status":"0",
"message":"NOTOK",
"result":"Missing/Invalid API Key"
});
let resp: ResponseData<Address> = serde_json::from_value(err).unwrap();
assert!(matches!(resp, ResponseData::Error { .. }));
}
Copy link
Member

Choose a reason for hiding this comment

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

this test succeeds on main, so still unclear what's the motivation for this

Copy link
Author

Choose a reason for hiding this comment

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

My mistake with writing the test.

It should have been

 let resp: ResponseData<String> = serde_json::from_value(err).unwrap();

or ResponseData<Option<String>> like in the contract_abi case I tested on.

I've updated the test but you will see it should fail on main without the updated serializer.

The problem is that the deserializer can match ResponseData of string/option string as a success or an error case. Since success takes precedence it deserializes as a success.

Copy link
Author

Choose a reason for hiding this comment

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

Hey @mattsse, just flagging my above comment this is still an issue and the broken case isn't solved.

I see you opted to make your own PR so I'll leave this one unless you would like the change I made in which case I can update it for you or make any updates you might want.

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