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

cuprated: P2P protocol request handler #303

Merged
merged 90 commits into from
Dec 3, 2024
Merged

cuprated: P2P protocol request handler #303

merged 90 commits into from
Dec 3, 2024

Conversation

Boog900
Copy link
Member

@Boog900 Boog900 commented Oct 4, 2024

What

Implements the P2P protocol handler

@github-actions github-actions bot removed the A-consensus Related to consensus. label Oct 14, 2024
@github-actions github-actions bot added the A-consensus Related to consensus. label Oct 27, 2024
@github-actions github-actions bot removed the A-consensus Related to consensus. label Oct 31, 2024
@Boog900 Boog900 marked this pull request as ready for review November 18, 2024 16:42
types/src/blockchain.rs Outdated Show resolved Hide resolved
types/src/blockchain.rs Outdated Show resolved Hide resolved
types/src/blockchain.rs Show resolved Hide resolved
storage/blockchain/Cargo.toml Outdated Show resolved Hide resolved
storage/blockchain/src/ops/block.rs Show resolved Hide resolved
binaries/cuprated/src/p2p/request_handler.rs Outdated Show resolved Hide resolved
binaries/cuprated/src/p2p/request_handler.rs Outdated Show resolved Hide resolved
binaries/cuprated/src/p2p/request_handler.rs Outdated Show resolved Hide resolved
Comment on lines +382 to +391
let BlockChainContextResponse::Context(context) = blockchain_context_service
.ready()
.await
.expect(PANIC_CRITICAL_SERVICE_ERROR)
.call(BlockChainContextRequest::Context)
.await
.expect(PANIC_CRITICAL_SERVICE_ERROR)
else {
unreachable!()
};
Copy link
Contributor

Choose a reason for hiding this comment

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

From #308 (review):

binaries/cuprated/src/rpc/request/*.rs which are fn versions of our tower::Service's Request -> Response to reduce noise

Since cuprated will be using Services like this in general, should I move this folder? Maybe cuprated/src/service/*.rs? I think this will reduce noise significantly through cuprated, example here would be:

let context = blockchain_context::context(&mut blockchain_context_service)
    .await
    .expect(PANIC_CRITICAL_SERVICE_ERROR)
    .unchecked_blockchain_context();

Copy link
Member Author

Choose a reason for hiding this comment

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

I have plans to rework this service to make getting the context not require a Service request, so for now I don't think so.

binaries/cuprated/src/p2p/request_handler.rs Outdated Show resolved Hide resolved
types/src/blockchain.rs Outdated Show resolved Hide resolved
types/src/types.rs Outdated Show resolved Hide resolved
storage/blockchain/src/ops/blockchain.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@hinto-janai hinto-janai left a comment

Choose a reason for hiding this comment

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

Reasoning for 83a3626

boog900: hinto: can we disable the rust-2024-compatibility lint: https://github.com/Cuprate/cuprate/actions/runs/12142669672/job/33857710159

hinto: I forgot our docs were nightly, yes btw maybe in #303?

boog900: sure

@Boog900 Boog900 merged commit 7b8756f into main Dec 3, 2024
5 of 7 checks passed
@Boog900 Boog900 deleted the p2p-request-handler branch December 3, 2024 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-binaries Related to binaries. A-dependency Related to dependencies, or changes to a Cargo.{toml,lock} file. A-net Related to networking. A-p2p Related to P2P. A-storage Related to storage. A-types Related to types. A-workspace Changes to a root workspace file or general repo file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants