[TRA-611] Get MarketPrice exponent from marketmap #2324
Conversation
WalkthroughThe pull request introduces significant modifications primarily focused on the handling of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range and nitpick comments (7)
proto/dydxprotocol/prices/market_price.proto (1)
Line range hint
1-22: Summary of changes and implicationsThis PR updates the comment for the
exponentfield in theMarketPricemessage, indicating that as of v7.1.x, the exponent value is now determined from the market map instead of needing to match theMarketParamfield. This change aligns with the PR objective of "Get exponent from marketmap".Implications:
- This change doesn't alter the protobuf structure itself, so it shouldn't cause any breaking changes in terms of serialization or deserialization.
- However, it does indicate a change in how the
exponentvalue is sourced, which could affect other parts of the system that interact withMarketPrice.- Developers and users of this protobuf definition should be aware of this change in behavior when working with the
exponentfield.To ensure system-wide consistency:
- Update any documentation that references how the
MarketPrice.exponentis determined.- Review and update any code that previously relied on
MarketPrice.exponentmatchingMarketParam.- Consider adding a migration guide or release note to inform users of this change in behavior.
protocol/x/prices/types/types.go (1)
34-34: LGTM! Consider adding documentation for the new method.The addition of the
GetExponentmethod to thePricesKeeperinterface looks good. It follows Go conventions and fits well with the existing methods in the interface.To improve code maintainability, consider adding a comment above the method to describe its purpose, parameters, and return values. For example:
// GetExponent retrieves the exponent associated with a given ticker. // Parameters: // - ctx: The SDK context // - ticker: The ticker string for which to retrieve the exponent // Returns: // - exponent: The int32 exponent value // - err: An error if the operation fails GetExponent(ctx sdk.Context, ticker string) (exponent int32, err error)protocol/x/prices/keeper/market.go (3)
60-74: Improved market exponent validation and readability.The changes look good. Using a named variable
marketMapDetailsimproves readability. The new validation check ensures consistency between the market price exponent and the market map decimals.Consider wrapping the error returned from
k.MarketMapKeeper.GetMarketwith additional context:marketMapDetails, err := k.MarketMapKeeper.GetMarket(ctx, currencyPairStr) if err != nil { - return types.MarketParam{}, errorsmod.Wrapf( + return types.MarketParam{}, errorsmod.Wrapf( types.ErrTickerNotFoundInMarketMap, - currencyPairStr, + "failed to get market details: %v", err, ) }This change would provide more context about the nature of the error, which could be helpful for debugging.
98-98: Using market price exponent as the source of truth.Good change. Using
marketPrice.Exponentaligns with the comment about it being the source of truth.Consider updating the inline comment for clarity:
- marketPrice.Exponent, // The exponent of the market price is the source of truth, the exponent of the param is deprecated as of v7.1.x + marketPrice.Exponent, // Use market price exponent (source of truth) instead of deprecated param exponent (as of v7.1.x)This change makes the comment more concise and easier to understand at a glance.
122-138: New GetExponent function looks good.The new
GetExponentfunction is a well-implemented, centralized way to retrieve the exponent for a market. It properly handles potential errors and aligns with using the market map as the source of truth for exponents.Consider improving error handling by wrapping the error returned from
slinky.MarketPairToCurrencyPair:currencyPair, err := slinky.MarketPairToCurrencyPair(ticker) if err != nil { - k.Logger(ctx).Error("Could not convert market pair to currency pair", "error", err) - return 0, err + return 0, errorsmod.Wrapf(err, "failed to convert market pair '%s' to currency pair", ticker) }This change provides more context in the error message and eliminates the need for separate logging, as the error will now contain all necessary information.
protocol/mocks/PricesKeeper.go (1)
153-179: LGTM! Consider a minor improvement for consistency.The implementation of the
GetExponentmethod is correct and follows the established pattern for mock methods in this file. It properly handles different return value scenarios and includes appropriate error checking.For consistency with other methods in this file, consider adding a comment above the method signature to describe its purpose, similar to other methods like
AddCurrencyPairIDToStore. For example:// GetExponent provides a mock function with given fields: ctx, ticker func (_m *PricesKeeper) GetExponent(ctx types.Context, ticker string) (int32, error) { // ... (rest of the implementation) }protocol/app/ante/market_update_test.go (1)
Line range hint
474-840: Consider refactoring test cases for improved maintainability.The test cases in
TestValidateMarketUpdateDecorator_AnteHandleare comprehensive and cover various scenarios. However, there's some repetition in the test setup and assertions. Consider the following improvements:
- Create helper functions for common setup tasks, such as creating markets and perpetuals.
- Use table-driven tests with clearly defined input and expected output structures to reduce code duplication.
- Group related test cases together (e.g., all cases related to adding providers, all cases related to disabling markets) for better organization.
These changes could make the tests more maintainable and easier to extend in the future.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
protocol/x/prices/types/market_param.pb.gois excluded by!**/*.pb.goprotocol/x/prices/types/market_price.pb.gois excluded by!**/*.pb.go
Files selected for processing (17)
- proto/dydxprotocol/prices/market_param.proto (1 hunks)
- proto/dydxprotocol/prices/market_price.proto (1 hunks)
- protocol/app/ante/market_update_test.go (6 hunks)
- protocol/mocks/PricesKeeper.go (1 hunks)
- protocol/testing/e2e/gov/prices_test.go (0 hunks)
- protocol/testutil/app/order.go (1 hunks)
- protocol/testutil/constants/genesis.go (6 hunks)
- protocol/x/prices/keeper/market.go (3 hunks)
- protocol/x/prices/keeper/market_param.go (0 hunks)
- protocol/x/prices/keeper/market_test.go (1 hunks)
- protocol/x/prices/keeper/msg_server_create_oracle_market.go (1 hunks)
- protocol/x/prices/keeper/msg_server_create_oracle_market_test.go (2 hunks)
- protocol/x/prices/keeper/msg_server_update_market_param_test.go (0 hunks)
- protocol/x/prices/module_test.go (0 hunks)
- protocol/x/prices/types/errors.go (1 hunks)
- protocol/x/prices/types/market_price.go (0 hunks)
- protocol/x/prices/types/types.go (1 hunks)
Files not reviewed due to no reviewable changes (5)
- protocol/testing/e2e/gov/prices_test.go
- protocol/x/prices/keeper/market_param.go
- protocol/x/prices/keeper/msg_server_update_market_param_test.go
- protocol/x/prices/module_test.go
- protocol/x/prices/types/market_price.go
Additional comments not posted (17)
proto/dydxprotocol/prices/market_price.proto (1)
13-15: Comment update clarifies exponent sourcing.The added comment provides valuable information about a change in how the
exponentfield is determined. This aligns with the PR objective of "Get exponent from marketmap".However, to ensure complete clarity:
- Consider specifying what "marketmap" refers to, as it's not immediately clear from the context.
- It might be helpful to briefly mention why this change was made or its benefits.
To ensure this change is consistently reflected across the codebase, let's verify references to the
exponentfield:Verification successful
Verification Complete: No Additional References to
MarketPrice.exponentFoundThe verification confirms that there are no other references to
MarketPrice.exponentin.protofiles or related comments. The comment update inmarket_price.protois safe and aligns with the PR objective of sourcing the exponent from the market map.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for references to MarketPrice.exponent and related comments # Search for references to MarketPrice.exponent echo "Searching for references to MarketPrice.exponent:" rg --type go --type proto "MarketPrice.exponent" -C 2 # Search for comments mentioning exponent determination echo "\nSearching for comments about exponent determination:" rg --type go --type proto "exponent.*determin" -C 2Length of output: 395
Script:
#!/bin/bash # Description: Check for references to MarketPrice.exponent and related comments in .proto files # Search for references to MarketPrice.exponent in .proto files echo "Searching for references to MarketPrice.exponent in .proto files:" rg "MarketPrice.exponent" -g "*.proto" -C 2 # Search for comments mentioning exponent determination in .proto files echo "\nSearching for comments about exponent determination in .proto files:" rg "exponent.*determin" -g "*.proto" -C 2Length of output: 379
proto/dydxprotocol/prices/market_param.proto (1)
Line range hint
1-38: Ensure backward compatibility and thorough testingWhile the deprecation of the
exponentfield has been correctly implemented, it's crucial to consider the broader implications of this change:
Backward Compatibility: Ensure that existing clients that may still be reading the
exponentfield continue to function correctly. Consider implementing a fallback mechanism if possible.Documentation Updates: Update any external documentation, API references, or client libraries that may be affected by this change.
Testing: Implement comprehensive tests to verify that:
- The system correctly retrieves the exponent from the market map.
- Existing functionality that previously relied on this field continues to work as expected.
- New code paths for retrieving the exponent are thoroughly tested.
Migration Guide: If not already done, create a migration guide for users of this protobuf definition, explaining how to adapt their code to the new approach of retrieving the exponent from the market map.
To ensure thorough testing, consider running the following checks:
#!/bin/bash # Search for test files related to market parameters fd -e go test.*market.*param # Search for usage of MarketParam in the codebase rg --type go "MarketParam" # Look for any TODOs or FIXMEs related to this change rg --type go "TODO|FIXME" -C 3 | rg -i "exponent|marketmap"These checks will help identify areas that might need updates or additional testing due to this change.
protocol/x/prices/keeper/msg_server_create_oracle_market.go (1)
47-50: LGTM! Consider adding a comment and verifying consistency.The changes align well with the PR objective of getting the exponent from the market map. Good job on adding proper error handling for the
GetExponentcall.Consider adding a comment explaining why the exponent is fetched from the Keeper instead of using the one from the message. This will help future developers understand the reasoning behind this change.
Please verify that this change doesn't introduce inconsistencies with other parts of the system that might expect the exponent from the message. It would be helpful to check if there are any other places in the codebase where
msg.Params.Exponentis used directly.This script will help identify any potential inconsistencies in how the exponent is used across the codebase.
Also applies to: 57-57
Verification successful
Verified no direct usages of
msg.Params.Exponentfound.The changes in
msg_server_create_oracle_market.gocorrectly retrieve theexponentwithout introducing inconsistencies. No further issues related tomsg.Params.Exponentwere detected.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of msg.Params.Exponent in the codebase # Test: Look for direct uses of msg.Params.Exponent echo "Searching for direct uses of msg.Params.Exponent:" rg --type go "msg\.Params\.Exponent" # Test: Look for uses of Exponent in MsgCreateOracleMarket echo "Searching for uses of Exponent in MsgCreateOracleMarket:" ast-grep --lang go --pattern 'type MsgCreateOracleMarket struct { $$$ Params $_ { $$$ Exponent $_ $$$ } $$$ }'Length of output: 925
Script:
#!/bin/bash # Description: Search for usages of msg.Params.Exponent in the codebase # Test: Look for direct uses of msg.Params.Exponent echo "Searching for direct uses of msg.Params.Exponent:" rg --type go "msg\.Params\.Exponent" # Test: Look for Exponent field within MsgCreateOracleMarket Params echo "Searching for Exponent within MsgCreateOracleMarket Params:" ast-grep --lang go --pattern 'type MsgCreateOracleMarket struct { $$$ Params struct { Exponent $_ } $$$ }'Length of output: 385
Script:
#!/bin/bash # Description: Comprehensive search for Exponent usage related to msg.Params.Exponent # Test 1: Search for the Exponent field within zeroMarketPrice struct echo "Searching for the Exponent field within zeroMarketPrice struct:" ast-grep --lang go --pattern 'type zeroMarketPrice struct { $$$ Exponent $_ $$$ }' # Test 2: Search for assignments to Exponent in CreateOracleMarket function echo "Searching for assignments to Exponent in CreateOracleMarket function:" rg --type go 'CreateOracleMarket.*\.Exponent\s*=' # Test 3: Search for any usages of Exponent across the codebase echo "Searching for any usages of Exponent across the codebase:" rg --type go '\.Exponent' # Test 4: Search for any aliases or different naming conventions for Exponent echo "Searching for aliases or different naming conventions for Exponent:" rg --type go '(Exp|exp)onent'Length of output: 179132
protocol/x/prices/types/types.go (1)
34-34: Changes align well with PR objectives.The addition of the
GetExponentmethod to thePricesKeeperinterface aligns well with the PR objective of "Get exponent from marketmap" as mentioned in the PR title. This new method provides a clear way to retrieve an exponent associated with a given ticker, which appears to fulfill the intended functionality.The implementation is straightforward and consistent with Go conventions. It integrates well with the existing interface structure, although there are some considerations regarding consistency with other methods that use market IDs instead of tickers.
Overall, this change seems to successfully implement the desired functionality while maintaining the integrity of the existing codebase structure.
protocol/x/prices/types/errors.go (2)
36-40: LGTM: New error variable is well-defined and properly integrated.The new
ErrInvalidMarketPriceExponenterror is correctly implemented:
- It follows the existing pattern for market-related errors (200-299 range).
- The error message is clear and specific, which will aid in debugging and error handling.
- Its placement after other market-related errors is logical and maintains code organization.
36-40: Verify the implementation of the market price exponent check.The new error suggests that there's now a check for the market price exponent against the Decimals value in the market map. This aligns with the PR objective of "Get exponent from marketmap". However, the implementation of this check is not visible in this file.
To ensure this error is properly utilized, let's verify its implementation:
Please ensure that the check triggering this error is implemented correctly in the appropriate location.
Verification successful
Market price exponent check implementation verified.
The new error
ErrInvalidMarketPriceExponentis properly utilized inprotocol/x/prices/keeper/market.goto validate the market price exponent against the Decimals value in the market map.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usage of ErrInvalidMarketPriceExponent # Test: Look for the usage of the new error in the codebase rg --type go "ErrInvalidMarketPriceExponent" -C 5 # Test: Search for functions that might be performing the exponent check rg --type go "func.*Exponent.*Market" -C 5Length of output: 8246
protocol/x/prices/keeper/msg_server_create_oracle_market_test.go (3)
59-65: Improved test case for market not found in MarketMap.The changes in this test case improve its clarity and accuracy:
- The test name now accurately describes the scenario being tested.
- The error message is updated to reflect the actual validation being performed.
- The test setup is simplified by using
testMarket1.Param.These modifications better align the test with the underlying functionality.
Let's verify the implementation of this check:
#!/bin/bash # Search for the validation logic that checks if a ticker exists in the market map rg --type go -g '!*_test.go' 'Ticker not found in market map'
Line range hint
1-150: Improved test coverage and integration validation.The overall changes to the test structure and logic enhance the test suite by:
- Adding checks for market enablement in the MarketMap before and after creating the Oracle market.
- Including new assertions to verify the state of the market in the MarketMap.
These additions improve the test coverage and validate the integration between the prices module and the market map module.
Let's verify the integration between the prices and market map modules:
#!/bin/bash # Search for interactions between the prices keeper and the market map keeper rg --type go -g '!*_test.go' 'MarketMapKeeper'
76-83: Enhanced setup for "oracle market id already exists" test case.The addition of
keepertest.CreateMarketsInMarketMapFromParamsin the setup function improves the test by ensuring that the market exists in both the prices keeper and the market map. This change makes the test more comprehensive and realistic.Let's verify the implementation of the
CreateMarketsInMarketMapFromParamsfunction:Verification successful
Let's generate a corrected shell script to search for the
CreateMarketsInMarketMapFromParamsfunction definition:
Verified implementation of
CreateMarketsInMarketMapFromParamsfunction.The function
CreateMarketsInMarketMapFromParamsis correctly defined inprotocol/testutil/keeper/prices.go, ensuring that markets are properly initialized in the market map for the test case.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the definition of CreateMarketsInMarketMapFromParams function rg --type go -g '!*_test.go' 'func CreateMarketsInMarketMapFromParams'Length of output: 4373
Script:
# Searching for the definition of CreateMarketsInMarketMapFromParams function rg --type go -g '!_test.go' 'func CreateMarketsInMarketMapFromParams'Length of output: 147
protocol/x/prices/keeper/market.go (1)
139-139: Improved code structure.The addition of an empty line improves readability by clearly separating functions.
protocol/x/prices/keeper/market_test.go (1)
113-118: Clarify the handling of exponent mismatches and add explanatory comment.The removal of the exponent mismatch test case aligns with the PR objective "[TRA-611] Get exponent from marketmap". However, to ensure comprehensive test coverage and code clarity:
Please clarify how exponent mismatches between market parameters and prices are now handled in the system. Are they no longer possible, or are they handled differently?
Consider adding a comment explaining why this test case was removed. For example:
// Note: Exponent mismatch test case removed as exponents are now derived from the market map.This will help future developers understand the reasoning behind this change and prevent potential reintroduction of unnecessary checks.
To ensure this change doesn't introduce regressions, please run the following script:
If these searches return results, we may need to update other parts of the codebase to reflect this change in exponent handling.
protocol/app/ante/market_update_test.go (2)
189-189: Increased precision in market decimal representation.The
Decimalsfield in various market configurations has been changed from 1 to 8. This increase in precision might affect calculations and representations throughout the system. Ensure that this change is consistently applied and that all dependent components (e.g., price calculations, display formatting) are updated accordingly.To verify the consistency of this change, let's check for any remaining instances of
Decimals: 1:Also applies to: 203-203, 222-222, 241-241
Verification successful
Re-Verifying
DecimalsField ChangesThe initial search for
Decimals: 1did not return any results. To ensure no instances were missed, please run the following script:
All instances of
Decimals: 1have been successfully updated toDecimals: 8.No remaining instances of
Decimals: 1found in the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
rg --type go 'Decimals:\s+1' -g '!*_test.go'Length of output: 65
Script:
#!/bin/bash rg --type go 'Decimals\s*:\s*[0-9]+'Length of output: 3251
767-767: Modified calculation of Decimals field in Ticker struct.The calculation of the
Decimalsfield has been changed fromuint64(pair.market.Exponent)touint64(pair.market.Exponent * -1). This change correctly inverts the exponent to represent the number of decimal places. For example, an exponent of -8 now correctly corresponds to 8 decimal places.This modification aligns the representation with common decimal place notation and should improve consistency across the system. However, ensure that this change is reflected in all relevant parts of the codebase to maintain consistency.
Let's verify if this change has been consistently applied throughout the codebase:
Verification successful
Decimals field correctly updated in
market_update_test.go.The
Decimalsfield has been updated touint64(pair.market.Exponent * -1)inprotocol/app/ante/market_update_test.goas specified. No other instances were found in the codebase, confirming that the change is correctly applied.Scripts executed
The following scripts were executed for the analysis:
Script:
rg --type go 'Decimals:\s+uint64\(.*Exponent\)' -g '!*_test.go'Length of output: 65
Script:
#!/bin/bash # Search for the modified Decimals field in Go files, excluding test files rg --type go 'Decimals:\s+uint64\(pair\.market\.Exponent\s*\*\s*-1\)' -g '!*_test.go'Length of output: 87
Script:
#!/bin/bash # Search for all assignments to the Decimals field involving pair.market.Exponent in Go files rg --type go 'Decimals:\s+uint64\s*\(\s*pair\.market\.Exponent\s*\*\s*-1\s*\)'Length of output: 177
protocol/testutil/constants/genesis.go (2)
Line range hint
3995-4379: Summary: Exponent changes require system-wide verificationThe changes in this PR modify the
exponentvalues for several markets in theprices.market_paramssection:
- LINK-USD (id: 2): Increased precision from -8 to -9
- MKR-USD (id: 23): Decreased precision from -7 to -6
- XLM-USD (id: 25): Slightly decreased precision from -11 to -10
While the increase in precision for LINK-USD is straightforward, the decreases for MKR-USD and XLM-USD require justification.
Given that these changes can have system-wide implications, please run a comprehensive check to ensure consistency and prevent any unintended consequences:
Please review the results of these checks and update any affected areas of the codebase accordingly.
4005-4005: Approve change with verification neededThe
exponentfor the market withid: 2(LINK-USD) has been changed from-8to-9, increasing the price precision. This change looks good as it allows for more precise price representation.However, please verify that this change is consistent with other parts of the system that may rely on this exponent value. Run the following script to check for any hardcoded uses of the old exponent:
Verification successful
Change Verified
The
exponentfor the market withid: 2(LINK-USD) has been successfully updated from-8to-9. All instances of the old exponent value are managed through theLinkUsdExponentconstant, and no hardcoded-8values were found elsewhere in the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any hardcoded uses of the old exponent (-8) for LINK-USD rg -i 'link.*usd.*-8' --type goLength of output: 1020
protocol/testutil/app/order.go (2)
44-47: Retrieve exponent from PricesKeeper usingGetExponentThe code correctly retrieves the exponent using
app.PricesKeeper.GetExponent(ctx, marketParams.Pair). Error handling is appropriate by panicking if the exponent is not found, which is acceptable in this test utility context.
53-53: Use retrieved exponent inMarketPriceThe
Exponentfield inMarketPriceis now updated to use the retrievedexponent, ensuring consistency with the current market data.
| // | ||
| // Deprecated since v7.1.x. This value is now determined from the marketmap. | ||
| sint32 exponent = 3 [ deprecated = true ]; |
There was a problem hiding this comment.
Tip
Codebase Verification
Deprecation of exponent Field Requires Further Updates
The exponent field in proto/dydxprotocol/prices/market_param.proto has been deprecated as intended. However, multiple references to exponent were identified across various parts of the codebase, including but not limited to:
protocol/x/vault/keeper/*.goprotocol/testutil/*.goprotocol/x/revshare/keeper/*.goprotocol/x/clob/keeper/*.goprotocol/daemons/slinky/client/*.go
This indicates that the deprecation is not fully propagated, and residual dependencies on the exponent field may lead to inconsistencies or legacy issues within the system.
Recommended Actions:
- Update References: Refactor the identified files to remove or replace usages of the
exponentfield, ensuring all components rely on the new mechanism provided by the market map. - Comprehensive Testing: After updating, perform thorough testing to verify that the removal of
exponentdoes not adversely affect system functionality. - Documentation: Ensure that all related documentation and client libraries are updated to reflect the deprecation and the new method of determining the exponent value.
Analysis chain
Deprecation of exponent field looks good, but consider additional documentation.
The deprecation of the exponent field has been correctly implemented using the deprecated = true option. The added comment provides clear information about when the field was deprecated and where the value is now determined.
However, to ensure a smooth transition for users of this protobuf definition, consider the following suggestions:
- Add information about how to retrieve the exponent value from the market map.
- Specify the version when this field will be removed entirely, if planned.
- Update any related documentation or client libraries to reflect this change.
Consider updating the comment to include more details:
// Static value. The exponent of the price.
// For example if `Exponent == -5` then a `Value` of `1,000,000,000`
// represents ``$10,000`. Therefore `10 ^ Exponent` represents the smallest
// price step (in dollars) that can be recorded.
- //
- // Deprecated since v7.1.x. This value is now determined from the marketmap.
+ //
+ // Deprecated since v7.1.x. This value is now determined from the marketmap.
+ // To retrieve the exponent, use [method to get from marketmap].
+ // This field will be removed in v8.0.0.
sint32 exponent = 3 [ deprecated = true ];Let's check if there are any other references to exponent in the file that might need updating:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other references to 'exponent' in the file
rg --type protobuf 'exponent' proto/dydxprotocol/prices/market_param.proto
Length of output: 134285
| @@ -31,6 +31,7 @@ type PricesKeeper interface { | |||
| GetAllMarketParams(ctx sdk.Context) (marketParams []MarketParam) | |||
| GetMarketPrice(ctx sdk.Context, id uint32) (marketPrice MarketPrice, err error) | |||
| GetAllMarketPrices(ctx sdk.Context) (marketPrices []MarketPrice) | |||
| GetExponent(ctx sdk.Context, ticker string) (exponent int32, err error) | |||
There was a problem hiding this comment.
Consider adding a market ID-based version of GetExponent for consistency.
While the new GetExponent method is consistent in its error handling approach, it uses a string ticker as a parameter, which differs from many other methods in the interface that use uint32 market IDs.
To maintain consistency with other methods and potentially improve performance, consider adding an additional method that uses a market ID:
GetExponentByMarketID(ctx sdk.Context, marketID uint32) (exponent int32, err error)This would allow callers to use either a ticker string or a market ID, depending on what information they have available, and would be more consistent with methods like GetMarketParam and GetMarketPrice.
Tip
Codebase Verification
Missing implementation of the GetExponent method in existing PricesKeeper implementations.
The shell script results indicate that none of the current implementations of the PricesKeeper interface include the new GetExponent method. This omission will likely cause compilation errors and disrupt functionality.
- Ensure all
PricesKeeperimplementations are updated to include theGetExponentmethod with the correct signature. - Review other interface changes to maintain consistency across implementations.
Analysis chain
Verify impact on existing implementations and consider consistency.
The addition of the GetExponent method to the PricesKeeper interface will require updates to all existing implementations. Also, note that this method uses a ticker string, while some other methods in the interface use market IDs.
- Please ensure all implementations of
PricesKeeperare updated to include this new method. - Consider whether using a ticker string instead of a market ID is the most appropriate approach for consistency and performance reasons.
To help verify the impact, you can run the following script:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of PricesKeeper and check if they include the new GetExponent method
# Find all Go files that might implement PricesKeeper
echo "Searching for potential PricesKeeper implementations:"
fd -e go | xargs rg -l 'type \w+ struct.*PricesKeeper'
echo -e "\nChecking for GetExponent method in potential implementations:"
fd -e go | xargs rg 'func \(.*\) GetExponent\(ctx sdk\.Context, ticker string\) \(exponent int32, err error\)'
echo -e "\nNote: If the second search returns no results, it means the new method hasn't been implemented yet."
Length of output: 643
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (4)
indexer/packages/v4-protos/src/codegen/dydxprotocol/prices/market_price.ts (1)
11-13: Approve the added comment with a minor suggestion.The added comment provides valuable information about the change in behavior for the
exponentfield. This is crucial for developers to understand how the value is now determined.Consider adding a brief explanation of why this change was made or its benefits. This could provide more context for developers. For example:
/** * Static value. The exponent of the price. See the comment on the duplicate * MarketParam field for more information. * * As of v7.1.x, this value is determined from the marketmap instead of * needing to match the MarketParam field. + * This change allows for more flexibility in managing market parameters. */indexer/packages/v4-protos/src/codegen/dydxprotocol/prices/market_param.ts (3)
21-26: LGTM with a minor suggestion.The deprecation notice for the
exponentproperty is clear and informative. It properly uses the JSDoc comment style and provides useful information about when the property was deprecated and where the value is now determined.Consider adding a link to the documentation or the specific location in the marketmap where developers can find more information about the new
exponentvalue determination. This could be helpful for developers transitioning to the new system.For example:
/** * Static value. The exponent of the price. * For example if `Exponent == -5` then a `Value` of `1,000,000,000` * represents ``$10,000`. Therefore `10 ^ Exponent` represents the smallest * price step (in dollars) that can be recorded. * * Deprecated since v7.1.x. This value is now determined from the marketmap. * @deprecated See https://link-to-marketmap-documentation for more information. */
65-70: LGTM. Ensure consistency with MarketParam interface.The deprecation notice for the
exponentproperty in theMarketParamSDKTypeinterface is consistent with the one added to theMarketParaminterface. This maintains the parallel structure between the two interfaces.If you implement the suggestion to add a documentation link in the
MarketParaminterface, please ensure you make the same change here for consistency.
Deprecation of
exponentProperty Impacts Multiple Codebase AreasThe
exponentproperty is extensively used across various modules and tests within the codebase. Deprecating it inMarketParamandMarketParamSDKTypewill affect:
- Configuration Files: Multiple configuration files reference
exponent, which are critical for system setup.- Test Suites: Numerous tests rely on
exponent, and these will need updates to reflect the deprecation.- Core Modules: Key services and modules depend on
exponentfor functionality, and their behavior may be disrupted.Recommended Actions:
- Develop a Comprehensive Migration Plan: Outline steps to update all instances where
exponentis used.- Update Documentation: Ensure all documentation reflects the deprecation and provides guidance on the new approach.
- Communicate with Teams: Inform all teams about the change to coordinate updates and avoid disruptions.
- Version Management: Bump the package version to indicate a major change, signaling to users the need for attention.
Analysis chain
Line range hint
1-190: Consider implications and next steps for deprecation.The changes to deprecate the
exponentproperty in bothMarketParamandMarketParamSDKTypeinterfaces are well-implemented. However, it's important to consider the broader implications of this deprecation:
- Usage in existing code: Ensure that all parts of the codebase that use these interfaces are updated to handle the deprecation appropriately.
- Documentation updates: Update any relevant documentation to reflect this change and guide developers on how to work with the new system.
- Migration plan: Consider creating a migration guide for users of this code to transition away from using the
exponentproperty.- Version bump: Ensure that the package version is updated appropriately to reflect this breaking change.
To help identify potential areas that might be affected by this change, you can run the following script:
This will help identify areas of the codebase that might need updates due to this deprecation.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of MarketParam or MarketParamSDKType with focus on the exponent property rg --type typescript -e 'MarketParam.*exponent' -e 'MarketParamSDKType.*exponent'Length of output: 118
Script:
#!/bin/bash # Search for usages of MarketParam or MarketParamSDKType with focus on the exponent property rg --type ts -e 'MarketParam.*exponent' -e 'MarketParamSDKType.*exponent'Length of output: 75
Script:
#!/bin/bash # Search for all usages of the 'exponent' property within TypeScript files rg --type ts 'exponent'Length of output: 12789
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/prices/market_param.ts (2 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/prices/market_price.ts (2 hunks)
Additional comments not posted (2)
indexer/packages/v4-protos/src/codegen/dydxprotocol/prices/market_price.ts (2)
32-34: Approve the consistent comment update.The comment update for the
MarketPriceSDKTypeinterface is consistent with the changes made to theMarketPriceinterface. This maintains clarity and consistency across the codebase.Good job on keeping the documentation synchronized between the main interface and the SDK type. This helps prevent confusion and ensures that developers have access to the same information regardless of which type they're working with.
Line range hint
1-105: Consider updating related documentation.The changes made to this file are informative and valuable. However, given that they indicate a significant change in behavior (how the
exponentvalue is determined), it might be beneficial to ensure that this information is reflected in other parts of the documentation or codebase.To ensure consistency across the codebase, please run the following script to check for other occurrences of
exponentthat might need updating:This will help identify any other locations where the relationship between
exponentandMarketParamis mentioned, which might need to be updated to reflect the new behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
protocol/x/prices/simulation/genesis.go (1)
54-54: LGTM with a minor optimization suggestion.The
strings.ToUpperfunction is correctly applied to themarketNameinside the loop, ensuring that any newly generated market name (in case of a collision) is also converted to uppercase. This maintains consistency with the uppercase requirement.Consider moving the uppercase conversion to the
simtypes.RandStringOfLengthcall to avoid redundant conversions:- marketName = simtypes.RandStringOfLength(r, simtypes.RandIntBetween(r, 3, 6)) + "-USD" - marketName = strings.ToUpper(marketName) + marketName = strings.ToUpper(simtypes.RandStringOfLength(r, simtypes.RandIntBetween(r, 3, 6))) + "-USD"This change would eliminate the need for a separate
strings.ToUppercall and slightly improve performance.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
<details>
Files selected for processing (2)
- protocol/x/prices/keeper/market.go (3 hunks)
- protocol/x/prices/simulation/genesis.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/prices/keeper/market.go
Additional comments not posted (3)
protocol/x/prices/simulation/genesis.go (3)
8-8: LGTM: Import added correctly.The
stringspackage import is necessary for thestrings.ToUpperfunction used in thegenMarketNamefunction. It's placed correctly in the import block.
51-51: LGTM: Uppercase conversion added correctly.The
strings.ToUpperfunction is applied to themarketNameimmediately after its initial generation. This ensures that all market names are consistently in uppercase format, which aligns with the apparent requirement for uppercase market names.
51-54: Summary: Uppercase market names consistently implemented.The changes in the
genMarketNamefunction effectively ensure that all generated market names are in uppercase format. This is achieved by applyingstrings.ToUpperboth after the initial name generation and in the collision-handling loop. These changes are consistent and correctly implement the apparent requirement for uppercase market names.The implementation will guarantee that all market names returned by this function are in uppercase, regardless of the original case of the randomly generated strings. This consistency in market name formatting can help prevent case-sensitivity issues in other parts of the system that may interact with these market names.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (5)
protocol/x/prices/keeper/msg_server_create_oracle_market_test.go (3)
60-66: LGTM: Updated test case for market map validation.The changes to this test case correctly reflect the new behavior of checking for market existence in the market map. The use of
testMarket1.Paramensures consistency with other test cases.Consider adding a comment explaining why the market is not found in the market map in this test case. For example:
// No markets are added to the market map in the setup, // so the test market should not be found.This would make the test's intention clearer to other developers.
77-84: LGTM: Added market map setup for duplicate ID test.The addition of
CreateMarketsInMarketMapFromParamsin the setup ensures that the market exists in the market map before attempting to create an oracle market with the same ID. This is consistent with the new requirement of markets existing in the market map.For improved clarity, consider adding a comment explaining the purpose of this setup:
// Create a market in the market map with the same ID as testMarket1 // to simulate the scenario where the oracle market ID already exists. keepertest.CreateMarketsInMarketMapFromParams( // ... existing code ... )This comment would help other developers understand the test scenario more quickly.
148-187: LGTM: New test case for market price exponent derivation.This new test case effectively verifies that the market price exponent is correctly derived from the market map's decimals during oracle market creation. It covers an important aspect of the new functionality, ensuring proper integration between the market map and oracle market creation process.
To make the test more robust, consider adding a test case with a different decimal value. This would ensure that the exponent derivation works correctly for various decimal values. For example:
testCases := []struct { name string decimals uint64 expected int32 }{ {"8 decimals", 8, -8}, {"6 decimals", 6, -6}, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { // ... (existing test logic with tc.decimals and tc.expected) }) }This approach would provide more comprehensive coverage of the exponent derivation logic.
protocol/x/prices/keeper/market_test.go (2)
114-119: LGTM: Improved test case structure and simplified exponent assignmentThe changes improve the readability of the test case struct and simplify the
Exponentfield assignment. These modifications are in line with the removal of the "Market param and price exponents don't match" test case.Consider adding a comment explaining why the exponent is always set to -6, as it might not be immediately clear to future readers.
Also applies to: 203-203
223-289: LGTM: Well-structured test for market price exponent validationThe new
TestValidateMarketPriceExponentfunction is a valuable addition to the test suite. It effectively validates the relationship between market price exponents and market map decimals, covering both success and failure scenarios.Suggestions for improvement:
- Consider adding a test case where the market map decimals are 0 to ensure edge cases are covered.
- Add a comment explaining the significance of this validation in the context of the prices module.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- protocol/x/prices/keeper/market_test.go (4 hunks)
- protocol/x/prices/keeper/msg_server_create_oracle_market_test.go (4 hunks)
Additional comments not posted (5)
protocol/x/prices/keeper/msg_server_create_oracle_market_test.go (3)
16-16: LGTM: New import for marketmap types.The addition of the
marketmaptypesimport is consistent with the changes in the test cases that now involve market map operations.
Line range hint
1-187: Overall assessment: Well-implemented changes with good test coverage.The modifications to existing test cases and the addition of the new test function effectively cover the new functionality related to market map integration in the
CreateOracleMarketprocess. The changes are consistent with the new requirements and maintain good code quality.Key points:
- Updated error messages and test scenarios align with the new market map validation.
- The new test case for market price exponent derivation covers an important aspect of the integration.
The minor suggestions provided for improved clarity and test robustness would further enhance the already solid test suite.
58-58: Verify error message consistency.The expected error message has been updated to "incorrectly formatted CurrencyPair", which is more specific and informative. However, as noted in a previous review, there might be a mismatch with the actual implementation if it appends additional details.
To ensure consistency, let's verify the error message in the actual implementation:
Please ensure that the test expectation matches the actual error message format used in the implementation.
Verification successful
Error message update is consistent with test assertions.
The test uses
require.ErrorContains, which verifies that the error message includes the expected substring. The updated error message "incorrectly formatted CurrencyPair" is more specific and aligns with the assertion method, ensuring the test remains valid.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the validation logic for CurrencyPair formatting rg --type go -g '!*_test.go' 'incorrectly formatted CurrencyPair'Length of output: 192
Script:
#!/bin/bash # Search for usage of expectedErr in the test file rg 'expectedErr' protocol/x/prices/keeper/msg_server_create_oracle_market_test.goLength of output: 450
protocol/x/prices/keeper/market_test.go (2)
14-14: LGTM: New import for marketmaptypesThe addition of the
marketmaptypesimport is appropriate for the new test function that validates market price exponents against market map decimals.
Line range hint
1-290: Overall assessment: Changes improve test coverage and align with PR objectivesThe modifications to this test file are well-implemented and consistent with the PR's goal of obtaining the exponent from the market map. The new
TestValidateMarketPriceExponentfunction adds valuable coverage, while the changes toTestCreateMarket_Errorssimplify the code by removing redundant checks. These changes enhance the overall quality and maintainability of the test suite.
chenyaoy
changed the title
|
Other places to update to use v4-chain/protocol/x/listing/keeper/listing.go Lines 66 to 67 in 07fc0b6 |
Thanks for catching the market_price.go one. Im choosing not to call it in the listing keeper method because it already needs marketMapDetails |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
protocol/x/prices/keeper/market_price.go (1)
177-190: Exponent retrieval implementation looks good, but consider refining error handling.The implementation of retrieving the exponent from the market map aligns well with the PR objectives. However, there are a couple of points to consider:
Returning
nilon error might lead to unexpected behavior in functions callingGetMarketIdToValidIndexPrice. Consider returning a partial result with the successfully processed market prices instead.It would be helpful to add a comment explaining the significance of the exponent in this context and why it's being retrieved from the market map.
Consider refactoring the error handling as follows:
exponent, err := k.GetExponent(ctx, marketParam.Pair) if err != nil { k.Logger(ctx).Error( "failed to get exponent for market", "market id", marketParam.Id, "market pair", marketParam.Pair, "error", err, ) - return nil + // Skip this market and continue processing others + continue }This change would allow the function to return partial results for markets where exponent retrieval was successful.
Consider adding a comment to explain the exponent's role:
// Retrieve the exponent for the market price from the market map. // This exponent is used to properly scale the price value for the given market. exponent, err := k.GetExponent(ctx, marketParam.Pair)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- protocol/testing/e2e/gov/prices_test.go (0 hunks)
- protocol/testutil/constants/genesis.go (6 hunks)
- protocol/x/prices/keeper/msg_server_update_market_param_test.go (0 hunks)
💤 Files with no reviewable changes (2)
- protocol/testing/e2e/gov/prices_test.go
- protocol/x/prices/keeper/msg_server_update_market_param_test.go
🔇 Additional comments (2)
protocol/testutil/constants/genesis.go (2)
4178-4178: Question: Justification for reducing MKR-USD price precisionThe
exponentfor the market withid: 23(MKR-USD) has been changed from-7to-6, which reduces the price precision. This change is unusual as it limits the granularity of price representation.Please provide justification for this change. Was this done intentionally to align with some specific requirements or limitations?
Also, let's verify that this change doesn't negatively impact any existing functionality:
#!/bin/bash # Search for any hardcoded uses of the old exponent (-7) for MKR-USD rg -i 'mkr.*usd.*-7' --type go # Check if there are any minimum price increment checks that might be affected rg -i 'mkr.*usd.*increment' --type go
4194-4194: Question: Reason for adjusting XLM-USD price precisionThe
exponentfor the market withid: 25(XLM-USD) has been changed from-11to-10, slightly reducing the price precision. While this change is less drastic than the previous one, it's still important to understand the reasoning behind it.Could you please explain the motivation for this change? Is it to align with specific market requirements or to optimize some aspect of the system?
Additionally, let's verify that this change doesn't cause any unintended consequences:
#!/bin/bash # Search for any hardcoded uses of the old exponent (-11) for XLM-USD rg -i 'xlm.*usd.*-11' --type go # Check for any price-related calculations specific to XLM that might be affected rg -i 'xlm.*usd.*price' --type go
| @@ -4007,7 +4007,7 @@ const GenesisState = `{ | |||
| }, | |||
| { | |||
| "exchange_config_json": "{\"exchanges\":[{\"exchangeName\":\"Binance\",\"ticker\":\"\\\"LINKUSDT\\\"\"},{\"exchangeName\":\"BinanceUS\",\"ticker\":\"\\\"LINKUSD\\\"\"},{\"exchangeName\":\"CoinbasePro\",\"ticker\":\"LINK-USD\"},{\"exchangeName\":\"CryptoCom\",\"ticker\":\"LINK_USD\"},{\"exchangeName\":\"Huobi\",\"ticker\":\"linkusdt\"},{\"exchangeName\":\"Kraken\",\"ticker\":\"LINKUSD\"},{\"exchangeName\":\"Kucoin\",\"ticker\":\"LINK-USDT\"},{\"exchangeName\":\"Okx\",\"ticker\":\"LINK-USDT\"}]}", | |||
| "exponent": -8, | |||
| "exponent": -9, | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistency Found in LinkUsdExponent Value
The LinkUsdExponent in protocol/testutil/constants/prices.go remains set to -8, which does not align with the updated exponent -9 in genesis.go. This discrepancy may lead to inconsistent price precision for LINK-USD across the system.
🔗 Analysis chain
Verify the intentionality and impact of increased LINK-USD price precision
The exponent for the LINK-USD market (id: 2) has been changed from -8 to -9, increasing the price precision by one decimal place.
Please confirm if this change is intentional and consistent with other parts of the system. Let's verify its impact:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any hardcoded uses of the old exponent (-8) for LINK-USD
rg -i 'link.*usd.*-8' --type go
# Check for any price-related calculations specific to LINK that might be affected
rg -i 'link.*usd.*price' --type go
# Look for any other occurrences of LINK-USD market ID (2) that might need updating
rg -i 'market.*id.*2.*link' --type go
Length of output: 798
Changelist
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests