feat(evm): randao support for evm #2151
Conversation
WalkthroughThis pull request introduces randao support for the Ethereum Virtual Machine (EVM) in the Nibiru blockchain project. The changes include adding a new Changes
Sequence DiagramsequenceDiagram
participant Block as Previous Block
participant EVM as Ethereum Virtual Machine
participant Contract as TestRandom Contract
Block-->>EVM: Provides prevrandao value
EVM->>Contract: Calls getRandom()
Contract-->>EVM: Returns block.prevrandao
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
x/evm/embeds/contracts/TestRandom.sol (1)
1-2: Fix incorrect contract name in comment headerThe comment header indicates "TestERC20.sol" but the file is "TestRandom.sol".
-// contracts/TestERC20.sol +// contracts/TestRandom.solx/evm/embeds/embeds.go (1)
139-143: Enhance documentation for TestRandom contractThe current comment is too brief and doesn't explain the RANDAO functionality or its implications.
- // SmartContract_TestRandom is a test contract which tests random function + // SmartContract_TestRandom is a test contract that demonstrates RANDAO functionality + // by accessing block.prevrandao values. This contract is used to verify that the EVM + // correctly supports post-merge randomness operations. Note that this randomness + // source has limitations and should not be used directly for high-stakes applications. SmartContract_TestRandom = CompiledEvmContract{ Name: "TestRandom.sol", EmbedJSON: testRandom, }x/evm/keeper/msg_server.go (1)
125-127: Implementation of RANDAO support looks correct but has predictability concerns.The implementation correctly provides RANDAO support by generating pseudo-random values from block time and last commit hash. However, be aware that these sources of entropy are predictable by validators, which may not be suitable for use cases requiring strong randomness.
Consider enhancing the randomness by incorporating additional entropy sources like:
- VDF (Verifiable Delay Function) output
- Multiple previous block hashes
- Validator signatures
Also applies to: 139-139
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)x/evm/embeds/artifacts/contracts/TestRandom.sol/TestRandom.json(1 hunks)x/evm/embeds/contracts/TestRandom.sol(1 hunks)x/evm/embeds/embeds.go(3 hunks)x/evm/embeds/embeds_test.go(1 hunks)x/evm/keeper/msg_server.go(3 hunks)x/evm/keeper/random_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/evm/embeds/artifacts/contracts/TestRandom.sol/TestRandom.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (4)
x/evm/embeds/embeds_test.go (1)
24-24: LGTM!The addition of
SmartContract_TestRandom.MustLoad()follows the established pattern for contract loading tests.x/evm/embeds/embeds.go (2)
44-45: LGTM!The embedding of the TestRandom contract JSON follows the established pattern.
160-160: LGTM!The initialization of TestRandom contract follows the established pattern.
CHANGELOG.md (1)
66-66: Changelog entry is well-formatted and clear.The entry properly documents the RANDAO support feature with correct PR reference and clear description.
| function getRandom() public view returns (uint256) { | ||
| return block.prevrandao; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and security considerations for randomness generation
The getRandom function directly exposes block.prevrandao which has important security implications:
- The value is predictable by validators
- Can be manipulated in certain scenarios
- Should not be used directly for high-stakes randomness
Add comprehensive NatSpec documentation:
+ /// @notice Retrieves the previous block's RANDAO value
+ /// @dev WARNING: This randomness source has limitations:
+ /// - Predictable by validators
+ /// - Should not be used for high-stakes applications
+ /// - Consider using a commit-reveal scheme for better unpredictability
+ /// @return uint256 The previous block's RANDAO value
function getRandom() public view returns (uint256) {
return block.prevrandao;
}Consider implementing a more secure randomness mechanism:
- Use a commit-reveal scheme
- Combine multiple entropy sources
- Consider using VRF (Verifiable Random Function) for critical applications
📝 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 getRandom() public view returns (uint256) { | |
| return block.prevrandao; | |
| } | |
| /// @notice Retrieves the previous block's RANDAO value | |
| /// @dev WARNING: This randomness source has limitations: | |
| /// - Predictable by validators | |
| /// - Should not be used for high-stakes applications | |
| /// - Consider using a commit-reveal scheme for better unpredictability | |
| /// @return uint256 The previous block's RANDAO value | |
| function getRandom() public view returns (uint256) { | |
| return block.prevrandao; | |
| } |
x/evm/keeper/random_test.go
Outdated
| // TestRandom tests the random value generation within the EVM. | ||
| func (s *Suite) TestRandom() { | ||
| deps := evmtest.NewTestDeps() | ||
| deployResp, err := evmtest.DeployContract(&deps, embeds.SmartContract_TestRandom) | ||
| s.Require().NoError(err) | ||
| randomContractAddr := deployResp.ContractAddr | ||
|
|
||
| // highjacked LoadERC20BigInt method as it perfectly fits the need of this test | ||
| resp, err := deps.EvmKeeper.LoadERC20BigInt( | ||
| deps.Ctx, embeds.SmartContract_TestRandom.ABI, randomContractAddr, "getRandom", | ||
| ) | ||
| s.Require().NoError(err) | ||
| s.Require().Greater(resp.Int64(), int64(0)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for random value generation
The current test only verifies that the random value is greater than zero. This is insufficient for testing randomness properties.
Add comprehensive test cases:
func (s *Suite) TestRandom() {
deps := evmtest.NewTestDeps()
deployResp, err := evmtest.DeployContract(&deps, embeds.SmartContract_TestRandom)
s.Require().NoError(err)
randomContractAddr := deployResp.ContractAddr
// Test multiple blocks to ensure values change
values := make(map[int64]bool)
for i := 0; i < 5; i++ {
resp, err := deps.EvmKeeper.LoadRandomValue(
deps.Ctx, embeds.SmartContract_TestRandom.ABI, randomContractAddr, "getRandom",
)
s.Require().NoError(err)
s.Require().Greater(resp.Int64(), int64(0))
// Verify uniqueness across blocks
s.Require().False(values[resp.Int64()], "Duplicate random value detected")
values[resp.Int64()] = true
// Advance block
deps.Ctx = deps.Ctx.WithBlockHeight(deps.Ctx.BlockHeight() + 1)
}
}
x/evm/keeper/random_test.go
Outdated
| // highjacked LoadERC20BigInt method as it perfectly fits the need of this test | ||
| resp, err := deps.EvmKeeper.LoadERC20BigInt( | ||
| deps.Ctx, embeds.SmartContract_TestRandom.ABI, randomContractAddr, "getRandom", | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid repurposing ERC20 methods for non-ERC20 functionality
The comment acknowledges that LoadERC20BigInt is being "hijacked" for this test. This is a code smell that could lead to maintenance issues.
Consider creating a dedicated method for loading random values:
- // highjacked LoadERC20BigInt method as it perfectly fits the need of this test
- resp, err := deps.EvmKeeper.LoadERC20BigInt(
+ resp, err := deps.EvmKeeper.LoadRandomValue(
deps.Ctx, embeds.SmartContract_TestRandom.ABI, randomContractAddr, "getRandom",
)Committable suggestion skipped: line range outside the PR's diff.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2151 +/- ##
==========================================
- Coverage 65.21% 65.20% -0.01%
==========================================
Files 277 277
Lines 22197 22208 +11
==========================================
+ Hits 14475 14480 +5
- Misses 6732 6739 +7
+ Partials 990 989 -1
|
Although without this feature, instead of a panic described by auditor contract simply returns 0.
Summary by CodeRabbit
Release Notes
New Features
TestRandomsmart contract to retrieve the previous block's random value.Testing
Documentation
These enhancements improve the EVM's capabilities by providing a mechanism to access block-level randomness, beneficial for various blockchain applications.