feat: add BuiltPayload associated type to EngineTypes #6028
Conversation
Rjected
added
C-enhancement
Rjected
force-pushed
the
dan/built-payload
branch
from
05973e7 to
38515eb
Compare
| /// Converts the type into the response expected by `engine_getPayloadV1` | ||
| fn into_v1_payload(self) -> ExecutionPayloadV1; | ||
|
|
||
| /// Converts the type into the response expected by `engine_getPayloadV2` | ||
| fn into_v2_payload(self) -> ExecutionPayloadEnvelopeV2; | ||
|
|
||
| /// Converts the type into the response expected by `engine_getPayloadV2` | ||
| fn into_v3_payload(self) -> ExecutionPayloadEnvelopeV3; |
There was a problem hiding this comment.
these are fine for now, but perhaps we want to reconsider this eventually and instead just require that the type is Serialize or can be converted into serde_Value
There was a problem hiding this comment.
agree, it at least just needs to work well with get_payload in EngineApi. I like that the outputs are typed in EngineApi currently. But yeah, we may need to change this
|
|
||
| /// Message type for the [PayloadBuilderService]. | ||
| pub enum PayloadServiceCommand<T> { | ||
| pub enum PayloadServiceCommand<A, P> { |
There was a problem hiding this comment.
can we use EngineT: EngineTypes here?
| Gen::Job: PayloadJob<PayloadAttributes = Engine::PayloadBuilderAttributes>, | ||
| Gen::Job: PayloadJob< | ||
| PayloadAttributes = Engine::PayloadBuilderAttributes, | ||
| BuiltPayload = Engine::BuiltPayload, |
There was a problem hiding this comment.
I think we want:
<Gen::Job as PayloadJob>: Into<Engine::BuiltPayload>this way the service can support any generator that creates jobs with a Payload that can be converted to the configured Engine::BuiltPayload.
I realize that this breaks the Arc wrapper, so I think we should remove the Arc from the Job trait resolve function signature.
cloning a payload isn't a big deal anyway and a job can take the payload on resolve
There was a problem hiding this comment.
alright, I had to get rid of pretty much all of the Arc, might be possible to make it work again with best_payload, but the bounds are now Into<Engine::BuiltPayload>
Rjected
force-pushed
the
dan/built-payload
branch
from
eb2d2cc to
57de0de
Compare
| /// Initializes the payload with the given initial block. | ||
| fn new(id: PayloadId, block: SealedBlock, fees: U256) -> Self; |
There was a problem hiding this comment.
hmm, I think this is fine now, but this effectively restricts how the built payload looks like.
I see that we're using this as the fallback for empty payloads, which is a default.
let's track this separately, so we can improve this a bit.
perhaps it should be the job's responsibility to create an empty payload
Adds a
BuiltPayloadassociated type and trait toEngineTypescloses #6026