Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add x/marketmap module #1922

Merged
merged 9 commits into from
Jul 16, 2024

Conversation

aljo242
Copy link
Contributor

@aljo242 aljo242 commented Jul 16, 2024

Changelist

[Describe or list the changes made in this PR]

Test Plan

[Describe how this PR was tested (if applicable)]

Author/Reviewer Checklist

  • If this PR has changes that result in a different app state given the same prior state and transaction list, manually add the state-breaking label.
  • If the PR has breaking postgres changes to the indexer add the indexer-postgres-breaking label.
  • If this PR isn't state-breaking but has changes that modify behavior in PrepareProposal or ProcessProposal, manually add the label proposal-breaking.
  • If this PR is one of many that implement a specific feature, manually label them all feature:[feature-name].
  • If you wish to for mergify-bot to automatically create a PR to backport your change to a release branch, manually add the label backport/[branch-name].
  • Manually add any of the following labels: refactor, chore, bug.

Summary by CodeRabbit

  • New Features

    • Introduced MarketMap module to manage market information and parameters.
    • Added new messages for market creation, updates, and authority management.
    • Updated genesis state to include market-related information.
  • Bug Fixes

    • Improved handling of module initialization order during upgrades.
  • Chores

    • Updated various dependencies to their latest versions for improved performance and stability.

Copy link
Contributor

coderabbitai bot commented Jul 16, 2024

Walkthrough

This update incorporates a new marketmap module into the existing protocol application. Key changes include initializing and integrating the MarketMapKeeper, adding new message types and handlers for the marketmap functionality, updating genesis state files, and modifying upgrade handlers. Additionally, dependencies in the go.mod file have been updated. These changes enhance the application's capabilities to manage markets more effectively and ensure compatibility with the latest libraries.

Changes

Files / File Groups Change Summary
protocol/app/app.go, .../app_test.go, .../basic_manager.go, .../module_accounts.go, .../module_accounts_test.go Integrated marketmap module: Added MarketMapKeeper, initialized modules, updated permissions, and added tests.
protocol/app/genesis_test.go Reordered import statements for better organization.
protocol/app/msgs/all_msgs.go, .../normal_msgs.go Added new message types related to marketmap, such as MsgCreateMarkets, MsgUpdateMarkets, etc.
protocol/app/testdata/default_genesis_state.json, .../sample_pregenesis.json Updated genesis state files with new entries for marketmap, including market data and parameters.
protocol/app/upgrades.go, .../v6.0.0/upgrade.go, .../upgrade_container_test.go Updated upgrade handlers to incorporate MarketMapKeeper and added new functions for market map state management.
protocol/go.mod Updated various dependencies to their latest versions.
protocol/testutil/app/app.go Modified imports and declarations for various modules and types.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant App
    participant MarketMapKeeper
    participant GenesisState
    User->>App: Initialize App
    App->>MarketMapKeeper: Initialize MarketMapKeeper
    App->>GenesisState: Load genesis state
    GenesisState->>MarketMapKeeper: Load market map data
    User->>App: Execute market map related messages
    App->>MarketMapKeeper: Process messages
    MarketMapKeeper->>App: Return results
    App->>User: Display processed results
Loading

Poem

In the code's vast, bright expanse,
A market map now takes its stance.
With keepers keen and messages new,
Market updates will ensue.
Genesis states so finely spun,
In the world of code, a new dawn begun.


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Outside diff range, codebase verification and nitpick comments (3)
protocol/app/module_accounts.go (1)

17-17: Incorrect import path for marketmapmoduletypes.

The import path github.com/skip-mev/slinky/x/marketmap/types does not exist in the repository. This will lead to build failures or runtime errors. Please verify and correct the import path or ensure the required directory and files are added to the repository.

  • protocol/app/module_accounts.go line 17
Analysis chain

Check the new import for marketmapmoduletypes.

Ensure that the import path github.com/skip-mev/slinky/x/marketmap/types is correct and accessible. Incorrect or inaccessible paths could lead to build failures or runtime errors.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if the import path is accessible.

# Test: Verify the existence of the specified path in the repository.
fd 'marketmap/types' --exec cat {}

Length of output: 36


Script:

#!/bin/bash
# Description: Check if the import path is accessible.

# Test: Verify the existence of the specified path in the repository.
fd 'x/marketmap/types'

Length of output: 22


Script:

#!/bin/bash
# Description: Check if the import path is accessible.

# Test: Verify the existence of the specified path in the repository and list its contents.
fd 'x/marketmap/types' -t d -x ls -l {}

Length of output: 41


Script:

#!/bin/bash
# Description: Check if the import path is accessible.

# Test: Verify the existence of the specified path in the repository and any references to it.
rg 'x/marketmap/types'

Length of output: 1043


Script:

#!/bin/bash
# Description: Check if the import path is accessible.

# Test: List the contents of the 'x/marketmap' directory to confirm the existence of the 'types' subdirectory.
ls -l x/marketmap

Length of output: 78

protocol/app/basic_manager/basic_manager.go (1)

43-43: Ensure marketmapmodule is included in ModuleBasics.

The module marketmapmodule is not currently included in ModuleBasics. Please ensure it is added to avoid potential issues with initialization and interactions with other modules.

  • File: protocol/app/basic_manager/basic_manager.go
  • Line: Add marketmapmodule to the ModuleBasics definition.
Analysis chain

Check the inclusion of marketmapmodule in ModuleBasics.

Ensure that the module path github.com/skip-mev/slinky/x/marketmap is correct and that the module is properly integrated into the system. This inclusion should not disrupt the initialization process or the interactions with other modules.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if the module path is correct and integrated properly.

# Test: Verify the existence of the module path in the repository and its integration.
fd 'marketmap' --exec cat {}

Length of output: 9904


Script:

#!/bin/bash
# Description: Check if `marketmapmodule` is included in `ModuleBasics`.

# Search for the definition of `ModuleBasics` and check for `marketmapmodule` inclusion.
rg 'ModuleBasics' -A 10 | rg 'marketmapmodule'

Length of output: 44


Script:

#!/bin/bash
# Description: Locate the definition of `ModuleBasics` and check for `marketmapmodule` inclusion.

# Search for the definition of `ModuleBasics`.
rg 'ModuleBasics' -A 20

Length of output: 17880

protocol/app/upgrades/v6.0.0/upgrade.go (1)

Line range hint 138-153: Complexity in CreateUpgradeHandler.

The function CreateUpgradeHandler is complex and handles multiple responsibilities. Consider refactoring to separate concerns, such as handling different types of upgrades in separate functions or classes.

// Example refactor to separate concerns
func handlePricesUpgrade(ctx context.Context, pricesKeeper pricestypes.PricesKeeper, mmKeeper marketmapkeeper.Keeper) error {
    // Logic to handle prices upgrade
}

func handleMarketMapUpgrade(ctx context.Context, mmKeeper marketmapkeeper.Keeper) error {
    // Logic to handle market map upgrade
}

func CreateUpgradeHandler(...) upgradetypes.UpgradeHandler {
    return func(ctx context.Context, plan upgradetypes.Plan, vm module.VersionMap) (module.VersionMap, error) {
        // Call separate functions
        handlePricesUpgrade(ctx, pricesKeeper, mmKeeper)
        handleMarketMapUpgrade(ctx, mmKeeper)
        // Other logic
    }
}
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between ad43f46 and 5b814b1.

Files ignored due to path filters (1)
  • protocol/go.sum is excluded by !**/*.sum
Files selected for processing (17)
  • protocol/app/app.go (9 hunks)
  • protocol/app/app_test.go (2 hunks)
  • protocol/app/basic_manager/basic_manager.go (2 hunks)
  • protocol/app/genesis_test.go (1 hunks)
  • protocol/app/module_accounts.go (2 hunks)
  • protocol/app/module_accounts_test.go (4 hunks)
  • protocol/app/msgs/all_msgs.go (1 hunks)
  • protocol/app/msgs/normal_msgs.go (2 hunks)
  • protocol/app/msgs/normal_msgs_test.go (1 hunks)
  • protocol/app/testdata/default_genesis_state.json (2 hunks)
  • protocol/app/upgrades.go (1 hunks)
  • protocol/app/upgrades/v6.0.0/constants.go (2 hunks)
  • protocol/app/upgrades/v6.0.0/upgrade.go (4 hunks)
  • protocol/app/upgrades/v6.0.0/upgrade_container_test.go (7 hunks)
  • protocol/go.mod (20 hunks)
  • protocol/scripts/genesis/sample_pregenesis.json (3 hunks)
  • protocol/testutil/app/app.go (4 hunks)
Files not summarized due to errors (1)
  • protocol/app/upgrades/v6.0.0/constants.go: Error: Message exceeds token limit
Files skipped from review due to trivial changes (2)
  • protocol/app/genesis_test.go
  • protocol/app/msgs/all_msgs.go
Additional comments not posted (30)
protocol/app/upgrades.go (1)

35-36: Review the initialization order of keepers.

The addition of MarketMapKeeper before RevShareKeeper in the initialization order could have implications on the system's behavior, depending on their interdependencies. It's crucial to ensure that this new order does not disrupt existing functionalities or introduce new issues.

protocol/app/module_accounts.go (1)

54-54: Review the permissions for marketmapmoduletypes.ModuleName.

The addition of marketmapmoduletypes.ModuleName with no specific permissions (nil) needs careful consideration. It's important to ensure that this aligns with the intended security model and access controls for the marketmap module.

protocol/app/module_accounts_test.go (2)

38-38: Review the new entries related to marketmapmoduletypes in test assertions.

The addition of marketmapmoduletypes.ModuleName in various maps and assertions should be carefully reviewed to ensure they correctly reflect the module's expected behavior and configurations. This includes checking the module address and permissions.

Also applies to: 78-78, 99-99


4-4: Check the new import in the test file for marketmapmoduletypes.

Ensure that the import path github.com/skip-mev/slinky/x/marketmap/types is correct and accessible, as it is crucial for the test configurations to reflect the actual module structure and dependencies.

protocol/app/upgrades/v6.0.0/upgrade.go (3)

13-19: Review of new imports for marketmap module integration.

The new imports for config, dydx, and marketmap are crucial for the integration of the new MarketMap module. Ensure these packages are properly versioned and maintained to avoid future compatibility issues.


107-111: Initialization of revenue sharing module state.

The function initRevShareModuleState initializes the revenue sharing module state. Ensure that the parameters set here are in line with the business requirements and that there are no unintended side effects or security implications.


28-29: Introduction of GovAuthority variable.

This variable is used to set a module account address for governance. Confirm that this address is secured and permissions are correctly set to prevent unauthorized access.

protocol/app/msgs/normal_msgs.go (3)

23-23: Added import for marketmapmoduletypes.

The import addition is necessary for the new message types related to the marketmap module. Ensure that the imported package is used correctly throughout the file.


244-256: Addition of NormalMsgsSlinky map with new message types.

The new message types for the slinky.marketmap module have been added correctly. Each message type is associated with a corresponding response type set to nil, which is typical for message definitions in this context.


28-28: Review the merging of message maps.

The merging function lib.MergeAllMapsMustHaveDistinctKeys is used to combine NormalMsgsDefault, NormalMsgsDydxCustom, and NormalMsgsSlinky. Ensure that these maps have distinct keys to prevent runtime errors.

protocol/app/testdata/default_genesis_state.json (1)

355-365: Addition of marketmap section in default_genesis_state.json.

The new marketmap section contains parameters and market information. Ensure that the JSON structure is correctly formatted and integrates well with the rest of the genesis state configuration.

protocol/go.mod (1)

11-11: Dependency version updates in go.mod.

Multiple dependencies have been updated to newer versions. It is crucial to verify that these updates are compatible with the project's existing codebase and do not introduce any conflicts or issues.

Also applies to: 27-27, 39-39, 63-63, 70-70, 73-73, 74-74, 83-83, 105-105, 116-116, 126-126, 132-132, 147-147, 150-150, 196-198, 222-222, 242-242, 253-253, 284-284, 303-303, 323-323, 326-326, 414-414, 424-424, 427-427, 430-430, 431-431, 436-436

Verification successful

Let's gather more information to verify the compatibility of the updated dependency github.com/cometbft/cometbft v0.38.9.


Let's gather the changelog details for version v0.38.9 of cometbft to identify any breaking changes or significant updates.


Let's fetch and display the entire changelog file for cometbft to manually inspect the updates in version v0.38.9.


Let's verify if the project uses the Mempool interface or the affected RPC endpoints.


Dependency version updates in go.mod verified.

The update to github.com/cometbft/cometbft v0.38.9 is compatible with the project's current implementation. The changes in v0.38.9 revert the Mempool interface to its state in v0.38.7, which aligns with the project's usage. Additionally, there are no references to the /tx_search RPC endpoint, indicating no impact from related changes.

  • protocol/mempool/noop.go
  • protocol/cmd/dydxprotocold/cmd/config.go
  • protocol/app/app.go
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that the updated dependency versions are compatible with the project.

# Test: Check for reported issues with new versions. Expect: No compatibility issues found.
# Example: Search for issues related to `github.com/cometbft/cometbft v0.38.9`.
echo "Check release notes and compatibility for updated dependencies."

Length of output: 136


Script:

#!/bin/bash
# Search for usage of the `cometbft` dependency in the codebase.
rg 'cometbft' -A 5

# Check the release notes of the `cometbft` repository for version v0.38.9 to identify any breaking changes or important updates.
curl -s https://api.github.com/repos/cometbft/cometbft/releases/tags/v0.38.9 | jq '.body'

Length of output: 83432


Script:

#!/bin/bash
# Fetch and display the changelog for version v0.38.9 of `cometbft`.
curl -s https://raw.githubusercontent.com/cometbft/cometbft/v0.38.9/CHANGELOG.md | grep -A 20 "## \[v0.38.9\]"

Length of output: 112


Script:

#!/bin/bash
# Fetch and display the entire changelog for `cometbft`.
curl -s https://raw.githubusercontent.com/cometbft/cometbft/v0.38.9/CHANGELOG.md

Length of output: 37487


Script:

#!/bin/bash
# Search for usage of the `Mempool` interface in the codebase.
rg 'Mempool' -A 5

# Search for usage of the affected RPC endpoints `/tx` and `/tx_search` in the codebase.
rg '/tx' -A 5
rg '/tx_search' -A 5

Length of output: 79596

protocol/testutil/app/app.go (2)

Line range hint 24-68: Review of Import Changes

The imports have been updated to include new modules and remove unnecessary ones. This is expected as the PR introduces a new module, x/marketmap.

  • Correctness: Ensure that the new imports are used within the file, and that there are no unused imports.
  • Security: Verify that the imported modules do not introduce any security vulnerabilities.
  • Performance: Check if the new imports affect the performance of the application.

Overall, the import changes seem aligned with the PR's objectives to integrate the new MarketMap module.


265-266: Integration of MarketMap Module Types

The addition of marketmapmoduletypes.GenesisState in the UpdateGenesisDocWithAppStateForModule function is a significant change. This integrates the new MarketMap module with the application's genesis state handling.

  • Correctness: Ensure that the marketmapmoduletypes.GenesisState is correctly used in the context of updating the genesis document.
  • Maintainability: This change makes the function handle more types, which may affect maintainability. Consider refactoring if the function becomes too complex.
  • Performance: Verify that the addition does not introduce performance bottlenecks during genesis state initialization.

This change is crucial for the integration of the new module and appears to be implemented correctly.

protocol/app/upgrades/v6.0.0/constants.go (4)

5-7: Imports look appropriate.

The imports are correctly structured and relevant to the file's functionality, including the new marketmapmoduletypes.


29-32: Default parameters setup for MarketMap.

The default parameters for MarketMap are correctly set with appropriate authorities and admin. However, ensure that the hardcoded addresses are valid and intended for production or testing as required.


34-950: Comprehensive review of MarketMap configurations.

The MarketMap configurations for various currency pairs are extensive and cover a wide range of scenarios. Each entry specifies a ticker with detailed provider configurations. Ensure that:

  • The CurrencyPair values are correctly formatted and valid.
  • The NormalizeByPair settings are appropriate for the context (e.g., USDT to USD conversions).
  • The Metadata_JSON fields, although mostly empty, are intended to be so or need to be populated based on further requirements.

Given the complexity and importance of this configuration, it is crucial to validate these settings with domain experts or through additional automated testing to ensure accuracy and completeness.

Verification successful

Comprehensive review of MarketMap configurations.

The MarketMap configurations for various currency pairs are extensive and cover a wide range of scenarios. Each entry specifies a ticker with detailed provider configurations. The CurrencyPair values are correctly formatted and valid, the NormalizeByPair settings are appropriate for the context, and the Metadata_JSON fields are intended to be empty. No issues were found in the configurations.

  • CurrencyPair values are correctly formatted.
  • NormalizeByPair settings are appropriate.
  • Metadata_JSON fields are correctly left empty.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify currency pairs and normalization settings across the configurations.
rg --type go -A 5 $'CurrencyPair'

Length of output: 133296


Script:

#!/bin/bash
# Extract CurrencyPair values from the constants.go file
rg --type go 'CurrencyPair' -A 2 protocol/app/upgrades/v6.0.0/constants.go

# Extract NormalizeByPair values from the constants.go file
rg --type go 'NormalizeByPair' -A 2 protocol/app/upgrades/v6.0.0/constants.go

Length of output: 79419


17-27: Verify the StoreUpgrades configuration.

The configuration for store upgrades includes keys for listingtypes, revsharetypes, and marketmapmoduletypes. Ensure these keys are correctly defined elsewhere in the codebase and that their addition here is consistent with the intended architectural changes.

Verification successful

Verify the StoreUpgrades configuration.

The StoreKey constants for listingtypes, revsharetypes, and marketmapmoduletypes are correctly defined and used elsewhere in the codebase:

  • listingtypes.StoreKey is defined in protocol/x/listing/types/keys.go.
  • revsharetypes.StoreKey is defined in protocol/x/revshare/types/keys.go.
  • marketmapmoduletypes.StoreKey is defined in protocol/x/marketmap/types/keys.go.

These definitions confirm that the StoreUpgrades configuration in protocol/app/upgrades/v6.0.0/constants.go is consistent with the intended architectural changes.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the store keys are defined and used correctly elsewhere.
rg --type go -A 5 $'StoreKey'

Length of output: 140793


Script:

#!/bin/bash
# Verify the definition and usage of listingtypes.StoreKey
rg --type go 'listingtypes.StoreKey'

# Verify the definition and usage of revsharetypes.StoreKey
rg --type go 'revsharetypes.StoreKey'

# Verify the definition and usage of marketmapmoduletypes.StoreKey
rg --type go 'marketmapmoduletypes.StoreKey'

Length of output: 474


Script:

#!/bin/bash
# Verify the definition of listingtypes.StoreKey
rg --type go 'StoreKey' | rg 'listingtypes'

# Verify the definition of revsharetypes.StoreKey
rg --type go 'StoreKey' | rg 'revsharetypes'

# Verify the definition of marketmapmoduletypes.StoreKey
rg --type go 'StoreKey' | rg 'marketmapmoduletypes'

Length of output: 489


Script:

#!/bin/bash
# Verify the definition of listingtypes.StoreKey in its module directory
rg --type go 'StoreKey' $(fd -t d -g 'listing*')

# Verify the definition of revsharetypes.StoreKey in its module directory
rg --type go 'StoreKey' $(fd -t d -g 'revshare*')

# Verify the definition of marketmapmoduletypes.StoreKey in its module directory
rg --type go 'StoreKey' $(fd -t d -g 'marketmap*')

Length of output: 39121


Script:

#!/bin/bash
# Verify the definition of marketmapmoduletypes.StoreKey in its module directory
rg --type go 'StoreKey' $(fd -t d -g 'marketmap*')

Length of output: 38104

protocol/scripts/genesis/sample_pregenesis.json (2)

861-872: Review of the new marketmap module configuration.

The new marketmap configuration has been added correctly. The structure includes last_updated, market_map, and params with admin and market_authorities. It's essential to ensure that the admin and market_authorities values are valid and authorized to perform their designated roles.


1862-1862: Update to app_version noted.

The app_version has been updated to "5.0.0-dev0-385-gb5b86472", reflecting a new development iteration. This change should be cross-verified with the version control system to ensure it corresponds to the correct commit or tag in the repository.

protocol/app/app.go (5)

192-194: Imports for MarketMap module are correctly added.

The import paths for the MarketMap module, its keeper, and types are correctly placed and follow the expected structure.


298-299: Addition of MarketMapKeeper to the App struct is correct.

The MarketMapKeeper has been properly added to the App struct, following the conventions used for other module keepers in the file.


913-913: Addition of marketmapModule to the module manager is correct.

The marketmapModule has been correctly added to the module manager, following the conventions used for other modules.


1223-1223: Correct addition of marketmapmoduletypes.ModuleName to various module setups.

The module name marketmapmoduletypes.ModuleName has been correctly added to various module setups such as SetOrderPreBlockers, SetOrderBeginBlockers, SetOrderEndBlockers, SetOrderInitGenesis, and SetOrderMigrations, ensuring the module is correctly integrated into the application lifecycle.

Also applies to: 1273-1273, 1319-1319, 1366-1366, 1409-1409


907-911: Initialization of MarketMapKeeper in the New function is mostly correct.

The initialization of MarketMapKeeper follows the pattern used for other keepers. However, verify that the govtypes.ModuleName is the correct authority for this module.

Verification successful

Initialization of MarketMapKeeper in the New function is correct.

The use of govtypes.ModuleName as the authority for MarketMapKeeper is consistent with its usage in other modules and tests across the codebase.

  • protocol/app/app.go
  • protocol/x/vest/keeper/msg_server_test.go
  • protocol/x/stats/keeper/msg_server_test.go
  • protocol/testing/e2e/gov/rewards_test.go
  • protocol/testing/e2e/gov/perpetuals_test.go
  • protocol/testing/e2e/gov/feetiers_test.go
  • protocol/testing/e2e/gov/vest_test.go
  • protocol/x/perpetuals/e2e/authorities_test.go
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that `govtypes.ModuleName` is the correct authority for `MarketMapKeeper`.

# Test: Search for the usage of `govtypes.ModuleName` as an authority in other keepers. Expect: Consistent usage across similar modules.
rg --type go -A 5 $'govtypes.ModuleName'

Length of output: 24370

protocol/app/app_test.go (2)

4-4: Check the import for marketmapmodule.

The import addition of marketmapmodule from "github.com/skip-mev/slinky/x/marketmap" is correctly placed and follows the convention of other module imports.


224-225: Integration of marketmapmodule in TestModuleBasics.

The addition of marketmapmodule.AppModuleBasic{} to the list of modules in TestModuleBasics function is correct. Ensure that this new module is fully compatible and integrated with the existing module setup.

Verification successful

Integration of marketmapmodule in TestModuleBasics.

The addition of marketmapmodule.AppModuleBasic{} to the list of modules in the TestModuleBasics function is correct. The test ensures that the module basics match the expected configuration, confirming the integration is successful.

  • protocol/app/app_test.go: TestModuleBasics function includes marketmapmodule.AppModuleBasic{} and validates the module basics.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the integration of `marketmapmodule` in `TestModuleBasics`.

# Test: Search for the function usage. Expect: Only occurrences of the new module in the expected test function.
rg --type go -A 5 $'TestModuleBasics'

Length of output: 423


Script:

#!/bin/bash
# Description: Verify the integration of `marketmapmodule` in `TestModuleBasics`.

# Test: Search for the usage of `marketmapmodule` in the `protocol/app/app_test.go` file.
rg --type go 'marketmapmodule' protocol/app/app_test.go

Length of output: 149


Script:

#!/bin/bash
# Description: Verify the initialization and usage of `marketmapmodule` in the `protocol/app/app_test.go` file.

# Test: Search for the initialization and usage of `marketmapmodule` in the `protocol/app/app_test.go` file.
rg --type go 'marketmapmodule' -A 10 protocol/app/app_test.go

Length of output: 754

protocol/app/msgs/normal_msgs_test.go (1)

220-230: Addition of slinky marketmap messages in TestNormalMsgs_Key.

The new messages related to the slinky marketmap have been correctly added to the expectedMsgs array. This ensures that the new messages are recognized and handled properly in the message handling tests.

protocol/app/upgrades/v6.0.0/upgrade_container_test.go (2)

10-10: Import addition for marketmapmoduletypes.

The addition of the marketmapmoduletypes import is appropriate and follows the convention used for other module type imports.


55-55: New functions for marketmap state checks.

The addition of preUpgradeMarketMapState and postUpgradeMarketMapState functions is crucial for ensuring the market map state is correctly handled during the upgrade process.

Also applies to: 60-60

Comment on lines +67 to +105
func migratePricesToMarketMap(ctx sdk.Context, pk pricestypes.PricesKeeper, mmk marketmapkeeper.Keeper) {
// fill out config with dummy variables to pass validation. This handler is only used to run the
// ConvertMarketParamsToMarketMap member function.
h, err := dydx.NewAPIHandler(zap.NewNop(), config.APIConfig{
Enabled: true,
Timeout: 1,
Interval: 1,
ReconnectTimeout: 1,
MaxQueries: 1,
Atomic: false,
Endpoints: []config.Endpoint{{URL: "upgrade"}},
BatchSize: 0,
Name: dydx.Name,
})
if err != nil {
panic(fmt.Sprintf("Failed to construct dydx handler %v", err))
}
allMarketParams := pk.GetAllMarketParams(ctx)
var mpr dydxtypes.QueryAllMarketParamsResponse
for _, mp := range allMarketParams {
mpr.MarketParams = append(mpr.MarketParams, dydxtypes.MarketParam{
Id: mp.Id,
Pair: mp.Pair,
Exponent: mp.Exponent,
MinExchanges: mp.MinExchanges,
MinPriceChangePpm: mp.MinPriceChangePpm,
ExchangeConfigJson: mp.ExchangeConfigJson,
})
}
mm, err := h.ConvertMarketParamsToMarketMap(mpr)
if err != nil {
panic(fmt.Sprintf("Couldn't convert markets %v", err))
}
for _, market := range mm.MarketMap.Markets {
err = mmk.CreateMarket(ctx, market)
if err != nil {
panic(fmt.Sprintf("Failed to create market %s", market.Ticker.String()))
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential optimization in migratePricesToMarketMap.

This function uses a loop to convert market parameters, which could be optimized. Consider using batch processing or parallel processing techniques to handle large datasets more efficiently.

// Example of batch processing
batchSize := 100 // Define suitable batch size
for i := 0; i < len(allMarketParams); i += batchSize {
    end := i + batchSize
    if end > len(allMarketParams) {
        end = len(allMarketParams)
    }
    batch := allMarketParams[i:end]
    // Process batch
}

Comment on lines +56 to +64
func setMarketMapParams(ctx sdk.Context, mmk marketmapkeeper.Keeper) {
err := mmk.SetParams(ctx, marketmaptypes.Params{
// init so that gov is the admin and a market authority
MarketAuthorities: []string{GovAuthority},
Admin: GovAuthority,
})
if err != nil {
panic(fmt.Sprintf("failed to set x/mm params %v", err))
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure proper error handling in setMarketMapParams.

The use of panic for error handling in setMarketMapParams could be improved. Consider returning an error to the caller instead, which allows for more graceful error handling and recovery in the application.

- panic(fmt.Sprintf("failed to set x/mm params %v", err))
+ return fmt.Errorf("failed to set x/mm params %v", err)
Committable 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. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func setMarketMapParams(ctx sdk.Context, mmk marketmapkeeper.Keeper) {
err := mmk.SetParams(ctx, marketmaptypes.Params{
// init so that gov is the admin and a market authority
MarketAuthorities: []string{GovAuthority},
Admin: GovAuthority,
})
if err != nil {
panic(fmt.Sprintf("failed to set x/mm params %v", err))
}
func setMarketMapParams(ctx sdk.Context, mmk marketmapkeeper.Keeper) error {
err := mmk.SetParams(ctx, marketmaptypes.Params{
// init so that gov is the admin and a market authority
MarketAuthorities: []string{GovAuthority},
Admin: GovAuthority,
})
if err != nil {
return fmt.Errorf("failed to set x/mm params %v", err)
}
return nil
}

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 5b814b1 and 218900e.

Files selected for processing (1)
  • protocol/scripts/genesis/sample_pregenesis.json (2 hunks)
Files skipped from review as they are similar to previous changes (1)
  • protocol/scripts/genesis/sample_pregenesis.json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants