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(evm): add oracle precompile #2056

Merged
merged 12 commits into from
Oct 3, 2024
Merged

Conversation

k-yang
Copy link
Member

@k-yang k-yang commented Oct 1, 2024

Purpose / Abstract

Summary by CodeRabbit

  • New Features

    • Introduced an Oracle contract for querying exchange rates within the Ethereum Virtual Machine.
    • Added support for Ethereum transaction types and enhanced inflation parameters.
    • Implemented an in-memory EventBus for real-time event distribution.
    • Added a new custom precompile for the Oracle contract.
  • Bug Fixes

    • Resolved issues with keyring import functions and FunToken state marshalling.
  • Documentation

    • Updated the changelog to reflect new features, improvements, and bug fixes.

@k-yang k-yang requested a review from a team as a code owner October 1, 2024 18:19
Copy link
Contributor

coderabbitai bot commented Oct 1, 2024

Walkthrough

The changes introduce a precompiled contract for querying exchange rates in the Nibiru blockchain's Ethereum Virtual Machine (EVM). Key updates include the addition of an Oracle interface, the implementation of methods for querying exchange rates, and enhancements to existing modules. The changelog documents various feature additions, improvements, and bug fixes, ensuring compatibility with Ethereum standards and updating dependencies.

Changes

File Path Change Summary
CHANGELOG.md Updates on new features, improvements, and bug fixes in the Nibiru blockchain project.
x/evm/embeds/embeds.go Adds embedded JSON for the Oracle contract and updates initialization logic.
x/evm/embeds/artifacts/contracts/IOracle.sol Introduces a new Solidity interface for the Oracle contract with a function to query exchange rates.
x/evm/embeds/contracts/IOracle.sol Defines the Oracle interface and constants for interaction with the oracle functionality.
x/evm/precompile/precompile.go Integrates the new PrecompileOracle into the initialization of precompiled contracts.
x/evm/precompile/oracle.go Implements the precompileOracle contract for querying exchange rates within the EVM.

Assessment against linked issues

Objective Addressed Explanation
Expose "Query/ExchangeRate" RPC method (#[2051])
Implement a precompile for x/oracle (#[2051])

Possibly related PRs

🐇 In the fields where the daisies bloom,
A new Oracle whispers, dispelling the gloom.
With rates to query, oh what a delight,
The EVM dances, all day and all night!
Hopping along, with code so fine,
Nibiru's future, forever will shine! 🌼


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>, please review it.
    • 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 gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @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 using 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.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

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.

@k-yang k-yang self-assigned this Oct 1, 2024
@k-yang k-yang added the x: evm Relates to Nibiru EVM or the EVM Module label Oct 1, 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.

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (13)
x/evm/embeds/contracts/IOracle.sol (1)

13-13: LGTM: Oracle gateway constant.

The ORACLE_GATEWAY constant provides a convenient way to interact with the oracle precompile. This approach is clean and allows for easy mocking in tests.

Consider adding a brief comment explaining the purpose of this constant, like so:

+// ORACLE_GATEWAY provides a convenient interface to interact with the oracle precompile
 IOracle constant ORACLE_GATEWAY = IOracle(ORACLE_PRECOMPILE_ADDRESS);
x/evm/embeds/artifacts/contracts/IOracle.sol/IOracle.json (2)

5-25: ABI definition looks good, but consider optimizing data types.

The ABI correctly defines the queryExchangeRate function as per the PR objectives. However, using string types for both input and output might not be the most gas-efficient approach for on-chain operations.

Consider using fixed-size bytes (e.g., bytes32) for the pair parameter if the trading pairs have a known maximum length. For the return value, you might want to use a struct or multiple return values to provide more structured data (e.g., price, timestamp, decimals).

Example optimization:

function queryExchangeRate(bytes32 pair) external view returns (uint256 price, uint256 timestamp, uint8 decimals);

This change would require updates to the contract implementation and any calling contracts, but it could significantly reduce gas costs and improve data handling.


1-30: Overall, the IOracle interface implementation meets the PR objectives.

This artifact file correctly defines the IOracle interface with the queryExchangeRate function, which aligns with the PR objective of exposing the "Query/ExchangeRate" RPC method to the EVM. The interface provides a foundation for implementing the precompile that will allow access to data feeds published by validator operators within the Oracle Module.

To fully meet the PR objectives, ensure that:

  1. The actual precompile implementation correctly uses this interface.
  2. The precompile is properly integrated into the EVM module.
  3. Adequate tests are written to verify the functionality of the precompile.

Consider the following next steps:

  1. Implement the precompile using this interface.
  2. Add unit and integration tests for the precompile.
  3. Update relevant documentation to explain how to use this new Oracle precompile in smart contracts.
  4. Evaluate the potential for extending this interface to support other types of oracle data (e.g., pseudo-random numbers) as mentioned in the linked issue [evm] Minimalist precompile for x/oracle #2051.
x/evm/precompile/oracle_test.go (3)

15-51: LGTM: Comprehensive test cases for ABI packing failures.

The TestOracle_FailToPackABI function effectively covers various failure scenarios for ABI packing, including wrong argument count, incorrect argument type, and invalid method name. The table-driven test approach is well-structured and easy to maintain.

Consider adding a test case for an empty string as the pair argument to ensure proper handling of edge cases.


53-73: LGTM: Happy path test for queryExchangeRate is implemented.

The TestOracle_HappyPath function effectively tests the successful execution of the queryExchangeRate method. It properly sets up the test environment, sets a price, packs and calls the contract, and checks the response.

Consider the following improvements:

  1. Add error checking for deps.App.OracleKeeper.SetPrice() call.
  2. Include additional assertions to verify the state after the call (e.g., check if the price in the keeper remains unchanged).
  3. Consider adding more test cases with different exchange rate values to ensure consistent behavior across various scenarios.

1-82: Overall, excellent test implementation for the Oracle precompile.

The test file is well-structured, covering both failure scenarios and the happy path for the Oracle precompile. The use of table-driven tests for failure cases and a separate test for the happy path is a good approach.

To further enhance the test suite, consider:

  1. Adding more edge cases to the TestOracle_FailToPackABI function (e.g., empty string for pair).
  2. Expanding the TestOracle_HappyPath function with multiple scenarios and additional assertions.
  3. Implementing a SetupTest method in the OracleSuite to handle common setup operations if needed in future tests.
  4. Adding tests for any other methods that might be added to the Oracle precompile in the future.
x/evm/embeds/embeds.go (1)

43-46: LGTM! Consider adjusting the contract name for consistency.

The addition of the SmartContract_Oracle instance is consistent with the existing pattern for other contracts. However, there's a minor discrepancy in the naming.

Consider updating the Name field to "IOracle.sol" for consistency with the embedded JSON file name:

 SmartContract_Oracle = CompiledEvmContract{
-		Name:      "Oracle.sol",
+		Name:      "IOracle.sol",
 		EmbedJSON: oracleContractJSON,
 }
x/evm/precompile/precompile.go (1)

49-49: Consider adding a comment for PrecompileOracle.

To improve code documentation, consider adding a brief comment explaining the purpose of PrecompileOracle. This will help maintain consistency with the existing comments for other planned precompiles and provide context for future developers.

Example:

// Custom precompiles
for _, precompileSetupFn := range []func(k keepers.PublicKeepers) vm.PrecompiledContract{
    PrecompileFunToken,
    // PrecompileOracle: Implements oracle functionality, allowing access to data feeds
    PrecompileOracle,
}
CHANGELOG.md (3)

Line range hint 12-15: Consider adding more context to the State Machine Breaking changes.

While the changes are listed, it might be helpful to provide brief explanations or reasons for these breaking changes. This can help users understand the impact and necessity of the changes.


Line range hint 112-126: Ensure consistency in formatting for added features.

The formatting of the added features is inconsistent. Some lines start with a hyphen, while others don't. Consider standardizing the format for better readability.


Line range hint 134-138: Consider grouping similar dependency updates.

The repeated updates for bufbuild/buf-setup-action could be consolidated into a single entry with all version changes listed. This would make the changelog more concise.

x/evm/precompile/oracle.go (2)

31-31: Correct typo in comment

In the comment, "it's value can be used to derive an appropriate value..." should be "its value can be used...", as "its" is the possessive form.

Apply this diff to fix the typo:

-	// a contract, it's value can be used to derive an appropriate value for the precompile call.
+	// a contract, its value can be used to derive an appropriate value for the precompile call.

74-74: Use %q for formatting error messages

In the error message, consider using %q instead of "%s" to automatically quote the method name.

Apply this diff:

-		err = fmt.Errorf("invalid method called with name \"%s\"", method.Name)
+		err = fmt.Errorf("invalid method called with name %q", method.Name)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 36533cd and 87744c1.

📒 Files selected for processing (7)
  • CHANGELOG.md (2 hunks)
  • x/evm/embeds/artifacts/contracts/IOracle.sol/IOracle.json (1 hunks)
  • x/evm/embeds/contracts/IOracle.sol (1 hunks)
  • x/evm/embeds/embeds.go (3 hunks)
  • x/evm/precompile/oracle.go (1 hunks)
  • x/evm/precompile/oracle_test.go (1 hunks)
  • x/evm/precompile/precompile.go (1 hunks)
🔇 Additional comments (17)
x/evm/embeds/contracts/IOracle.sol (2)

1-2: LGTM: Appropriate license and Solidity version.

The use of the MIT license is suitable for open-source projects, and the specified Solidity version (>=0.8.19) is recent, ensuring access to the latest language features and security improvements.


11-11: LGTM: Precompile address constant.

The use of a constant for the oracle precompile address is a good practice. The address (0x0000000000000000000000000000000000000801) follows the expected pattern for precompile addresses.

To ensure consistency, please verify that this address matches the implementation in the EVM module. You can use the following command to search for references to this address:

✅ Verification successful

Verified: ORACLE_PRECOMPILE_ADDRESS is consistently used.

The ORACLE_PRECOMPILE_ADDRESS constant (0x0000000000000000000000000000000000000801) is consistently referenced in both IOracle.sol and oracle.go, ensuring proper coordination with the EVM implementation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

rg --type-add 'go:*.go' --type go '0x0000000000000000000000000000000000000801'

Length of output: 203

x/evm/embeds/artifacts/contracts/IOracle.sol/IOracle.json (3)

1-4: LGTM: Metadata section is correctly formatted.

The metadata section of the artifact is well-structured and contains the necessary information about the contract name and source file.


26-27: LGTM: Empty bytecode is correct for an interface.

The empty bytecode ("0x") for both bytecode and deployedBytecode fields is correct, as this is an interface definition without any implementation code.


28-30: LGTM: Empty link references are appropriate.

The empty objects for linkReferences and deployedLinkReferences are correct, as this interface doesn't use any external libraries that would require linking.

x/evm/precompile/oracle_test.go (2)

3-13: LGTM: Imports are well-organized and relevant.

The imports are appropriately chosen for the test file's purpose, including necessary testing, Cosmos SDK, Ethereum, and custom Nibiru packages. The import order follows a good convention.


75-82: LGTM: Test suite is properly defined and executed.

The OracleSuite struct and TestOracleSuite function are correctly implemented using the testify package conventions. This structure allows for proper setup and teardown of test environments if needed in the future.

x/evm/embeds/embeds.go (3)

56-56: LGTM! Oracle contract initialization added correctly.

The addition of SmartContract_Oracle.MustLoad() in the init function ensures that the Oracle contract is properly loaded during initialization, consistent with the handling of other contracts.


Line range hint 22-56: Summary: Oracle precompile successfully integrated

The changes in this file successfully introduce the Oracle contract to the embeds package, aligning with the PR objective of adding an oracle precompile for the EVM. The implementation follows existing patterns and is well-integrated into the current structure.

Key points:

  1. The Oracle contract JSON is properly embedded.
  2. A new CompiledEvmContract instance for the Oracle is created.
  3. The init function is updated to load the Oracle contract.

These changes lay the groundwork for exposing the "Query/ExchangeRate" RPC method through the EVM, as specified in the PR objectives. The next steps would involve implementing the actual precompile logic to interact with the Oracle module and retrieve exchange rate data.


22-23: LGTM! Verify the existence of the Oracle contract artifact.

The addition of the oracleContractJSON variable is consistent with the existing pattern for other contracts.

Please run the following script to verify the existence of the Oracle contract artifact:

✅ Verification successful

Error: Oracle contract artifact not found at the specified path.

Please run the following script to search for IOracle.json within the artifacts directory:


Error: Oracle contract artifact not found at the specified path.

The artifacts/ directory does not exist. Let's search the entire codebase to locate the IOracle.json artifact.

Please run the following script:


Verified: Oracle contract artifact is correctly embedded.

The oracleContractJSON variable successfully embeds the IOracle.json artifact at x/evm/embeds/artifacts/contracts/IOracle.sol/IOracle.json.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the existence of the Oracle contract artifact.

# Test: Check if the IOracle.json file exists
if [ -f "artifacts/contracts/IOracle.sol/IOracle.json" ]; then
    echo "Oracle contract artifact exists."
else
    echo "Error: Oracle contract artifact not found."
    exit 1
fi

Length of output: 161


Script:

#!/bin/bash
# Description: Search for IOracle.json within artifacts directory.

fd "IOracle.json" artifacts/ --exec echo "Found: {}"

Length of output: 150


Script:

#!/bin/bash
# Description: Search for IOracle.json in the entire codebase.

fd "IOracle.json" --type f --exec echo "Found: {}"

Length of output: 117

x/evm/precompile/precompile.go (1)

49-49: LGTM! Verify PrecompileOracle implementation.

The addition of PrecompileOracle to the list of custom precompiles aligns well with the PR objectives. It's a minimal change that extends the EVM functionality to include the Oracle Module precompile.

To ensure completeness:

  1. Confirm that PrecompileOracle is properly implemented in a separate file.
  2. Verify that it exposes the "Query/ExchangeRate" RPC method as mentioned in the objectives.

Let's verify the implementation of PrecompileOracle:

✅ Verification successful

PrecompileOracle Implementation Verified.

The PrecompileOracle is properly implemented in x/evm/precompile/oracle.go, and the "Query/ExchangeRate" RPC method is correctly exposed in the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the implementation of PrecompileOracle

# Test 1: Check if PrecompileOracle is implemented
echo "Checking for PrecompileOracle implementation:"
rg --type go "func PrecompileOracle" -A 10

# Test 2: Verify if Query/ExchangeRate method is exposed
echo "Checking for Query/ExchangeRate method:"
rg --type go "Query/ExchangeRate"

Length of output: 2095

CHANGELOG.md (5)

Line range hint 1-4: LGTM: Proper changelog header and format declaration.

The header of the changelog follows the recommended format, clearly stating its purpose and referencing the Keep a Changelog and Semantic Versioning standards. This is good practice for maintaining a clear and standardized changelog.


Line range hint 6-10: LGTM: Clear structure for unreleased changes.

The "Unreleased" section is properly placed at the top of the changelog, following best practices. This allows for easy tracking of upcoming changes.


Line range hint 128-132: LGTM: Clear documentation of dependencies.

The Dependencies section is well-organized and clearly lists all the version bumps. This is helpful for users to understand the project's external dependencies and their versions.


Line range hint 140-1730: LGTM: Comprehensive historical changelog.

The historical changelog is well-maintained, providing a detailed history of changes across multiple versions. This is excellent for tracking the project's evolution over time.


Line range hint 1-1730: Overall, the CHANGELOG.md is well-maintained and informative.

The changelog provides a comprehensive history of changes to the Nibiru project, following the Keep a Changelog format and Semantic Versioning. It's well-organized, with clear sections for different types of changes and a proper "Unreleased" section at the top.

Some minor improvements could be made:

  1. Adding more context to some of the breaking changes.
  2. Ensuring consistent formatting for feature lists.
  3. Considering consolidation of repeated dependency updates.

These small adjustments would further enhance the already high-quality changelog.

x/evm/precompile/oracle.go (1)

62-63: Verify DecomposeInput function is properly imported or defined

The function DecomposeInput is used but its definition or import is not shown in the code. Ensure that DecomposeInput is properly defined or imported to avoid undefined function errors.

Run the following script to check for the definition of DecomposeInput in the codebase:

✅ Verification successful

DecomposeInput is properly defined within the package. No import issues detected.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for the definition of DecomposeInput in the codebase.

# Expected: Function definition of DecomposeInput should be found.
rg 'func DecomposeInput' --type go

Length of output: 88


Script:

#!/bin/bash
# Description: Retrieve package declarations of oracle.go and precompile.go.

# Get package declaration of oracle.go
echo "Package declaration of oracle.go:"
rg '^package ' x/evm/precompile/oracle.go

# Get package declaration of precompile.go
echo "Package declaration of precompile.go:"
rg '^package ' x/evm/precompile/precompile.go

# Retrieve import statements from oracle.go
echo "Import statements in oracle.go:"
rg '^import ' x/evm/precompile/oracle.go

Length of output: 416

x/evm/embeds/contracts/IOracle.sol Outdated Show resolved Hide resolved
x/evm/precompile/oracle.go Outdated Show resolved Hide resolved
x/evm/precompile/oracle.go Show resolved Hide resolved
@NibiruChain NibiruChain deleted a comment from coderabbitai bot Oct 2, 2024
Copy link
Member

@Unique-Divine Unique-Divine left a comment

Choose a reason for hiding this comment

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

LGTM, added a few small changes

x/evm/embeds/contracts/IOracle.sol Outdated Show resolved Hide resolved
x/evm/embeds/contracts/IOracle.sol Outdated Show resolved Hide resolved
x/evm/precompile/oracle.go Outdated Show resolved Hide resolved
x/evm/precompile/oracle.go Outdated Show resolved Hide resolved
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

🧹 Outside diff range and nitpick comments (7)
x/evm/precompile/precompile.go (1)

Line range hint 62-70: Note: Consider future enhancements mentioned in TODO comments.

The file contains TODO comments for potential future implementations of precompiled contracts for IBC transfer and staking. While these are not directly related to the current PR objectives, they provide valuable information for future enhancements. Consider creating separate issues to track these potential features if they align with the project's roadmap.

CHANGELOG.md (6)

Line range hint 33-35: Consider providing more context for the oracle precompile addition.

The addition of the oracle precompile for EVM is a significant feature. It would be helpful to briefly explain its purpose and potential impact on the system.

Consider adding a brief explanation, such as:
"Added an oracle precompile for the Ethereum Virtual Machine (EVM), allowing EVM contracts to access oracle data from the Nibiru blockchain."


Line range hint 36-37: Clarify the impact of removing CosmosMsg::Custom bindings.

The removal of CosmosMsg::Custom bindings is marked as a breaking change. It would be beneficial to briefly explain the implications of this removal for users or developers.

Consider adding a note about potential migration steps or alternatives, if applicable.


112-113: Provide more context for the EVM tx indexer service.

The addition of the EVM tx indexer service is mentioned, but its purpose and benefits are not clear. It would be helpful to provide a brief explanation of what this service does and why it's important.

Consider adding a brief description, such as:
"Implemented an EVM transaction indexer service to improve query performance and enable more efficient tracking of EVM transactions on the Nibiru blockchain."


122-123: Clarify the purpose of the debug_traceCall method.

The implementation of the debug_traceCall method is mentioned, but its purpose and use case are not clear. It would be helpful to provide a brief explanation of what this method does and why it's useful.

Consider adding a brief description, such as:
"Implemented the debug_traceCall method to enable detailed tracing and debugging of EVM contract calls, helping developers troubleshoot and optimize their smart contracts."


Line range hint 131-132: Consider elaborating on the impact of migrating Go-sdk.

The migration of Go-sdk into the Nibiru blockchain repo is mentioned, but its implications are not clear. It would be helpful to briefly explain why this change was made and how it benefits the project or developers.

Consider adding a brief explanation, such as:
"Migrated the Go-sdk into the Nibiru blockchain repo to streamline development, improve maintainability, and ensure tighter integration between the SDK and the core blockchain code."


Line range hint 133-134: Highlight the significance of the dependency updates.

The update to cometbft v0.37.5, cosmos-sdk v0.47.11, and proto-builder v0.14.0 is mentioned, but the impact of these updates is not clear. It would be helpful to briefly explain any notable improvements or fixes that come with these updates.

Consider adding a brief note about key improvements or fixes in these updates, if applicable. For example:
"Updated to cometbft v0.37.5, cosmos-sdk v0.47.11, and proto-builder v0.14.0, bringing performance improvements and bug fixes from upstream projects."

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 87744c1 and 9ef7dd9.

📒 Files selected for processing (3)
  • CHANGELOG.md (2 hunks)
  • x/evm/embeds/embeds.go (3 hunks)
  • x/evm/precompile/precompile.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • x/evm/embeds/embeds.go
🔇 Additional comments (2)
x/evm/precompile/precompile.go (1)

52-53: LGTM: PrecompileOracle addition aligns with PR objectives.

The addition of PrecompileOracle to the list of custom precompiles is correct and aligns with the PR objectives to implement a precompile for accessing oracle data feeds. The implementation follows the existing pattern for adding custom precompiles.

To ensure the PrecompileOracle is properly implemented, let's verify its existence and basic structure:

CHANGELOG.md (1)

Line range hint 31-127: Overall, the Unreleased section provides a comprehensive overview of recent changes.

The changelog is well-structured and follows the Keep a Changelog format. It clearly separates breaking changes, new features, and improvements. Here are some key observations:

  1. State Machine Breaking changes:

    • Significant refactoring and improvements in various modules (oracle, perp, spot, etc.).
    • Addition of new modules like devgas and tokenfactory.
    • Removal of some modules and features.
  2. Features:

    • Addition of oracle precompile for EVM.
    • Implementation of new queries and commands.
    • Improvements in existing functionalities.
  3. Non-breaking Improvements:

    • Dependency updates.
    • Code refactoring and cleanup.
    • Test improvements.

The changelog provides good traceability with links to pull requests for each change.

Copy link

codecov bot commented Oct 2, 2024

Codecov Report

Attention: Patch coverage is 53.06122% with 23 lines in your changes missing coverage. Please review.

Project coverage is 65.03%. Comparing base (629aea3) to head (51f49fe).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
x/evm/precompile/oracle.go 51.06% 15 Missing and 8 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2056      +/-   ##
==========================================
- Coverage   65.13%   65.03%   -0.11%     
==========================================
  Files         269      270       +1     
  Lines       17007    17078      +71     
==========================================
+ Hits        11078    11106      +28     
- Misses       5000     5028      +28     
- Partials      929      944      +15     
Files with missing lines Coverage Δ
x/evm/embeds/embeds.go 65.21% <100.00%> (+1.58%) ⬆️
x/evm/precompile/precompile.go 67.39% <100.00%> (+0.72%) ⬆️
x/evm/precompile/oracle.go 51.06% <51.06%> (ø)

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: 4

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 9ef7dd9 and ad136c7.

📒 Files selected for processing (3)
  • x/evm/embeds/artifacts/contracts/IOracle.sol/IOracle.json (1 hunks)
  • x/evm/embeds/contracts/IOracle.sol (1 hunks)
  • x/evm/precompile/oracle.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • x/evm/embeds/artifacts/contracts/IOracle.sol/IOracle.json
  • x/evm/embeds/contracts/IOracle.sol
🔇 Additional comments (1)
x/evm/precompile/oracle.go (1)

115-117: Correct expected argument count in error message

The error message states "expected 3 arguments but got %d", but only one argument is expected in this function.

Comment on lines +29 to +33
func (p precompileOracle) RequiredGas(input []byte) (gasPrice uint64) {
// Since [gethparams.TxGas] is the cost per (Ethereum) transaction that does not create
// a contract, it's value can be used to derive an appropriate value for the precompile call.
return gethparams.TxGas
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider refining the gas calculation in RequiredGas method

The RequiredGas method currently returns a constant gethparams.TxGas, which is a fixed cost for transactions that do not create contracts. To ensure accurate gas estimation for this precompile, consider calculating the gas based on the actual computational complexity of the queryExchangeRate method.

Comment on lines +120 to +124
pair, ok := args[0].(string)
if !ok {
err = ErrArgTypeValidation("string pair", args[0])
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Validate argument type more robustly

While the code checks if args[0] is a string, additional validation may be helpful to ensure that the string conforms to expected formats (e.g., correct asset pair format). This can prevent potential errors downstream.

Consider adding a validation step:

if !asset.IsValidPairFormat(pair) {
    err = fmt.Errorf("invalid asset pair format: %s", pair)
    return
}

Assuming asset.IsValidPairFormat is a function that validates the format of the asset pair string.

Comment on lines +95 to +106
if err != nil {
return nil, err
}
assetPair, err := asset.TryNewPair(pair)
if err != nil {
return nil, err
}

price, err := p.oracleKeeper.GetExchangeRate(ctx, assetPair)
if err != nil {
return nil, err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Wrap errors with context in queryExchangeRate method

To improve debuggability, consider wrapping the errors returned from called functions with additional context. This provides clearer information about where and why a failure occurred.

Apply this diff to wrap the errors:

      pair, err := p.decomposeQueryExchangeRateArgs(args)
      if err != nil {
-         return nil, err
+         return nil, fmt.Errorf("failed to decompose arguments: %w", err)
      }

      assetPair, err := asset.TryNewPair(pair)
      if err != nil {
-         return nil, err
+         return nil, fmt.Errorf("invalid asset pair \"%s\": %w", pair, err)
      }

      price, err := p.oracleKeeper.GetExchangeRate(ctx, assetPair)
      if err != nil {
-         return nil, err
+         return nil, fmt.Errorf("failed to get exchange rate for \"%s\": %w", assetPair, 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
if err != nil {
return nil, err
}
assetPair, err := asset.TryNewPair(pair)
if err != nil {
return nil, err
}
price, err := p.oracleKeeper.GetExchangeRate(ctx, assetPair)
if err != nil {
return nil, err
}
if err != nil {
return nil, fmt.Errorf("failed to decompose arguments: %w", err)
}
assetPair, err := asset.TryNewPair(pair)
if err != nil {
return nil, fmt.Errorf("invalid asset pair \"%s\": %w", pair, err)
}
price, err := p.oracleKeeper.GetExchangeRate(ctx, assetPair)
if err != nil {
return nil, fmt.Errorf("failed to get exchange rate for \"%s\": %w", assetPair, err)
}

Comment on lines +71 to +72
err = fmt.Errorf("invalid method called with name \"%s\"", method.Name)
return
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance the error message for unsupported methods

The error message "invalid method called with name \"%s\"" could be improved to provide more guidance to the user. Consider listing the supported methods in the error to aid in debugging.

Apply this diff to enhance the error message:

-		err = fmt.Errorf("invalid method called with name \"%s\"", method.Name)
+		err = fmt.Errorf("invalid method \"%s\" called. Supported methods: \"%s\"", method.Name, OracleMethod_QueryExchangeRate)
📝 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
err = fmt.Errorf("invalid method called with name \"%s\"", method.Name)
return
err = fmt.Errorf("invalid method \"%s\" called. Supported methods: \"%s\"", method.Name, OracleMethod_QueryExchangeRate)
return

@Unique-Divine Unique-Divine enabled auto-merge (squash) October 3, 2024 12:34
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

🧹 Outside diff range and nitpick comments (5)
CHANGELOG.md (5)

Line range hint 32-33: Removal of CosmosMsg::Custom bindings may break existing integrations.

The removal of CosmosMsg::Custom bindings in the app-wasmext could potentially break existing smart contracts or integrations that rely on these custom messages. It's important to ensure that all affected contracts are updated and that proper migration paths are provided for users.

Consider providing a detailed migration guide for affected contracts and users.


Line range hint 34-35: New inflation parameters collection may require data migration.

The introduction of inflation parameters as a collection could require data migration from the previous format. Ensure that there's a clear upgrade path for existing chain state.

Verify that an upgrade handler is in place to migrate existing inflation data to the new collection format.


Line range hint 36-37: Removal of proto stringer may affect logging and debugging.

Removing the proto stringer as per Cosmos SDK migration guidelines could impact logging and debugging processes that rely on these string representations.

Ensure that alternative logging mechanisms are in place and that this change doesn't negatively impact observability.


Line range hint 137-138: Changes to collateral handling and volume-based rebates may impact user economics.

Making the collateral denom a stateful collections.Item and adding volume-based rebates could significantly change the economic model for users.

  1. Analyze the potential economic impacts of these changes on different user segments.
  2. Ensure that the new rebate system is fair and doesn't introduce unintended incentives.
  3. Consider running economic simulations to validate the effects of these changes.

Also applies to: 139-140


Line range hint 165-166: Migration of Go-sdk into the Nibiru blockchain repo may affect external tools.

The migration of the Go-sdk into the main repository could impact external tools and integrations that depend on the separate SDK.

  1. Ensure that clear migration instructions are provided for users of the external Go-sdk.
  2. Update all relevant documentation to reflect this change.
  3. Consider maintaining backwards compatibility for a transition period if possible.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 2584155 and 51f49fe.

📒 Files selected for processing (1)
  • CHANGELOG.md (2 hunks)
🔇 Additional comments (8)
CHANGELOG.md (8)

Line range hint 38-39: Fix to keyring import functions may alter existing behavior.

The fix to keyring import functions could potentially change the behavior of existing key management processes.

It's crucial to verify that this fix doesn't introduce any regressions or unexpected behavior changes in key management.


Line range hint 42-43: Significant EVM updates may require thorough testing and auditing.

The introduction of Ethereum transaction types, conversion functions, and keeper logic for managing Ethereum accounts represents a major update to the EVM implementation. These changes could have significant implications for smart contract compatibility, security, and overall system behavior.

  1. Ensure comprehensive testing is performed, including compatibility tests with existing Ethereum tools and contracts.
  2. Consider conducting a security audit focused on these EVM changes.
  3. Verify that these changes maintain compatibility with the Ethereum ecosystem while adhering to Nibiru's specific requirements.

Also applies to: 44-45, 46-47


Line range hint 48-49: In-memory EventBus implementation may have performance implications.

The introduction of an in-memory EventBus for real-time event distribution could impact system performance and scalability.

Conduct performance testing to ensure that the in-memory EventBus can handle the expected load and doesn't introduce bottlenecks in high-throughput scenarios.


Line range hint 133-134: Perp module changes may affect existing positions and market behavior.

The introduction of market closing, settlement price computation, and position settling features in the perp module could have significant impacts on existing user positions and overall market dynamics.

  1. Ensure that these changes don't adversely affect existing user positions.
  2. Verify that the settlement price computation is accurate and resistant to manipulation.
  3. Consider providing clear documentation and user guides for these new features.

Also applies to: 135-136


Line range hint 141-142: New market manipulation features require careful consideration.

The introduction of MsgShiftPegMultiplier and MsgShiftSwapInvariant, along with the ability to donate to the perp fund, could potentially be used for market manipulation if not properly controlled.

  1. Implement strict access controls for these powerful features.
  2. Consider adding monitoring and alerting for unusual activity related to these new messages.
  3. Conduct thorough testing to ensure these features cannot be exploited to manipulate markets unfairly.

Also applies to: 143-144, 145-146


Line range hint 167-168: Significant dependency updates may introduce subtle changes.

The updates to cometbft, cosmos-sdk, and proto-builder could introduce subtle changes in behavior or compatibility.

  1. Conduct thorough regression testing to ensure these updates don't introduce unexpected behavior changes.
  2. Review the changelogs of these dependencies to understand any potential impacts on the Nibiru system.
  3. Update any internal tools or scripts that may be affected by these dependency changes.

Line range hint 173-190: Multiple dependency updates require careful integration testing.

The large number of dependency updates, including critical components like gRPC, protobuf, and various Cosmos ecosystem packages, necessitates comprehensive integration testing.

  1. Conduct end-to-end testing of all major system flows to ensure compatibility with the updated dependencies.
  2. Pay special attention to any changes in cryptographic libraries (e.g., github.com/holiman/uint256) to ensure they don't introduce security vulnerabilities.
  3. Verify that all build and development tools are compatible with these updated dependencies.

129-130: ⚠️ Potential issue

Addition of oracle precompile requires security review.

The introduction of an oracle precompile in the EVM is a significant feature that could have security implications if not implemented correctly.

  1. Conduct a thorough security review of the oracle precompile implementation.
  2. Ensure that proper access controls and rate limiting are in place to prevent abuse.
  3. Verify that the oracle data cannot be manipulated by malicious actors through the EVM interface.

@Unique-Divine Unique-Divine merged commit 585ebe7 into main Oct 3, 2024
14 checks passed
@Unique-Divine Unique-Divine deleted the feat/evm/oracle-precompile branch October 3, 2024 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x: evm Relates to Nibiru EVM or the EVM Module
Projects
Status: ✅ Completed
Development

Successfully merging this pull request may close these issues.

[evm] Minimalist precompile for x/oracle
3 participants