Add version check for block header creation #3510
Conversation
| @@ -732,7 +733,9 @@ pub struct CommitmentAndMetadata<TYPES: NodeType> { | |||
| /// Metadata for the block payload | |||
| pub metadata: <TYPES::BlockPayload as BlockPayload<TYPES>>::Metadata, | |||
| /// Builder fee data | |||
| pub fee: BuilderFee<TYPES>, | |||
| pub fees: Vec1<BuilderFee<TYPES>>, | |||
There was a problem hiding this comment.
I'm not sure a Vec1 is the best option. With the marketplace it will be possible to receive no builder bundles and therefore have no builder fees in a block.
There was a problem hiding this comment.
For some reason I thought we were using a Vec1 for this in some other place, but let me look at it again!
There was a problem hiding this comment.
I think the rationale in my original design was that, since BuilderFee was not an Option type, then we needed always put something in the fee field. Therefore, it seemed sensible to use vec1 for backwards compatibility to guarantee that the existing behavior stayed.
There was a problem hiding this comment.
What does the current code do if there is an empty block? That would also result in no builder fee, right?
There was a problem hiding this comment.
I think we just put in null_block::builder_fee() currently, if we receive no bundles
There was a problem hiding this comment.
Okay, then I suppose we can do the same for the marketplace, which means a Vec1 would work fine.
| /// This trait guarantees that a particular type has urls that can be extracted from it. This trait | ||
| /// essentially ensures that the results returned by the [`AuctionResultsProvider`] trait includes a | ||
| /// list of urls for the builders that HotShot must request from. | ||
| pub trait HasUrls { |
There was a problem hiding this comment.
This is a good change, great idea here.
This PR:
Correctly propagates marketplace fees and the auction result from
PackedBundlethrough to block header creation, and adds a version check to callBlockHeader::new_marketplacewhen appropriate.This PR does not:
Key places to review:
This is unfortunately a lot of small changes rather than a single big change, so there's not any specific place to focus on. You can safely ignore all the test files, since they were just updated to account for type changes.