feat(miner): Allow allowlisted sdk.Msgs (mainly for validator configuration)
#1269
Conversation
WalkthroughThe changes introduced in the codebase primarily revolve around the handling of validator transactions in the Changes
Poem
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (17)
- build/tools/go.mod
- build/tools/go.sum
- contracts/go.mod
- contracts/go.sum
- cosmos/go.mod
- cosmos/go.sum
- e2e/localnet/go.mod
- e2e/localnet/go.sum
- e2e/precompile/go.mod
- e2e/precompile/go.sum
- e2e/testapp/go.mod
- e2e/testapp/go.sum
- eth/go.mod
- eth/go.sum
- go.work.sum
- lib/go.mod
- lib/go.sum
Files selected for processing (4)
- cosmos/miner/miner.go (6 hunks)
- cosmos/miner/msgs.go (1 hunks)
- cosmos/runtime/runtime.go (1 hunks)
- e2e/testapp/app.go (2 hunks)
Files skipped from review due to trivial changes (2)
- cosmos/miner/msgs.go
- cosmos/runtime/runtime.go
Additional comments: 8
e2e/testapp/app.go (2)
57-63: The import statement for the "miner" package has been added. This is necessary to access the
DefaultAllowedMsgsmap defined in theminerpackage.192-197: The call to the
Buildfunction of thePolarisstruct now includesminer.DefaultAllowedMsgsas an argument. This change affects the signature of theBuildfunction in thePolarisstruct. Ensure that theBuildfunction in thePolarisstruct has been updated to accept this new argument.cosmos/miner/miner.go (6)
24-34: Imports look fine. No changes needed.
52-56: The
Appinterface has been updated to includebaseapp.ProposalTxVerifier. Ensure that all implementations of this interface have been updated accordingly.67-85: The
Minerstruct andNewfunction have been updated to include avalTxSelectorfield and anallowedValMsgsparameter respectively. Ensure that all calls toNewhave been updated to match the new signature.91-109: The
PrepareProposalfunction has been updated to process validator messages. Ensure that the function is being used correctly in the context of these changes.122-135: The
PrepareProposalfunction now returns a combination of the payload envelope and the validator transactions. Ensure that the function's return value is being handled correctly in all places where it is called.194-225: The new function
processValidatorMsgshas been added to process validator messages. Ensure that this function is being used correctly in the context of these changes.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1269 +/- ##
=======================================
Coverage 48.54% 48.54%
=======================================
Files 84 84
Lines 4857 4857
=======================================
Hits 2358 2358
Misses 2325 2325
Partials 174 174 |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- cosmos/miner/miner.go (6 hunks)
Additional comments: 5
cosmos/miner/miner.go (5)
24-34: The new import statements are correctly placed and formatted. Ensure that the imported packages are used in the code.
52-56: The
baseapp.ProposalTxVerifierinterface has been added to theAppinterface. This is a good practice as it ensures that any type that satisfies theAppinterface also satisfies thebaseapp.ProposalTxVerifierinterface.67-84: The
Minerstruct andNewfunction have been updated to include avalTxSelectorfield and anallowedValMsgsparameter respectively. This is a good practice as it allows for more flexibility and control over the behavior of theMiner.91-109: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [91-135]
The
PrepareProposalfunction has been modified to process validator messages and include them in the proposal. This is a good practice as it allows for more control over the proposal preparation process.
- 194-225: The
processValidatorMsgsfunction has been added to process validator messages. This is a good practice as it allows for more control over the processing of validator messages.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (3)
- cosmos/go.sum
- e2e/testapp/go.sum
- go.work.sum
Files selected for processing (1)
- cosmos/miner/miner.go (6 hunks)
Additional comments: 8
cosmos/miner/miner.go (8)
24-36: The import statements look fine. Ensure that all the imported packages are being used in the code.
54-58: The
Appinterface now includes thebaseapp.ProposalTxVerifierinterface. This change is fine as long as all implementations of theAppinterface have been updated to include the methods ofbaseapp.ProposalTxVerifier.69-76: The
Minerstruct now includes avalTxSelectorfield of typebaseapp.TxSelectorand anallowedValMsgsmap. This is fine as long as all instances ofMinerare updated to include these new fields.79-87: The
Newfunction for creating aMinernow takes an additionalallowedValMsgsparameter. This is fine as long as all calls toNewhave been updated to include this new parameter.122-157: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [95-144]
The
PrepareProposalfunction now takes areqparameter of type*abci.RequestPrepareProposal. This is fine as long as all calls toPrepareProposalhave been updated to include this new parameter. The function now also processes validator messages and combines them with the payload envelope. This is a significant change and should be thoroughly tested.
146-155: The
buildBlockfunction now returns theethGasUsedvalue in addition to the payload. This is fine as long as all calls tobuildBlockhave been updated to handle this additional return value.186-197: The
resolveEnvelopefunction now returns thegasUsedvalue in addition to the envelope. This is fine as long as all calls toresolveEnvelopehave been updated to handle this additional return value.204-239: The
processValidatorMsgsfunction is added to process validator messages. This is a significant addition and should be thoroughly tested.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- e2e/testapp/go.mod
- e2e/testapp/go.sum
Files selected for processing (6)
- cosmos/runtime/miner/abci.go (2 hunks)
- cosmos/runtime/miner/interfaces.go (1 hunks)
- cosmos/runtime/miner/miner.go (3 hunks)
- cosmos/runtime/miner/msgs.go (1 hunks)
- cosmos/runtime/runtime.go (1 hunks)
- e2e/testapp/app.go (2 hunks)
Files skipped from review due to trivial changes (2)
- cosmos/runtime/miner/msgs.go
- cosmos/runtime/runtime.go
Additional comments: 13
cosmos/runtime/miner/interfaces.go (1)
- 42-46: The new method
TxDecodehas been added to theAppinterface. Ensure that all implementations of this interface have been updated to include this method.TxDecode(txBytes []byte) (sdk.Tx, error)e2e/testapp/app.go (2)
58-63: The import statement for the "miner" package is added. Ensure that the package path is correct and accessible.
192-197: The
Buildfunction of thePolarisstruct is called with an additional argumentminer.DefaultAllowedMsgs. Ensure that theBuildfunction is updated to accept this new argument in its signature.cosmos/runtime/miner/abci.go (4)
41-46: The introduction of
valTxsandethGasUsedvariables is consistent with the changes described in the pull request summary. Ensure that these variables are used correctly throughout the function.50-53: The
PreBlockerfunction is now called with anilargument instead ofreq. Ensure that this change does not cause any issues, especially if thePreBlockerfunction uses the argument in its implementation.68-75: The
buildBlockfunction now returns an additionalethGasUsedvariable and theprocessValidatorMsgsfunction is called with additional arguments:req.MaxTxBytes,ethGasUsed, andreq.Txs. Ensure that these changes are consistent with the updated function signatures.78-83: The
PrepareProposalfunction now returns a new variablevalTxsand modifies the return value to include bothpayloadEnvelopeBzandvalTxsin theTxsfield ofabci.ResponsePrepareProposal. This change is consistent with the pull request summary.cosmos/runtime/miner/miner.go (6)
24-37: New import statements have been added. Ensure that these packages are included in your project dependencies.
41-48: The
Minerstruct has been updated to includevalTxSelectorandallowedValMsgsfields. Ensure that these fields are properly initialized and used throughout the codebase.51-59: The
Newfunction now acceptsallowedValMsgsas a parameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.68-75: The
buildBlockfunction now returns the resolved envelope and gas used. Ensure that all calls to this function throughout the codebase have been updated to handle the new return values.107-117: The
resolveEnvelopefunction now returns the resolved envelope and gas used. Ensure that all calls to this function throughout the codebase have been updated to handle the new return values.124-159: The new
processValidatorMsgsfunction processes validator messages. Ensure that this function is properly called and its return values are correctly handled.
|
@BrickBera please verify |
itsdevbear
force-pushed
the
allow-msgs
branch
from
303af58
to
5c4a0ee
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (17)
- build/tools/go.mod
- build/tools/go.sum
- contracts/go.mod
- contracts/go.sum
- cosmos/go.mod
- cosmos/go.sum
- e2e/localnet/go.mod
- e2e/localnet/go.sum
- e2e/precompile/go.mod
- e2e/precompile/go.sum
- e2e/testapp/go.mod
- e2e/testapp/go.sum
- eth/go.mod
- eth/go.sum
- go.work.sum
- lib/go.mod
- lib/go.sum
Files selected for processing (7)
- cosmos/runtime/miner/miner.go (3 hunks)
- cosmos/runtime/txpool/handler.go (2 hunks)
- cosmos/runtime/txpool/handler_test.go (1 hunks)
- cosmos/runtime/txpool/mocks/geth_tx_pool.go (2 hunks)
- cosmos/runtime/txpool/mocks/tx_sub_provider.go (2 hunks)
- eth/eth.go (1 hunks)
- eth/polar/api_backend.go (1 hunks)
Additional comments: 14
eth/eth.go (1)
- 50-56: The
SubscribeTransactionsfunction has been renamed and its signature has been updated. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, make sure to handle the returnedevent.Subscriptioncorrectly in the calling code.cosmos/runtime/txpool/handler.go (1)
- 53-54: The
SubscribeTransactionsmethod now accepts areorgsparameter. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.cosmos/runtime/txpool/mocks/tx_sub_provider.go (2)
22-34: The
SubscribeTransactionsmethod has been updated to include a new parameterreorgs. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.38-68: The
TxSubProvider_SubscribeTransactions_Callstruct and its methods have been updated to reflect the changes in theSubscribeTransactionsmethod. This looks fine as long as the changes are consistent with the rest of the codebase.cosmos/runtime/txpool/handler_test.go (1)
- 57-63: Ensure that the
SubscribeTransactionsfunction is correctly implemented and that the two arguments it now requires are being provided correctly. Also, verify that all instances ofSubscribeTransactionsin the codebase have been updated to match the new function signature.- subprovider.On("SubscribeTransactions", mock.Anything, mock.Anything).Return(subscription) + subprovider.On("SubscribeTransactions", mock.Anything, mock.Anything).Return(subscription)cosmos/runtime/txpool/mocks/geth_tx_pool.go (2)
209-221: The function
SubscribeTransactionshas been updated to take two arguments and return a different type. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.228-254: The
GethTxPool_SubscribeTransactions_Callstruct and its methods have been updated to reflect the changes in theSubscribeTransactionsfunction. Ensure that these changes are consistent with the intended functionality.eth/polar/api_backend.go (1)
- 523-527: The function call
b.polar.txPool.SubscribeNewTxsEvent(ch)has been replaced withb.polar.txPool.SubscribeTransactions(ch, true). Ensure that the new functionSubscribeTransactionsis implemented correctly and that it behaves as expected. Also, verify that all instances ofSubscribeNewTxsEventhave been replaced withSubscribeTransactionsthroughout the codebase.- return b.polar.txPool.SubscribeNewTxsEvent(ch) + return b.polar.txPool.SubscribeTransactions(ch, true)cosmos/runtime/miner/miner.go (6)
24-37: Ensure that all the new imports are being used in the code. Unused imports can lead to confusion and unnecessary dependencies.
41-48: The
Minerstruct has been updated to include avalTxSelectorfield and anallowedValMsgsmap. Ensure that these new fields are being used appropriately throughout the code.51-59: The
Newfunction now accepts anallowedValMsgsmap as a parameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.65-75: The
buildBlockfunction now returns the resolved envelope and the gas used. Ensure that all calls to this function throughout the codebase have been updated to handle the new return values.104-117: The
resolveEnvelopefunction now returns the resolved envelope and the gas used. Ensure that all calls to this function throughout the codebase have been updated to handle the new return values.124-159: The new
processValidatorMsgsfunction processes validator messages. Ensure that this function is being called appropriately throughout the codebase.
itsdevbear
force-pushed
the
allow-msgs
branch
from
f67ff31
to
50a0736
Compare
Signed-off-by: Devon Bear <[email protected]>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- cosmos/runtime/miner/abci.go (2 hunks)
Additional comments: 5
cosmos/runtime/miner/abci.go (5)
41-46: The introduction of new variables
valTxsandethGasUsedis clear and straightforward. Ensure that these variables are used correctly throughout the function.75-77: The
buildBlockfunction now returnspayloadEnvelopeBzandethGasUsedin addition to an error. Ensure that the function is updated accordingly and that the returned values are used correctly.80-82: The
processValidatorMsgsfunction is called to process validator messages and returnsvalTxsand an error. Ensure that this function is implemented correctly and that the returnedvalTxsare used correctly.84-90: The
payloadEnvelopeBzandvalTxsare combined into a new slice calledallTxs. This is a logical step to combine all transactions into a single slice.93-93: The
allTxsslice is returned as a transaction in the proposal. This is a logical step to return all transactions in the proposal.
Summary by CodeRabbit