feat(ABCI): New Proposal Struct with Associated Metadata #126
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nivasan1
reviewed
Sep 27, 2023
block/proposals/update.go
Outdated
| return fmt.Errorf("err retriveing transaction info: %s", err) | ||
| } | ||
|
|
||
| // Invarient check: Ensure that the transaction is not already in the proposal. |
There was a problem hiding this comment.
shii, i cant spell
nivasan1
reviewed
Sep 27, 2023
block/proposals/update.go
Outdated
| for index, tx := range partialProposal { | ||
| txInfo, err := utils.GetTxInfo(p.TxEncoder, tx) | ||
| if err != nil { | ||
| return fmt.Errorf("err retriveing transaction info: %s", err) |
There was a problem hiding this comment.
ong i cant spell
nivasan1
reviewed
Sep 27, 2023
block/proposals/update.go
Outdated
| } | ||
|
|
||
| // Invarient check: Ensure we have not already prepared a partial proposal for this lane. | ||
| if _, ok := p.Info.TxsByLane[lane]; ok { |
There was a problem hiding this comment.
nit: would make this check the first check (avoids redundant work)
nivasan1
reviewed
Sep 27, 2023
block/proposals/update.go
Outdated
| // the lane. | ||
| // 5. The lane must not have already prepared a partial proposal. | ||
| // 6. The transaction must not already be in the proposal. | ||
| func (p *Proposal) UpdateProposal(lane string, partialProposal []sdk.Tx, limit LaneLimits) error { |
There was a problem hiding this comment.
Why would the caller pass in their LaneLimits? Why wouldn't the proposal j get them here from the lane-string? I.e if the purpose of this abstraction is to have devs only aware of adding txs to proposals, does it not make more sense to j hide the notion of a LaneLimit entirely?
There was a problem hiding this comment.
yea i agree, will update
davidterpay
added
backport/v1.x.x
mergify bot
pushed a commit
that referenced
this pull request
Sep 28, 2023
* new proto types for proposal info * new proposal type * nits * lane input * lint * feat(ABCI): Deprecating `CheckOrderHandler` with new Proposal MetaData (#127) * refactor without checkorder * nits * more nits * lint * nits * feat(ABCI): Updating MEV lane to have no `CheckOrder` handler + testing (#128) * updating mev lane * nits * preventing adding multiple bid txs in prepare * update (cherry picked from commit b9d6761) # Conflicts: # abci/abci.go # abci/abci_test.go # block/base/handlers.go # block/base/types.go # block/proposals.go # block/utils/utils_test.go # lanes/mev/abci.go # tests/app/ante.go # tests/app/app.go # tests/integration/block_sdk_suite.go
mergify bot
pushed a commit
that referenced
this pull request
Sep 28, 2023
* new proto types for proposal info * new proposal type * nits * lane input * lint * feat(ABCI): Deprecating `CheckOrderHandler` with new Proposal MetaData (#127) * refactor without checkorder * nits * more nits * lint * nits * feat(ABCI): Updating MEV lane to have no `CheckOrder` handler + testing (#128) * updating mev lane * nits * preventing adding multiple bid txs in prepare * update (cherry picked from commit b9d6761)
This was referenced Sep 28, 2023
davidterpay
added a commit
that referenced
this pull request
Sep 28, 2023
* new proto types for proposal info * new proposal type * nits * lane input * lint * feat(ABCI): Deprecating `CheckOrderHandler` with new Proposal MetaData (#127) * refactor without checkorder * nits * more nits * lint * nits * feat(ABCI): Updating MEV lane to have no `CheckOrder` handler + testing (#128) * updating mev lane * nits * preventing adding multiple bid txs in prepare * update (cherry picked from commit b9d6761) Co-authored-by: David Terpay <35130517+davidterpay@users.noreply.github.com>
davidterpay
added a commit
that referenced
this pull request
Oct 4, 2023
…) (#132) * feat(ABCI): New Proposal Struct with Associated Metadata (#126) * new proto types for proposal info * new proposal type * nits * lane input * lint * feat(ABCI): Deprecating `CheckOrderHandler` with new Proposal MetaData (#127) * refactor without checkorder * nits * more nits * lint * nits * feat(ABCI): Updating MEV lane to have no `CheckOrder` handler + testing (#128) * updating mev lane * nits * preventing adding multiple bid txs in prepare * update (cherry picked from commit b9d6761) # Conflicts: # abci/abci.go # abci/abci_test.go # block/base/handlers.go # block/base/types.go # block/proposals.go # block/utils/utils_test.go # lanes/mev/abci.go # tests/app/ante.go # tests/app/app.go # tests/integration/block_sdk_suite.go * fix * nit * docs nit * height nit --------- Co-authored-by: David Terpay <35130517+davidterpay@users.noreply.github.com> Co-authored-by: David Terpay <david.terpay@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This PR creates a new proposal type that replace the current proposal building object. The premise here is the following:
By attaching metadata to the proposal about how many transactions were included by each lane, we can deprecate the
CheckOrderHandlerand solely have a singleProcessLanehandler. Given a constructed proposal with meta data attached to it - lane x has 1 txs, lane y has 2 txs, etc. - we no longer have to do any introspection into what transactions belong to what lane. Definitively, we can take the proposal and break it up into its corresponding partial proposals - the proposals each lane built - and have stronger guarantees about the properties of the proposed block.Additionally, this will both decrease the complexity of the code base and will allow us to subsequently do more nuanced things when building/verifying proposals. For example, this can include