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.

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

refactor: move network protocol related variables to SupportProtocols #2096

Merged
merged 1 commit into from
Jun 22, 2020
Merged

refactor: move network protocol related variables to SupportProtocols #2096

merged 1 commit into from
Jun 22, 2020

Conversation

quake
Copy link
Member

@quake quake commented Jun 2, 2020 •
edited
Loading

To eliminate dependence of ckb-sync crate, this PR refactored network protocol related variables and move them to a new enum: SupportProtocols

@quake quake requested review from a team, zhangsoledad and keroro520 June 2, 2020 03:41
@quake quake force-pushed the quake/refactor-network-protocol-constant branch from 9e4ce2a to af7c5b2 Compare June 2, 2020 04:45
@quake quake force-pushed the quake/refactor-network-protocol-constant branch 2 times, most recently from 51a4456 to 6338eae Compare June 8, 2020 06:23
@quake quake force-pushed the quake/refactor-network-protocol-constant branch from 6338eae to 63a699b Compare June 8, 2020 08:16
keroro520
keroro520 previously approved these changes Jun 8, 2020
View reviewed changes
zhangsoledad
zhangsoledad requested changes Jun 16, 2020
View reviewed changes
test/src/specs/sync/invalid_locator_size.rs Outdated
@@ -11,6 +11,9 @@ use log::info;

pub struct InvalidLocatorSize;

// test same value as ckb_sync::MAX_LOCATOR_SIZE;
const MAX_LOCATOR_SIZE: usize = 101;

impl Spec for InvalidLocatorSize {
Copy link
Member

Choose a reason for hiding this comment

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

ckb/sync/src/lib.rs

Line 53 in 63a699b

pub const MAX_LOCATOR_SIZE: usize = 101;

Copy link
Member

@zhangsoledad zhangsoledad Jun 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This refractory does not affect MAX_LOCATOR_SIZE, it should not be defined twice. remain unchanged.

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR want to remove the dependence of ckb-sync crate in the integration test, 3 constants are copied to test code: RETRY_ASK_TX_TIMEOUT_INCREASE / BLOCK_DOWNLOAD_TIMEOUT / MAX_LOCATOR_SIZE, we may move them and more shared constants to a new crate (ckb-constants), and change ckb-sync and integration test to use this new crate in the future PR.

I prefer to re-define it in test code regarding this PR, let's keep changes to a minimum.

@quake quake requested a review from zhangsoledad June 16, 2020 09:14
zhangsoledad
zhangsoledad requested changes Jun 16, 2020
View reviewed changes
Copy link
Member

@zhangsoledad zhangsoledad left a comment

Choose a reason for hiding this comment

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

I insist on this should remain unchanged, since this PR does not move it.
I think this is the so-called minimum change.

@quake quake force-pushed the quake/refactor-network-protocol-constant branch from 63a699b to a44c04a Compare June 17, 2020 02:04
@quake
Copy link
Member Author

quake commented Jun 17, 2020

I insist on this should remain unchanged, since this PR does not move it.
I think this is the so-called minimum change.

OK, I have reverted related change and will create a new PR for integration-test change, please review again, thanks.

@quake quake force-pushed the quake/refactor-network-protocol-constant branch from a44c04a to 9af130b Compare June 17, 2020 02:39
keroro520
keroro520 previously approved these changes Jun 19, 2020
View reviewed changes
network/src/protocols/support_protocols.rs
};
use tokio_util::codec::length_delimited;

pub enum SupportProtocols {
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI. SupportProtocols is just like a "constants wrapper", which wraps the protocols and their contributions. It seems that the protocols and contributions are pre-defined. I think the plain style would be more intuitive(define as constants):

pub const PING_PROTOCOL_NAME = "ping";
pub const PING_SUPPORTED_VERSIONS = vec!["1"];

@quake quake force-pushed the quake/refactor-network-protocol-constant branch from 9af130b to dcdb45d Compare June 21, 2020 02:45
@quake quake requested a review from doitian as a code owner June 21, 2020 02:45
@quake quake requested a review from keroro520 June 21, 2020 02:46
keroro520
keroro520 previously approved these changes Jun 21, 2020
View reviewed changes
@doitian
Copy link
Member

doitian commented Jun 22, 2020

ping @zhangsoledad

zhangsoledad
zhangsoledad previously approved these changes Jun 22, 2020
View reviewed changes
@quake quake dismissed stale reviews from zhangsoledad and keroro520 via a076992 June 22, 2020 05:39
@quake quake force-pushed the quake/refactor-network-protocol-constant branch from dcdb45d to a076992 Compare June 22, 2020 05:39
@quake
Copy link
Member Author

quake commented Jun 22, 2020

@doitian @zhangsoledad rebased this PR, please review again

@doitian
Copy link
Member

doitian commented Jun 22, 2020

bors r=doitian,zhangsoledad

@bors
Copy link
Contributor

bors bot commented Jun 22, 2020

Build succeeded:

@bors bors bot merged commit 199c643 into develop Jun 22, 2020
@bors bors bot deleted the quake/refactor-network-protocol-constant branch June 22, 2020 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@doitian doitian doitian approved these changes

@zhangsoledad zhangsoledad zhangsoledad approved these changes

@keroro520 keroro520 Awaiting requested review from keroro520

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@quake @doitian @keroro520 @zhangsoledad