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(p2p_proto): remove redundant address fields #1672

Merged
merged 10 commits into from
Jan 19, 2024

Conversation

CHr15F0x
Copy link
Member

@CHr15F0x CHr15F0x commented Jan 11, 2024

I added those fields in anticipation of a spec update but it turns out I didn't notice these fields are redundant and can just be computed locally.


Edit: I updated the code to defer computing those addresses after all other parsing is done.

@CHr15F0x CHr15F0x marked this pull request as ready for review January 11, 2024 17:47
@CHr15F0x CHr15F0x requested review from pierre-l and a team as code owners January 11, 2024 17:47
crates/common/src/lib.rs Outdated Show resolved Hide resolved
crates/common/src/lib.rs Outdated Show resolved Hide resolved
crates/p2p/src/client/types.rs Outdated Show resolved Hide resolved
@CHr15F0x CHr15F0x marked this pull request as draft January 17, 2024 10:02
@CHr15F0x CHr15F0x force-pushed the chris/rm-addr-in-depl-acc branch 2 times, most recently from 179c94b to 507eb07 Compare January 18, 2024 14:57
@CHr15F0x CHr15F0x force-pushed the chris/rm-addr-in-depl-acc branch from 507eb07 to b3709d7 Compare January 18, 2024 15:01
@CHr15F0x CHr15F0x marked this pull request as ready for review January 18, 2024 15:14
Copy link
Contributor

@kkovaacs kkovaacs left a comment

Choose a reason for hiding this comment

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

LGTM!

@CHr15F0x CHr15F0x requested a review from sistemd January 19, 2024 10:51
Copy link
Contributor

@sistemd sistemd left a comment

Choose a reason for hiding this comment

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

👍

@@ -37,6 +37,7 @@ pathfinder-common = { path = "../common" }
pathfinder-crypto = { path = "../crypto" }
prost = "0.12.1"
rand = { workspace = true }
rayon = "1.8.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should become a workspace dep I think.

@CHr15F0x CHr15F0x force-pushed the chris/rm-addr-in-depl-acc branch from 7b71efd to 112d488 Compare January 19, 2024 13:45
@CHr15F0x CHr15F0x merged commit fcfdb52 into main Jan 19, 2024
7 checks passed
@CHr15F0x CHr15F0x deleted the chris/rm-addr-in-depl-acc branch January 19, 2024 13:58
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