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

cmd/devp2p/internal/ethtest: use shared message types #22315

Merged
merged 5 commits into from
Feb 16, 2021

Conversation

renaynay
Copy link
Contributor

No description provided.

Hash common.Hash // Hash of one particular block being announced
Number uint64 // Number of one particular block being announced
type NewBlockHashes struct {
*eth.NewBlockHashesPacket
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be better not to embed it here. If you just use

type NewBlockHashes eth.NewBlockHashesPacket

you can still use the fields of the upstream definition, but don't need to wrap all the constructor call sites

@renaynay renaynay force-pushed the replace_message_types_ethtestsuite branch from 270c44e to e3505e2 Compare February 15, 2021 17:21
@renaynay renaynay force-pushed the replace_message_types_ethtestsuite branch from e3505e2 to 5b0e3cb Compare February 15, 2021 17:24
@renaynay renaynay requested a review from fjl February 15, 2021 17:57
@renaynay renaynay marked this pull request as ready for review February 15, 2021 17:58
@renaynay renaynay changed the title [DRAFT]: replace message types ethtestsuite cmd/devp2p: replace message types ethtestsuite Feb 15, 2021
Copy link
Contributor

@fjl fjl left a comment

Choose a reason for hiding this comment

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

Looks very good now, just need to remove this extra type.

cmd/devp2p/internal/ethtest/types.go Outdated Show resolved Hide resolved
@fjl fjl changed the title cmd/devp2p: replace message types ethtestsuite cmd/devp2p/internal/ethtest: replace message types ethtestsuite Feb 16, 2021
@fjl fjl changed the title cmd/devp2p/internal/ethtest: replace message types ethtestsuite cmd/devp2p/internal/ethtest: use shared message types Feb 16, 2021
@fjl fjl merged commit f9445e9 into ethereum:master Feb 16, 2021
@renaynay renaynay deleted the replace_message_types_ethtestsuite branch February 16, 2021 14:56
filipescuc pushed a commit to EthereumGenesys/go-ethereum that referenced this pull request Mar 2, 2021
This updates the eth protocol test suite to use the message type
definitions of the 'production' protocol implementation in eth/protocols/eth.
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