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

dev: add send_and_resolve #10847

Merged
merged 2 commits into from
Sep 13, 2024
Merged

Conversation

greged93
Copy link
Contributor

Adds a send and resolve helper on the PayloadBuilderHandle. This helper allows to request for a payload to be build based on the provided payload builder attribute and returns a future that resolves to the payload.

Resolves #10845

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.

one suggestion

Comment on lines 31 to 32
type MaybePayloadFuture<P> =
Pin<Box<dyn Future<Output = Option<Result<P, PayloadBuilderError>>> + Send + Sync>>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need this

@@ -127,6 +129,31 @@ where
}
}

/// Sends a message to the service to start building a new payload for the given payload
/// attributes and returns a future that resolves to the payload.
pub fn send_and_resolve_payload(
Copy link
Collaborator

Choose a reason for hiding this comment

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

this can just be an async function

Comment on lines 142 to 153
let id = match rx.await {
Err(e) => return Some(Err(e.into())),
Ok(Err(e)) => return Some(Err(e)),
Ok(Ok(id)) => id,
};

let (tx, rx) = oneshot::channel();
to_service.send(PayloadServiceCommand::Resolve(id, tx)).ok()?;
match rx.await.transpose()? {
Ok(fut) => Some(fut.await),
Err(e) => Some(Err(e.into())),
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

then you can do all of this without the Box::pin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok I thought you wanted to return a future, will update

Copy link
Collaborator

@mattsse mattsse Sep 12, 2024

Choose a reason for hiding this comment

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

yeah but you can return Option<PayloadFuture>

or result PayloadFuture, PayloadNotfound

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 added this pull request to the merge queue Sep 13, 2024
Merged via the queue into paradigmxyz:main with commit 6fc81f2 Sep 13, 2024
34 checks passed
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.

Add PayloadStore::send_and_resolve helper function
2 participants