fix: unwire spot module for main branch #1831
Conversation
WalkthroughThe recent updates reflect a significant shift in the Nibiru project, focusing on streamlining its components by removing the 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.
Actionable comments posted: 7
Review Status
Configuration used: CodeRabbit UI
Files ignored due to path filters (15)
x/perp/v2/types/event.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/perp/v2/types/genesis.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/perp/v2/types/query.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/perp/v2/types/query.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.gox/perp/v2/types/state.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/perp/v2/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/spot/types/event.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/spot/types/genesis.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/spot/types/misc/stabletests.csvis excluded by!**/*.csv,!**/*.csvx/spot/types/params.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/spot/types/pool.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/spot/types/query.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/spot/types/query.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.gox/spot/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/spot/types/tx.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.go
Files selected for processing (14)
- INSTALL.md (7 hunks)
- README.md (5 hunks)
- app/keepers.go (12 hunks)
- cmd/nibid/cmd/root.go (2 hunks)
- contrib/docker/chaosnet.Dockerfile (1 hunks)
- contrib/scripts/chaosnet.sh (3 hunks)
- contrib/scripts/localnet.sh (6 hunks)
- justfile (1 hunks)
- simapp/sim_test.go (2 hunks)
- x/common/testutil/cli/network.go (2 hunks)
- x/common/testutil/cli/query.go (3 hunks)
- x/common/testutil/testapp/testapp.go (2 hunks)
- x/oracle/client/cli/gen_pricefeeder_delegation_test.go (1 hunks)
- x/oracle/keeper/hooks.go (2 hunks)
Files skipped from review due to trivial changes (2)
- simapp/sim_test.go
- x/oracle/client/cli/gen_pricefeeder_delegation_test.go
Additional Context Used
Additional comments not posted (10)
contrib/docker/chaosnet.Dockerfile (1)
23-28: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-31]
The Dockerfile changes seem aligned with the PR's objective to remove the
spotandperpmodules, as indicated by the removal of thefeat-perp.shscript copying. Ensure that any dependencies or references to this script in other parts of the project or in documentation are also updated or removed to maintain consistency.x/oracle/keeper/hooks.go (1)
7-12: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-39]
The significant alteration within the
AfterEpochEndfunction, removing logic related to reward handling and allocation, aligns with the PR's objective to simplify or refocus the project by removing thespotandperpmodules. Ensure that this change does not inadvertently affect other functionalities that might rely on the previous behavior of this function. It's also important to verify that the removal of this logic is part of a broader strategy or refactoring plan that includes handling these operations differently or deems them unnecessary.justfile (1)
42-42: Splitting the command example into two lines in the comment improves readability and clarity, making it easier for users to understand how to run a Nibiru local network. This change is a good practice for documentation within code files, ensuring instructions are clear and accessible.INSTALL.md (1)
46-78: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-127]
The updates and reorganization in
INSTALL.md, including the addition of troubleshooting and contributing sections, enhance the clarity and usefulness of the installation guide. These changes make it easier for users to follow the installation process and contribute to the project. However, consider updating "MacOS" to "macOS" to align with the official naming convention by Apple.- Error when running `make install` on MacOS + Error when running `make install` on macOSx/common/testutil/cli/query.go (1)
16-21: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-93]
The removal of functions related to querying markets and positions in the
perp/v2module fromquery.gois consistent with the PR's objective to remove thespotandperpmodules. Ensure that any references to these removed functions in documentation, tests, or other parts of the codebase are also updated or removed to maintain consistency and avoid broken references.x/common/testutil/testapp/testapp.go (1)
19-24: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-103]
The removal of the import statement related to
perp/v2/typesand the function call toPerpKeeperV2.Collateral.Setintestapp.goaligns with the PR's objective to remove thespotandperpmodules. This change likely affects test setups and should be accompanied by updates to any tests or documentation that relied on the removed functionality. Ensure that the removal of these elements does not leave any orphaned references or broken tests in the codebase.cmd/nibid/cmd/root.go (1)
29-34: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-263]
Given the PR's objective to remove the
perpv2clipackage import and associated function call, ensure that:
- All references to
perpv2cliare completely removed from the file.- No orphaned variables or functions that depended on
perpv2cliremain.- Any tests or documentation that referenced
perpv2cliare updated accordingly.Additionally, verify that the removal does not affect the functionality of related modules or commands, potentially requiring adjustments elsewhere in the codebase.
README.md (2)
25-25: Ensure the link to the Nibiru Chain documentation is correct and accessible. It's crucial for users to have accurate and working links for documentation.
47-47: The description of CosmWasm Smart Contracts mentions enhanced security, performance, and interoperability. Ensure that the link to the CosmWasm sandbox monorepo is correct and that the repository contains relevant and up-to-date information about the protocol's core smart contracts.x/common/testutil/cli/network.go (1)
42-47: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-263]
Given the PR's objective to remove the
perpV2typesimport and its usage, ensure that:
- All references to
perpV2typesare completely removed from the file.- No orphaned variables or functions that depended on
perpV2typesremain.- Any tests or documentation that referenced
perpV2typesare updated accordingly.Additionally, verify that the removal does not affect the functionality of related modules or configurations, potentially requiring adjustments elsewhere in the codebase.
README.md
Outdated
| @@ -22,17 +22,17 @@ | |||
|
|
|||
| ## ⚙️ — Documentation | |||
|
|
|||
| - [Docs | Nibiru Chain](https://nibiru.fi/docs/): Conceptual and technical documentation can be found here. | |||
| - [Docs | Nibiru Chain](https://nibiru.fi/docs/): Conceptual and technical documentation can be found here. | |||
| - [Complete Golang reference docs](https://pkg.go.dev/github.com/NibiruChain/nibiru): (`pkg.go.dev`) For the blockchain implementation . | |||
There was a problem hiding this comment.
The phrase "For the blockchain implementation." should not have a space before the period. Consider revising to maintain grammatical correctness.
- For the blockchain implementation .
+ For the blockchain implementation.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.
| - [Complete Golang reference docs](https://pkg.go.dev/github.com/NibiruChain/nibiru): (`pkg.go.dev`) For the blockchain implementation . | |
| - [Complete Golang reference docs](https://pkg.go.dev/github.com/NibiruChain/nibiru): (`pkg.go.dev`) For the blockchain implementation. |
README.md
Outdated
|
|
||
| ## ⚙️ — Documentation | ||
|
|
||
| - [Docs | Nibiru Chain](https://nibiru.fi/docs/): Conceptual and technical documentation can be found here. | ||
| - [Docs | Nibiru Chain](https://nibiru.fi/docs/): Conceptual and technical documentation can be found here. | ||
| - [Complete Golang reference docs](https://pkg.go.dev/github.com/NibiruChain/nibiru): (`pkg.go.dev`) For the blockchain implementation . | ||
| - ["X Module"](https://nibiru.fi/docs/dev/x/)-specific documentation | ||
|
|
||
| ## 💬 — Community | ||
|
|
||
| If you have questions or concerns, feel free to connect with a developer or community member in the [Nibiru Discord][social-discord]. We also have active communities on [Twitter][social-twitter] and [Telegram][social-telegram]. | ||
|
|
||
| <!-- Markdown versions of the social badges | ||
| [![description][discord-badge]][social-discord] | ||
| [![description][twitter-badge]][social-twitter] | ||
| <!-- Markdown versions of the social badges | ||
| [![description][discord-badge]][social-discord] | ||
| [![description][twitter-badge]][social-twitter] | ||
| [![description][telegram-badge]][social-telegram] | ||
| --> | ||
|
|
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [79-79]
The operating system from Apple should be written as “macOS” instead of “MacOS” to align with the official branding and naming conventions.
- Unix system: MacOS or Ubuntu 18+
+ Unix system: macOS or Ubuntu 18+
README.md
Outdated
| ``` | ||
| and open another terminal. | ||
| ``` | ||
|
|
There was a problem hiding this comment.
This sentence does not start with an uppercase letter. Ensure that the documentation follows consistent grammatical rules for readability and professionalism.
- and open another terminal.
+ And open another terminal.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.
contrib/scripts/chaosnet.sh
Outdated
| #!/usr/bin/env bash | ||
| # | ||
| # This script is used in tandem with `contrib/docker/chaosnet.Dockerfile` to | ||
| # run nodes for Nibiru Chain networks inside docker containers. | ||
| # run nodes for Nibiru Chain networks inside docker containers. | ||
| # - See CHAOSNET.md for usage instructions. | ||
| set -e | ||
|
|
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [16-16]
Consider quoting variables to prevent globbing and word splitting, which enhances the script's robustness and security. For example:
- rm -rf "$HOME/.nibid"
+ rm -rf "${HOME}/.nibid"
contrib/scripts/chaosnet.sh
Outdated
| curr_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" | ||
| source "$curr_dir/feat-perp.sh" | ||
| add_genesis_perp_markets_offline | ||
| curr_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" &>/dev/null && pwd)" |
There was a problem hiding this comment.
The variable curr_dir appears unused in the script. If it's not needed, consider removing it to clean up the script and avoid confusion.
There was a problem hiding this comment.
Actionable comments posted: 3
Actionable comments outside the diff hunks (1)
contrib/scripts/localnet.sh (1)
Line range hint
235-237: Ensure that thejqcommand arguments are properly quoted to prevent globbing and word splitting. This is crucial for maintaining the integrity of the command's input and output.- cat $CHAIN_DIR/config/genesis.json | jq "$1" >$CHAIN_DIR/config/tmp_genesis.json + cat "$CHAIN_DIR/config/genesis.json" | jq "$1" > "$CHAIN_DIR/config/tmp_genesis.json" - mv $CHAIN_DIR/config/tmp_genesis.json $CHAIN_DIR/config/genesis.json + mv "$CHAIN_DIR/config/tmp_genesis.json" "$CHAIN_DIR/config/genesis.json"
contrib/scripts/localnet.sh
Outdated
| echo_info "config/app.toml: Enabling API server" | ||
| sed -i $SEDOPTION '/\[api\]/,+3 s/enable = false/enable = true/' $CHAIN_DIR/config/app.toml | ||
| echo_info "Enabling API server" | ||
| if sed -i $SEDOPTION '/\[api\]/,+3 s/enable = false/enable = true/' $CHAIN_DIR/config/app.toml; then |
There was a problem hiding this comment.
Ensure that variables and paths in shell commands are properly quoted to prevent globbing and word splitting. This is particularly important for the sed command modifications in the script.
- if sed -i $SEDOPTION '/\[api\]/,+3 s/enable = false/enable = true/' $CHAIN_DIR/config/app.toml; then
+ if sed -i "$SEDOPTION" '/\[api\]/,+3 s/enable = false/enable = true/' "$CHAIN_DIR/config/app.toml"; then
- if sed -i $SEDOPTION 's/swagger = false/swagger = true/' $CHAIN_DIR/config/app.toml; then
+ if sed -i "$SEDOPTION" 's/swagger = false/swagger = true/' "$CHAIN_DIR/config/app.toml"; then
- if sed -i $SEDOPTION 's/enabled-unsafe-cors = false/enabled-unsafe-cors = true/' $CHAIN_DIR/config/app.toml; then
+ if sed -i "$SEDOPTION" 's/enabled-unsafe-cors = false/enabled-unsafe-cors = true/' "$CHAIN_DIR/config/app.toml"; thenAlso applies to: 182-182, 190-190
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.
| if sed -i $SEDOPTION '/\[api\]/,+3 s/enable = false/enable = true/' $CHAIN_DIR/config/app.toml; then | |
| if sed -i "$SEDOPTION" '/\[api\]/,+3 s/enable = false/enable = true/' "$CHAIN_DIR/config/app.toml"; then |
contrib/scripts/localnet.sh
Outdated
| if sed -i $SEDOPTION '/\[api\]/,+3 s/enable = false/enable = true/' $CHAIN_DIR/config/app.toml; then | ||
| echo_success "Successfully enabled API server" | ||
| else | ||
| echo_error "Failed to enable API server" | ||
| fi | ||
|
|
||
| # Enable Swagger Docs | ||
| echo_info "config/app.toml: Enabling Swagger Docs" | ||
| sed -i $SEDOPTION 's/swagger = false/swagger = true/' $CHAIN_DIR/config/app.toml | ||
| echo_info "Enabling Swagger Docs" | ||
| if sed -i $SEDOPTION 's/swagger = false/swagger = true/' $CHAIN_DIR/config/app.toml; then | ||
| echo_success "Successfully enabled Swagger Docs" | ||
| else | ||
| echo_error "Failed to enable Swagger Docs" | ||
| fi | ||
|
|
||
| # Enable CORS for localnet | ||
| echo_info "config/app.toml: Enabling CORS" | ||
| sed -i $SEDOPTION 's/enabled-unsafe-cors = false/enabled-unsafe-cors = true/' $CHAIN_DIR/config/app.toml | ||
| echo_info "Enabling CORS" | ||
| if sed -i $SEDOPTION 's/enabled-unsafe-cors = false/enabled-unsafe-cors = true/' $CHAIN_DIR/config/app.toml; then | ||
| echo_success "Successfully enabled CORS" | ||
| else | ||
| echo_error "Failed to enable CORS" | ||
| fi |
There was a problem hiding this comment.
Ensure variables in sed commands are properly quoted to prevent globbing and word splitting.
- if sed -i $SEDOPTION '/\[api\]/,+3 s/enable = false/enable = true/' $CHAIN_DIR/config/app.toml; then
+ if sed -i "$SEDOPTION" '/\[api\]/,+3 s/enable = false/enable = true/' "$CHAIN_DIR/config/app.toml"; then
- if sed -i $SEDOPTION 's/swagger = false/swagger = true/' $CHAIN_DIR/config/app.toml; then
+ if sed -i "$SEDOPTION" 's/swagger = false/swagger = true/' "$CHAIN_DIR/config/app.toml"; then
- if sed -i $SEDOPTION 's/enabled-unsafe-cors = false/enabled-unsafe-cors = true/' $CHAIN_DIR/config/app.toml; then
+ if sed -i "$SEDOPTION" 's/enabled-unsafe-cors = false/enabled-unsafe-cors = true/' "$CHAIN_DIR/config/app.toml"; thenCommittable 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.
| if sed -i $SEDOPTION '/\[api\]/,+3 s/enable = false/enable = true/' $CHAIN_DIR/config/app.toml; then | |
| echo_success "Successfully enabled API server" | |
| else | |
| echo_error "Failed to enable API server" | |
| fi | |
| # Enable Swagger Docs | |
| echo_info "config/app.toml: Enabling Swagger Docs" | |
| sed -i $SEDOPTION 's/swagger = false/swagger = true/' $CHAIN_DIR/config/app.toml | |
| echo_info "Enabling Swagger Docs" | |
| if sed -i $SEDOPTION 's/swagger = false/swagger = true/' $CHAIN_DIR/config/app.toml; then | |
| echo_success "Successfully enabled Swagger Docs" | |
| else | |
| echo_error "Failed to enable Swagger Docs" | |
| fi | |
| # Enable CORS for localnet | |
| echo_info "config/app.toml: Enabling CORS" | |
| sed -i $SEDOPTION 's/enabled-unsafe-cors = false/enabled-unsafe-cors = true/' $CHAIN_DIR/config/app.toml | |
| echo_info "Enabling CORS" | |
| if sed -i $SEDOPTION 's/enabled-unsafe-cors = false/enabled-unsafe-cors = true/' $CHAIN_DIR/config/app.toml; then | |
| echo_success "Successfully enabled CORS" | |
| else | |
| echo_error "Failed to enable CORS" | |
| fi | |
| if sed -i "$SEDOPTION" '/\[api\]/,+3 s/enable = false/enable = true/' "$CHAIN_DIR/config/app.toml"; then | |
| echo_success "Successfully enabled API server" | |
| else | |
| echo_error "Failed to enable API server" | |
| fi | |
| # Enable Swagger Docs | |
| echo_info "Enabling Swagger Docs" | |
| if sed -i "$SEDOPTION" 's/swagger = false/swagger = true/' "$CHAIN_DIR/config/app.toml"; then | |
| echo_success "Successfully enabled Swagger Docs" | |
| else | |
| echo_error "Failed to enable Swagger Docs" | |
| fi | |
| # Enable CORS for localnet | |
| echo_info "Enabling CORS" | |
| if sed -i "$SEDOPTION" 's/enabled-unsafe-cors = false/enabled-unsafe-cors = true/' "$CHAIN_DIR/config/app.toml"; then | |
| echo_success "Successfully enabled CORS" | |
| else | |
| echo_error "Failed to enable CORS" | |
| fi |
contrib/scripts/localnet.sh
Outdated
|
|
||
| echo_info "Adding genesis accounts..." | ||
|
|
||
| val_key_name="validator" | ||
|
|
||
| echo "$MNEMONIC" | $BINARY keys add $val_key_name --recover | ||
| $BINARY add-genesis-account $($BINARY keys show $val_key_name -a) $GENESIS_COINS | ||
| echo_success "Successfully added genesis account: $val_key_name" | ||
| if $BINARY add-genesis-account $($BINARY keys show $val_key_name -a) $GENESIS_COINS; then |
There was a problem hiding this comment.
Quote the subshell command to prevent word splitting when adding a genesis account.
- if $BINARY add-genesis-account $($BINARY keys show $val_key_name -a) $GENESIS_COINS; then
+ if $BINARY add-genesis-account "$($BINARY keys show $val_key_name -a)" $GENESIS_COINS; thenCommittable 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.
| if $BINARY add-genesis-account $($BINARY keys show $val_key_name -a) $GENESIS_COINS; then | |
| if $BINARY add-genesis-account "$($BINARY keys show $val_key_name -a)" $GENESIS_COINS; then |
There was a problem hiding this comment.
Actionable comments posted: 1
Actionable comments outside the diff hunks (5)
contrib/scripts/localnet.sh (5)
[!TIP]
Codebase VerificationThe search results confirm that
FLAG_SPOTis only set in thelocalnet.shscript and does not appear to be used elsewhere in the repository. This suggests that the variable is indeed unused, which aligns with the concern raised in the review comment. It would be advisable to either implement the necessary logic usingFLAG_SPOTor remove the variable if it is not needed.Analysis chain
Line range hint
75-75: Verify the usage ofFLAG_SPOTsince it appears unused in the script.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for FLAG_SPOT usage in the repository rg --type sh 'FLAG_SPOT'Length of output: 78
Line range hint
109-109: Ensure that thesedcommand uses double quotes to prevent globbing and word splitting.- sed -i $SEDOPTION '/\[api\]/,+3 s/enable = false/enable = true/' $CHAIN_DIR/config/app.toml + sed -i "$SEDOPTION" '/\[api\]/,+3 s/enable = false/enable = true/' "$CHAIN_DIR/config/app.toml"
Line range hint
150-150: Ensure that thesedcommand uses double quotes to prevent globbing and word splitting.- sed -i $SEDOPTION 's/swagger = false/swagger = true/' $CHAIN_DIR/config/app.toml + sed -i "$SEDOPTION" 's/swagger = false/swagger = true/' "$CHAIN_DIR/config/app.toml"
Line range hint
158-158: Ensure that thesedcommand uses double quotes to prevent globbing and word splitting.- sed -i $SEDOPTION 's/enabled-unsafe-cors = false/enabled-unsafe-cors = true/' $CHAIN_DIR/config/app.toml + sed -i "$SEDOPTION" 's/enabled-unsafe-cors = false/enabled-unsafe-cors = true/' "$CHAIN_DIR/config/app.toml"
Line range hint
165-165: Quote the subshell command to prevent word splitting when adding a genesis account.- $BINARY add-genesis-account $($BINARY keys show $val_key_name -a) $GENESIS_COINS + $BINARY add-genesis-account "$($BINARY keys show $val_key_name -a)" $GENESIS_COINS
| @@ -24,7 +24,7 @@ console_log_text_color() { | |||
| } | |||
|
|
|||
| if [ console_log_text_color ]; then | |||
There was a problem hiding this comment.
Fix the conditional check to correctly execute the console_log_text_color function.
- if [ console_log_text_color ]; then
+ if console_log_text_color; thenCommittable 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.
| if [ console_log_text_color ]; then | |
| if console_log_text_color; then |
#1830 should be merged first
Summary by CodeRabbit
Documentation
New Features
Refactor
spotandperp) from the application, including keepers and types.Chores
localnet.sh.