feat(evm-oracle): add Solidity contract that we can use to expose the Nibiru Oracle in the ChainLink interface #2149
Conversation
… Nibiru Oracle in the ChainLink interface
WalkthroughThis pull request introduces a comprehensive enhancement to the Nibiru Oracle system by adding Solidity contracts and ABIs for ChainLink-like interactions. The changes include creating a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant NibiruOracleChainLinkLike
participant OraclePrecompile
participant OracleKeeper
Client->>NibiruOracleChainLinkLike: latestRoundData()
NibiruOracleChainLinkLike->>OraclePrecompile: chainLinkLatestRoundData(pair)
OraclePrecompile->>OracleKeeper: Query exchange rate
OracleKeeper-->>OraclePrecompile: Return rate and metadata
OraclePrecompile-->>NibiruOracleChainLinkLike: Return round data
NibiruOracleChainLinkLike-->>Client: Return roundId, answer, timestamps
Assessment against linked issues
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2149 +/- ##
=======================================
Coverage 65.21% 65.21%
=======================================
Files 277 277
Lines 22208 22234 +26
=======================================
+ Hits 14482 14499 +17
- Misses 6736 6742 +6
- Partials 990 993 +3
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
x/evm/embeds/gen-abi/main.go (2)
17-26: Consider using go-git for repository root detection.The current implementation using
exec.Commandis fragile and may fail in certain environments. Consider usinggo-gitto reliably detect the repository root:-func findRootPath() (string, error) { - rootPathBz, err := exec.Command("go", "list", "-m", "-f", "{{.Dir}}").Output() - if err != nil { - return "", err - } - rootPath := strings.Trim(string(rootPathBz), "\n") - return rootPath, nil -} +import "github.com/go-git/go-git/v5" + +func findRootPath() (string, error) { + wd, err := os.Getwd() + if err != nil { + return "", err + } + r, err := git.PlainOpenWithOptions(wd, &git.PlainOpenOptions{ + DetectDotGit: true, + }) + if err != nil { + return "", err + } + wt, err := r.Worktree() + if err != nil { + return "", err + } + return wt.Filesystem.Root(), nil +}
49-53: Add error handling for malformed filenames.The file filtering logic should handle potential edge cases with malformed filenames.
- if !info.IsDir() && - filepath.Ext(path) == ".json" && // .json files that - !strings.Contains(path, ".dbg") && // are NOT "dbg" files - !strings.HasPrefix(info.Name(), "Test") { // are NOT for tests + filename := info.Name() + if info.IsDir() || + !strings.HasSuffix(strings.ToLower(filename), ".json") || + strings.Contains(filename, ".dbg") || + strings.HasPrefix(filename, "Test") { + return nil + }x/evm/precompile/oracle.go (1)
178-178: Consider handling potential timestamp overflow.The division of
BlockTimestampMsby 1000 could potentially lose precision. Consider using a safer conversion method.- timestampSeconds := big.NewInt(priceAtBlock.BlockTimestampMs / 1000) + timestampSeconds := new(big.Int).Div( + new(big.Int).SetUint64(priceAtBlock.BlockTimestampMs), + big.NewInt(1000), + )x/evm/embeds/package.json (1)
4-4: Consider semantic versioning for the initial release.Since this is a new package with breaking changes from the previous implementation, consider starting with version 1.0.0 instead of 0.0.2.
- "version": "0.0.2", + "version": "1.0.0",evm-e2e/package.json (1)
30-30: Pin the package version for stabilitySince
@nibiruchain/solidityis in early development (v0.0.2), using^could allow breaking changes in minor versions. Consider pinning the exact version:- "@nibiruchain/solidity": "^0.0.2", + "@nibiruchain/solidity": "0.0.2",x/evm/embeds/abi/IOracle.json (2)
3-9: Consider using bytes32 for pair parameterUsing
stringfor thepairparameter is gas-intensive. ChainLink typically usesbytes32for asset pair identifiers. Consider:- "internalType": "string", - "name": "pair", - "type": "string" + "internalType": "bytes32", + "name": "pair", + "type": "bytes32"
1-70: Consider adding events for price updatesThe interface could benefit from events to allow monitoring of price updates. Consider adding:
event PriceUpdated( bytes32 indexed pair, uint256 price, uint256 timestamp );x/evm/embeds/README.md (1)
1-14: Enhance documentation with usage examples and API detailsConsider adding:
- Usage examples showing how to interact with the Oracle
- API documentation for each function
- Version compatibility information (Solidity, ChainLink, etc.)
Example structure:
## Usage ```solidity // Import the contract import "@nibiruchain/solidity/contracts/NibiruOracleChainLinkLike.sol"; // Initialize NibiruOracleChainLinkLike oracle = new NibiruOracleChainLinkLike("NIBI/USD"); // Get latest price ( uint80 roundId, int256 answer, uint256 startedAt, uint256 updatedAt, uint80 answeredInRound ) = oracle.latestRoundData();API Reference
chainLinkLatestRoundData
Returns the latest round data in ChainLink format...
</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 8e477cb03fd9b2853456d0e7775aa157165dce0e and c3e38bd330a757c6e7ac4e9fe3d4c9aae4c41636. </details> <details> <summary>⛔ Files ignored due to path filters (2)</summary> * `evm-e2e/package-lock.json` is excluded by `!**/package-lock.json` * `x/evm/embeds/package-lock.json` is excluded by `!**/package-lock.json` </details> <details> <summary>📒 Files selected for processing (20)</summary> * `CHANGELOG.md` (1 hunks) * `evm-e2e/package.json` (1 hunks) * `justfile` (1 hunks) * `x/evm/embeds/README.md` (1 hunks) * `x/evm/embeds/abi/ChainLinkAggregatorV3Interface.json` (1 hunks) * `x/evm/embeds/abi/ERC20Minter.json` (1 hunks) * `x/evm/embeds/abi/IFunToken.json` (1 hunks) * `x/evm/embeds/abi/INibiruEvm.json` (1 hunks) * `x/evm/embeds/abi/IOracle.json` (1 hunks) * `x/evm/embeds/abi/IWasm.json` (1 hunks) * `x/evm/embeds/abi/NibiruOracleChainLinkLike.json` (1 hunks) * `x/evm/embeds/artifacts/contracts/IOracle.sol/ChainLinkAggregatorV3Interface.json` (1 hunks) * `x/evm/embeds/artifacts/contracts/IOracle.sol/IOracle.json` (1 hunks) * `x/evm/embeds/artifacts/contracts/NibiruOracleChainLinkLike.sol/NibiruOracleChainLinkLike.json` (1 hunks) * `x/evm/embeds/contracts/IOracle.sol` (1 hunks) * `x/evm/embeds/contracts/NibiruOracleChainLinkLike.sol` (1 hunks) * `x/evm/embeds/gen-abi/main.go` (1 hunks) * `x/evm/embeds/package.json` (1 hunks) * `x/evm/precompile/oracle.go` (5 hunks) * `x/evm/precompile/oracle_test.go` (1 hunks) </details> <details> <summary>✅ Files skipped from review due to trivial changes (2)</summary> * x/evm/embeds/artifacts/contracts/IOracle.sol/ChainLinkAggregatorV3Interface.json * x/evm/embeds/artifacts/contracts/NibiruOracleChainLinkLike.sol/NibiruOracleChainLinkLike.json </details> <details> <summary>👮 Files not reviewed due to content moderation or server errors (5)</summary> * x/evm/embeds/abi/NibiruOracleChainLinkLike.json * x/evm/embeds/abi/IWasm.json * x/evm/embeds/abi/IFunToken.json * x/evm/embeds/abi/ERC20Minter.json * CHANGELOG.md </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms (1)</summary> * GitHub Check: integration-tests </details> <details> <summary>🔇 Additional comments (13)</summary><blockquote> <details> <summary>x/evm/embeds/contracts/IOracle.sol (3)</summary> `21-32`: **LGTM! Function signature matches ChainLink's interface.** The new `chainLinkLatestRoundData` function correctly implements the ChainLink interface signature, enabling compatibility with existing ChainLink consumers. --- `39-72`: **LGTM! Interface matches ChainLink's standard.** The `ChainLinkAggregatorV3Interface` is correctly implemented with proper documentation and source attribution. --- `37-37`: **Verify impact of constant rename.** The constant has been renamed from `ORACLE_GATEWAY` to `NIBIRU_ORACLE`. While the new name better reflects its purpose, this is a breaking change. </details> <details> <summary>justfile (1)</summary> `42-43`: **LGTM! Build step properly integrated.** The ABI generation step is correctly integrated into the existing build process. </details> <details> <summary>x/evm/precompile/oracle.go (3)</summary> `36-37`: **LGTM! Clear method naming for ChainLink compatibility.** The new constant `OracleMethod_chainLinkLatestRoundData` follows the existing naming convention and clearly indicates its purpose. --- `56-59`: **LGTM! Clean integration of ChainLink interface.** The switch case addition is well-documented and properly integrated into the existing flow. --- `176-179`: _:warning: Potential issue_ **Verify the `answeredInRound` implementation.** The hardcoded value `420` for `answeredInRound` doesn't follow ChainLink's specification. According to ChainLink's documentation, this should represent the round ID in which the answer was computed. Apply this diff to fix the implementation: ```diff - answeredInRound := big.NewInt(420) // for no reason in particular / unused + answeredInRound := roundId // should be the same as roundId per ChainLink's specx/evm/precompile/oracle_test.go (2)
142-146: LGTM! Thorough verification of exchange rate conversion.The test properly verifies the 18 decimal precision required by ChainLink's interface.
147-148: LGTM! Proper timestamp verification.The test correctly verifies both
startedAtandupdatedAttimestamps.x/evm/embeds/abi/INibiruEvm.json (1)
1-21: LGTM! Well-structured event definition.The event definition follows Solidity best practices with appropriate parameter indexing for efficient event filtering.
x/evm/embeds/package.json (1)
7-9: LGTM! Appropriate file patterns for package distribution.The file patterns correctly include the necessary Solidity contracts and ABIs while excluding test files.
x/evm/embeds/artifacts/contracts/IOracle.sol/IOracle.json (1)
Line range hint
1-44: Implementation looks correctThe contract artifact correctly defines the interface with empty bytecode as expected.
x/evm/embeds/abi/ChainLinkAggregatorV3Interface.json (1)
1-113: Add standard error definitionsConsider adding standard ChainLink error definitions for consistency:
error StalePrice(uint256 updatedAt); error InvalidRoundId(uint80 roundId);Also verify the implementation handles these cases:
- Stale prices (when
updatedAtis too old)- Invalid round IDs
- Zero or negative prices
| constructor(string memory _pair, uint8 _dec) { | ||
| require(_dec <= 18, "Decimals cannot exceed 18"); | ||
| pair = _pair; | ||
| _decimals = _dec; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for empty pair string.
The constructor should validate that the _pair parameter is not empty.
constructor(string memory _pair, uint8 _dec) {
require(_dec <= 18, "Decimals cannot exceed 18");
+ require(bytes(_pair).length > 0, "Pair string cannot be empty");
pair = _pair;
_decimals = _dec;
}📝 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.
| constructor(string memory _pair, uint8 _dec) { | |
| require(_dec <= 18, "Decimals cannot exceed 18"); | |
| pair = _pair; | |
| _decimals = _dec; | |
| } | |
| constructor(string memory _pair, uint8 _dec) { | |
| require(_dec <= 18, "Decimals cannot exceed 18"); | |
| require(bytes(_pair).length > 0, "Pair string cannot be empty"); | |
| pair = _pair; | |
| _decimals = _dec; | |
| } |
| function scaleAnswerToDecimals( | ||
| int256 answer18Dec | ||
| ) internal view returns (int256 answer) { | ||
| // Default answers are in 18 decimals. | ||
| // Scale down to the decimals specified in the constructor. | ||
| uint8 pow10 = 18 - _decimals; | ||
| return answer18Dec / int256(10 ** pow10); | ||
| } |
There was a problem hiding this comment.
Prevent potential overflow in decimal scaling.
The decimal scaling operation should check for potential overflow conditions.
function scaleAnswerToDecimals(
int256 answer18Dec
) internal view returns (int256 answer) {
// Default answers are in 18 decimals.
// Scale down to the decimals specified in the constructor.
uint8 pow10 = 18 - _decimals;
+ require(
+ answer18Dec > type(int256).min / (10 ** pow10) &&
+ answer18Dec < type(int256).max / (10 ** pow10),
+ "Decimal scaling would overflow"
+ );
return answer18Dec / int256(10 ** pow10);
}📝 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.
| function scaleAnswerToDecimals( | |
| int256 answer18Dec | |
| ) internal view returns (int256 answer) { | |
| // Default answers are in 18 decimals. | |
| // Scale down to the decimals specified in the constructor. | |
| uint8 pow10 = 18 - _decimals; | |
| return answer18Dec / int256(10 ** pow10); | |
| } | |
| function scaleAnswerToDecimals( | |
| int256 answer18Dec | |
| ) internal view returns (int256 answer) { | |
| // Default answers are in 18 decimals. | |
| // Scale down to the decimals specified in the constructor. | |
| uint8 pow10 = 18 - _decimals; | |
| require( | |
| answer18Dec > type(int256).min / (10 ** pow10) && | |
| answer18Dec < type(int256).max / (10 ** pow10), | |
| "Decimal scaling would overflow" | |
| ); | |
| return answer18Dec / int256(10 ** pow10); | |
| } |
| function getRoundData( | ||
| uint80 | ||
| ) | ||
| external | ||
| view | ||
| returns ( | ||
| uint80 roundId, | ||
| int256 answer, | ||
| uint256 startedAt, | ||
| uint256 updatedAt, | ||
| uint80 answeredInRound | ||
| ) | ||
| { | ||
| return latestRoundData(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document unsupported historical data retrieval.
The getRoundData function should revert when attempting to retrieve historical data instead of silently returning the latest data.
function getRoundData(
- uint80
+ uint80 _roundId
)
external
view
returns (
uint80 roundId,
int256 answer,
uint256 startedAt,
uint256 updatedAt,
uint80 answeredInRound
)
{
+ (roundId, answer, startedAt, updatedAt, answeredInRound) = latestRoundData();
+ require(
+ _roundId == roundId,
+ "Historical data retrieval not supported"
+ );
- return latestRoundData();
return (roundId, answer, startedAt, updatedAt, answeredInRound);
}📝 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.
| function getRoundData( | |
| uint80 | |
| ) | |
| external | |
| view | |
| returns ( | |
| uint80 roundId, | |
| int256 answer, | |
| uint256 startedAt, | |
| uint256 updatedAt, | |
| uint80 answeredInRound | |
| ) | |
| { | |
| return latestRoundData(); | |
| } | |
| function getRoundData( | |
| uint80 _roundId | |
| ) | |
| external | |
| view | |
| returns ( | |
| uint80 roundId, | |
| int256 answer, | |
| uint256 startedAt, | |
| uint256 updatedAt, | |
| uint80 answeredInRound | |
| ) | |
| { | |
| (roundId, answer, startedAt, updatedAt, answeredInRound) = latestRoundData(); | |
| require( | |
| _roundId == roundId, | |
| "Historical data retrieval not supported" | |
| ); | |
| return (roundId, answer, startedAt, updatedAt, answeredInRound); | |
| } |
| // answeredInRound | ||
| s.Equal(out[4].(*big.Int), big.NewInt(420)) |
There was a problem hiding this comment.
Update test to match the corrected implementation.
The test expects answeredInRound to be 420, but this should match the roundId value per ChainLink's specification.
Apply this diff to fix the test:
- s.Equal(out[4].(*big.Int), big.NewInt(420))
+ s.Equal(out[4].(*big.Int), out[0].(*big.Int)) // answeredInRound should match roundId📝 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.
| // answeredInRound | |
| s.Equal(out[4].(*big.Int), big.NewInt(420)) | |
| // answeredInRound | |
| s.Equal(out[4].(*big.Int), out[0].(*big.Int)) // answeredInRound should match roundId |
Purpose / Abstract
[STILL DRAFT: Will add more context]
ChainLink interface.
@nibiruchain/soliditypackage.Summary by CodeRabbit
New Features
@nibiruchain/soliditynpm packageDocumentation
Dependencies
@nibiruchain/soliditypackage (version 0.0.2)