Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.

[sync]: rust 2018#11067

Merged
niklasad1 merged 6 commits into
masterfrom
na-sync-rust2018
Sep 19, 2019
Merged

[sync]: rust 2018#11067
niklasad1 merged 6 commits into
masterfrom
na-sync-rust2018

Conversation

@niklasad1
Copy link
Copy Markdown
Collaborator

@niklasad1 niklasad1 commented Sep 18, 2019 •
edited
Loading

Address #11061 (review)

Will not be addressed in this PR:

I think there are additional cleanups to do in the crate such:

  • replace enum_from_primitive with a proc macro for SyncPacket
  • rename/unify types such as TransactionStats, LightSync and similar, because several things are named the same thing which is confusing at least to me
  • split sync into different crates for light and full sync

@niklasad1 niklasad1 added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Sep 18, 2019
dvdplm
dvdplm approved these changes Sep 18, 2019
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

LGTM, just a few nits.

Thank you for caring about this stuff. Much appreciated.

Comment thread ethcore/sync/Cargo.toml Outdated
Comment thread ethcore/sync/Cargo.toml Outdated
Comment thread ethcore/sync/src/lib.rs
#[macro_use]
extern crate trace_time;
// needed to make the procedural macro `MallocSizeOf` to work
#[macro_use] extern crate parity_util_mem as malloc_size_of;
Copy link
Copy Markdown
Collaborator

@dvdplm dvdplm Sep 18, 2019

Choose a reason for hiding this comment

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

@cheme I've run into this as well in other crates. Are we using it wrong or is this a "known issue"?

Copy link
Copy Markdown
Collaborator Author

@niklasad1 niklasad1 Sep 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think the crate need proc-macro = true in its manifest to work with rust 2018?

Comment thread ethcore/sync/src/chain/supplier.rs Outdated
Comment thread ethcore/sync/src/chain/propagator.rs
snapshot::TestSnapshotService,
};

use super::{*, super::{*, tests::*}};
Copy link
Copy Markdown
Collaborator

@dvdplm dvdplm Sep 18, 2019

Choose a reason for hiding this comment

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

Lovely to see these go.

@dvdplm
Copy link
Copy Markdown
Collaborator

dvdplm commented Sep 18, 2019 

* replace `enum_from_primitive` with a proc macro for `SyncPacket`

We had a PR open that did exactly this and was really close to being finished. Maybe now's the time to pick that up and push it over the finish line?

@dvdplm
Copy link
Copy Markdown
Collaborator

dvdplm commented Sep 18, 2019 

* split `sync` into different crates for light and full sync

Can you elaborate on this? Does light use sync atm? Sounds like a great idea on paper.

@dvdplm dvdplm added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 18, 2019
@niklasad1
Copy link
Copy Markdown
Collaborator Author

niklasad1 commented Sep 19, 2019 •
edited
Loading

Can you elaborate on this? Does light use sync atm? Sounds like a great idea on paper.

We have two different types for LightSync and FullSync a.k.a EthSync if we could extract the traits and handle light protocol handler for EthSync it should be doable IIRC. I can have a look myself at this :P

We had a PR open that did exactly this and was really close to being finished. Maybe now's the time to pick that up and push it over the finish line?

Right, yeah would be cool.

@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Sep 19, 2019
@ordian ordian added this to the 2.7 milestone Sep 19, 2019
ordian
ordian approved these changes Sep 19, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

LGTM, feel free to merge if you want to address the bullet points in a separate PR.

@niklasad1 niklasad1 merged commit 19184e8 into master Sep 19, 2019
@niklasad1 niklasad1 deleted the na-sync-rust2018 branch September 19, 2019 11:12
dvdplm added a commit that referenced this pull request Sep 20, 2019
@dvdplm
Merge branch 'master' into dp/chore/new-ethereum-consensus-tests
dfaee44 
* master:
  ethcore/res: activate Istanbul on Ropsten, Görli, Rinkeby, Kovan (#11068)
  [sync]: rust 2018 (#11067)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@dvdplm dvdplm dvdplm approved these changes

@ordian ordian ordian approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.

Projects

None yet

Milestone

2.7

Development

Successfully merging this pull request may close these issues.

3 participants

@niklasad1 @dvdplm @ordian