{"action":"created","comment":{"url":"https://api.github.com/repos/ethereum/py-evm/pulls/comments/211416028","pull_request_review_id":147832969,"id":211416028,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDIxMTQxNjAyOA==","diff_hunk":"@@ -123,6 +126,27 @@ def encode(self, data: _DecodedMsgType) -> Tuple[bytes, bytes]:\n         return header, body\n \n \n+TCommandPayload = TypeVar('TCommandPayload', bound=_DecodedMsgType)","path":"p2p/protocol.py","position":19,"original_position":19,"commit_id":"7ca13388d003bd42aa4c79a84040fe740c8cc5ce","original_commit_id":"7ca13388d003bd42aa4c79a84040fe740c8cc5ce","user":{"login":"carver","id":205327,"node_id":"MDQ6VXNlcjIwNTMyNw==","avatar_url":"https://avatars0.githubusercontent.com/u/205327?v=4","gravatar_id":"","url":"https://api.github.com/users/carver","html_url":"https://github.com/carver","followers_url":"https://api.github.com/users/carver/followers","following_url":"https://api.github.com/users/carver/following{/other_user}","gists_url":"https://api.github.com/users/carver/gists{/gist_id}","starred_url":"https://api.github.com/users/carver/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/carver/subscriptions","organizations_url":"https://api.github.com/users/carver/orgs","repos_url":"https://api.github.com/users/carver/repos","events_url":"https://api.github.com/users/carver/events{/privacy}","received_events_url":"https://api.github.com/users/carver/received_events","type":"User","site_admin":false},"body":"This is actually the outgoing one, but we'll end up with much clearer `TRequestPayload` and `TResponsePayload`.","created_at":"2018-08-20T21:39:34Z","updated_at":"2018-08-20T21:39:34Z","html_url":"https://github.com/ethereum/py-evm/pull/1184#discussion_r211416028","pull_request_url":"https://api.github.com/repos/ethereum/py-evm/pulls/1184","author_association":"CONTRIBUTOR","_links":{"self":{"href":"https://api.github.com/repos/ethereum/py-evm/pulls/comments/211416028"},"html":{"href":"https://github.com/ethereum/py-evm/pull/1184#discussion_r211416028"},"pull_request":{"href":"https://api.github.com/repos/ethereum/py-evm/pulls/1184"}},"in_reply_to_id":211077235},"pull_request":{"url":"https://api.github.com/repos/ethereum/py-evm/pulls/1184","id":208452574,"node_id":"MDExOlB1bGxSZXF1ZXN0MjA4NDUyNTc0","html_url":"https://github.com/ethereum/py-evm/pull/1184","diff_url":"https://github.com/ethereum/py-evm/pull/1184.diff","patch_url":"https://github.com/ethereum/py-evm/pull/1184.patch","issue_url":"https://api.github.com/repos/ethereum/py-evm/issues/1184","number":1184,"state":"open","locked":false,"title":"Reducing responsibility of BaseRequestManager","user":{"login":"carver","id":205327,"node_id":"MDQ6VXNlcjIwNTMyNw==","avatar_url":"https://avatars0.githubusercontent.com/u/205327?v=4","gravatar_id":"","url":"https://api.github.com/users/carver","html_url":"https://github.com/carver","followers_url":"https://api.github.com/users/carver/followers","following_url":"https://api.github.com/users/carver/following{/other_user}","gists_url":"https://api.github.com/users/carver/gists{/gist_id}","starred_url":"https://api.github.com/users/carver/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/carver/subscriptions","organizations_url":"https://api.github.com/users/carver/orgs","repos_url":"https://api.github.com/users/carver/repos","events_url":"https://api.github.com/users/carver/events{/privacy}","received_events_url":"https://api.github.com/users/carver/received_events","type":"User","site_admin":false},"body":"### What was wrong?\r\n\r\n- Logic for a new request/response cycle was spread around\r\n- BaseRequestManager had a lot of responsibilities and mutual dependencies with subclasses\r\n\r\n### How was it fixed?\r\n\r\nA lot going on here, so I'll try to summarize. Everything was kind of connected, so I'm sorry this came in one big dump.\r\n\r\n- Consistently refer to `Message` as the decoded data that the p2p protocol returned, and `Result` as the normalized data that is finally returned\r\n- Removed the `BaseRequestManager` in favor of a `MessageManager` which is responsible for returning a stream of messages that match the command of interest\r\n- Added an `ExchangeManager` (where `Exchange` is the IETF word for a paired Request/Response) which is responsible for validating potential messages from the `MessageManager` and handing back the normalized result\r\n- The `Handler` now uses `Exchange` objects\r\n- `Exchange` is meant to be a very small class that ties things together: a normalizing component, a validation component, a `BaseRequest`, the `ExchangeManager`, and other data munging (more details below)\r\n- Changed the `BaseRequest` to only track: the outgoing command type, the response command type, and the parameters that p2p needs to send\r\n- Added the ability to send a command to a peer, with only a `BaseRequest` instance\r\n- Reduced the `Validator` classes so they *only* validate the final result\r\n- Added option for custom lightweight message validator (eg~ for validating LES)\r\n- Updated all the currently implemented commands to work this way\r\n\r\nThe most important piece to understand when implementing a new command is the `Exchange`. It ties together everything new you need. Here are two annotated examples of `Exchange`, and some related definitions:\r\n\r\n```py\r\nclass BaseExchange(ABC, Generic[TCommandPayload, TMsg, TResult]):\r\n    \"\"\"\r\n    TCommandPayload is the data as passed directly to the p2p command\r\n    TMsg is the data as received directly from the p2p command response\r\n    TResult is the response data after normalization\r\n    \"\"\"\r\n    def __init__(self, manager: ExchangeManager[TCommandPayload, TMsg, TResult]) -> None:\r\n        self._manager = manager\r\n    ...\r\n\r\n\r\n# This is a core example of an exchange. It will be exposed directly on `peer.requests.get_node_data`\r\nclass GetNodeDataExchange(BaseExchange[Tuple[Hash32, ...], Tuple[bytes, ...], NodeDataBundles]):\r\n    _normalizer = GetNodeDataNormalizer()  # the normalizer is responsible for message->result conversion (and counting the number of results)\r\n\r\n    # this is the core await-able call, which returns the normalized result\r\n    async def __call__(self, node_hashes: Tuple[Hash32, ...]) -> NodeDataBundles:\r\n\r\n        # prepare to validate the result by saving a copy of the request arguments\r\n        validator = GetNodeDataValidator(node_hashes)\r\n\r\n        # form the request, which also knows its own command type, and response command type\r\n        request = GetNodeDataRequest(node_hashes)\r\n\r\n        # This is a convenience method on the super-class. It mostly just calls out to the \r\n        # exchange manager, which waits for a result that validates, and returns the normalized version\r\n        return await self.get_result(request, self._normalizer, validator)\r\n\r\n\r\n#\r\n# The following helper function and exchange shows how to validate the underlying message\r\n#\r\n\r\ndef _match_message_request_id(payload: Dict[str, Any], message: Dict[str, Any]) -> None:\r\n    if payload['request_id'] != message['request_id']:\r\n        raise ValidationError(\"Request `id` does not match\")\r\n\r\n\r\nclass GetBlockHeadersExchange(BaseExchange[Dict[str, Any], Dict[str, Any], Tuple[BlockHeader, ...]]):\r\n    _normalizer = BlockHeadersNormalizer()\r\n    async def __call__(\r\n            self,\r\n            block_number_or_hash: BlockIdentifier,\r\n            max_headers: int = None,\r\n            skip: int = 0,\r\n            reverse: bool = True) -> Tuple[BlockHeader, ...]:\r\n\r\n        validator = GetBlockHeadersValidator(block_number_or_hash, max_headers, skip, reverse)\r\n\r\n        # add the request ID so that the request object has everything it needs\r\n        command_args = (block_number_or_hash, max_headers, skip, reverse, gen_request_id())\r\n\r\n        request = GetBlockHeadersRequest(*command_args)\r\n\r\n        # Wait for a result that matches *both* the message and result validators\r\n        return await self.get_result(\r\n            request,\r\n            self._normalizer,\r\n            validator,\r\n            # the superclass passes the request command arguments to the msg validator:\r\n            _match_message_request_id,  \r\n        )\r\n```\r\n\r\nStill TODO, probably in a different PR:\r\n- Not many docs besides this writeup right now :/\r\n\r\n#### Cute Animal Picture\r\n\r\n![put a cute animal picture link inside the parentheses](http://www.thatcutesite.com/uploads/2010/07/tiny_little_snail.jpg)\r\n","created_at":"2018-08-15T01:33:39Z","updated_at":"2018-08-20T21:39:34Z","closed_at":null,"merged_at":null,"merge_commit_sha":"97696674d753b0e36dc05530aa9e92dd6ad46b95","assignee":null,"assignees":[],"requested_reviewers":[],"requested_teams":[],"labels":[],"milestone":null,"commits_url":"https://api.github.com/repos/ethereum/py-evm/pulls/1184/commits","review_comments_url":"https://api.github.com/repos/ethereum/py-evm/pulls/1184/comments","review_comment_url":"https://api.github.com/repos/ethereum/py-evm/pulls/comments{/number}","comments_url":"https://api.github.com/repos/ethereum/py-evm/issues/1184/comments","statuses_url":"https://api.github.com/repos/ethereum/py-evm/statuses/7ca13388d003bd42aa4c79a84040fe740c8cc5ce","head":{"label":"carver:request-manager-refactor","ref":"request-manager-refactor","sha":"7ca13388d003bd42aa4c79a84040fe740c8cc5ce","user":{"login":"carver","id":205327,"node_id":"MDQ6VXNlcjIwNTMyNw==","avatar_url":"https://avatars0.githubusercontent.com/u/205327?v=4","gravatar_id":"","url":"https://api.github.com/users/carver","html_url":"https://github.com/carver","followers_url":"https://api.github.com/users/carver/followers","following_url":"https://api.github.com/users/carver/following{/other_user}","gists_url":"https://api.github.com/users/carver/gists{/gist_id}","starred_url":"https://api.github.com/users/carver/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/carver/subscriptions","organizations_url":"https://api.github.com/users/carver/orgs","repos_url":"https://api.github.com/users/carver/repos","events_url":"https://api.github.com/users/carver/events{/privacy}","received_events_url":"https://api.github.com/users/carver/received_events","type":"User","site_admin":false},"repo":{"id":111029002,"node_id":"MDEwOlJlcG9zaXRvcnkxMTEwMjkwMDI=","name":"py-evm","full_name":"carver/py-evm","owner":{"login":"carver","id":205327,"node_id":"MDQ6VXNlcjIwNTMyNw==","avatar_url":"https://avatars0.githubusercontent.com/u/205327?v=4","gravatar_id":"","url":"https://api.github.com/users/carver","html_url":"https://github.com/carver","followers_url":"https://api.github.com/users/carver/followers","following_url":"https://api.github.com/users/carver/following{/other_user}","gists_url":"https://api.github.com/users/carver/gists{/gist_id}","starred_url":"https://api.github.com/users/carver/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/carver/subscriptions","organizations_url":"https://api.github.com/users/carver/orgs","repos_url":"https://api.github.com/users/carver/repos","events_url":"https://api.github.com/users/carver/events{/privacy}","received_events_url":"https://api.github.com/users/carver/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/carver/py-evm","description":"A Python implementation of the Ethereum Virtual Machine","fork":true,"url":"https://api.github.com/repos/carver/py-evm","forks_url":"https://api.github.com/repos/carver/py-evm/forks","keys_url":"https://api.github.com/repos/carver/py-evm/keys{/key_id}","collaborators_url":"https://api.github.com/repos/carver/py-evm/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/carver/py-evm/teams","hooks_url":"https://api.github.com/repos/carver/py-evm/hooks","issue_events_url":"https://api.github.com/repos/carver/py-evm/issues/events{/number}","events_url":"https://api.github.com/repos/carver/py-evm/events","assignees_url":"https://api.github.com/repos/carver/py-evm/assignees{/user}","branches_url":"https://api.github.com/repos/carver/py-evm/branches{/branch}","tags_url":"https://api.github.com/repos/carver/py-evm/tags","blobs_url":"https://api.github.com/repos/carver/py-evm/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/carver/py-evm/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/carver/py-evm/git/refs{/sha}","trees_url":"https://api.github.com/repos/carver/py-evm/git/trees{/sha}","statuses_url":"https://api.github.com/repos/carver/py-evm/statuses/{sha}","languages_url":"https://api.github.com/repos/carver/py-evm/languages","stargazers_url":"https://api.github.com/repos/carver/py-evm/stargazers","contributors_url":"https://api.github.com/repos/carver/py-evm/contributors","subscribers_url":"https://api.github.com/repos/carver/py-evm/subscribers","subscription_url":"https://api.github.com/repos/carver/py-evm/subscription","commits_url":"https://api.github.com/repos/carver/py-evm/commits{/sha}","git_commits_url":"https://api.github.com/repos/carver/py-evm/git/commits{/sha}","comments_url":"https://api.github.com/repos/carver/py-evm/comments{/number}","issue_comment_url":"https://api.github.com/repos/carver/py-evm/issues/comments{/number}","contents_url":"https://api.github.com/repos/carver/py-evm/contents/{+path}","compare_url":"https://api.github.com/repos/carver/py-evm/compare/{base}...{head}","merges_url":"https://api.github.com/repos/carver/py-evm/merges","archive_url":"https://api.github.com/repos/carver/py-evm/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/carver/py-evm/downloads","issues_url":"https://api.github.com/repos/carver/py-evm/issues{/number}","pulls_url":"https://api.github.com/repos/carver/py-evm/pulls{/number}","milestones_url":"https://api.github.com/repos/carver/py-evm/milestones{/number}","notifications_url":"https://api.github.com/repos/carver/py-evm/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/carver/py-evm/labels{/name}","releases_url":"https://api.github.com/repos/carver/py-evm/releases{/id}","deployments_url":"https://api.github.com/repos/carver/py-evm/deployments","created_at":"2017-11-16T22:23:49Z","updated_at":"2017-11-16T22:23:51Z","pushed_at":"2018-08-17T23:48:32Z","git_url":"git://github.com/carver/py-evm.git","ssh_url":"git@github.com:carver/py-evm.git","clone_url":"https://github.com/carver/py-evm.git","svn_url":"https://github.com/carver/py-evm","homepage":"","size":4881,"stargazers_count":0,"watchers_count":0,"language":"Python","has_issues":false,"has_projects":true,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":0,"mirror_url":null,"archived":false,"open_issues_count":0,"license":null,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"ethereum:master","ref":"master","sha":"dee977ee25a99bf662c0f4a15b49cef8e137895e","user":{"login":"ethereum","id":6250754,"node_id":"MDEyOk9yZ2FuaXphdGlvbjYyNTA3NTQ=","avatar_url":"https://avatars3.githubusercontent.com/u/6250754?v=4","gravatar_id":"","url":"https://api.github.com/users/ethereum","html_url":"https://github.com/ethereum","followers_url":"https://api.github.com/users/ethereum/followers","following_url":"https://api.github.com/users/ethereum/following{/other_user}","gists_url":"https://api.github.com/users/ethereum/gists{/gist_id}","starred_url":"https://api.github.com/users/ethereum/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ethereum/subscriptions","organizations_url":"https://api.github.com/users/ethereum/orgs","repos_url":"https://api.github.com/users/ethereum/repos","events_url":"https://api.github.com/users/ethereum/events{/privacy}","received_events_url":"https://api.github.com/users/ethereum/received_events","type":"Organization","site_admin":false},"repo":{"id":77478378,"node_id":"MDEwOlJlcG9zaXRvcnk3NzQ3ODM3OA==","name":"py-evm","full_name":"ethereum/py-evm","owner":{"login":"ethereum","id":6250754,"node_id":"MDEyOk9yZ2FuaXphdGlvbjYyNTA3NTQ=","avatar_url":"https://avatars3.githubusercontent.com/u/6250754?v=4","gravatar_id":"","url":"https://api.github.com/users/ethereum","html_url":"https://github.com/ethereum","followers_url":"https://api.github.com/users/ethereum/followers","following_url":"https://api.github.com/users/ethereum/following{/other_user}","gists_url":"https://api.github.com/users/ethereum/gists{/gist_id}","starred_url":"https://api.github.com/users/ethereum/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ethereum/subscriptions","organizations_url":"https://api.github.com/users/ethereum/orgs","repos_url":"https://api.github.com/users/ethereum/repos","events_url":"https://api.github.com/users/ethereum/events{/privacy}","received_events_url":"https://api.github.com/users/ethereum/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/ethereum/py-evm","description":"A Python implementation of the Ethereum Virtual Machine","fork":false,"url":"https://api.github.com/repos/ethereum/py-evm","forks_url":"https://api.github.com/repos/ethereum/py-evm/forks","keys_url":"https://api.github.com/repos/ethereum/py-evm/keys{/key_id}","collaborators_url":"https://api.github.com/repos/ethereum/py-evm/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/ethereum/py-evm/teams","hooks_url":"https://api.github.com/repos/ethereum/py-evm/hooks","issue_events_url":"https://api.github.com/repos/ethereum/py-evm/issues/events{/number}","events_url":"https://api.github.com/repos/ethereum/py-evm/events","assignees_url":"https://api.github.com/repos/ethereum/py-evm/assignees{/user}","branches_url":"https://api.github.com/repos/ethereum/py-evm/branches{/branch}","tags_url":"https://api.github.com/repos/ethereum/py-evm/tags","blobs_url":"https://api.github.com/repos/ethereum/py-evm/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/ethereum/py-evm/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/ethereum/py-evm/git/refs{/sha}","trees_url":"https://api.github.com/repos/ethereum/py-evm/git/trees{/sha}","statuses_url":"https://api.github.com/repos/ethereum/py-evm/statuses/{sha}","languages_url":"https://api.github.com/repos/ethereum/py-evm/languages","stargazers_url":"https://api.github.com/repos/ethereum/py-evm/stargazers","contributors_url":"https://api.github.com/repos/ethereum/py-evm/contributors","subscribers_url":"https://api.github.com/repos/ethereum/py-evm/subscribers","subscription_url":"https://api.github.com/repos/ethereum/py-evm/subscription","commits_url":"https://api.github.com/repos/ethereum/py-evm/commits{/sha}","git_commits_url":"https://api.github.com/repos/ethereum/py-evm/git/commits{/sha}","comments_url":"https://api.github.com/repos/ethereum/py-evm/comments{/number}","issue_comment_url":"https://api.github.com/repos/ethereum/py-evm/issues/comments{/number}","contents_url":"https://api.github.com/repos/ethereum/py-evm/contents/{+path}","compare_url":"https://api.github.com/repos/ethereum/py-evm/compare/{base}...{head}","merges_url":"https://api.github.com/repos/ethereum/py-evm/merges","archive_url":"https://api.github.com/repos/ethereum/py-evm/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/ethereum/py-evm/downloads","issues_url":"https://api.github.com/repos/ethereum/py-evm/issues{/number}","pulls_url":"https://api.github.com/repos/ethereum/py-evm/pulls{/number}","milestones_url":"https://api.github.com/repos/ethereum/py-evm/milestones{/number}","notifications_url":"https://api.github.com/repos/ethereum/py-evm/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/ethereum/py-evm/labels{/name}","releases_url":"https://api.github.com/repos/ethereum/py-evm/releases{/id}","deployments_url":"https://api.github.com/repos/ethereum/py-evm/deployments","created_at":"2016-12-27T19:36:00Z","updated_at":"2018-08-20T19:28:11Z","pushed_at":"2018-08-20T20:49:48Z","git_url":"git://github.com/ethereum/py-evm.git","ssh_url":"git@github.com:ethereum/py-evm.git","clone_url":"https://github.com/ethereum/py-evm.git","svn_url":"https://github.com/ethereum/py-evm","homepage":"https://trinity.ethereum.org/","size":4903,"stargazers_count":616,"watchers_count":616,"language":"Python","has_issues":true,"has_projects":true,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":270,"mirror_url":null,"archived":false,"open_issues_count":123,"license":{"key":"mit","name":"MIT License","spdx_id":"MIT","url":"https://api.github.com/licenses/mit","node_id":"MDc6TGljZW5zZTEz"},"forks":270,"open_issues":123,"watchers":616,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/ethereum/py-evm/pulls/1184"},"html":{"href":"https://github.com/ethereum/py-evm/pull/1184"},"issue":{"href":"https://api.github.com/repos/ethereum/py-evm/issues/1184"},"comments":{"href":"https://api.github.com/repos/ethereum/py-evm/issues/1184/comments"},"review_comments":{"href":"https://api.github.com/repos/ethereum/py-evm/pulls/1184/comments"},"review_comment":{"href":"https://api.github.com/repos/ethereum/py-evm/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/ethereum/py-evm/pulls/1184/commits"},"statuses":{"href":"https://api.github.com/repos/ethereum/py-evm/statuses/7ca13388d003bd42aa4c79a84040fe740c8cc5ce"}},"author_association":"CONTRIBUTOR"}}
{"actor":{"display_login":"carver"}}