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

feat: add Web3 namespace RPC handler #990

Merged
merged 6 commits into from
Jan 23, 2023
Merged

feat: add Web3 namespace RPC handler #990

merged 6 commits into from
Jan 23, 2023

Conversation

leruaa
Copy link
Contributor

@leruaa leruaa commented Jan 23, 2023

Closes #983

@codecov-commenter
Copy link

codecov-commenter commented Jan 23, 2023 •
edited
Loading

Codecov Report

Merging #990 (def59c3) into main (c7289bd) will increase coverage by 0.07%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #990      +/-   ##
==========================================
+ Coverage   74.01%   74.08%   +0.07%     
==========================================
  Files         294      298       +4     
  Lines       32564    32818     +254     
==========================================
+ Hits        24102    24314     +212     
- Misses       8462     8504      +42
Flag Coverage Δ
unit-tests 74.08% <0.00%> (+0.07%) ⬆️

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

Impacted Files Coverage Δ
crates/net/network-api/src/lib.rs 33.33% <ø> (ø)
crates/net/network/src/network.rs 52.77% <0.00%> (ø)
crates/net/rpc-api/src/web3.rs 0.00% <ø> (ø)
crates/net/rpc/src/lib.rs 100.00% <ø> (ø)
crates/net/rpc/src/web3.rs 0.00% <0.00%> (ø)
crates/metrics/metrics-derive/src/expand.rs 86.51% <0.00%> (-11.99%) ⬇️
crates/transaction-pool/src/test_utils/mock.rs 53.11% <0.00%> (-6.60%) ⬇️
crates/interfaces/src/p2p/error.rs 64.28% <0.00%> (-3.58%) ⬇️
crates/metrics/metrics-derive/src/metric.rs 75.00% <0.00%> (-3.13%) ⬇️
crates/stages/src/stages/bodies.rs 90.94% <0.00%> (-1.81%) ⬇️
... and 21 more

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

@onbjerg onbjerg added C-enhancement New feature or request A-rpc Related to the RPC implementation labels Jan 23, 2023
onbjerg
onbjerg requested changes Jan 23, 2023
View reviewed changes
crates/net/rpc/src/web3.rs Outdated Show resolved Hide resolved
onbjerg
onbjerg requested changes Jan 23, 2023
View reviewed changes
crates/net/rpc/src/web3.rs Outdated Show resolved Hide resolved
mattsse
mattsse requested changes Jan 23, 2023
View reviewed changes
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.

there's no need for an additional client_version field, we should to reuse the client_version of the SessionManager.HelloMessage

this is also already included in the NetworkStatus, should probably rename the field from client_name to client_version

crates/net/network/src/config.rs Outdated Show resolved Hide resolved
mattsse
mattsse approved these changes Jan 23, 2023
View reviewed changes
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.

awesome, only 1 nit, re #993 that was just merged

crates/net/network/src/network.rs
Comment on lines 235 to +238
let status = self.get_status().await?;

Ok(NetworkStatus {
client_name: "Reth".to_string(),
client_version: "Reth".to_string(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should actually be included in the fn get_status

do you want to modify the fn get_status in a followup, so that it returns NetworkStatus instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate? Not sure to get what is needed

Copy link
Collaborator

Choose a reason for hiding this comment

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

Network:: get_status

reth/crates/net/network/src/network.rs

Lines 132 to 137 in def59c3

/// Get the current status of the node.
pub async fn get_status(&self) -> Result<Status, oneshot::error::RecvError> {
let (tx, rx) = oneshot::channel();
let _ = self.manager().send(NetworkHandleMessage::GetStatus(tx));
rx.await
}

should return NetworkStatus directly

Copy link
Collaborator

Choose a reason for hiding this comment

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

created #996

crates/net/rpc/src/web3.rs Show resolved Hide resolved
crates/net/rpc/src/web3.rs Outdated Show resolved Hide resolved
mattsse
mattsse approved these changes Jan 23, 2023
View reviewed changes
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.

lgtm

@mattsse mattsse merged commit 6f047a5 into paradigmxyz:main Jan 23, 2023
literallymarvellous pushed a commit to literallymarvellous/reth that referenced this pull request Feb 5, 2023
@leruaa @literallymarvellous
feat: add Web3 namespace RPC handler (paradigmxyz#990)
ffc5a16
literallymarvellous pushed a commit to literallymarvellous/reth that referenced this pull request Feb 5, 2023
@leruaa @literallymarvellous
feat: add Web3 namespace RPC handler (paradigmxyz#990)
46f469f
literallymarvellous pushed a commit to literallymarvellous/reth that referenced this pull request Feb 6, 2023
@leruaa @literallymarvellous
feat: add Web3 namespace RPC handler (paradigmxyz#990)
9685a8b
@leruaa leruaa deleted the impl_web3_api branch July 12, 2024 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattsse mattsse mattsse approved these changes

@onbjerg onbjerg onbjerg requested changes

@Rjected Rjected Awaiting requested review from Rjected Rjected is a code owner

Assignees
No one assigned
Labels
A-rpc Related to the RPC implementation C-enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement Web3 namespace RPC handler
4 participants
@leruaa @codecov-commenter @onbjerg @mattsse