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

feat(etherscan): account endpoints #939

Merged
merged 4 commits into from
Feb 26, 2022

Conversation

tomtau
Copy link
Contributor

@tomtau tomtau commented Feb 21, 2022

Motivation

#485

Add etherscan.io account endpoints

Solution

Added them.

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog

@tomtau tomtau force-pushed the feat/etherscan-account branch 4 times, most recently from e6cb381 to 4f2c57f Compare February 21, 2022 06:21
@tomtau
Copy link
Contributor Author

tomtau commented Feb 21, 2022

FYI I checked forks and noticed @1wilkens had a WIP version https://github.com/1wilkens/ethers-rs/commit/f2eb53732fca388b0af21a756828e8efab8106d0 -- feel free to review if this PR works for you!

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for this.

A few nits and suggestions.

I'm fine with the AsRef<str> approach.

It'd be nice if you could copy some of the tests as examples to the API functions' docs

ethers-etherscan/src/account.rs Outdated Show resolved Hide resolved
ethers-etherscan/src/account.rs Outdated Show resolved Hide resolved
ethers-etherscan/src/account.rs Outdated Show resolved Hide resolved
ethers-etherscan/src/account.rs Outdated Show resolved Hide resolved
ethers-etherscan/src/account.rs Outdated Show resolved Hide resolved
ethers-etherscan/src/account.rs Outdated Show resolved Hide resolved
ethers-etherscan/src/account.rs Outdated Show resolved Hide resolved
ethers-etherscan/src/account.rs Outdated Show resolved Hide resolved
ethers-etherscan/src/account.rs Outdated Show resolved Hide resolved
ethers-etherscan/src/account.rs Outdated Show resolved Hide resolved
Copy link
Owner

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

nice work! just small nit around type defs

ethers-etherscan/src/account.rs Outdated Show resolved Hide resolved
ethers-etherscan/src/account.rs Outdated Show resolved Hide resolved
ethers-etherscan/src/account.rs Outdated Show resolved Hide resolved
@tomtau
Copy link
Contributor Author

tomtau commented Feb 24, 2022

@gakonst I tried to put more restricted types on the response fields, but I'm not sure whether it'll be worth it: e14d054

There are a few issues at this moment:

  • I don't know if Etherscan API response schema is documented anywhere, this page only shows a few examples: https://docs.etherscan.io/api-endpoints/accounts so I'm not sure whether there are more edge cases it may fail to parse etc.
  • I haven't checked in detail whether every field is interpreted correctly -- for example, whether the nonce value is a plain decimal string or a hexadecimal string (and I think U256's default deserialization assumes the latter?)
  • I'm not sure how common the edge case values are -- my sense is that they are not (at least for the GENESIS ones), so it may be unnecessary for many end use cases to worry about them.

Maybe a middle ground could be to keep the returned response types flexible as before (maybe they could be renamed as e.g. RawNormalTransaction to make it clear they are raw responses that weren't fully checked) + add more restricted cleaned up domain types with a TryFrom conversion from the raw types.
What do you think?

@tomtau tomtau requested a review from gakonst February 24, 2022 07:54
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

Awesome!
can we also use Address, U256 etc in the new InternalTransaction and Erc20TokenTransferEven types?

@mattsse
Copy link
Collaborator

mattsse commented Feb 24, 2022

  • I don't know if Etherscan API response schema is documented anywhere, this page only shows a few examples: https://docs.etherscan.io/api-endpoints/accounts so I'm not sure whether there are more edge cases it may fail to parse etc.
  • I haven't checked in detail whether every field is interpreted correctly -- for example, whether the nonce value is a plain decimal string or a hexadecimal string (and I think U256's default deserialization assumes the latter?)

that'll be discovered eventually so we can fix once that happens if there's a serde issue

  • I'm not sure how common the edge case values are -- my sense is that they are not (at least for the GENESIS ones), so it may be unnecessary for many end use cases to worry about them.

not familiar with the GENESIS values, but that sounds reasonable

@tomtau
Copy link
Contributor Author

tomtau commented Feb 25, 2022

can we also use Address, U256 etc in the new InternalTransaction and Erc20TokenTransferEvent types?

They are now updated to be like that if that's the preferred way (as opposed to keeping them flexible and having conversions to domain types).

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Owner

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

Sick - thank you for bearing with our nits sir.

@gakonst gakonst merged commit e7befca into gakonst:master Feb 26, 2022
@1wilkens
Copy link

1wilkens commented Mar 3, 2022

FYI I checked forks and noticed @1wilkens had a WIP version 1wilkens@f2eb537 -- feel free to review if this PR works for you!

Sorry didn't even see your mention. Awesome that this is now merged, my branch was indeed early WIP so thanks for finishing this :)

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.

None yet

4 participants