fix(Config): Simplifying config interface #104
Conversation
x/builder/ante/ante.go
Outdated
| GetAuctionBidInfo(tx sdk.Tx) (mempool.AuctionBidInfo, error) | ||
| GetBundleSigners(txs [][]byte) ([]map[string]struct{}, error) | ||
| GetAuctionBidInfo(tx sdk.Tx) (*mempool.AuctionBidInfo, error) | ||
| GetTransactionSigners(tx []byte) (map[string]struct{}, error) |
There was a problem hiding this comment.
I would put GetTransactionSigners() as a function on the AuctionBidInfo
| // We only need to verify the auction bid relative to the local validator's mempool if the mode | ||
| // is checkTx or recheckTx. Otherwise, the ABCI handlers (VerifyVoteExtension, ExtendVoteExtension, etc.) | ||
| // will always compare the auction bid to the highest bidding transaction in the mempool leading to | ||
| // poor liveness guarantees. | ||
| topBid := sdk.Coin{} |
There was a problem hiding this comment.
by forcing bid to be an sdk.Coins() we get less generalizability. For instance in the polaris implementation, this forces us to pass evmDenom around, whereas if topBid could be just an uint64 for instance, then we could get rid of this simplifiying the integration.
Really the only thing that bids are required to be are comparable
There was a problem hiding this comment.
Agreed. Using sdk.Coin would make more sense in a world where we have multiple fee tokens. Opening an issue for this.
x/builder/keeper/auction_test.go
Outdated
| @@ -327,3 +327,18 @@ func (suite *KeeperTestSuite) TestValidateBundle() { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| func (suite *KeeperTestSuite) getBundleSigners(bundle [][]byte) ([]map[string]struct{}, error) { | |||
There was a problem hiding this comment.
as mentioned above, this function should go on the AuctionBidInfo imo.
| @@ -1,242 +0,0 @@ | |||
| package mempool | |||
There was a problem hiding this comment.
Moved all this code to auction_bid.go
mempool/auction_bid.go
Outdated
| // AuctionBid defines the interface for processing auction transactions. It is | ||
| // a wrapper around all of the functionality that each application chain must implement | ||
| // in order for auction processing to work. | ||
| AuctionBid interface { |
There was a problem hiding this comment.
Thought about this some more...what about AuctionFactory?
There was a problem hiding this comment.
Big factory pattern enthusiast.
Decoupling auction bid from mempool also makes sense to me.
There was a problem hiding this comment.
Down for AuctionFactory. I have some ideas on how we could still use the ExtensionOptions on sdk.Tx to completely decouple the bid from the mempool. If we only have it store the priority and validate that the bid and priority within the tx and extension option match, then the mempool doesn't need to have conception of what the underlying tx is. This might mess with general UX tho since most wallets and users dont add extension options to begin with. We should maybe explore this approach in the BlockBuster refactor.
There was a problem hiding this comment.
Thinking extension over msg interface?
There was a problem hiding this comment.
We're still evaluating what the best approach is in the context of the refactor we're planning but msg interface is probably going to be our approach. We'll share the RFC of the refactor with you in the coming days (that'll address the coupling concerns you've been discussing).
There was a problem hiding this comment.
Definitely going to be curious to hear your thoughts on design and such since the refactor will be a new approach to general mempool design.
mempool/mempool.go
Outdated
| } | ||
| } | ||
|
|
||
| // Insert inserts a transaction into the mempool based on the transaction type (normal or auction). | ||
| func (am *AuctionMempool) Insert(ctx context.Context, tx sdk.Tx) error { | ||
| isAuctionTx, err := am.IsAuctionTx(tx) | ||
| bidInfo, err := am.GetAuctionBidInfo(tx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Insert the transactions into the appropriate index. | ||
| switch { |
There was a problem hiding this comment.
I think an if/else would be a bit cleaner here IMO. if bidInfo != nil {} else {}
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Remove the transactions from the appropriate index. | ||
| switch { | ||
| case !isAuctionTx: |
(cherry picked from commit d58b36b) # Conflicts: # abci/abci_test.go # abci/vote_extensions.go # abci/vote_extensions_test.go
| // that is included in the bundle into a sdk.Tx. In the default case, the transaction | ||
| // that is included in the bundle will be the raw bytes of an sdk.Tx so we can just | ||
| // decode it. | ||
| func (config *DefaultAuctionFactory) WrapBundleTransaction(tx []byte) (sdk.Tx, error) { |
There was a problem hiding this comment.
config -> factory or daf @davidterpay
| txDecoder: txDecoder, | ||
| txEncoder: txEncoder, | ||
| txIndex: make(map[string]struct{}), | ||
| AuctionFactory: config, |
No description provided.