feat: Proposal logic for Slinky #1135
Conversation
WalkthroughThe overall change introduces a new architecture for handling market price updates within a blockchain protocol. It transitions from directly using a Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![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: 11
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
protocol/go.modis excluded by:!**/*.modprotocol/go.sumis excluded by:!**/*.sum
Files selected for processing (25)
- protocol/app/app.go (5 hunks)
- protocol/app/prepare/expected_keepers.go (2 hunks)
- protocol/app/prepare/prepare_proposal.go (5 hunks)
- protocol/app/prepare/prepare_proposal_test.go (17 hunks)
- protocol/app/prepare/prices/default_price_update_generator.go (1 hunks)
- protocol/app/prepare/prices/errors.go (1 hunks)
- protocol/app/prepare/prices/price_update_generator.go (1 hunks)
- protocol/app/prepare/prices/slinky_price_update_generator.go (1 hunks)
- protocol/app/prepare/prices/slinky_price_update_generator_test.go (1 hunks)
- protocol/app/process/default_market_price_decoder.go (1 hunks)
- protocol/app/process/errors.go (1 hunks)
- protocol/app/process/full_node_process_proposal.go (2 hunks)
- protocol/app/process/full_node_process_proposal_test.go (1 hunks)
- protocol/app/process/market_price_decoder.go (1 hunks)
- protocol/app/process/market_prices_test.go (4 hunks)
- protocol/app/process/process_proposal.go (2 hunks)
- protocol/app/process/process_proposal_test.go (1 hunks)
- protocol/app/process/slinky_market_price_decoder.go (1 hunks)
- protocol/app/process/slinky_market_price_decoder_test.go (1 hunks)
- protocol/app/process/transactions.go (4 hunks)
- protocol/app/process/transactions_test.go (4 hunks)
- protocol/mocks/Makefile (1 hunks)
- protocol/mocks/PriceUpdateGenerator.go (1 hunks)
- protocol/mocks/PricesKeeper.go (15 hunks)
- protocol/mocks/UpdateMarketPriceTxDecoder.go (1 hunks)
Files skipped from review due to trivial changes (2)
- protocol/mocks/PriceUpdateGenerator.go
- protocol/mocks/UpdateMarketPriceTxDecoder.go
Additional comments: 42
protocol/app/prepare/prices/price_update_generator.go (1)
- 8-12: The introduction of the
PriceUpdateGeneratorinterface with theGetValidMarketPriceUpdatesmethod is well-defined and aligns with the PR's objectives to enhance proposal handling mechanisms related to market price updates. The method signature is appropriate, and the use ofsdk.Contextis consistent with Cosmos SDK standards.protocol/app/prepare/prices/errors.go (1)
- 7-17: The
InvalidPriceErrorstruct and its associatedErrormethod are well-implemented. The error message is clear and provides valuable information about the invalid price, including the market ID and the reason, which aligns with best practices for error handling.protocol/app/prepare/expected_keepers.go (1)
- 7-12: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-18]
The removal of the
PreparePricesKeeperinterface and its methodGetValidMarketPriceUpdatesis consistent with the PR's objectives to refactor the proposal handling mechanism. The remaining keeper interfaces (PrepareClobKeeper,PreparePerpetualsKeeper,PrepareBridgeKeeper) are correctly defined and focus on different aspects of thePrepareProposalprocess.protocol/app/process/market_price_decoder.go (1)
- 8-28: The introduction of the
UpdateMarketPriceTxDecoderinterface with methodsDecodeUpdateMarketPricesTxandGetTxOffsetis well-defined, clearly outlining responsibilities for decoding market price transactions and managing transaction offsets. TheNewUpdateMarketPricesTxfunction is correctly implemented, facilitating the creation ofUpdateMarketPricesTxinstances with the necessary parameters.protocol/app/prepare/prices/default_price_update_generator.go (1)
- 9-36: The
PricesKeeperinterface and theDefaultPriceUpdateGeneratorstruct, along with itsGetValidMarketPriceUpdatesmethod, are correctly implemented. TheDefaultPriceUpdateGeneratoradheres to thePriceUpdateGeneratorinterface's contract, and the error handling inGetValidMarketPriceUpdatesensures that a nilMsgUpdateMarketPricesis correctly managed, which is crucial for robustness.protocol/app/process/errors.go (1)
- 13-49: The new error types related to proposed price validation are correctly registered with the
process_proposalmodule, and the functions for generating specific error instances are appropriately implemented. These changes enhance error reporting and provide detailed context for each error scenario, which is crucial for debugging and robustness.protocol/app/process/default_market_price_decoder.go (1)
- 7-42: The
DefaultUpdateMarketPriceTxDecoderstruct, implementing theUpdateMarketPriceTxDecoderinterface, is correctly implemented. Its methodsDecodeUpdateMarketPricesTxandGetTxOffsetare clearly defined and align with the expected functionality. The constructor function is appropriately defined, ensuring that necessary dependencies are correctly passed.protocol/app/process/full_node_process_proposal.go (1)
- 15-21: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-37]
The modification to the
FullNodeProcessProposalHandlerfunction, replacing thepricesKeeperparameter withpricesTxDecoderof typeUpdateMarketPriceTxDecoder, aligns with the PR's objectives to enhance the proposal handling mechanism. The implementation of the proposal handling logic, including decoding transactions and measuring MEV metrics, is correctly done, ensuring robust and expected behavior.protocol/app/process/full_node_process_proposal_test.go (1)
- 88-94: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-91]
The modifications to the
TestFullNodeProcessProposalHandlerfunction, specifically the use ofprocess.NewDefaultUpdateMarketPriceTxDecoderwith additional arguments, are correctly done. These changes ensure that the test aligns with the updated implementation of theFullNodeProcessProposalHandlerfunction, providing the necessary dependencies for the decoder and reflecting the PR's objectives.protocol/app/prepare/prices/slinky_price_update_generator.go (1)
- 14-104: The
SlinkyPriceUpdateGeneratorstruct, implementing thePriceUpdateGeneratorinterface, is correctly implemented. ItsGetValidMarketPriceUpdatesmethod is comprehensive, including checks for vote extensions, aggregation of votes, and mapping of currency pairs to market IDs. The constructor function is appropriately defined, ensuring that necessary dependencies are correctly passed. The error handling is clear, contributing to the robustness of the implementation.protocol/app/process/process_proposal.go (1)
- 40-40: The addition of the
pricesTxDecoderparameter to theProcessProposalHandlerfunction signature is a significant change. It replaces the previouspricesKeeperparameter, indicating a shift towards a more decoupled and flexible approach for decoding market price transactions. This change aligns with the PR's objective to introduce new interfaces for handling proposals, specifically for market price updates. Ensure that all calls toProcessProposalHandlerthroughout the codebase have been updated to pass the correctpricesTxDecoderimplementation.protocol/app/process/slinky_market_price_decoder.go (2)
- 12-29: The
SlinkyMarketPriceDecoderstruct and its constructorNewSlinkyMarketPriceDecoderare well-defined, encapsulating the logic for decoding market price updates with consideration for vote-extensions. This design adheres to the principles of modularity and encapsulation, allowing for easy extension or modification in the future. It's important to ensure that thedecoderandaggfields are properly initialized and used throughout the struct's methods to maintain consistency and reliability in the decoding process.- 80-88: The
GetTxOffsetmethod provides a straightforward way to determine the offset for injected transactions based on whether vote-extensions are enabled. This method enhances the flexibility of transaction processing by allowing dynamic adjustment of indices based on the application's configuration. It's a good practice to ensure that this method is used consistently wherever transaction offsets need to be calculated, to maintain coherence in handling injected transactions.protocol/app/process/transactions.go (1)
- 126-128: The adjustments to transaction index handling based on injected extensions, as seen in the handling of
OtherTxs, demonstrate a thoughtful approach to accommodating dynamic transaction structures. This change ensures that the application can correctly process transactions even when additional data (e.g., vote-extensions) is injected. It's crucial to ensure that these adjustments are consistently applied across all relevant parts of the codebase to maintain the integrity of transaction processing.protocol/mocks/Makefile (1)
- 59-60: The addition of mock generation commands for
PriceUpdateGeneratorandUpdateMarketPriceTxDecoderin theMakefileis a necessary step to support unit testing of the new functionalities introduced in this PR. These commands facilitate the creation of mocks for the newly introduced interfaces, enabling comprehensive testing of their implementations and interactions. It's important to ensure that these mocks are utilized effectively in unit tests to validate the behavior of the interfaces under various conditions.protocol/mocks/PricesKeeper.go (4)
- 22-24: Adding panic checks for unspecified return values in the
CreateMarketmock function improves the robustness of the test suite by ensuring that tests fail fast if the mock's return values are not properly set up. This is a good practice for mock functions as it helps identify issues in test setup early in the test execution process. Ensure that similar checks are consistently applied across all mock functions for consistency.- 50-52: The addition of panic checks for unspecified return values in the
GetAllMarketParamPricesmock function follows best practices for mock setup and error handling. This ensures that tests using this mock will fail immediately if the return values are not correctly specified, aiding in quicker debugging and test reliability.- 80-82: Including panic checks for unspecified return values in the
GetAllMarketParamsmock function is a good practice. It ensures that any test relying on this mock function will encounter an immediate failure if the mock's return values are not properly configured, promoting test reliability and maintainability.- 100-102: The panic checks added for unspecified return values in the
GetAllMarketPricesmock function enhance the test suite's robustness by ensuring immediate failure if the mock's return values are not set up correctly. This practice helps in early detection of issues in test setup, contributing to more reliable and maintainable tests.protocol/app/process/process_proposal_test.go (1)
- 288-288: The addition of
process.NewDefaultUpdateMarketPriceTxDecoder(pricesKeeper, constants.TestEncodingCfg.TxConfig.TxDecoder())as a new argument to theProcessProposalHandlerfunction call in theTestProcessProposalHandler_Errortest is a significant change. This ensures that the test accurately reflects the updated function signature and its dependencies. It's important to verify that this change aligns with the intended design and that theDefaultUpdateMarketPriceTxDecoderis correctly utilized within theProcessProposalHandler. This addition should also prompt a review of related tests to ensure they are updated accordingly to cover the new logic introduced by theDefaultUpdateMarketPriceTxDecoder.protocol/app/prepare/prepare_proposal.go (3)
- 13-16: The addition of new imports for handling price updates and market price decoders aligns with the PR's objectives to enhance proposal logic for Slinky. Ensure that these packages are used effectively within the file and that there are no unused imports to maintain code cleanliness.
- 59-59: Replacing
PreparePricesKeeperwithpriceUpdateGeneratorin thePrepareProposalHandlerfunction is a significant change. It's crucial to ensure that all references and usages ofPreparePricesKeeperare updated to usepriceUpdateGeneratorcorrectly. This change should also be reflected in any related documentation or comments to avoid confusion.Verification successful
The verification process confirms that
PreparePricesKeeperis no longer actively used in the project's main codebase, with occurrences limited to mocks and tests. ThepriceUpdateGeneratoris correctly integrated and used throughout the project. The review comment's concerns have been addressed, indicating a successful refactoring fromPreparePricesKeepertopriceUpdateGenerator. The only minor issue noted is an outdated comment indefault_price_update_generator.go, which does not impact the correctness of the implementation.* 77-88: The logic for handling external commit bytes (`extCommitBzTx`) and generating valid market price updates seems correct. However, ensure that error handling is robust and that any potential issues with `priceUpdateGenerator.GetValidMarketPriceUpdates` are logged and handled appropriately. It's also important to verify that the `EmptyResponse` is the desired behavior in case of an error, as it might impact the proposal's preparation process.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that `PreparePricesKeeper` is no longer used in the project. rg --type go "PreparePricesKeeper" # Verify that `priceUpdateGenerator` is used correctly throughout the project. rg --type go "priceUpdateGenerator"Length of output: 1746
protocol/app/process/slinky_market_price_decoder_test.go (6)
- 16-38: The setup for the
SlinkyMarketPriceDecoderSuitetest suite is well-structured, with mock objects for the price update generator and the update market price tx decoder. This setup facilitates isolated testing of theSlinkyMarketPriceDecoderfunctionality. Ensure that all necessary teardown or cleanup steps are included if any stateful components are introduced in future tests.- 41-88: The tests for scenarios where vote extensions are not enabled (
TestVoteExtensionsNotEnabled) are comprehensive, covering both the case where a non-empty proposed market-price update fails and the case where proposed prices must be empty. It's important to ensure that these tests accurately reflect the expected behavior of the system under these conditions. Additionally, consider adding more detailed comments or documentation to explain the rationale behind these expected behaviors for future maintainers.- 89-122: The tests under
TestVoteExtensionsEnabledcover critical scenarios, including missing extended commit and failure of the price-update generator. These tests are crucial for ensuring the robustness of the vote extension logic. However, ensure that the error messages and failure conditions are clearly documented and that any mock expectations are verified at the end of each test to ensure test integrity.- 124-253: The tests for market price update validation (
TestMarketPriceUpdateValidation_WithVoteExtensionsEnabled) are thorough, covering various failure scenarios such as decoding failures, conflicting updates, and validation failures. These tests are essential for ensuring the correctness of market price updates. Consider adding positive test cases as well, where the validation passes, to ensure comprehensive coverage of both success and failure paths.- 256-295: The happy path test (
TestHappyPath_VoteExtensionsEnabled) effectively validates the successful decoding and validation of market price updates when vote extensions are enabled. This test is crucial for ensuring that the system behaves as expected under normal conditions. Ensure that this test is kept up-to-date as the implementation evolves to continue providing confidence in the system's correctness.- 297-320: The tests for
GetTxOffsetare well-designed, covering scenarios where vote extensions are either enabled or not. These tests ensure that the offset calculation behaves correctly under different conditions. It's important to maintain these tests and consider adding more scenarios if the logic for calculating the offset becomes more complex in the future.protocol/app/process/transactions_test.go (4)
- 119-119: The replacement of
pricesKeeperwithprocess.NewDefaultUpdateMarketPriceTxDecoder(pricesKeeper, constants.TestEncodingCfg.TxConfig.TxDecoder())in theTestDecodeProcessProposalTxs_Errorfunction correctly aligns with the PR objectives to enhance proposal handling with new decoding functionalities. This change is crucial for testing the new market price update mechanism and is implemented correctly.- 200-200: The use of
process.NewDefaultUpdateMarketPriceTxDecoder(pricesKeeper, constants.TestEncodingCfg.TxConfig.TxDecoder())in theTestDecodeProcessProposalTxs_Validfunction is consistent with the PR objectives and correctly implements the new decoding functionality for market price updates. This ensures comprehensive testing of the new proposal handling mechanism across different scenarios.- 348-348: The replacement of
pricesKeeperwithprocess.NewDefaultUpdateMarketPriceTxDecoder(pricesKeeper, constants.TestEncodingCfg.TxConfig.TxDecoder())in theTestProcessProposalTxs_Validate_ErrorandTestProcessProposalTxs_Validate_Validfunctions is correctly implemented. This change ensures that the validation logic for process proposal transactions is tested using the updated decoding functionality, aligning with the PR objectives.- 455-455: The use of
process.NewDefaultUpdateMarketPriceTxDecoder(pricesKeeper, constants.TestEncodingCfg.TxConfig.TxDecoder())in theTestProcessProposalTxs_Validate_Validfunction is consistent with the PR objectives and correctly implements the new decoding functionality for market price updates. This ensures comprehensive testing of the validation logic for process proposal transactions, aligning with the PR objectives.protocol/app/prepare/prepare_proposal_test.go (9)
- 37-49: The encoder functions (
failingTxEncoder,emptyTxEncoder,passingTxEncoderOne, etc.) are well-defined for simulating different outcomes in the encoding process. This approach is effective for testing various scenarios, including failures and successes in transaction encoding.- 60-75: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [49-370]
The test function
TestPrepareProposalHandlerthoroughly tests various scenarios, including error cases and valid cases for handling price updates, funding, bridge acknowledgments, and operations. The use of mock objects and encoder functions to simulate different outcomes is effective for isolating the test cases from external dependencies. Ensure that the test cases accurately reflect the expected behavior of thePrepareProposalHandler, especially in light of any new logic introduced in the PR.
- 446-453: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [370-450]
The test function
TestPrepareProposalHandler_OtherTxseffectively tests the handling of "other" transactions by thePrepareProposalHandler, focusing on scenarios where other transactions contain disallowed messages. The test cases are well-designed to validate the filtering logic, ensuring that it is consistent with the application's requirements. Ensure comprehensive coverage of all relevant scenarios in the filtering logic.
- 465-642: The test function
TestSlinkyPrepareProposalHandlereffectively tests the behavior of theSlinkyPrepareProposalHandlerin scenarios where vote extensions (VEs) are enabled or not. The scenarios include testing the insertion of an emptyUpdateMarketPricestransaction when VEs are not enabled and a validUpdateMarketPricesTxwhen VEs are enabled. The use of mock objects and specific context configurations to simulate these scenarios is appropriate. Ensure that the test cases accurately reflect the expected behavior in the context of vote extensions.- 686-698: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [643-695]
The test function
TestGetUpdateMarketPricesTxthoroughly tests thegetMarketPriceUpdatesfunction, covering scenarios such as nil messages, empty messages, encoding failures, and valid messages. The comprehensive range of test cases and the use of mock objects ensure that the function's behavior is accurately validated under various conditions. Ensure that the test cases reflect the expected behavior of thegetMarketPriceUpdatesfunction, particularly with any new logic introduced in the PR.
- 704-723: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [716-762]
The test function
TestGetAcknowledgeBridgesTxeffectively validates the behavior of theGetAcknowledgeBridgesTxfunction under various scenarios, including empty lists of messages, encoding failures, and valid bridge events. The comprehensive test cases and the use of mock objects ensure accurate validation of the function's behavior under different conditions. Ensure that the test cases reflect the expected behavior of theGetAcknowledgeBridgesTxfunction.
- 779-785: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [782-824]
The test function
TestGetAddPremiumVotesTxthoroughly tests theGetAddPremiumVotesTxfunction, covering scenarios such as nil messages, empty messages, encoding failures, and valid messages with votes. The comprehensive range of test cases and the use of mock objects ensure that the function's behavior is accurately validated under various conditions. Ensure that the test cases reflect the expected behavior of theGetAddPremiumVotesTxfunction, particularly with any new logic introduced in the PR.
- 841-847: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [844-889]
The test function
TestGetProposedOperationsTxeffectively validates the behavior of theGetProposedOperationsTxfunction under various scenarios, including nil messages, empty messages, encoding failures, and valid messages with operations. The comprehensive test cases and the use of mock objects ensure accurate validation of the function's behavior under different conditions. Ensure that the test cases reflect the expected behavior of theGetProposedOperationsTxfunction.
- 904-910: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [907-929]
The test function
TestEncodeMsgsIntoTxBytesthoroughly tests theEncodeMsgsIntoTxBytesutility function, covering scenarios such as setting messages failures, encoding failures, and valid encoding scenarios. The comprehensive range of test cases and the use of mock objects ensure that the function's behavior is accurately validated under various conditions. Ensure that the test cases reflect the expected behavior of theEncodeMsgsIntoTxBytesfunction.
| func (suite *SlinkyPriceUpdateGeneratorSuite) TestWithVoteExtensionsNotEnabled() { | ||
| // setup | ||
| ctx := testutils.UpdateContextWithVEHeight(testutils.CreateBaseSDKContext(suite.T()), 5) | ||
| // ctx.BlockHeight() < ctx.ConsensusParams.VoteExtensionsEnableHeight (ves are not enabled rn) | ||
| ctx = ctx.WithBlockHeight(4) | ||
|
|
||
| // expect | ||
| msg, err := suite.spug.GetValidMarketPriceUpdates(ctx, []byte{}) // 2nd argument shld be irrelevant | ||
|
|
||
| // assert | ||
| suite.NoError(err) | ||
| suite.NotNil(msg) | ||
| suite.Empty(msg.MarketPriceUpdates) // no updates | ||
| } |
There was a problem hiding this comment.
In the test TestWithVoteExtensionsNotEnabled, it's good practice to ensure that the test case is isolated and does not depend on external state or configurations that might change. This test assumes that vote extensions are not enabled based on the block height. It's recommended to explicitly document this assumption or, if possible, mock or control the enabling of vote extensions within the test setup to make the test more robust and understandable.
| func (suite *SlinkyPriceUpdateGeneratorSuite) TestVoteExtensionAggregationFails() { | ||
| // setup | ||
| ctx := testutils.UpdateContextWithVEHeight(testutils.CreateBaseSDKContext(suite.T()), 5) | ||
| ctx = ctx.WithBlockHeight(6) // ves enabled | ||
|
|
||
| // create vote-extensions | ||
| validator := []byte("validator") | ||
| // we j mock what the actual wire-transmitted bz are for this vote-extension | ||
| voteExtensionBz := []byte("vote-extension") | ||
| extCommitBz := []byte("ext-commit") // '' for ext-commit | ||
| extCommit := cmtabci.ExtendedCommitInfo{ | ||
| Votes: []cmtabci.ExtendedVoteInfo{ | ||
| { | ||
| Validator: cmtabci.Validator{ | ||
| Address: validator, | ||
| }, | ||
| VoteExtension: voteExtensionBz, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| // mock codecs | ||
| suite.extCommitCodec.On("Decode", extCommitBz).Return(extCommit, nil) | ||
|
|
||
| ve := vetypes.OracleVoteExtension{ | ||
| Prices: map[uint64][]byte{ | ||
| 1: []byte("price"), | ||
| }, | ||
| } | ||
| suite.veCodec.On("Decode", voteExtensionBz).Return(ve, nil) | ||
|
|
||
| // expect an error from the vote-extension aggregator | ||
| suite.va.On("AggregateOracleVotes", ctx, []aggregator.Vote{ | ||
| { | ||
| ConsAddress: sdk.ConsAddress(validator), | ||
| OracleVoteExtension: ve, | ||
| }, | ||
| }).Return(nil, fmt.Errorf("error in aggregation")) | ||
|
|
||
| // execute | ||
| msg, err := suite.spug.GetValidMarketPriceUpdates(ctx, extCommitBz) | ||
| suite.Nil(msg) | ||
| suite.Error(err, "error in aggregation") |
There was a problem hiding this comment.
In the test TestVoteExtensionAggregationFails, the error handling for the aggregation of oracle votes is correctly tested. However, it's important to ensure that the error message being checked ("error in aggregation") matches exactly what is expected from the AggregateOracleVotes method. This is crucial for the test to accurately validate the error handling logic. Additionally, consider using constants for repeated string literals like error messages to reduce the chance of typos and make maintenance easier.
| // test that if price / ID conversion fails we fail | ||
| func (suite *SlinkyPriceUpdateGeneratorSuite) TestCurrencyPairConversionFails() { | ||
| // setup | ||
| ctx := testutils.UpdateContextWithVEHeight(testutils.CreateBaseSDKContext(suite.T()), 5) | ||
| ctx = ctx.WithBlockHeight(6) // ves enabled | ||
|
|
||
| // create vote-extensions | ||
| validator := []byte("validator") | ||
| // we j mock what the actual wire-transmitted bz are for this vote-extension | ||
| voteExtensionBz := []byte("vote-extension") | ||
| extCommitBz := []byte("ext-commit") // '' for ext-commit | ||
| extCommit := cmtabci.ExtendedCommitInfo{ | ||
| Votes: []cmtabci.ExtendedVoteInfo{ | ||
| { | ||
| Validator: cmtabci.Validator{ | ||
| Address: validator, | ||
| }, | ||
| VoteExtension: voteExtensionBz, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| // mock codecs | ||
| suite.extCommitCodec.On("Decode", extCommitBz).Return(extCommit, nil) | ||
|
|
||
| ve := vetypes.OracleVoteExtension{ | ||
| Prices: map[uint64][]byte{ | ||
| 1: []byte("price"), | ||
| }, | ||
| } | ||
| suite.veCodec.On("Decode", voteExtensionBz).Return(ve, nil) | ||
|
|
||
| mogBtc := oracletypes.NewCurrencyPair("MOG", "BTC") | ||
| // expect an error from the vote-extension aggregator | ||
| suite.va.On("AggregateOracleVotes", ctx, []aggregator.Vote{ | ||
| { | ||
| ConsAddress: sdk.ConsAddress(validator), | ||
| OracleVoteExtension: ve, | ||
| }, | ||
| }).Return(map[oracletypes.CurrencyPair]*big.Int{ | ||
| mogBtc: big.NewInt(1), | ||
| }, nil) | ||
|
|
||
| // expect an error from the currency-pair strategy | ||
| suite.cps.On("ID", ctx, mogBtc).Return(uint64(0), fmt.Errorf("error in currency-pair conversion")) | ||
|
|
||
| // execute | ||
| msg, err := suite.spug.GetValidMarketPriceUpdates(ctx, extCommitBz) | ||
| suite.Nil(msg) | ||
| suite.Error(err, "error in currency-pair conversion") |
There was a problem hiding this comment.
The test TestCurrencyPairConversionFails effectively checks the error handling when currency pair ID conversion fails. It's a good practice to ensure that the error message being asserted ("error in currency-pair conversion") is exactly what the system under test is expected to return. This ensures that the test is accurately validating the system's behavior. As with previous comments, using constants for error messages can improve maintainability.
| func (suite *SlinkyPriceUpdateGeneratorSuite) TestValidMarketPriceUpdate() { | ||
| // setup | ||
| ctx := testutils.UpdateContextWithVEHeight(testutils.CreateBaseSDKContext(suite.T()), 5) | ||
| ctx = ctx.WithBlockHeight(6) // ves enabled | ||
|
|
||
| // create vote-extensions | ||
| validator := []byte("validator") | ||
| // we j mock what the actual wire-transmitted bz are for this vote-extension | ||
| voteExtensionBz := []byte("vote-extension") | ||
| extCommitBz := []byte("ext-commit") // '' for ext-commit | ||
| extCommit := cmtabci.ExtendedCommitInfo{ | ||
| Votes: []cmtabci.ExtendedVoteInfo{ | ||
| { | ||
| Validator: cmtabci.Validator{ | ||
| Address: validator, | ||
| }, | ||
| VoteExtension: voteExtensionBz, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| // mock codecs | ||
| suite.extCommitCodec.On("Decode", extCommitBz).Return(extCommit, nil) | ||
|
|
||
| ve := vetypes.OracleVoteExtension{ | ||
| Prices: map[uint64][]byte{ | ||
| 1: []byte("price"), | ||
| }, | ||
| } | ||
| suite.veCodec.On("Decode", voteExtensionBz).Return(ve, nil) | ||
|
|
||
| mogBtc := oracletypes.NewCurrencyPair("MOG", "BTC") | ||
| pepeEth := oracletypes.NewCurrencyPair("PEPE", "ETH") | ||
| // expect an error from the vote-extension aggregator | ||
| suite.va.On("AggregateOracleVotes", ctx, []aggregator.Vote{ | ||
| { | ||
| ConsAddress: sdk.ConsAddress(validator), | ||
| OracleVoteExtension: ve, | ||
| }, | ||
| }).Return(map[oracletypes.CurrencyPair]*big.Int{ | ||
| mogBtc: big.NewInt(1), | ||
| pepeEth: big.NewInt(2), | ||
| }, nil) | ||
|
|
||
| // expect no error from currency-pair strategies | ||
| suite.cps.On("ID", ctx, mogBtc).Return(uint64(0), nil) | ||
| suite.cps.On("ID", ctx, pepeEth).Return(uint64(1), nil) | ||
|
|
||
| // execute | ||
| msg, err := suite.spug.GetValidMarketPriceUpdates(ctx, extCommitBz) | ||
| suite.NoError(err) | ||
|
|
||
| // check the message | ||
| suite.Len(msg.MarketPriceUpdates, 2) | ||
| expectedPrices := map[uint64]uint64{ | ||
| 0: 1, | ||
| 1: 2, | ||
| } | ||
| for _, mpu := range msg.MarketPriceUpdates { | ||
| price, ok := expectedPrices[uint64(mpu.MarketId)] | ||
| suite.True(ok) | ||
|
|
||
| suite.Equal(price, mpu.Price) | ||
| } |
There was a problem hiding this comment.
The test TestValidMarketPriceUpdate successfully validates the correct generation of MsgUpdateMarketPricesTx when valid inputs are provided. This test covers the happy path scenario well. It's important to ensure that all edge cases and potential failure modes are also covered in separate tests to ensure comprehensive test coverage. Additionally, consider adding assertions for the entire structure of msg.MarketPriceUpdates to ensure that all fields are correctly populated, not just the MarketPriceUpdates length and prices.
| @@ -72,7 +73,7 @@ func ProcessProposalHandler( | |||
| error_lib.LogErrorWithOptionalContext(ctx, "UpdateSmoothedPrices failed", err) | |||
| } | |||
|
|
|||
| txs, err := DecodeProcessProposalTxs(ctx, txConfig.TxDecoder(), req, bridgeKeeper, pricesKeeper) | |||
| txs, err := DecodeProcessProposalTxs(ctx, txConfig.TxDecoder(), req, bridgeKeeper, pricesTxDecoder) | |||
There was a problem hiding this comment.
The usage of pricesTxDecoder within the ProcessProposalHandler function to decode market price transactions is a key part of the new proposal handling logic. This change ensures that the decoding process is abstracted away, allowing for different decoding strategies to be implemented and injected as needed. It's crucial to ensure that the pricesTxDecoder implementation correctly handles the decoding logic, including error handling and validation of market price updates. Comprehensive testing around this functionality is recommended to ensure its reliability and correctness.
Would you like assistance in creating additional unit tests for the pricesTxDecoder implementation?
| // DecodeUpdateMarketPricesTx returns a new `UpdateMarketPricesTx` after validating the following: | ||
| // - the underlying decoder decodes successfully | ||
| // - the UpdateMarketPricesTx follows correctly from the vote-extensions | ||
| // - vote-extensions are enabled: each price per market-id is derived from the injected extended commit | ||
| // - vote-extensions are disabled: no price updates are proposed | ||
| // - vote-extensions are inserted into the block if necessary | ||
| // | ||
| // If error occurs during any of the checks, returns error. | ||
| func (mpd *SlinkyMarketPriceDecoder) DecodeUpdateMarketPricesTx( | ||
| ctx sdk.Context, txs [][]byte) (*UpdateMarketPricesTx, error) { | ||
| expectedMsg := &pricestypes.MsgUpdateMarketPrices{} | ||
|
|
||
| // check if vote-extensions are enabled | ||
| if ve.VoteExtensionsEnabled(ctx) { | ||
| // if there isn't a vote-extension in the block when there should be, fail | ||
| if len(txs) < slinkyabci.NumInjectedTxs { | ||
| return nil, getDecodingError( | ||
| msgUpdateMarketPricesType, | ||
| fmt.Errorf("expected %v txs, got %v", slinkyabci.NumInjectedTxs, len(txs))) | ||
| } | ||
|
|
||
| // get the expected message from the injected vote-extensions | ||
| var err error | ||
| expectedMsg, err = mpd.agg.GetValidMarketPriceUpdates(ctx, txs[slinkyabci.OracleInfoIndex]) | ||
| if err != nil { | ||
| return nil, getDecodingError(msgUpdateMarketPricesType, err) | ||
| } | ||
| } | ||
|
|
||
| // use the underlying decoder to get the UpdateMarketPricesTx | ||
| updateMarketPrices, err := mpd.decoder.DecodeUpdateMarketPricesTx(ctx, txs) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| updateMarketPricesMsg, ok := updateMarketPrices.GetMsg().(*pricestypes.MsgUpdateMarketPrices) | ||
| if !ok { | ||
| return nil, getDecodingError( | ||
| msgUpdateMarketPricesType, fmt.Errorf("expected %T, got %T", expectedMsg, updateMarketPricesMsg)) | ||
| } | ||
|
|
||
| // check that the UpdateMarketPricesTx matches the expected message | ||
| if err := checkEqualityOfMarketPriceUpdate(expectedMsg, updateMarketPricesMsg); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return updateMarketPrices, nil |
There was a problem hiding this comment.
The DecodeUpdateMarketPricesTx method in the SlinkyMarketPriceDecoder struct implements complex logic for decoding market price updates, taking into account whether vote-extensions are enabled. This method demonstrates a clear separation of concerns by first checking for vote-extensions, then using the underlying decoder for the actual decoding, and finally validating the decoded message against expected values. It's crucial to ensure comprehensive testing around this method, particularly to cover scenarios with and without vote-extensions, to validate its correctness and robustness.
Would you like assistance in creating additional unit tests for the DecodeUpdateMarketPricesTx method to cover various scenarios?
| pricesTxDecoder UpdateMarketPriceTxDecoder, | ||
| ) (*ProcessProposalTxs, error) { | ||
| // Check len. | ||
| // Check len (accounting for offset from injected vote-extensions if applicable) | ||
| offset := pricesTxDecoder.GetTxOffset(ctx) | ||
| injectedTxCount := minTxsCount + offset | ||
| numTxs := len(req.Txs) | ||
| if numTxs < minTxsCount { | ||
| if numTxs < injectedTxCount { | ||
| return nil, errorsmod.Wrapf( | ||
| ErrUnexpectedNumMsgs, | ||
| "Expected the proposal to contain at least %d txs, but got %d", | ||
| minTxsCount, | ||
| injectedTxCount, | ||
| numTxs, | ||
| ) | ||
| } | ||
|
|
||
| // Price updates. | ||
| updatePricesTx, err := pricesTxDecoder.DecodeUpdateMarketPricesTx( | ||
| ctx, | ||
| req.Txs, | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
There was a problem hiding this comment.
The addition of UpdateMarketPriceTxDecoder to the DecodeProcessProposalTxs function and its subsequent usage for decoding market price updates is a key enhancement. This change aligns with the PR's objective to introduce a more flexible and decoupled approach for handling market price updates. It's important to ensure that the pricesTxDecoder is correctly implemented and integrated, particularly in terms of error handling and validation of decoded transactions. Comprehensive testing around this functionality is recommended to ensure its reliability and correctness.
Would you like assistance in creating additional unit tests for the DecodeProcessProposalTxs function to cover various decoding scenarios?
| pricesTxDecoder := process.NewDefaultUpdateMarketPriceTxDecoder(k, encodingCfg.TxConfig.TxDecoder()) | ||
| umpt, err := pricesTxDecoder.DecodeUpdateMarketPricesTx(ctx, [][]byte{tc.txBytes}) |
There was a problem hiding this comment.
The introduction of the pricesTxDecoder object in the test functions for decoding market prices is a positive change, enhancing the clarity and structure of the test logic. This modification aligns with the changes made in the application code, ensuring that the tests accurately reflect the new decoding process. It's important to ensure that these tests cover a wide range of scenarios, including error cases and different configurations of the pricesTxDecoder, to thoroughly validate the decoding logic.
Would you like assistance in expanding the test coverage for the pricesTxDecoder usage in these test functions?
| // Gather "FixedSize" group messages. | ||
| pricesTxResp, err := GetUpdateMarketPricesTx(ctx, txConfig, pricesKeeper) | ||
| pricesTxResp, err := EncodeMarketPriceUpdates(txConfig, msg) |
There was a problem hiding this comment.
Introducing the EncodeMarketPriceUpdates function to handle encoding market price updates is a good practice, as it encapsulates this specific logic into a separate, reusable component. Ensure that this function is thoroughly tested, especially for edge cases such as invalid inputs or encoding failures, to maintain the integrity of the proposal preparation process.
Would you like me to help with generating additional unit tests for EncodeMarketPriceUpdates to cover edge cases?
| @@ -71,8 +74,21 @@ func PrepareProposalHandler( | |||
| return &EmptyResponse, nil | |||
| } | |||
|
|
|||
| var extCommitBzTx []byte | |||
| if len(req.Txs) >= slinkyabci.NumInjectedTxs { | |||
| extCommitBzTx = req.Txs[slinkyabci.OracleInfoIndex] | |||
There was a problem hiding this comment.
is extended commit always at OracleInfoIndex?
There was a problem hiding this comment.
Yes, the VEs commit info is always injected as the first Tx in the block. See comment above.
| } | ||
|
|
||
| // get the update market prices tx | ||
| msg, err := priceUpdateGenerator.GetValidMarketPriceUpdates(ctx, extCommitBzTx) |
There was a problem hiding this comment.
Maybe I'm missing something but this doesn't exclude the markets whose prices changed less than a threshold, right?
There was a problem hiding this comment.
My understanding is that this is handled in the Prices Keeper.
https://github.com/dydxprotocol/v4-chain/pull/1139/files#diff-0da397be90196d47fd5225c12b6bfd6c7c4adc27b46e334f33776f510c0c1031R54
We only extend vote with "valid" market price updates. If a price update is not above the min_price threshold it would not be returned by that keeper method. Perhaps I'm mistaken though and this needs to be handled/validated outside the keeper?
| "github.com/dydxprotocol/v4-chain/protocol/lib/metrics" | ||
| pricetypes "github.com/dydxprotocol/v4-chain/protocol/x/prices/types" | ||
| slinkyabci "github.com/skip-mev/slinky/abci/types" |
There was a problem hiding this comment.
What is the best way to ensure that future changes to slinky packages do not result in a breaking change for dYdX chain?
There was a problem hiding this comment.
Same way that comet and the sdk are maintained. Breaking changes will only occur along with a major version bump. Dependency updates should be looked at and tested to make sure regression aren't introduced. Security and bugfixes will be backported to all supported versions.
| @@ -71,8 +74,21 @@ func PrepareProposalHandler( | |||
| return &EmptyResponse, nil | |||
| } | |||
|
|
|||
| var extCommitBzTx []byte | |||
| if len(req.Txs) >= slinkyabci.NumInjectedTxs { | |||
There was a problem hiding this comment.
If I understand correctly, the first tx of the PrepareProposalRequest should be the VE of the previous block right? If len(req.Txs) == 0, does that mean that there were no VE in the previous block? Can you include in a comment?
Additionally similar to the OracleInfoIndex, can we have this constant stored in the dYdX code base?
There was a problem hiding this comment.
Yeah the prepare proposal handler will inject the VEs as the first tx in the block. If there are no txs, it means there were no VEs injected. This is basically only going to happen if the protocol hasn't enabled VEs yet.
There was a problem hiding this comment.
can we include this in a comment?
There was a problem hiding this comment.
Could we also change this constant name? NumInjectedTxs doesn't really mean anything. Ideally this would be a function named TxsContainsVEs or the constant would be something like MinTxRequiredForVEEnabled
There was a problem hiding this comment.
Ended up at OracleVEInjectedTxs. Let me know if you'd prefer something else.
| pricestypes "github.com/dydxprotocol/v4-chain/protocol/x/prices/types" | ||
| ) | ||
|
|
||
| // MarketPriceDecoder is an interface for decoding market price transactions, This interface is responsible |
There was a problem hiding this comment.
Shouldnt this be UpdateMarketPriceTxDecoder since that's the name of the type? I'm confused where MarketPriceDecoder is defined
There was a problem hiding this comment.
That is correct, the name of the type in the docs should be UpdateMarketPricesDecoder
| // If error occurs during any of the checks, returns error. | ||
| func (mpd *DefaultUpdateMarketPriceTxDecoder) DecodeUpdateMarketPricesTx( | ||
| ctx sdk.Context, txs [][]byte) (*UpdateMarketPricesTx, error) { | ||
| return DecodeUpdateMarketPricesTx(ctx, mpd.pk, mpd.txDecoder, txs[len(txs)+updateMarketPricesTxLenOffset]) |
There was a problem hiding this comment.
Where is this inner DecodeUpdateMarketPricesTx defined?
There was a problem hiding this comment.
This is defined in app/process/market_prices.go this is used in the current dydx protocol to unmarshal price-update txs
| } | ||
|
|
||
| // use the underlying decoder to get the UpdateMarketPricesTx | ||
| updateMarketPrices, err := mpd.decoder.DecodeUpdateMarketPricesTx(ctx, txs) |
There was a problem hiding this comment.
what is the purpose of this inner decoder? Is it so SlinkyMarketPriceDecoder can call DefaultMarketPriceDecoder?
There was a problem hiding this comment.
@nivasan1 can probably comment more on this as he implemented it.
The idea is twofold:
- The existing behavior is preserved via just using
DefaultMarketPricesDecoderso all of these changes are backwards compatible in the case where slinky is not yet enabled. - The new implementation wraps the default encoder because we still want all the same behavior. But additionally we want to do checks on the equality of VE data with the changes being made in the UpdateMarketPrices tx.
So we first get the injected VE data, then get the UpdateMarketPricesTx via the default behavior, then verify that the two match.
There was a problem hiding this comment.
That's correct. The SlinkyUpdateMarketPriceDecoder's responsibility is to decode the price-update + determine whether or not the proposed prices followed exactly from the injected vote-extensions
There was a problem hiding this comment.
Why do we need to define this whole decoder struct? Maybe I'm missing something but it doesn't seem to be doing much
There was a problem hiding this comment.
The decoder struct is responsible for encapsulating the logical differences between dydx's ProcessProposal pre / post vote-extensions. Specifically
- The
DefaultMarketPriceDecoderdecodes price-updates from proposals as they are done in the pre-vote extensions protoocl - The
SlinkyMarketPriceDecoderdecodes price-updates + verfies that they follow exactly from the injected vote-extensions
There was a problem hiding this comment.
If we stop supporting our own Price Daemon and remove VEs, would you then delete this decoder struct and remove DefaultMarketPriceDecoder?
There was a problem hiding this comment.
If I understand correctly I think yes.
The interface is just in place to support having the existing implementation and the new one. If we clean up the old code we don't really need the interface/struct anymore and it can just be the function defined in slinky market price decoder.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (5)
- protocol/app/prepare/prepare_proposal.go (5 hunks)
- protocol/app/prepare/prices/slinky_price_update_generator.go (1 hunks)
- protocol/app/prepare/prices/slinky_price_update_generator_test.go (1 hunks)
- protocol/app/process/slinky_market_price_decoder_test.go (1 hunks)
- protocol/app/process/transactions.go (4 hunks)
Files skipped from review as they are similar to previous changes (4)
- protocol/app/prepare/prices/slinky_price_update_generator.go
- protocol/app/prepare/prices/slinky_price_update_generator_test.go
- protocol/app/process/slinky_market_price_decoder_test.go
- protocol/app/process/transactions.go
Additional comments: 3
protocol/app/prepare/prepare_proposal.go (3)
- 13-13: The addition of new imports for handling price updates and market price decoders aligns with the PR's objectives. Ensure that these packages are properly versioned to avoid potential breaking changes in the future.
- 59-59: Replacing
PreparePricesKeeperwithpriceUpdateGeneratorin thePrepareProposalHandlerfunction is a significant change. It's crucial to ensure that all instances wherePreparePricesKeeperwas used are now correctly utilizingpriceUpdateGenerator. Additionally, verify thatpriceUpdateGenerator's methods are fully implemented and tested, especially in edge cases.- 74-96: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [77-216]
The changes from lines 77 to 216 introduce logic to handle market price updates in the
PrepareProposalHandlerfunction. This includes handling injected VEs from the previous block, generating and encoding market price updates, and integrating these updates into the proposal preparation process. Ensure that this logic is thoroughly tested, particularly the handling of edge cases such as empty transactions or errors during the encoding process. Additionally, consider the performance implications of these changes, especially in scenarios with a large number of market price updates.
| } | ||
| } | ||
|
|
||
| // GetUpdateMarketPricesTx returns a tx containing `MsgUpdateMarketPrices`. | ||
| func GetUpdateMarketPricesTx( | ||
| ctx sdk.Context, | ||
| // EncodeMarketPriceUpdates returns a tx containing `MsgUpdateMarketPrices`. | ||
| func EncodeMarketPriceUpdates( | ||
| txConfig client.TxConfig, | ||
| pricesKeeper PreparePricesKeeper, | ||
| msg *pricetypes.MsgUpdateMarketPrices, | ||
| ) (PricesTxResponse, error) { | ||
| // Get prices to update. | ||
| msgUpdateMarketPrices := pricesKeeper.GetValidMarketPriceUpdates(ctx) | ||
| if msgUpdateMarketPrices == nil { | ||
| return PricesTxResponse{}, fmt.Errorf("MsgUpdateMarketPrices cannot be nil") | ||
| } | ||
|
|
||
| tx, err := EncodeMsgsIntoTxBytes(txConfig, msgUpdateMarketPrices) | ||
| tx, err := EncodeMsgsIntoTxBytes(txConfig, msg) | ||
| if err != nil { | ||
| return PricesTxResponse{}, err | ||
| } |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [201-216]
Introducing the EncodeMarketPriceUpdates function to handle encoding market price updates is a good practice, as it encapsulates this specific logic into a separate, reusable component. However, the error message for an empty transaction could be more informative. Consider specifying the reason why the transaction is considered invalid, such as "transaction bytes are empty" instead of just showing the empty transaction in the error message.
- return PricesTxResponse{}, fmt.Errorf("Invalid tx: %v", tx)
+ return PricesTxResponse{}, fmt.Errorf("Invalid tx: transaction bytes are empty")
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- protocol/app/constants/constants.go (1 hunks)
- protocol/app/prepare/prepare_proposal.go (6 hunks)
- protocol/app/process/slinky_market_price_decoder.go (1 hunks)
- protocol/app/process/slinky_market_price_decoder_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- protocol/app/prepare/prepare_proposal.go
- protocol/app/process/slinky_market_price_decoder.go
- protocol/app/process/slinky_market_price_decoder_test.go
Additional comments: 1
protocol/app/constants/constants.go (1)
- 11-16: The introduction of
OracleInfoIndexandOracleVEInjectedTxsconstants is clear and aligns well with the purpose described in the PR objectives. However, to enhance code readability and maintainability, consider adding more detailed comments explaining the rationale behind these specific values and how they are intended to be used within the Slinky integration context.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/app/process/market_prices.go (1 hunks)
Additional comments: 1
protocol/app/process/market_prices.go (1)
- 66-66: The change from
truetofalsein the call toPerformStatefulPriceUpdateValidationalters the behavior of the price update validation process. It's crucial to understand the implications of this modification:
- Ensure that this change aligns with the intended behavior and security requirements of the system.
- Verify that this adjustment does not inadvertently weaken any validation checks that are critical for maintaining the integrity of price updates.
- Consider adding comments in the code to explain the rationale behind using
falseinstead oftrue, especially if this parameter controls significant validation logic.Given the potential impact on the system's behavior, it's advisable to review the documentation or comments associated with the
PerformStatefulPriceUpdateValidationmethod to understand the significance of this parameter change. Additionally, testing should be conducted to ensure that this modification does not introduce any unintended consequences.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/app/process/market_prices_test.go (5 hunks)
- protocol/app/process/slinky_market_price_decoder.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/app/process/market_prices_test.go
- protocol/app/process/slinky_market_price_decoder.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to path filters (3)
protocol/go.modis excluded by:!**/*.modprotocol/go.sumis excluded by:!**/*.sumprotocol/scripts/genesis/sample_pregenesis.jsonis excluded by:!**/*.json
Files selected for processing (3)
- protocol/Dockerfile (3 hunks)
- protocol/app/app.go (5 hunks)
- protocol/mocks/Makefile (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/mocks/Makefile
Additional comments: 7
protocol/Dockerfile (3)
- 3-3: The Dockerfile has been updated to use Golang version
1.22. This change is necessary for compatibility with the latest features or security patches. Ensure that all dependencies of the project are compatible with this Golang version to avoid any build or runtime issues.- 12-12: The base image for the builder stage has been updated to Golang
1.22. This is consistent with the earlier change to the Golang version. It's important to ensure that the entire build process is tested to confirm that there are no issues arising from this version change.- 59-61: The Dockerfile now copies the
oraclebinary and its configuration files (oracle.jsonandmarket.json) from theslinkybuild directory to the final image. This is a crucial step for integrating Slinky-specific functionality. Ensure that the configuration files (oracle.jsonandmarket.json) are properly reviewed and secure, as they could contain sensitive information or settings critical to the application's operation.protocol/app/app.go (4)
- 1340-1353: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1334-1344]
The introduction of
priceUpdateGeneratorin thePrepareProposalhandler is a significant change that aligns with the PR's objectives to integrate Slinky-specific proposal handlers. Ensure that the implementation ofNewDefaultPriceUpdateGeneratorcorrectly generates price updates according to the Slinky protocol's requirements. It's also important to verify that this change does not disrupt existing operations and maintains backward compatibility as intended.
- 1359-1365: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1350-1374]
Similarly, the use of
priceUpdateDecoderin theProcessProposalhandler is crucial for decoding market price updates according to the Slinky protocol. Ensure thatNewDefaultUpdateMarketPriceTxDecoderis implemented correctly and that it accurately decodes price updates. This change should enhance the application's functionality without affecting existing behavior, in line with the PR's objectives. It's essential to verify that these changes are thoroughly tested and validated to ensure robust error handling and validation.
- 1340-1353: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1334-1374]
While reviewing the changes related to
priceUpdateGeneratorandpriceUpdateDecoder, consider the performance implications of these implementations. Ensure that they are optimized for efficiency and do not introduce unnecessary computational overhead, especially in the context of handling large volumes of price updates. It may be beneficial to benchmark these changes to assess their impact on the application's overall performance.
- 1340-1353: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1334-1374]
The modularization of price update generation and decoding through
priceUpdateGeneratorandpriceUpdateDecoderis a positive step towards maintaining clean and maintainable code. Ensure that these components are well-documented and that their interfaces are clearly defined. Additionally, robust error handling and validation are crucial, especially given the critical nature of price updates in the application. Consider adding more comprehensive tests to cover edge cases and potential error conditions.
protocol/Dockerfile
Outdated
| RUN git clone https://github.com/skip-mev/slinky.git | ||
| WORKDIR /slinky | ||
| RUN make build |
There was a problem hiding this comment.
The Dockerfile now includes steps to clone the slinky repository and navigate to its directory. While this enables the building of the oracle binary, consider pinning the repository to a specific commit or tag to ensure reproducible builds. Cloning the latest version could introduce unexpected changes or incompatibilities.
- RUN git clone https://github.com/skip-mev/slinky.git
+ RUN git clone --branch <tag_name> --depth 1 https://github.com/skip-mev/slinky.gitCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| RUN git clone https://github.com/skip-mev/slinky.git | |
| WORKDIR /slinky | |
| RUN make build | |
| RUN git clone --branch <tag_name> --depth 1 https://github.com/skip-mev/slinky.git | |
| WORKDIR /slinky | |
| RUN make build |
commit e806d67 Author: Eric <eric.warehime@gmail.com> Date: Mon Mar 11 12:21:02 2024 -0700 Run update-sample-pregenesis commit ea01d17 Merge: 97b29d4 340801a Author: Eric <eric.warehime@gmail.com> Date: Mon Mar 11 11:53:03 2024 -0700 Merge remote-tracking branch 'upstream' into eric/proposal-logic commit 97b29d4 Author: Nikhil <nikhil@skip.money> Date: Fri Mar 8 14:17:49 2024 -0500 update Dockerfile commit 3bbf627 Author: Nikhil Vasan <nikhil@skip.money> Date: Fri Mar 8 11:02:19 2024 -0800 lint commit 37720da Author: Nikhil Vasan <nikhil@skip.money> Date: Fri Mar 8 10:50:24 2024 -0800 remove non-deterministic validation logic commit 0af5a49 Author: Eric <eric.warehime@gmail.com> Date: Thu Mar 7 16:48:50 2024 -0800 Update constants w/ slinky values commit 9cf8a67 Author: Eric <eric.warehime@gmail.com> Date: Wed Mar 6 20:41:22 2024 -0800 Update docs commit 7feaddb Author: Eric <eric.warehime@gmail.com> Date: Sun Mar 3 21:28:32 2024 -0800 Fix import order commit 382d45c Author: Eric <eric.warehime@gmail.com> Date: Sun Mar 3 21:24:41 2024 -0800 Prepare and Process logic for Slinky
Eric-Warehime
force-pushed
the
eric/proposal-logic
branch
from
e806d67 to
5ad38a3
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (3)
protocol/go.modis excluded by:!**/*.modprotocol/go.sumis excluded by:!**/*.sumprotocol/scripts/genesis/sample_pregenesis.jsonis excluded by:!**/*.json
Files selected for processing (28)
- protocol/Dockerfile (3 hunks)
- protocol/app/app.go (5 hunks)
- protocol/app/constants/constants.go (1 hunks)
- protocol/app/prepare/expected_keepers.go (2 hunks)
- protocol/app/prepare/prepare_proposal.go (6 hunks)
- protocol/app/prepare/prepare_proposal_test.go (17 hunks)
- protocol/app/prepare/prices/default_price_update_generator.go (1 hunks)
- protocol/app/prepare/prices/errors.go (1 hunks)
- protocol/app/prepare/prices/price_update_generator.go (1 hunks)
- protocol/app/prepare/prices/slinky_price_update_generator.go (1 hunks)
- protocol/app/prepare/prices/slinky_price_update_generator_test.go (1 hunks)
- protocol/app/process/default_market_price_decoder.go (1 hunks)
- protocol/app/process/errors.go (1 hunks)
- protocol/app/process/full_node_process_proposal.go (2 hunks)
- protocol/app/process/full_node_process_proposal_test.go (1 hunks)
- protocol/app/process/market_price_decoder.go (1 hunks)
- protocol/app/process/market_prices.go (1 hunks)
- protocol/app/process/market_prices_test.go (5 hunks)
- protocol/app/process/process_proposal.go (2 hunks)
- protocol/app/process/process_proposal_test.go (1 hunks)
- protocol/app/process/slinky_market_price_decoder.go (1 hunks)
- protocol/app/process/slinky_market_price_decoder_test.go (1 hunks)
- protocol/app/process/transactions.go (4 hunks)
- protocol/app/process/transactions_test.go (4 hunks)
- protocol/mocks/Makefile (1 hunks)
- protocol/mocks/PriceUpdateGenerator.go (1 hunks)
- protocol/mocks/PricesKeeper.go (15 hunks)
- protocol/mocks/UpdateMarketPriceTxDecoder.go (1 hunks)
Files skipped from review as they are similar to previous changes (26)
- protocol/Dockerfile
- protocol/app/constants/constants.go
- protocol/app/prepare/expected_keepers.go
- protocol/app/prepare/prepare_proposal.go
- protocol/app/prepare/prices/default_price_update_generator.go
- protocol/app/prepare/prices/errors.go
- protocol/app/prepare/prices/price_update_generator.go
- protocol/app/prepare/prices/slinky_price_update_generator.go
- protocol/app/prepare/prices/slinky_price_update_generator_test.go
- protocol/app/process/default_market_price_decoder.go
- protocol/app/process/errors.go
- protocol/app/process/full_node_process_proposal.go
- protocol/app/process/full_node_process_proposal_test.go
- protocol/app/process/market_price_decoder.go
- protocol/app/process/market_prices.go
- protocol/app/process/market_prices_test.go
- protocol/app/process/process_proposal.go
- protocol/app/process/process_proposal_test.go
- protocol/app/process/slinky_market_price_decoder.go
- protocol/app/process/slinky_market_price_decoder_test.go
- protocol/app/process/transactions.go
- protocol/app/process/transactions_test.go
- protocol/mocks/Makefile
- protocol/mocks/PriceUpdateGenerator.go
- protocol/mocks/PricesKeeper.go
- protocol/mocks/UpdateMarketPriceTxDecoder.go
Additional comments: 11
protocol/app/prepare/prepare_proposal_test.go (8)
- 19-52: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [8-28]
The addition of new imports from
github.com/skip-mev/slinky/abci/strategies/aggregator,github.com/skip-mev/slinky/abci/strategies/codec, and related packages indicates the integration of Slinky-specific logic into the proposal preparation tests. Ensure that these dependencies are properly versioned and maintained to avoid potential issues with breaking changes or security vulnerabilities.
- 35-49: The introduction of various
TxEncoderfunctions (failingTxEncoder,emptyTxEncoder,passingTxEncoderOne, etc.) is a good practice for testing different scenarios of transaction encoding. However, ensure that these encoders are thoroughly tested to simulate realistic encoding outcomes accurately.- 62-72: The test cases now include
pricesEncoder,fundingEncoder,clobEncoder, andbridgeEncoderas part of their setup. This change allows for more granular control over the encoding process in different transaction types, which is crucial for testing the proposal preparation logic comprehensively. It's important to verify that these encoders are correctly implemented to reflect the actual behavior of the encoding process in production.- 366-373: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [339-370]
The creation of
mockTxConfigwith an array ofsdk.TxEncoderand the subsequent setup of various mock keepers (mockPricesKeeper,mockPerpKeeper,mockBridgeKeeper,mockClobKeeper) are critical for setting up the test environment. This setup ensures that the tests can simulate the proposal preparation process accurately. It's essential to ensure that these mocks behave as expected and that the tests cover a wide range of scenarios to catch potential issues early.
- 465-642: The tests for
TestSlinkyPrepareProposalHandlerfocus on scenarios where vote extensions (VEs) are either enabled or disabled. These tests are crucial for ensuring that the proposal preparation logic correctly handles Slinky-specific logic under different conditions. Given the previous comment regarding the use of "ves" in the codebase, it's good to see clarity in the test descriptions regarding vote extensions. However, ensure that the tests cover all relevant scenarios, including edge cases that might arise from the interaction between vote extensions and other parts of the proposal preparation logic.- 686-698: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [643-716]
The function
TestGetUpdateMarketPricesTxand its associated tests demonstrate thorough testing of the market price updates transaction generation process. It's important to ensure that these tests cover all possible outcomes of thegetMarketPriceUpdatesfunction, including error handling and successful transaction encoding. This will help ensure the robustness of the market price updates logic in the proposal preparation process.
- 704-723: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [717-889]
Similar to the previous comment, the tests for
TestGetAcknowledgeBridgesTx,TestGetAddPremiumVotesTx, andTestGetProposedOperationsTxare essential for verifying the correctness of transaction generation for different types of operations within the proposal preparation logic. Ensure that these tests are comprehensive and cover a wide range of scenarios, including error cases and successful transaction encoding, to maintain the integrity of the proposal preparation process.
- 904-910: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [890-943]
The
TestEncodeMsgsIntoTxBytesfunction and its tests are crucial for ensuring that messages can be correctly encoded into transaction bytes. This functionality is fundamental to the proposal preparation process, as it directly affects the ability to generate valid transactions. Ensure that the tests cover various scenarios, including errors during message setting and encoding failures, to guarantee the reliability of this functionality.protocol/app/app.go (3)
- 1340-1353: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1334-1344]
The introduction of
priceUpdateGeneratorin thePrepareProposalhandler is a significant change. It's crucial to ensure that thePriceUpdateGeneratoris properly implemented and tested, especially since it directly affects the proposal handling logic. Additionally, consider whether thepriceUpdateGeneratorneeds to be configurable to support different strategies or protocols in the future.
- 1359-1365: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1350-1374]
Similarly, the use of
priceUpdateDecoderin theProcessProposalhandler is a critical addition. It's important to validate the correctness and efficiency of theUpdateMarketPriceTxDecoderimplementation. Given that this decoder plays a key role in interpreting market price updates, thorough testing and validation are essential to prevent incorrect price data from affecting the system.
- 1340-1353: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-1374]
The
Newfunction is quite large and contains a significant amount of initialization logic. Consider refactoring some of this logic into separate, well-named functions or methods. This could improve readability and maintainability by making the main application setup more concise and easier to follow. For example, initialization of keepers, modules, and middleware could each be moved to their own functions.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/app/app.go (5 hunks)
- protocol/mocks/Makefile (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/mocks/Makefile
Additional comments: 1
protocol/app/app.go (1)
- 1366-1379: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1360-1400]
The implementation of
PrepareProposalandProcessProposalhandlers introduces new functionality related to price updates. It's crucial to ensure that these handlers are thoroughly tested, especially since they interact with critical components like thePricesKeeper,BridgeKeeper,ClobKeeper, andPerpetualsKeeper. Consider adding comprehensive unit tests covering various scenarios to ensure the correctness and robustness of these handlers.Would you like assistance in generating unit tests for these handlers?
| "github.com/dydxprotocol/v4-chain/protocol/app/flags" | ||
| "github.com/dydxprotocol/v4-chain/protocol/app/middleware" | ||
| "github.com/dydxprotocol/v4-chain/protocol/app/prepare" | ||
| "github.com/dydxprotocol/v4-chain/protocol/app/prepare/prices" | ||
| "github.com/dydxprotocol/v4-chain/protocol/app/process" | ||
|
|
||
| "github.com/dydxprotocol/v4-chain/protocol/lib" |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [726-726]
The gRPC server is initialized without specifying credentials, which could lead to insecure communication. It's recommended to secure the gRPC server by adding SSL/TLS credentials. This can be achieved by creating credentials using credentials.NewServerTLSFromFile("cert.pem", "cert.key") and passing them to the grpc.NewServer function using the grpc.Creds() option. This change will ensure that the communication between the client and server is encrypted and secure.
Changelist
This PR introduces interfaces and implementations of those interfaces in the Prepare and Process Proposal handlers.
We introduce Slinky-specific proposal handlers via price update generator and market price decoders. Existing behavior of the application is preserved in order to allow the changes to be introduced to the repo and turned on in a later PR which will include the end to end flow and feature flagging.
Test Plan
New changes include unit tests and are fully backwards compatible by using the default interfaces as existing impl.
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.