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

feat: add fundamental types #1717

Closed
wants to merge 2 commits into from
Closed

feat: add fundamental types #1717

wants to merge 2 commits into from

Conversation

mattsse
Copy link
Collaborator

@mattsse mattsse commented Mar 12, 2023

adds a new crate that provides types used by reth,revm,ethers

having this separately makes it easier to unify types.

roadmap

  • add crate
  • make revm-primitives depend on this crate (for H256 type)
  • likely move additional revm-primitives to this crate (Account, Log)

cc @prestwich

@mattsse mattsse requested a review from gakonst as a code owner March 12, 2023 10:49
@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2023

Codecov Report

Merging #1717 (5930ec5) into main (1846f2d) will increase coverage by 0.89%.
The diff coverage is 30.99%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #1717      +/-   ##
==========================================
+ Coverage   71.87%   72.77%   +0.89%     
==========================================
  Files         388      390       +2     
  Lines       46468    46710     +242     
==========================================
+ Hits        33401    33994     +593     
+ Misses      13067    12716     -351     
Flag Coverage Δ
integration-tests 20.31% <0.00%> (+2.17%) ⬆️
unit-tests 67.56% <30.99%> (-0.25%) ⬇️

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

Impacted Files Coverage Δ
crates/fundamental-types/src/bits.rs 30.70% <30.70%> (ø)
crates/fundamental-types/src/lib.rs 100.00% <100.00%> (ø)

... and 44 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mattsse mattsse added the C-enhancement New feature or request label Mar 12, 2023
@rakita
Copy link
Collaborator

rakita commented Mar 12, 2023

This would make dependency hell if revm depends on reth that depends on revm.
reth-primitives are made for this reason to have just a subset of the exportable types from revm, you can reexport them here if needed.
Or we can move H types to ruint

@mattsse
Copy link
Collaborator Author

mattsse commented Mar 12, 2023

yeh, didn't think about that when discussing this with @prestwich

main motivation for this is to provide a solid foundation for all types for revm,reth,ethers(migration away from primitive-types)

Initially, I thought we need this for all the traits/codecs etc. but turns out we already have control over the rlp trait and can just solve this with features.

So, I perhaps the better solution is to make revm-primitives the base crate for everything type related?
however, there some types that revm only like Database and env.
The purpose of this fundamental type crate would to provide just the shared types.

Or we can move H types to ruint

I think ruint has its own B256 type?
don't know what the difference is

@rakita
Copy link
Collaborator

rakita commented Mar 12, 2023

Asked remco: recmo/uint#228

@prestwich
Copy link
Collaborator

This would make dependency hell if revm depends on reth that depends on revm.

I think this gets resolved if we publish the types crate (which should be well-ossified) and stop depending on git versions for this crate. If revm and reth both depend on a published version of this types crate there should be no dep hell

@gakonst
Copy link
Member

gakonst commented Mar 14, 2023

Let's avoid spending time on this as it's debt

@gakonst gakonst added the C-debt Refactor of code section that is hard to understand or maintain label Mar 14, 2023
@prestwich
Copy link
Collaborator

Let's avoid spending time on this as it's debt

Some solution is a blocker for ethers, I think

@gakonst
Copy link
Member

gakonst commented Mar 14, 2023

Yeh just referring to Reth team needs to focus on the missing feats right now, I think realistically once we're "live" we'll want to do a zoomed out pass from our side, but would like to protect our time in the next few coming weeks. Closing and will reopen then

@gakonst gakonst closed this Mar 14, 2023
@prestwich
Copy link
Collaborator

If the reth team doesn't have time to maintain core types relied on by ethers, revm, foundry, and others, maybe those types should be maintained by another org?

@gakonst
Copy link
Member

gakonst commented Mar 14, 2023

What I mean is that we do want to maintain these types and gather everyone around them, but right now dont want to do this refactor because we're changing things across 5+ repos (incl. all the cryptography etc. version bumps). Just asking for some patience hehe

@prestwich
Copy link
Collaborator

making changes across 5+ repos without a commitment to fundamental shared types is how we got into this mess in the first place

@gakonst gakonst deleted the matt/add-fundamental-types branch July 2, 2023 14:11
@gakonst gakonst restored the matt/add-fundamental-types branch July 2, 2023 14:11
@gakonst gakonst deleted the matt/add-fundamental-types branch July 2, 2023 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-debt Refactor of code section that is hard to understand or maintain C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants