Skip to content

Conversation

@juanbono
Copy link
Collaborator

@juanbono juanbono commented Jul 1, 2024

Motivation

Description

Closes #125

@juanbono juanbono changed the title Implement rlpx handshake feat: implement rlpx handshake Jul 1, 2024
@MegaRedHand MegaRedHand self-assigned this Jul 8, 2024
@MegaRedHand MegaRedHand marked this pull request as ready for review July 8, 2024 21:56
@MegaRedHand MegaRedHand requested a review from a team as a code owner July 8, 2024 21:56
};
use tokio::io::AsyncReadExt;
use tokio::{
io::AsyncWriteExt,
Copy link
Contributor

Choose a reason for hiding this comment

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

AsyncReadExt and AsyncWriteExt could be declared in the same use line.
cargo fmt does not fit these together? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

cargo fmt doesn't merge them by default, so I did it manually. We might want to modify the imports_granularity setting for this.

auth.encode(&mut encoded_auth_msg);
encoded_auth_msg.resize(encoded_auth_msg.len() + padding_length, 0);

let ecies_data_size = 65 + 16 + 32;
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we comment the reason behind these numbers?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done!

self.handshake_data.is_some()
}

pub fn encode_auth_message(
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this function could benefit from some inline comments

Copy link
Collaborator

Choose a reason for hiding this comment

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

Added!

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome!

H520(signature_bytes)
}

pub fn decode_ack_message(&mut self, static_key: &SecretKey, msg: &mut [u8]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function could have inline comments, as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Added!

Copy link
Contributor

@ElFantasma ElFantasma left a comment

Choose a reason for hiding this comment

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

Other than (optional) minor comments, it LGTM

@MegaRedHand MegaRedHand merged commit 263fccb into main Jul 12, 2024
@MegaRedHand MegaRedHand deleted the implement_rlpx_handshake branch July 12, 2024 18:38
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.

Implement RLPx handshake

4 participants