Skip to content

eth/protocols/eth: remove magic numbers in tests #21999

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
Merged
merged 1 commit into from
Dec 14, 2020
Merged

eth/protocols/eth: remove magic numbers in tests #21999

merged 1 commit into from
Dec 14, 2020

Conversation

@Mr-Leshiy
Copy link
Contributor

@Mr-Leshiy Mr-Leshiy commented Dec 11, 2020 •
edited
Loading

Have removed magic numbers, have replaced it with the variables usage from the protocol.go

// eth protocol message codes
const (
	StatusMsg          = 0x00
	NewBlockHashesMsg  = 0x01
	TransactionMsg     = 0x02
	GetBlockHeadersMsg = 0x03
	BlockHeadersMsg    = 0x04
	GetBlockBodiesMsg  = 0x05
	BlockBodiesMsg     = 0x06
	NewBlockMsg        = 0x07
	GetNodeDataMsg     = 0x0d
	NodeDataMsg        = 0x0e
	GetReceiptsMsg     = 0x0f
	ReceiptsMsg        = 0x10

	// New protocol message codes introduced in eth65
	//
	// Previously these message ids were used by some legacy and unsupported
	// eth protocols, reown them here.
	NewPooledTransactionHashesMsg = 0x08
	GetPooledTransactionsMsg      = 0x09
	PooledTransactionsMsg         = 0x0a
)

MariusVanDerWijden
MariusVanDerWijden approved these changes Dec 11, 2020
View reviewed changes
Copy link
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

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

LGTM, even fixes a small bug in l.334

holiman
holiman approved these changes Dec 11, 2020
View reviewed changes
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

eth/handler_test.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Ha, this was wrong before, good catch there!

@holiman
Copy link
Contributor

holiman commented Dec 11, 2020

Let's hold this PR though, and rebase+apply it after #21482 is merged

@Mr-Leshiy
Copy link
Contributor Author

@holiman , no problem ! I am not in hurry :)

@holiman holiman force-pushed the feature/refactor_magic_numbers branch from ae75e7e to f3da993 Compare December 14, 2020 09:36
@holiman holiman changed the title P2P tests: remove magic numbers eth/protocols/eth: remove magic numbers in tests Dec 14, 2020
@holiman
Copy link
Contributor

holiman commented Dec 14, 2020

Rebased

@holiman holiman added this to the 1.10.0 milestone Dec 14, 2020
@holiman holiman merged commit 0fe66f8 into ethereum:master Dec 14, 2020
@holiman
Copy link
Contributor

holiman commented Dec 14, 2020

Thanks!

@Mr-Leshiy Mr-Leshiy deleted the feature/refactor_magic_numbers branch December 14, 2020 15:16
@quorumbot quorumbot mentioned this pull request Sep 3, 2021
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MariusVanDerWijden MariusVanDerWijden MariusVanDerWijden approved these changes

@karalabe karalabe Awaiting requested review from karalabe

@rjl493456442 rjl493456442 Awaiting requested review from rjl493456442 rjl493456442 is a code owner

+1 more reviewer

@holiman holiman holiman approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.10.0

Development

Successfully merging this pull request may close these issues.

3 participants

@Mr-Leshiy @holiman @MariusVanDerWijden