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.

Sign up for GitHub

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

Jump to bottom

Implement eth_submitHashrate and eth_submitWork #1280

Merged
merged 2 commits into from
Mar 15, 2019
Merged

Implement eth_submitHashrate and eth_submitWork #1280

merged 2 commits into from
Mar 15, 2019

Conversation

njgheorghita
Copy link
Contributor

@njgheorghita njgheorghita commented Mar 14, 2019 •
edited
Loading

What was wrong?

eth_submitHashrate needed implementation according to ERC 1474

Related to Issue #1273

Cute Animal Picture

image

@njgheorghita njgheorghita force-pushed the eth_submitHashrate branch 2 times, most recently from 32017cf to b5d74a8 Compare March 14, 2019 11:58
pipermerriam
pipermerriam reviewed Mar 14, 2019
View reviewed changes
Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

@kclowes let me know if you want me to also take a look at this.

@njgheorghita njgheorghita changed the title Implement eth_submitHashrate and integration test Implement eth_submitHashrate and eth_submitWork Mar 14, 2019
@njgheorghita njgheorghita force-pushed the eth_submitHashrate branch 5 times, most recently from 68b3b03 to 727b865 Compare March 14, 2019 15:26
carver
carver reviewed Mar 14, 2019
View reviewed changes
web3/_utils/rpc_abi.py Outdated
@@ -51,6 +51,7 @@
'eth_sendRawTransaction': ['bytes'],
'eth_sendTransaction': TRANSACTION_PARAMS_ABIS,
'eth_sign': ['address', 'bytes'],
'eth_submitHashrate': ['uint', None],
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is right (not sure if it's always bytes32)

Suggested change
'eth_submitHashrate': ['uint', None],
'eth_submitHashrate': ['uint', 'bytes'],

Also, maybe ['bytes8', 'bytes32', 'bytes32'] for eth_submitWork?

Adding this extra fields should automatically handle arguments like submitting hex strings, hex literals, or python bytes, as well as length checks.

Copy link
Collaborator

@kclowes kclowes Mar 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

geth and parity both specify 32 bytes for both the hashrate and the id, but I don't think the EIP does ¯_(ツ)_/¯

Copy link
Collaborator

Choose a reason for hiding this comment

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

Eh, the EIP is still a draft. Since geth & parity implement the same way, that wins over the doc. :)

web3/middleware/pythonic.py Outdated
apply_formatter_at_index(nonce_formatter, 0),
apply_formatter_at_index(hexstr_if_str(to_hex), 1),
apply_formatter_at_index(hexstr_if_str(to_hex), 2),
),
Copy link
Collaborator

Choose a reason for hiding this comment

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

All these changes to pythonic can go away with the rpc_abi change, I think.

@njgheorghita njgheorghita force-pushed the eth_submitHashrate branch 2 times, most recently from 75eb226 to 1d34cc4 Compare March 15, 2019 16:17
@njgheorghita
Copy link
Contributor Author

Ready for final review

kclowes
kclowes reviewed Mar 15, 2019
View reviewed changes
web3/_utils/rpc_abi.py Outdated
@@ -51,6 +51,8 @@
'eth_sendRawTransaction': ['bytes'],
'eth_sendTransaction': TRANSACTION_PARAMS_ABIS,
'eth_sign': ['address', 'bytes'],
'eth_submitHashrate': ['uint', 'bytes'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this line should be [bytes32, bytes32]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While parity's fine with that, it seems like geth doesn't like that, going with ['uint', 'bytes32']

Copy link
Collaborator

Choose a reason for hiding this comment

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

Iiiinteresting. geth's docs are wrong then. Sounds good!

kclowes
kclowes approved these changes Mar 15, 2019
View reviewed changes
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.

LGTM! :shipit:

@njgheorghita njgheorghita force-pushed the eth_submitHashrate branch 2 times, most recently from b04f966 to ac274db Compare March 15, 2019 17:40
@kclowes
Copy link
Collaborator

kclowes commented Mar 15, 2019

I just merged an Unreleased section to docs/releases.rst. Before you merge, will you add a blurb there please? Thanks!

@njgheorghita njgheorghita merged commit 86c4457 into ethereum:master Mar 15, 2019
@njgheorghita njgheorghita deleted the eth_submitHashrate branch March 15, 2019 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carver carver carver left review comments

@pipermerriam pipermerriam pipermerriam left review comments

@kclowes kclowes kclowes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@njgheorghita @kclowes @carver @pipermerriam