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

standalone rpc_types #4088

Merged
merged 8 commits into from
Aug 9, 2023
Merged

Conversation

supernovahs
Copy link
Contributor

Ref #3964

@supernovahs supernovahs changed the title draft wip standalone rpc_types Aug 6, 2023
@supernovahs
Copy link
Contributor Author

@codecov
Copy link

codecov bot commented Aug 6, 2023

Codecov Report

Merging #4088 (64062fb) into main (b823cc0) will decrease coverage by 54.48%.
Report is 1 commits behind head on main.
The diff coverage is 1.08%.

Impacted file tree graph

Files Changed Coverage Δ
crates/rpc/rpc-types-compat/src/block.rs 0.00% <0.00%> (ø)
crates/rpc/rpc-types-compat/src/transaction/mod.rs 0.00% <0.00%> (ø)
crates/rpc/rpc-types/src/eth/block.rs 0.00% <ø> (-39.85%) ⬇️
crates/rpc/rpc-types/src/eth/transaction/mod.rs 0.00% <ø> (-42.08%) ⬇️
crates/rpc/rpc/src/eth/api/transactions.rs 16.26% <0.00%> (-15.15%) ⬇️
crates/rpc/rpc/src/eth/pubsub.rs 6.66% <0.00%> (ø)
crates/rpc/rpc/src/txpool.rs 0.00% <0.00%> (ø)
crates/rpc/rpc/src/eth/api/block.rs 50.94% <66.66%> (-0.44%) ⬇️

... and 391 files with indirect coverage changes

Flag Coverage Δ
integration-tests 14.21% <1.08%> (-2.70%) ⬇️
unit-tests ?

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

Components Coverage Δ
reth binary 5.48% <ø> (-19.90%) ⬇️
blockchain tree 0.00% <ø> (-83.05%) ⬇️
pipeline 0.00% <ø> (-90.08%) ⬇️
storage (db) 16.48% <ø> (-58.15%) ⬇️
trie 0.00% <ø> (-94.72%) ⬇️
txpool 20.94% <ø> (-26.65%) ⬇️
networking 20.16% <ø> (-57.21%) ⬇️
rpc 23.81% <1.08%> (-34.87%) ⬇️
consensus 0.98% <ø> (-62.78%) ⬇️
revm 0.94% <ø> (-31.33%) ⬇️
payload builder 6.57% <ø> (ø)
primitives 19.09% <ø> (-68.70%) ⬇️

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

perhaps there was a misunderstanding,

all type defs should remain in rpc-types crate, and we only want to extract all the functions

right @DaniPopes ?

Cargo.toml Outdated
@@ -45,7 +45,7 @@ members = [
"crates/transaction-pool",
"crates/trie",
"testing/ef-tests",

"crates/rpc/rpc-types-util",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer something other than util

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think compat or convert are better alternatives

crates/rpc/rpc-types-util/src/transaction/mod.rs Outdated Show resolved Hide resolved
@mattsse mattsse added C-debt Refactor of code section that is hard to understand or maintain A-rpc Related to the RPC implementation labels Aug 7, 2023
@supernovahs
Copy link
Contributor Author

supernovahs commented Aug 7, 2023

Don't know why I am getting cyclic dependency errors on cargo test

error: cyclic package dependency: package `reth-rpc-types v0.1.0-alpha.4 (/Users/supernovahs/Desktop/reth/crates/rpc/rpc-types)` depends on itself. Cycle:
package `reth-rpc-types v0.1.0-alpha.4 (/Users/supernovahs/Desktop/reth/crates/rpc/rpc-types)`
    ... which satisfies path dependency `reth-rpc-types` of package `reth-rpc-types-compat v0.1.0-alpha.4 (/Users/supernovahs/Desktop/reth/crates/rpc/rpc-types-compat)`
    ... which satisfies path dependency `reth-rpc-types-compat` of package `reth-rpc-types v0.1.0-alpha.4 (/Users/supernovahs/Desktop/reth/crates/rpc/rpc-types)`
    ... which satisfies path dependency `reth-rpc-types` of package `reth-network-api v0.1.0-alpha.4 (/Users/supernovahs/Desktop/reth/crates/net/network-api)`
    ... which satisfies path dependency `reth-network-api` of package `reth-interfaces v0.1.0-alpha.4 (/Users/supernovahs/Desktop/reth/crates/interfaces)`
    ... which satisfies path dependency `reth-interfaces` of package `reth-consensus-common v0.1.0-alpha.4 (/Users/supernovahs/Desktop/reth/crates/consensus/common)`
    ... which satisfies path dependency `reth-consensus-common` of package `reth-beacon-consensus v0.1.0-alpha.4 (/Users/supernovahs/Desktop/reth/crates/consensus/beacon)`
    ... which satisfies path dependency `reth-beacon-consensus` of package `reth-auto-seal-consensus v0.1.0-alpha.4 (/Users/supernovahs/Desktop/reth/crates/consensus/auto-seal)`
    ... which satisfies path dependency `reth-auto-seal-consensus` of package `reth v0.1.0-alpha.4 (/Users/supernovahs/Desktop/reth/bin/reth)`
    ... which satisfies path dependency `reth` of package `additional-rpc-namespace-in-cli v0.0.0 (/Users/supernovahs/Desktop/reth/examples/additional-rpc-namespace-in-cli)`

@supernovahs
Copy link
Contributor Author

Got it, I would have to separate every function , such that rpc_types does not import compat in any case

@supernovahs
Copy link
Contributor Author

I have made the changes. But +nightly fmt could not be done, due to some issues with my mac .
Ty

@supernovahs supernovahs marked this pull request as ready for review August 8, 2023 16:56
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

amazing, this looks great, ty!

I'll take care of the fmt issue

@mattsse mattsse added this pull request to the merge queue Aug 9, 2023
Merged via the queue into paradigmxyz:main with commit ba7fa1a Aug 9, 2023
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc Related to the RPC implementation C-debt Refactor of code section that is hard to understand or maintain
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants