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

Update Docs to Show Return of Eth.sendRawTransaction() to be HexBytes, not string. #1384

Merged
merged 1 commit into from Jul 15, 2019
Merged

Conversation

ghost
Copy link

@ghost ghost commented Jul 13, 2019

What was wrong?

The documentation was not clear that the return type of Eth.sendRawTransaction is a HexBytes object, as originally pointed out by @wjmelements in Issue #1367.

How was it fixed?

updating web3.eth.rst, line ~638.

Cute Animal Picture

IMG_3703

Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

Thanks for the doc update @crawfordleeds! What was your reasoning behind changing the address in the example?

@ghost
Copy link
Author

ghost commented Jul 15, 2019

Good catch, that was a mistake, and likely a remnant of some copy and pasting with another project. Sorry about that. I pushed a reversion to the original address. Thanks!

Copy link
Collaborator

@kclowes kclowes 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! 🚢

@ghost
Copy link
Author

ghost commented Jul 15, 2019

Actually, now that I look into it, don't these addresses need to use the EIP 55 checksum address encoding? I just tried to run the code with the original address, '0xd3cda913deb6f67967b99d67acdfa1712c293601', and receive the following error:

TypeError: Transaction had invalid fields: {'to': '0xd3cda913deb6f67967b99d67acdfa1712c293601'}

It works fine with the one I originally changed it to, '0xF0109fC8DF283027b6285cc889F5aA624EaC1F55'.

Am I mistaken about that? If not, perhaps this is deserving of its own issue because there are a few instances of the docs not using EIP 55 checksummed addresses. I'm happy to take a crack at it too.

@kclowes
Copy link
Collaborator

kclowes commented Jul 15, 2019

Yep, you're right. They should be using checksummed addresses. Let's keep this PR as-is and open up a new issue to make sure all addresses in the docs are checksums

@ghost
Copy link
Author

ghost commented Jul 15, 2019

Sounds great, I'll keep an eye out for that issue.

@kclowes
Copy link
Collaborator

kclowes commented Jul 15, 2019

@crawfordleeds added the issue! #1388 if you want to take a crack at it. Thanks again!

@kclowes kclowes merged commit 68aced7 into ethereum:master Jul 15, 2019
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