Skip to content
This repository was archived by the owner on Jul 1, 2021. It is now read-only.

Add get_cmd_id_offsets helper function #836

Merged

Conversation

pipermerriam
Copy link
Member

@pipermerriam pipermerriam commented Jul 24, 2019

extracted from #684

What was wrong?

For a given set of protocols we need to compute the offsets for their command ids

How was it fixed?

Added a helper function for computing these offset values.

To-Do

  • Clean up commit history

Cute Animal Picture

15-01-17 Sport - P1150162 C1000

@pipermerriam pipermerriam force-pushed the piper/add-get_cmd_id_offsets_helper branch from a8c5f5b to 49c0f02 Compare July 24, 2019 18:05
@pipermerriam pipermerriam requested a review from lithp July 24, 2019 18:06
@pipermerriam pipermerriam mentioned this pull request Jul 24, 2019
2 tasks
Copy link
Contributor

@lithp lithp left a comment

Choose a reason for hiding this comment

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

Looks good to me!



# Reserved command length for the base `p2p` protocol
P2P_PROTOCOL_COMMAND_LENGTH = 16
Copy link
Contributor

Choose a reason for hiding this comment

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

It could be nice to include a link here to https://github.com/ethereum/devp2p/blob/master/rlpx.md#message-id-based-multiplexing, as explanation for why cmd length is 16 even though there are only four commands.

p2p/protocol.py Outdated
lambda prev_offset, protocol_class: prev_offset + protocol_class.cmd_length,
protocol_types,
P2P_PROTOCOL_COMMAND_LENGTH,
))[:-1]
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that I know what it does the code as-is makes sense but the easiest way for me to figure out what it was doing was to play with the implementation. Dropping what I came up here with for inspiration, I think a comment explaining why the [:-1] exists could be valuable.

def get_cmd_offsets(protocol_types: Tuple[Type[ProtocolAPI], ...]) -> Tuple[int, ...]:
    protocol_lengths = (protocol_type.cmd_length for protocol_type in protocol_types)
    offsets = accumulate(operator.add, protocol_lengths, P2P_PROTOCOL_COMMAND_LENGTH)
    return tuple(take(len(protocol_types), offsets))  # don't compute more offsets than necessary```

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I don't know how you plan on using this method, but it might be slightly cleaner and more general if, instead of hard-coding P2P_PROTOCOL_COMMAND_LENGTH, it was always passed P2PProtocol as its first argument.

@pipermerriam pipermerriam force-pushed the piper/add-get_cmd_id_offsets_helper branch from 49c0f02 to cea75e3 Compare July 25, 2019 19:23
@pipermerriam pipermerriam merged commit 1cebcf5 into ethereum:master Jul 25, 2019
@pipermerriam pipermerriam deleted the piper/add-get_cmd_id_offsets_helper branch July 25, 2019 20:01
@carver carver mentioned this pull request Aug 29, 2019
42 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants