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(payload): add support for stack of payload builders #11004

Merged

Conversation

aroralanuk
Copy link
Contributor

@aroralanuk aroralanuk commented Sep 18, 2024

Problem

We need a flexible way to combine and sequentially execute multiple payload builders of the same type, allowing for more complex and customizable payload building strategies. Fixes #10438

Solution

Implement a new PayloadBuilderStack<L, R> that can hold and execute two builders of potentially different types in sequence, with the ability to nest these stacks for more complex arrangements.

Implementation Details

  • Create PayloadBuilderStack<L, R> with two generic type parameters for left and right builders
  • Allow for nested stacks, enabling complex arrangements like Stack<Stack<A, B>, C>

Testing

  • None

@nkysg
Copy link
Contributor

nkysg commented Sep 18, 2024

no just something that wraps two instances of the same kind (ethereum eg)
similar to https://docs.rs/tower/latest/tower/layer/util/struct.Stack.html
the idea here is that you can use two diff payload builders
and the attributes would be an Either<A, B> where A,B are the payload attributes types of the respective payload builders

Originally posted by @mattsse. FYI @aroralanuk

Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

I think the original issue requested a stack where you could use more than one type of payload builder - in this, because of the single generic, you can only do multiple of one type of payload builder. So this needs to be changed to support two different payload builders with two different types

Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

Don't think this works because it assumes all builders are the same type?


/// A stack of payload builders that can be used in sequence.
#[derive(Clone, Debug)]
pub struct PayloadBuilderStack<B>(Vec<B>);
Copy link
Collaborator

Choose a reason for hiding this comment

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

take a look at this type for reference:

https://docs.rs/tower/latest/tower/layer/util/struct.Stack.html

because this has two instances, this allows "stacking" of multiple different types, which can be nested and is very flexible, for example: Stack<Stack<A, B>, C> etc.

A vec would work if all payloadtypes are the same but ideally we can be a bit more flexible here by using two generics

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I understand now, will fix soon!

Comment on lines 26 to 27
type Attributes = B::Attributes;
type BuiltPayload = B::BuiltPayload;
Copy link
Collaborator

Choose a reason for hiding this comment

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

if we do Stack<A,B> then these could be Either<A,b>:

https://docs.rs/futures/latest/futures/future/enum.Either.html

&self,
args: BuildArguments<Pool, Client, Self::Attributes, Self::BuiltPayload>,
) -> Result<BuildOutcome<Self::BuiltPayload>, PayloadBuilderError> {
let mut args = Some(args);
Copy link
Collaborator

Choose a reason for hiding this comment

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

and these functions would then do a match on the Either and delegate the to A or B, we likely needs some mapper helpers on BuildArguments and Outcome

Copy link
Collaborator

Choose a reason for hiding this comment

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

these changes we don't need, because those are independent of the payloadbuilder type itself

@mattsse mattsse added the A-block-building Related to block building label Sep 20, 2024
@aroralanuk
Copy link
Contributor Author

aroralanuk commented Sep 21, 2024

Both L and R can be different implementations of PayloadBuilder<Pool, Client> but R must have the same Attributes and BuiltPayload associated types as L. This makes it simpler then to have Either<L,R> which would require us to implement PayloadBuilderAttributes for Either<L::Attributes, R::Attributes> and similarly for Either<L::BuildPayload, R::BuildPayload> and this felt a bit unnecessary.

@mattsse does this feel still too restrictive, we can do:

type StackAB = PayloadBuilderStack<BuilderA, BuilderB>;
type StackABC = PayloadBuilderStack<StackAB, BuilderC>;
type StackABCCA = PayloadBuilderStack<StackABC, PayloadBuilderStack<BuilderC, BuilderA>>;

@aroralanuk aroralanuk changed the title feat(payload): add support for multiple payload builders in BasicPayloadJobGenerator feat(payload): add support for stack of payload builders Sep 23, 2024
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.

cool, the last thing that is missing is changing the stack attribute types

Comment on lines 29 to 30
type Attributes = L::Attributes;
type BuiltPayload = L::BuiltPayload;
Copy link
Collaborator

Choose a reason for hiding this comment

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

there is now no way to determine which builder should be used, hence we should use Either<L::Attr, R::Atrr> here

ref https://github.com/paradigmxyz/reth/pull/11004/files#r1768054937

@aroralanuk
Copy link
Contributor Author

It's a bit more verbose now (with the PayloadBuilderAttributes and BuiltPayload implementation for Either and map_payload), but it uses Either<L::Attr, R::Attr> now. @mattsse

Comment on lines 5 to 6
use futures_util::future::Either as EitherFuture;

Copy link
Member

Choose a reason for hiding this comment

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

While it might be code duplication, we're not really implementing Future for an either type here, so it seems better to just hand roll an Either type for the crate and implement Error / PayloadBuilderAttributes / BuiltPayload on it. It would help naming as well

@aroralanuk
Copy link
Contributor Author

Friendly ping @Rjected @mattsse :)

@aroralanuk
Copy link
Contributor Author

@mattsse Can you merge this?

@mattsse
Copy link
Collaborator

mattsse commented Nov 7, 2024

ah oops, forgot to pull the trigger

ty and sorry for the long delay on this -.-

@mattsse mattsse added this pull request to the merge queue Nov 7, 2024
Merged via the queue into paradigmxyz:main with commit b1642f9 Nov 7, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-block-building Related to block building
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add PayloadBuilder Implementation that supports two types of builders
5 participants