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

Fork ethereum-types, ethbloom and serialize, remove deprecated macros #132

Closed
wants to merge 1 commit into from
Closed

Fork ethereum-types, ethbloom and serialize, remove deprecated macros #132

wants to merge 1 commit into from

Conversation

vlopes11
Copy link
Contributor

@vlopes11 vlopes11 commented Jan 5, 2019

Issue Addressed

Implement hash types removing dependency on ethereum_types
#10

Proposed Changes

Fork ethereum-types, ethbloom and serialize inside the project

Additional Info

N/A

@paulhauner
Copy link
Member

Thanks for this @vlopes11. I'm in the middle of a big PR, but I'll start mulling it over and get back to you in the coming days :)

@paulhauner
Copy link
Member

Sorry, I've been lagging on this one.

@drozdziak1 and @ralexstokes, keen to hear your thoughts on this.

@vlopes11
Copy link
Contributor Author

Oh, seems we have a CI break with the introduction of default cargo fmt on 7df6481

I'll fix it.

@ralexstokes
Copy link
Contributor

it seems that this PR just vendors the dependency, rather than re-implementing it which I believe was the original spirit of #10. i assumed the idea was that we wanted lighthouse to use as few external crates as possible so that it provides a small base for other users in the ecosystem who want to build on top. and with this in mind, it would make sense to provide as minimal implementations as possible of the types we are currently using in the codebase.

i don't really see what vendoring the target crate really does for us, other than forgo any maintenance benefits provided by the crate's maintainers. we in fact now have an increased burden to either sync their changes or maintain our own. given how good cargo seems to be at managing dependencies, i would elect to just leave this as is w/ the original crate

@vlopes11
Copy link
Contributor Author

vlopes11 commented Jan 17, 2019

It is true, this one is merely a fork without additional functionalities. I agree with you, but this would be a starting point for lighthouse own types. Anyway, to implement from scratch could also be an option - much more work demanding, tho.

Shall we close this PR without merging?

@ralexstokes
Copy link
Contributor

I would want to know which types we need to reimplement based on current usage -- if it is just a handful i would favor reimplementing, if we need most of the types in the source crate then we should carefully consider reasons to not just use that crate (as a full-on dependency)

@drozdziak1
Copy link
Contributor

@ralexstokes @paulhauner I think we should see how much impact each dep has first and whether losing that is worth the rewrite effort.

e.g. ethereum-types 0.4.0 built independently has a 20-crate dep tree, amounts\ to ~19MB in debug *.rlib, builds in 28s from scratch on my XPS13 9360 laptop, release build is <1s longer and amounts to 2.7MB. The crate is even nostd-friendly.

@vlopes11
Copy link
Contributor Author

Maybe we should close this PR and implement types from zero?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants