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

fix(inflation-docs): 1 Document inflation, 2 delete unused code, 3 fix CI #1799

Merged
merged 16 commits into from
Feb 18, 2024

Conversation

Unique-Divine
Copy link
Member

@Unique-Divine Unique-Divine commented Feb 17, 2024

Purpose / Abstract

Originally, I opened this to add some doc comments and delete unused code. When I opened the PR, I noticed that the localnet.sh was not running on the branch and was also broken on main. I've gone ahead and fixed that.

In addition, both just localnet and make localnet now use feature flags to enable modules like x/perp and x/spot, which will make tests a lot less finicky.

  • Fixed scripts/chaosnet.sh
  • Fixed scripts/localnet.sh
  • Documented x/inflation more thoroughly in each file.
  • Slight no-op refactor in x/inflation to delete unused code

Summary

  • New Features
    • Added functions to configure initial reserve values for genesis perpetual markets and query market prices.
  • Enhancements
    • Enhanced localnet script with bash, flag parsing for build options, and feature flags for perp and spot.
    • Improved clarity and organization in scripts and documentation, particularly in the inflation module.
  • Refactor
    • Code related to inflation refactored for better clarity and efficiency, including removal of unused code.
  • Documentation
    • Added and refined comments across various modules for better understanding of functionalities and parameters.
  • Chores
    • Updated workflow settings and configurations for e2e testing.

@Unique-Divine Unique-Divine requested a review from a team as a code owner February 17, 2024 23:42
Copy link
Contributor

coderabbitai bot commented Feb 17, 2024

Warning

Rate Limit Exceeded

@Unique-Divine has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 52 seconds before requesting another review.

How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.
Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.
Please see our FAQ for further information.

Commits Files that changed from the base of the PR and between 005113a and b8a0483.

Walkthrough

The recent updates encompass a variety of enhancements including the addition of a cherry-picking feature for WebAssembly messages, significant code refactoring around inflation mechanisms, updates to the librocksdb library, and improvements in script functionalities for better feature management and testing. These changes aim to streamline development, improve maintainability, and extend the functionality of the system, particularly in handling genesis perpetual markets and inflation calculations.

Changes

Files Summary
CHANGELOG.md Added cherry-pick feature, refactored inflation code, updated librocksdb.
contrib/make/localnet.mk, contrib/scripts/localnet.sh, justfile, .github/workflows/e2e-wasm.yml Enhanced script functionalities for better feature management and testing.
contrib/scripts/feat-perp.sh, contrib/scripts/chaosnet.sh Improved handling of genesis perpetual markets with Coingecko prices.
x/.../keeper.go, x/.../hooks.go, x/.../hooks_test.go, x/.../grpc_query.go, x/.../msg_server.go, x/.../sudo.go Refactored inflation code, added comments, and introduced new functionalities.
x/epochs/..., x/inflation/doc.go, x/inflation/types/inflation_calculation_test.go Updated epoch handling and clarified inflation calculations.

🐰✨
In the realm of code, where logic threads weave,
A rabbit hopped, changes to conceive.
With a cherry-pick here, and a refactor there,
Inflation's tale, now clearer, fair.
Through scripts and tests, it danced with glee,
A leap towards efficiency, for you and me.
🌟📜🚀

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-tests for this file.
  • 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 tests 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 from git and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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 a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

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

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • 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/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

@Unique-Divine Unique-Divine changed the title refactor,docs(inflation): Document everything + delete unused code refactor(inflation-docs): Document everything + delete unused code Feb 17, 2024
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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 8d63d64 and 45d3346.
Files selected for processing (13)
  • CHANGELOG.md (1 hunks)
  • x/devgas/v1/keeper/keeper.go (1 hunks)
  • x/epochs/keeper/hooks_test.go (1 hunks)
  • x/epochs/types/hooks.go (1 hunks)
  • x/inflation/doc.go (1 hunks)
  • x/inflation/keeper/grpc_query.go (1 hunks)
  • x/inflation/keeper/hooks.go (5 hunks)
  • x/inflation/keeper/hooks_test.go (4 hunks)
  • x/inflation/keeper/inflation.go (4 hunks)
  • x/inflation/keeper/keeper.go (2 hunks)
  • x/inflation/keeper/msg_server.go (1 hunks)
  • x/inflation/keeper/sudo.go (3 hunks)
  • x/inflation/types/inflation_calculation_test.go (2 hunks)
Files skipped from review due to trivial changes (1)
  • x/inflation/keeper/grpc_query.go
Additional comments: 13
x/inflation/doc.go (1)
  • 1-11: The package documentation for the inflation module is clear and informative, providing a good overview of its purpose and functionality within the Nibiru Chain ecosystem. The inclusion of references is helpful for those seeking more detailed information.
x/epochs/keeper/hooks_test.go (1)
  • 36-41: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-48]

The test file hooks_test.go is well-structured, with appropriate setup for mock hooks and testing of epoch-related functionalities. Ensure that the removal of the AfterEpochEnd function call from the TestAfterEpochEnd test case aligns with the intended testing strategy and module behavior.

x/inflation/keeper/msg_server.go (1)
  • 33-40: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [36-42]

The addition of the ToggleInflation function to the msgServer struct is a valuable enhancement, allowing for the enabling or disabling of token inflation with appropriate sudo access control. The implementation follows best practices and aligns with the PR's objectives to refine the inflation mechanism.

x/epochs/types/hooks.go (1)
  • 7-41: The introduction of the EpochHooks interface and the enhancements to the MultiEpochHooks struct, including updated comments and sequential execution of hook functions, significantly improve the clarity, modularity, and documentation of the epoch handling functionality. These changes align well with best practices and contribute to the maintainability of the code.
x/inflation/keeper/inflation.go (4)
  • 13-27: The updated comments for the MintAndAllocateInflation function provide a clear and detailed explanation of the function's purpose, its arguments, and its return values. This enhances the readability and maintainability of the code.
  • 50-50: The comment for the MintCoins function succinctly explains its purpose, which is to mint tokens using the BankKeeper. This is a good practice as it clarifies the role of external dependencies in the function's operation.
  • 57-66: The comments for the AllocatePolynomialInflation function are well-written, providing a clear overview of the function's purpose and the distribution of minted tokens. This is crucial for understanding the allocation logic and its impact on different parts of the system.
  • 74-99: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [77-114]

The AllocatePolynomialInflation function's logic has been adjusted to improve error handling and clarity in the allocation process. The changes include better error messages and a more structured approach to allocating tokens to different modules. These improvements are beneficial for maintainability and debugging.

However, it's important to ensure that the error handling does not inadvertently swallow errors that should be propagated or logged for further investigation. The use of fmt.Errorf to wrap errors with additional context is a good practice, as it aids in understanding the source and nature of errors when they occur.

x/inflation/keeper/hooks_test.go (3)
  • 239-239: The replacement of inflationKeeper.AfterEpochEnd with inflationKeeper.Hooks().AfterEpochEnd in the TestManual test function is a significant change. This modification aligns with the PR's objective of enhancing modularity and maintainability by utilizing the Hooks interface for epoch event handling. It's important to verify that this change does not alter the intended behavior of the test, ensuring that it still accurately simulates and verifies inflation logic.
  • 261-261: The same modification as mentioned previously is applied here in the loop simulating epoch endings for period 0. This change is consistent with the approach of using the Hooks interface for epoch event handling. As with the previous instance, it's crucial to ensure that this modification does not impact the test's ability to accurately simulate and verify inflation logic during period 0.
  • 285-285: This modification applies the Hooks interface for epoch event handling in the loop simulating epoch endings for period 1. Consistency in using the Hooks interface across different test scenarios is a positive change, enhancing the code's modularity and maintainability. As with the other instances, verifying that this change does not negatively affect the test's accuracy in simulating and verifying inflation logic during period 1 is essential.
CHANGELOG.md (2)
  • 101-103: The entries under "Non-breaking/Compatible Improvements" for cherrypicking GetBuildWasmMsg() to the main branch and bumping librocksdb to v8.9.1 are clear and well-categorized. However, it's important to ensure that the pull request links (if provided) are valid and lead to the correct GitHub pull request pages for users seeking more details.
  • 103-103: The entry under "Non-breaking/Compatible Improvements" for refactoring and documenting inflation-related code, including the deletion of unused code, is appropriately categorized. This entry highlights the effort to improve code quality and maintainability, which is crucial for the long-term success of the project.

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.

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 8d63d64 and 397b4c9.
Files selected for processing (17)
  • CHANGELOG.md (1 hunks)
  • contrib/make/localnet.mk (1 hunks)
  • contrib/scripts/feat-perp.sh (1 hunks)
  • contrib/scripts/localnet.sh (6 hunks)
  • justfile (1 hunks)
  • x/devgas/v1/keeper/keeper.go (1 hunks)
  • x/epochs/keeper/hooks_test.go (1 hunks)
  • x/epochs/types/hooks.go (1 hunks)
  • x/inflation/doc.go (1 hunks)
  • x/inflation/keeper/grpc_query.go (1 hunks)
  • x/inflation/keeper/hooks.go (5 hunks)
  • x/inflation/keeper/hooks_test.go (4 hunks)
  • x/inflation/keeper/inflation.go (4 hunks)
  • x/inflation/keeper/keeper.go (2 hunks)
  • x/inflation/keeper/msg_server.go (1 hunks)
  • x/inflation/keeper/sudo.go (3 hunks)
  • x/inflation/types/inflation_calculation_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (15)
  • CHANGELOG.md
  • contrib/make/localnet.mk
  • contrib/scripts/feat-perp.sh
  • justfile
  • x/devgas/v1/keeper/keeper.go
  • x/epochs/keeper/hooks_test.go
  • x/epochs/types/hooks.go
  • x/inflation/doc.go
  • x/inflation/keeper/grpc_query.go
  • x/inflation/keeper/hooks_test.go
  • x/inflation/keeper/inflation.go
  • x/inflation/keeper/keeper.go
  • x/inflation/keeper/msg_server.go
  • x/inflation/keeper/sudo.go
  • x/inflation/types/inflation_calculation_test.go
Additional comments: 6
x/inflation/keeper/hooks.go (6)
  • 15-29: The introduction of the Hooks struct and the Hooks() method on the Keeper struct is a good practice for encapsulating hook-related logic. This approach enhances modularity and maintainability by separating the hook handling logic from the main keeper logic.
  • 31-34: The BeforeEpochStart function is correctly implemented as a no-op, adhering to the interface requirement without performing any unnecessary operations. This is a clean way to fulfill interface obligations when no action is needed at the start of an epoch.
  • 37-42: The AfterEpochEnd function's initial check for epochIdentifier ensures that inflation logic is only executed for the specified epoch type (DayEpochID). This is a good practice for ensuring that the inflation logic is triggered at the correct intervals.
  • 47-61: The logic to skip inflation if it is disabled and to increment the number of skipped epochs is well-implemented. However, it's important to ensure that the NumSkippedEpochs state is correctly managed across epochs, especially in scenarios where inflation might be toggled on and off.
Verification successful

The examination of the codebase and test implementations related to NumSkippedEpochs and InflationEnabled indicates that the logic to manage NumSkippedEpochs when toggling inflation on and off is well-implemented and tested. The code includes mechanisms to correctly increment NumSkippedEpochs when inflation is disabled and manage its state across different scenarios, as evidenced by the references in hooks.go, keeper.go, and the tests in hooks_test.go. Therefore, the review comment is verified as the implementation aligns with the expectations for managing NumSkippedEpochs correctly.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify that NumSkippedEpochs is correctly managed when toggling inflation on and off.
# This script is a placeholder for the actual verification logic.
echo "Verify NumSkippedEpochs management in various scenarios."

Length of output: 122



Script:

#!/bin/bash
# Search for references to NumSkippedEpochs and any logic related to toggling inflation.
rg "NumSkippedEpochs" -C 5
rg "InflationEnabled" -C 5

Length of output: 50643

* 93-95: Error handling in the `MintAndAllocateInflation` call is correctly implemented, logging the error and returning early to prevent further execution. This is a good practice for robust error handling. Ensure that all possible error paths in `MintAndAllocateInflation` are well-documented and tested. * 167-169: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [159-169]

The use of defer for telemetry updates is a clever way to ensure that metrics are updated regardless of how the function exits. This pattern enhances the reliability of telemetry data collection. Just ensure that the telemetry labels and metrics are consistent and meaningful for monitoring purposes.

Comment on lines +72 to +81
period := h.K.CurrentPeriod.Peek(ctx)
epochsPerPeriod := h.K.GetEpochsPerPeriod(ctx)

epochMintProvision := types.CalculateEpochMintProvision(
params,
period,
)

if !epochMintProvision.IsPositive() {
k.Logger(ctx).Error(
h.K.Logger(ctx).Error(
Copy link
Contributor

Choose a reason for hiding this comment

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

The calculation of epochMintProvision using types.CalculateEpochMintProvision is a good example of encapsulating complex logic within a dedicated function. This keeps the AfterEpochEnd function focused on higher-level logic. Ensure that CalculateEpochMintProvision is thoroughly tested, especially for edge cases like negative provisions.

Would you like me to help with adding or reviewing tests for CalculateEpochMintProvision?

Comment on lines +108 to +123
// Given, epochNumber = 1, period = 0, epochPerPeriod = 30, skippedEpochs = 0
// => 1 - 30 * 0 - 0 < 30
// => nothing to do here
// Given, epochNumber = 70, period = 1, epochPerPeriod = 30, skippedEpochs = 10
// => 70 - 1 * 30 - 10 >= 30
// => a period has ended! we set a new period
// Given, epochNumber = 42099, period = 0, epochPerPeriod = 30, skippedEpochs = 42069
// => 42099 - 0 * 30 - 42069 >= 30
// => a period has ended! we set a new period
numSkippedEpochs := h.K.NumSkippedEpochs.Peek(ctx)
if int64(epochNumber)-
int64(epochsPerPeriod*period)-
int64(numSkippedEpochs) >= int64(epochsPerPeriod) {
prevPeriod := k.CurrentPeriod.Next(ctx)
periodBeforeIncrement := h.K.CurrentPeriod.Next(ctx)

k.Logger(ctx).Info(fmt.Sprintf("setting new period: %d", prevPeriod+1))
h.K.Logger(ctx).Info(fmt.Sprintf("setting new period: %d", periodBeforeIncrement+1))
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic for updating the period based on the current epoch number, epochs per period, and skipped epochs is complex but appears to be correctly implemented. It's crucial to ensure this logic is covered by unit tests to prevent regressions and to validate the behavior in various scenarios.

Would you like assistance in creating or reviewing unit tests for the period update logic?

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.

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 397b4c9 and 59aecf2.
Files selected for processing (4)
  • .github/workflows/e2e-wasm.yml (3 hunks)
  • contrib/scripts/chaosnet.sh (1 hunks)
  • contrib/scripts/localnet.sh (5 hunks)
  • x/inflation/keeper/keeper.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • x/inflation/keeper/keeper.go
Additional comments: 10
.github/workflows/e2e-wasm.yml (5)
  • 8-8: The concurrency settings have been adjusted to allow concurrent runs on main/release branches but isolate other branches. This is a good practice to prevent unnecessary workflow runs on branches that are frequently updated, potentially saving CI/CD resources.
  • 31-31: The change to use the latest: true option in the release downloader step is a significant improvement. It ensures that the workflow always uses the latest release (excluding drafts), which is crucial for e2e testing to validate the most recent changes. This change enhances the reliability of the testing process.
  • 41-42: Moving the nibid binary to /usr/local/bin/ and printing its version is a good practice for ensuring that the correct version of the binary is used during testing. This step also improves the maintainability of the workflow by making it easier to identify the version being tested in the logs.
  • 44-48: The addition of the "Install just" step using taiki-e/install-action@just is a positive change. It ensures that the "just" command-line utility is installed, which is necessary for the subsequent "launch localnet" step. This change enhances the workflow's modularity by clearly separating the installation of dependencies from their usage.
  • 51-51: Modifying the localnet launch command to include --no-build is a thoughtful adjustment. It likely speeds up the testing process by skipping the build step if the necessary binaries are already prepared, which can significantly reduce the overall runtime of the workflow.
contrib/scripts/chaosnet.sh (1)
  • 47-47: Sourcing the feat-perp.sh script is a good practice for modularizing the script and improving maintainability. By moving the logic for adding genesis perpetual markets into a separate script, it becomes easier to manage and update this functionality independently of the main chaosnet.sh script. Ensure that feat-perp.sh is properly maintained and documented to facilitate future updates and debugging.
contrib/scripts/localnet.sh (4)
  • 1-1: Switching the shebang to #!/bin/bash is appropriate for scripts that may utilize bash-specific features not available in sh. This change ensures compatibility and reliability across different environments where bash features might be required.
  • 4-10: Introducing localnet settings at the beginning of the script enhances readability and maintainability. By centralizing configuration variables, it becomes easier to modify the script's behavior without searching through the entire script for hardcoded values.
  • 74-118: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [51-104]

The addition of flag parsing logic is a significant improvement in the script's usability and flexibility. It allows users to customize the script's behavior through command-line flags, such as --no-build to skip building from source and --features to enable specific features. This change makes the script more adaptable to different use cases and environments.

  • 199-208: The conditional logic for setting genesis perp markets based on the FLAG_PERP variable demonstrates good practice in script flexibility and feature toggling. However, ensure that both add_genesis_perp_markets_with_coingecko_prices and add_genesis_perp_markets_offline functions are robustly implemented in the sourced feat-perp.sh script, with proper error handling and validation to prevent script failure.

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.

Review Status

Actionable comments generated: 5

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 59aecf2 and eae1a1f.
Files selected for processing (2)
  • contrib/scripts/chaosnet.sh (1 hunks)
  • contrib/scripts/localnet.sh (5 hunks)
Files skipped from review as they are similar to previous changes (1)
  • contrib/scripts/chaosnet.sh
Additional comments: 3
contrib/scripts/localnet.sh (3)
  • 1-1: The shebang line correctly specifies bash instead of sh, ensuring that bash-specific features can be used safely in the script.
  • 197-209: The conditional execution of add_genesis_perp_markets_with_coingecko_prices and add_genesis_perp_markets_offline based on the FLAG_PERP is a good practice. However, ensure that the scripts feat-perp.sh and the functions it contains are properly documented and error-handled to maintain the robustness of the script.
  • 216-216: The use of add_genesis_param to set various genesis parameters, including setting a validator as a sudoer, is a clear and modular approach. Ensure that the jq command and the handling of temporary files are secure and correctly manage potential errors.

Comment on lines +4 to +9
# Set localnet settings
BINARY="nibid"
CHAIN_ID="nibiru-localnet-0"
MNEMONIC="guard cream sadness conduct invite crumble clock pudding hole grit liar hotel maid produce squeeze return argue turtle know drive eight casino maze host"
GENESIS_COINS="10000000000000unibi,10000000000000unusd,10000000000000uusdt,10000000000000uusdc"
CHAIN_DIR="$HOME/.nibid"
Copy link
Contributor

Choose a reason for hiding this comment

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

The localnet settings are clearly defined and easy to understand. However, consider externalizing sensitive or environment-specific values like MNEMONIC and GENESIS_COINS to a configuration file or environment variables for better security and flexibility.

Comment on lines +1 to 20
#!/bin/bash
set -e

# Set localnet settings
BINARY="nibid"
CHAIN_ID="nibiru-localnet-0"
MNEMONIC="guard cream sadness conduct invite crumble clock pudding hole grit liar hotel maid produce squeeze return argue turtle know drive eight casino maze host"
GENESIS_COINS="10000000000000unibi,10000000000000unusd,10000000000000uusdt,10000000000000uusdc"
CHAIN_DIR="$HOME/.nibid"

echo "CHAIN_DIR: $CHAIN_DIR"
echo "CHAIN_ID: $CHAIN_ID"

# ------------------------------------------------------------
# Set up colored text logging
# ------------------------------------------------------------

# Console log text colour
console_log_text_color() {
red=$(tput setaf 9)
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [17-45]

The function console_log_text_color and the subsequent check for its success are not correctly implemented. The if condition will always evaluate to true because it checks the existence of the function, not its execution result. Consider removing the if condition or fixing the logic to correctly handle the installation of libncurses5-dbg only if needed.

- if [ console_log_text_color ]; then
+ console_log_text_color
+ if [ $? -ne 0 ]; then

Comment on lines 74 to 118
fi
}

echo_info "Parsing flags for the script..."
# Initialize an associative array for feature flags with default values
declare -A features=( ["perp"]=0 ["spot"]=0 )

# Iterate over all arguments to the script
for arg in "$@"; do
if [ "$arg" == "--no-build" ]; then
FLAG_NO_BUILD=true
fi
# enable_feature_flag: Enables feature flags variables if present
enable_feature_flag() {
case $1 in
perp) FLAG_PERP=true ;;
spot) FLAG_SPOT=true ;;
*) echo_error "Unknown feature: $1" ;;
esac
}

# Iterate over flags, handling the cases: "--no-build" and "--features"
while [[ $# -gt 0 ]]; do
case $1 in
--no-build)
FLAG_NO_BUILD=true
shift
;;
--features)
shift # Remove '--features' from arguments
while [[ $# -gt 0 && $1 != --* ]]; do
enable_feature_flag "$1"
shift # Remove the feature name from arguments
done
;;
*) shift ;; # Unknown arg
esac
done

# Check if FLAG_NO_BUILD was set to true
if ! $FLAG_NO_BUILD; then
build_from_source
fi

# Set localnet settings
BINARY="nibid"
CHAIN_ID="nibiru-localnet-0"
RPC_PORT="26657"
GRPC_PORT="9090"
MNEMONIC="guard cream sadness conduct invite crumble clock pudding hole grit liar hotel maid produce squeeze return argue turtle know drive eight casino maze host"
GENESIS_COINS="10000000000000unibi,10000000000000unusd,10000000000000uusdt,10000000000000uusdc"
CHAIN_DIR="$HOME/.nibid"
echo "CHAIN_DIR: $CHAIN_DIR"
echo "CHAIN_ID: $CHAIN_ID"
echo_info "Features flags:"
echo "FLAG_NO_BUILD: $FLAG_NO_BUILD"
echo "FLAG_PERP: $FLAG_PERP"
echo "FLAG_SPOT: $FLAG_SPOT"

SEDOPTION=""
if [[ "$OSTYPE" == "darwin"* ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [52-104]

The flag parsing logic is well-implemented, providing clear handling for --no-build and --features flags. However, it's important to document the expected format and usage of these flags at the top of the script or in a help message function to improve usability.

@@ -92,10 +131,10 @@

# Remove previous data
echo_info "Removing previous chain data from $CHAIN_DIR..."
rm -rf $CHAIN_DIR
rm -rf "$CHAIN_DIR"
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing previous chain data is a critical operation. As previously commented, adding a warning or confirmation prompt to prevent accidental data loss is crucial for user experience and safety.

+ echo "Warning: This will remove all existing chain data in $CHAIN_DIR. Continue? (y/N)"
+ read -r confirm
+ if [[ $confirm =~ ^[Yy]$ ]]; then
+   rm -rf "$CHAIN_DIR"
+ else
+   echo "Operation cancelled."
+   exit 1
+ fi

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.

Suggested change
rm -rf "$CHAIN_DIR"
echo "Warning: This will remove all existing chain data in $CHAIN_DIR. Continue? (y/N)"
read -r confirm
if [[ $confirm =~ ^[Yy]$ ]]; then
rm -rf "$CHAIN_DIR"
else
echo "Operation cancelled."
exit 1
fi

Comment on lines +213 to +215
# if $FLAG_SPOT; then
# # Perform any actions specific to the x/spot feature
# fi
Copy link
Contributor

Choose a reason for hiding this comment

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

The commented-out section for FLAG_SPOT indicates incomplete implementation. If this feature is intended for future development, consider adding a TODO comment to clarify its status and ensure it doesn't get overlooked.

Would you like me to help with implementing the x/spot feature handling or open a GitHub issue to track this task?

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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between eae1a1f and c60d0a5.
Files selected for processing (1)
  • contrib/scripts/feat-perp.sh (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • contrib/scripts/feat-perp.sh

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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between c60d0a5 and 005113a.
Files selected for processing (2)
  • contrib/scripts/chaosnet.sh (1 hunks)
  • contrib/scripts/feat-perp.sh (1 hunks)
Files skipped from review as they are similar to previous changes (2)
  • contrib/scripts/chaosnet.sh
  • contrib/scripts/feat-perp.sh

@Unique-Divine Unique-Divine changed the title refactor(inflation-docs): Document everything + delete unused code fix(inflation-docs): Document everything + delete unused code + fix CI Feb 18, 2024
@Unique-Divine Unique-Divine changed the title fix(inflation-docs): Document everything + delete unused code + fix CI fix(inflation-docs): (1) Document inflation + (2) delete unused code + (3) fix CI Feb 18, 2024
@Unique-Divine Unique-Divine changed the title fix(inflation-docs): (1) Document inflation + (2) delete unused code + (3) fix CI fix(inflation-docs): [1] Document inflation + [2] delete unused code + [3] fix CI Feb 18, 2024
@Unique-Divine Unique-Divine changed the title fix(inflation-docs): [1] Document inflation + [2] delete unused code + [3] fix CI fix(inflation-docs): ① Document inflation + ② delete unused code + ③ fix CI Feb 18, 2024
@Unique-Divine Unique-Divine enabled auto-merge (squash) February 18, 2024 05:53
@Unique-Divine Unique-Divine changed the title fix(inflation-docs): ① Document inflation + ② delete unused code + ③ fix CI fix(inflation-docs): ① Document inflation ② delete unused code ③ fix CI Feb 18, 2024
@Unique-Divine Unique-Divine changed the title fix(inflation-docs): ① Document inflation ② delete unused code ③ fix CI fix(inflation-docs): 1 Document inflation, 2 delete unused code, 3 fix CI Feb 18, 2024
@Unique-Divine Unique-Divine merged commit 688ac5d into main Feb 18, 2024
20 of 29 checks passed
@Unique-Divine Unique-Divine deleted the ud/inf-docs branch February 18, 2024 08:00
k-yang pushed a commit that referenced this pull request Feb 24, 2024
…fix CI (#1799)

* refactor,docs(inflation): Document everything + delete unused code

* changelog

* feat(localnet): make perp and spot modules optional features

* fix(e2e-wasm.yml-ci): Use consistent command runner

* wip! move nibid to path after downlaoding release

* wip! fix syntax error in localnet.sh

* wip!: what version is running?

* wip! try: simplify and use fresh build

* wip! fix param space

* wip!: fix source path in chaosnet and localnet

* wip!: fix source path in chaosnet and localnet

* wip!: fix source path in chaosnet and localnet

* fix(localent.sh): missing prices in oralce genesis

* wait a bit since localnet is fixed now

* fix(deploy-wasm): using wrong binary name

* ci: Runs well but needs a better name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants