Skip to content

Conversation

@sug0
Copy link
Collaborator

@sug0 sug0 commented Dec 14, 2022

Based on #897

Adds some validation of the BlockSpaceAllocator logic into ProcessProposal.

SPECS: https://hackmd.io/unZg8ia4SZCKDQIY-s8d_Q?view=

@sug0 sug0 added the ledger label Dec 14, 2022
@sug0 sug0 mentioned this pull request Dec 14, 2022
8 tasks
@sug0 sug0 marked this pull request as ready for review December 15, 2022 14:35
Comment on lines +128 to +129
AllocationError = 8, /* NOTE: keep these values in sync with
* [`ErrorCodes::is_recoverable`] */
Copy link
Contributor

Choose a reason for hiding this comment

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

ErrorCodes::is_recoverable should be matching on this enum rather than returning (self as u32) <= 3, to enforce this (related to this NOTE but not the PR itself though, we shouldn't change this as part of this PR)

Copy link
Collaborator Author

@sug0 sug0 Dec 16, 2022

Choose a reason for hiding this comment

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

that's how it was defined in ProcessProposal before, this is_recoverable() method came from a ProcessProposal refactor from back when I was working on eth events vote extensions

@sug0 sug0 changed the title Plug block space allocator logic into ProcessProposal Plug BlockSpaceAllocator logic into ProcessProposal Dec 16, 2022
Copy link
Collaborator

@batconjurer batconjurer left a comment

Choose a reason for hiding this comment

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

nit: Can the tx building fns in prepare proposal be defined in the same order they are used in the main method?

@sug0 sug0 marked this pull request as draft December 20, 2022 09:28
@sug0 sug0 marked this pull request as ready for review December 20, 2022 09:37
@sug0 sug0 requested a review from batconjurer December 20, 2022 09:37
@sug0 sug0 merged commit a8f5abf into tiago/ethbridge/optimize-block-proposal Dec 20, 2022
@sug0 sug0 deleted the tiago/ethbridge/process-proposal-allocator branch December 20, 2022 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants