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

DevP2P: add snap protocol #1883

Merged
merged 7 commits into from
Jul 16, 2022
Merged

DevP2P: add snap protocol #1883

merged 7 commits into from
Jul 16, 2022

Conversation

jochem-brouwer
Copy link
Member

@jochem-brouwer jochem-brouwer commented May 13, 2022

Closes #1876

TODO:

  • Add tests

@codecov
Copy link

codecov bot commented May 13, 2022

Codecov Report

Merging #1883 (319d42b) into master (319d42b) will not change coverage.
The diff coverage is n/a.

❗ Current head 319d42b differs from pull request most recent head 16ab448. Consider uploading reports for the commit 16ab448 to get more accurate results

Impacted file tree graph

Flag Coverage Δ
block 84.01% <0.00%> (ø)
blockchain 80.10% <0.00%> (ø)
client 78.37% <0.00%> (ø)
common 94.90% <0.00%> (ø)
devp2p 82.41% <0.00%> (ø)
ethash 90.81% <0.00%> (ø)
evm 40.97% <0.00%> (ø)
statemanager 84.55% <0.00%> (ø)
trie 80.84% <0.00%> (ø)
tx 92.18% <0.00%> (ø)
util 87.22% <0.00%> (ø)
vm 78.66% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@@ -4,9 +4,10 @@ import { EventEmitter } from 'events'
import { devp2pDebug } from '../util'
import { Peer, DISCONNECT_REASONS } from '../rlpx/peer'

export enum EthProtocol {
export enum EthProtocol { // What does this represent?
Copy link
Contributor

Choose a reason for hiding this comment

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

This represents the protocol running on top of RLPx. Could probably just be renamed to Protocol, but that may conflict with the base class? RlpxProtocol could also work, but not sure if we want to rename/breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could rename this enum to ProtocolType?

@scorbajio
Copy link
Contributor

Two tests for the SNAP protocol are proposed in #1893

g11tech
g11tech previously approved these changes May 28, 2022
@holgerd77
Copy link
Member

Please do not merge here before last round of master releases, will give it a "Blocked" label for now (feel free to remove afterwards).

Relatively directly after the releases we actually should merge to clear the field for the latest develop rebase.

@g11tech
Copy link
Contributor

g11tech commented Jul 16, 2022

@jochem-brouwer @holgerd77 this is good to merge in my opinion

Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

LGTM!

@holgerd77 holgerd77 marked this pull request as ready for review July 16, 2022 15:23
@holgerd77
Copy link
Member

Ok, then let's do it. 😃

@holgerd77 holgerd77 deleted the snap-devp2p branch July 16, 2022 15:45
@g11tech g11tech mentioned this pull request Aug 4, 2022
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

devp2p: SNAP Protocol Support
5 participants