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

p2p: Rework transport to be async #1116

Merged
merged 2 commits into from
Apr 9, 2022
Merged

Conversation

xla
Copy link
Contributor

@xla xla commented Apr 8, 2022

While it was well-intended the reality is that it's going to be
impractical and hinder potential adoption trying to not give in to the
sweet siren sounds of the async ecosystem. The author since has changed
their stance and adopted a conviction that well tuned and guard-railed
async can be workable and lead to maintainable solutions. This
change-set is in preparation of bringing in peer and supervisor
abstractions as described in the ADR.

Signed-off-by: xla self@xla.is

Individual commits are significant please review accordingly.

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
    * [ ] Wrote tests
  • Added entry in .changelog/

@xla xla requested a review from thanethomson April 8, 2022 08:05
@xla xla self-assigned this Apr 8, 2022
@xla xla force-pushed the xla/692-async-transport branch from 64d53be to 113013a Compare April 8, 2022 08:14
@xla xla force-pushed the xla/692-async-transport branch from 113013a to 07e3ab1 Compare April 9, 2022 00:42
@xla xla requested review from romac and soareschen April 9, 2022 00:42
@codecov-commenter
Copy link

Codecov Report

Merging #1116 (8694b23) into master (d2b9f33) will decrease coverage by 0.0%.
The diff coverage is n/a.

❗ Current head 8694b23 differs from pull request most recent head 07e3ab1. Consider uploading reports for the commit 07e3ab1 to get more accurate results

@@           Coverage Diff            @@
##           master   #1116     +/-   ##
========================================
- Coverage    64.8%   64.8%   -0.1%     
========================================
  Files         240     240             
  Lines       20822   20822             
========================================
- Hits        13500   13493      -7     
- Misses       7322    7329      +7     
Impacted Files Coverage Δ
light-client-verifier/src/types.rs 37.9% <0.0%> (-1.2%) ⬇️
p2p/src/secret_connection/protocol.rs 59.8% <0.0%> (-1.0%) ⬇️
testgen/src/commit.rs 90.6% <0.0%> (-0.7%) ⬇️
testgen/src/light_block.rs 76.1% <0.0%> (-0.6%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2b9f33...07e3ab1. Read the comment docs.

@xla
Copy link
Contributor Author

xla commented Apr 9, 2022

Fails because of the unmaintained serde_cbor advisory, temporarily addressed in #1119

xla added 2 commits April 9, 2022 22:23
Signed-off-by: xla <self@xla.is>
While it was well-intended the reality is that it's going to be
impractical and hinder potential adoption trying to not give in to the
sweet siren sounds of the async ecosystem. The author since has changed
their stance and adopted a conviction that well tuned and guard-railed
async can be workable and lead to maintainable solutions. This
change-set is in preparation of bringing in peer and supervisor
abstractions as described in the ADR.

Signed-off-by: xla <self@xla.is>
@xla xla force-pushed the xla/692-async-transport branch from 07e3ab1 to be27b6a Compare April 9, 2022 12:23
Copy link
Contributor

@thanethomson thanethomson left a comment

Choose a reason for hiding this comment

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

Oh those sweet siren sounds 🧜‍♀️

@thanethomson thanethomson merged commit c828262 into master Apr 9, 2022
@thanethomson thanethomson deleted the xla/692-async-transport branch April 9, 2022 12:39
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.

3 participants