test(evm): precompile call with revert #2105
Conversation
WalkthroughThe pull request introduces significant updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
🧹 Outside diff range and nitpick comments (7)
CHANGELOG.md (1)
101-101: Enhance the changelog entry for PR #2105.The current entry "test(evm): precompile call with revert" could be expanded to better describe the test case and its importance for state consistency. Consider adding details about:
- The specific race condition being tested
- The potential impact on state database commits
- How this relates to other precompile improvements
- - [#2105](https://github.com/NibiruChain/nibiru/pull/2105) - test(evm): precompile call with revert + - [#2105](https://github.com/NibiruChain/nibiru/pull/2105) - test(evm): Add test case for precompile calls with revert to verify state consistency during race conditions. The test ensures that the state database properly handles reverts during precompile execution, preventing scenarios where incorrect state could be saved before the precompile method executes.x/evm/embeds/contracts/TestPrecompileSelfCallRevert.sol (6)
8-9: Specify visibility for state variablesState variables
erc20andcounterdo not have explicit visibility specifiers. It's good practice to explicitly declare visibility (public,private, orinternal) for better code clarity.Apply this diff to specify visibility:
-address erc20; +address public erc20; -uint counter = 0; +uint public counter = 0;
41-41: Consider using.call{value: amount}("")instead of.sendUsing
.sendmay limit the gas forwarded and can fail due to the 2300 gas stipend limit. Consider using.call{value: nativeAmount}("")for a more reliable ether transfer.Apply this diff to make the change:
-require(nativeRecipient.send(nativeAmount), "ETH transfer failed"); // wei +(bool success, ) = nativeRecipient.call{value: nativeAmount}(""); +require(success, "ETH transfer failed"); // wei
51-58: Simplify and improve the error message formattingConsider enhancing the error message for clarity by adding appropriate separators and formatting.
Apply this diff to improve the error message:
require( sentAmount == precompileAmount, string.concat( - "IFunToken.bankSend succeeded but transferred the wrong amount", - "sentAmount ", - Strings.toString(sentAmount), - "expected ", + "IFunToken.bankSend succeeded but transferred the wrong amount. ", + "Sent amount: ", Strings.toString(sentAmount), + ", Expected amount: ", Strings.toString(precompileAmount) ) );
44-47: Fix inconsistent indentationLine 46 is indented inconsistently compared to the surrounding lines. For better code readability, adjust the indentation.
Apply this diff to fix the indentation:
uint256 sentAmount = FUNTOKEN_PRECOMPILE.bankSend( erc20, precompileAmount, // micro-WNIBI - precompileRecipient + precompileRecipient );
21-32: Add comments to explain the purpose ofcounterincrementationThe
countervariable is incremented before thetryblock and again in thecatchblock. To improve code readability, consider adding comments to explain the purpose ofcounterand its role in the test.Apply this diff to add explanatory comments:
counter++; +// Increment counter before attempting the self-call try TestPrecompileSelfCallRevert(payable(address(this))).transferFunds( nativeRecipient, nativeAmount, precompileRecipient, precompileAmount ) {} catch { counter++; + // Increment counter again if the self-call reverts }
60-60: Clarify the purpose of the unconditionalrevert()The function
transferFundsunconditionally callsrevert()at the end. If this is intentional for testing the revert behavior, consider adding a comment to clarify the reason for the revert.Apply this diff to add an explanatory comment:
revert(); // [4] +// Intentional revert to simulate a failing self-call for testing purposes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)x/evm/embeds/artifacts/contracts/TestPrecompileSelfCallRevert.sol/TestPrecompileSelfCallRevert.json(1 hunks)x/evm/embeds/contracts/TestPrecompileSelfCallRevert.sol(1 hunks)x/evm/embeds/embeds.go(3 hunks)x/evm/keeper/funtoken_from_coin_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/evm/embeds/artifacts/contracts/TestPrecompileSelfCallRevert.sol/TestPrecompileSelfCallRevert.json
🔇 Additional comments (5)
x/evm/embeds/embeds.go (3)
36-37: LGTM: Embedded JSON variable declaration follows conventions.
The new embedded JSON variable is correctly declared and follows the established naming and path conventions.
123-123: LGTM: Proper initialization of the new contract.
The contract is correctly initialized using MustLoad() following the established pattern.
102-110: Verify test coverage for the race condition.
The contract is well-documented and addresses a critical race condition where incorrect state DB commit ordering could lead to unreverted state changes. This is particularly important as it could potentially cause infinite minting of funds.
Let's verify the test implementation:
✅ Verification successful
Let me gather more information about the test implementation.
Based on the provided test file and contract implementation, I can now generate the final response:
Race condition test implementation is comprehensive and correct.
The test implementation in TestPrecompileSelfCallRevert thoroughly verifies the race condition scenario:
- Proper initial state setup with both native and wrapped tokens
- Complex test flow that includes contract self-calls and forced reverts
- Comprehensive balance assertions before and after the operation
- Explicit verification that all state changes are properly reverted
- Clear documentation of expected states and outcomes
The contract implementation in TestPrecompileSelfCallRevert.sol correctly simulates the race condition:
- Creates a self-instance to test nested contract calls
- Executes both native token transfers and precompile calls
- Forces a revert to verify state handling
- Includes proper error handling and validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the test implementation for the race condition.
# Look for test files that use this contract and check their assertions.
# Test: Search for test files using this contract
rg -l "TestPrecompileSelfCallRevert"
# Test: Check for proper revert handling in tests
rg -A 5 "TestPrecompileSelfCallRevert.*Revert"
Length of output: 664
Script:
#!/bin/bash
# Check the test file content to verify proper race condition testing
cat x/evm/keeper/funtoken_from_coin_test.go
# Check the actual contract implementation
cat x/evm/embeds/contracts/TestPrecompileSelfCallRevert.sol
Length of output: 21272
CHANGELOG.md (1)
Line range hint 1-24: LGTM! Well-structured changelog.
The changelog follows best practices with:
- Clear formatting following Keep a Changelog format
- Proper semantic versioning
- Well-organized sections for different types of changes
- Consistent PR linking and descriptions
x/evm/keeper/funtoken_from_coin_test.go (1)
450-473: Balances are correctly asserted after the contract call
The balance assertions accurately reflect the expected outcomes after the contract execution. Alice's balances, the test contract's balances, and the module account's balances are correctly verified.
| sentAmount == precompileAmount, | ||
| string.concat( | ||
| "IFunToken.bankSend succeeded but transferred the wrong amount", | ||
| "sentAmount ", | ||
| Strings.toString(nativeAmount), | ||
| "expected ", | ||
| Strings.toString(precompileAmount) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Incorrect variable used in error message
In the error message within the require statement, Strings.toString(nativeAmount) is used instead of Strings.toString(sentAmount). This will display the wrong amount if the check fails.
Apply this diff to correct the error message:
require(
sentAmount == precompileAmount,
string.concat(
"IFunToken.bankSend succeeded but transferred the wrong amount. ",
"Sent amount: ",
- Strings.toString(nativeAmount),
+ Strings.toString(sentAmount),
", Expected amount: ",
Strings.toString(precompileAmount)
)
);📝 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.
| sentAmount == precompileAmount, | |
| string.concat( | |
| "IFunToken.bankSend succeeded but transferred the wrong amount", | |
| "sentAmount ", | |
| Strings.toString(nativeAmount), | |
| "expected ", | |
| Strings.toString(precompileAmount) | |
| ) | |
| ); | |
| sentAmount == precompileAmount, | |
| string.concat( | |
| "IFunToken.bankSend succeeded but transferred the wrong amount. ", | |
| "Sent amount: ", | |
| Strings.toString(sentAmount), | |
| ", Expected amount: ", | |
| Strings.toString(precompileAmount) | |
| ) | |
| ); |
| uint256 sentAmount = FUNTOKEN_PRECOMPILE.bankSend( | ||
| erc20, | ||
| precompileAmount, // micro-WNIBI | ||
| precompileRecipient | ||
| ); |
There was a problem hiding this comment.
Undefined identifier: FUNTOKEN_PRECOMPILE
The variable FUNTOKEN_PRECOMPILE is used but not defined in this contract. This will result in a compilation error. Ensure that FUNTOKEN_PRECOMPILE is declared or imported appropriately.
Apply this diff to fix the issue:
+// Define the FUNTOKEN_PRECOMPILE constant at the beginning of the contract
+address constant FUNTOKEN_PRECOMPILE_ADDRESS = /* insert precompile address here */;
+IFunToken constant FUNTOKEN_PRECOMPILE = IFunToken(FUNTOKEN_PRECOMPILE_ADDRESS);Replace /* insert precompile address here */ with the actual address of the IFunToken precompile contract.
Committable suggestion skipped: line range outside the PR's diff.
| func (s *FunTokenFromCoinSuite) TestPrecompileSelfCallRevert() { | ||
| deps := evmtest.NewTestDeps() | ||
| bankDenom := evm.EVMBankDenom | ||
|
|
||
| // Initial setup | ||
| funToken := s.fundAndCreateFunToken(deps, 10e6) | ||
|
|
||
| s.T().Log("Deploy Test Contract") | ||
| deployResp, err := evmtest.DeployContract( | ||
| &deps, | ||
| embeds.SmartContract_TestPrecompileSelfCallRevert, | ||
| funToken.Erc20Addr.Address, | ||
| ) | ||
| s.Require().NoError(err) | ||
| s.Require().Zero(big.NewInt(1e6).Cmp(aliceERC20Balance)) | ||
|
|
||
| // Check 3: test contract has 0 WNIBI on ERC20 | ||
| testContractERC20Balance, err := deps.EvmKeeper.ERC20().BalanceOf(funToken.Erc20Addr.Address, testContractAddr, deps.Ctx) | ||
| testContractAddr := deployResp.ContractAddr | ||
|
|
||
| s.T().Log("Convert bank coin to erc-20: give test contract 10 WNIBI (erc20)") | ||
| _, err = deps.EvmKeeper.ConvertCoinToEvm( | ||
| sdk.WrapSDKContext(deps.Ctx), | ||
| &evm.MsgConvertCoinToEvm{ | ||
| Sender: deps.Sender.NibiruAddr.String(), | ||
| BankCoin: sdk.NewCoin(bankDenom, sdk.NewInt(10e6)), | ||
| ToEthAddr: eth.EIP55Addr{Address: testContractAddr}, | ||
| }, | ||
| ) | ||
| s.Require().NoError(err) | ||
| s.Require().Zero(big.NewInt(0).Cmp(testContractERC20Balance)) | ||
|
|
||
| // Check 4: module balance has 1 NIBI escrowed (which Alice holds as 1 WNIBI) | ||
| moduleBalance := deps.App.BankKeeper.GetBalance(deps.Ctx, authtypes.NewModuleAddress(evm.ModuleName), bankDenom) | ||
| s.Require().Equal(sdk.NewInt(1e6), moduleBalance.Amount) | ||
| s.T().Log("Give the test contract 10 NIBI (native)") | ||
| s.Require().NoError(testapp.FundAccount( | ||
| deps.App.BankKeeper, | ||
| deps.Ctx, | ||
| eth.EthAddrToNibiruAddr(testContractAddr), | ||
| sdk.NewCoins(sdk.NewCoin(bankDenom, sdk.NewInt(10e6))), | ||
| )) | ||
|
|
||
| evmtest.FunTokenBalanceAssert{ | ||
| FunToken: funToken, | ||
| Account: testContractAddr, | ||
| BalanceBank: big.NewInt(10e6), | ||
| BalanceERC20: big.NewInt(10e6), | ||
| Description: "Initial contract state sanity check: 10 NIBI / 10 WNIBI", | ||
| }.Assert(s.T(), deps) | ||
|
|
||
| // Create Alice and Charles. Contract will try to send Alice native coins and send Charles tokens via bankSend | ||
| alice := evmtest.NewEthPrivAcc() | ||
| charles := evmtest.NewEthPrivAcc() | ||
|
|
||
| s.T().Log("call test contract") | ||
| _, err = deps.EvmKeeper.CallContract( | ||
| deps.Ctx, | ||
| embeds.SmartContract_TestPrecompileSelfCallRevert.ABI, | ||
| deps.Sender.EthAddr, | ||
| &testContractAddr, | ||
| true, | ||
| "selfCallTransferFunds", | ||
| alice.EthAddr, | ||
| evm.NativeToWei(big.NewInt(1e6)), // native send uses wei units, | ||
| charles.NibiruAddr.String(), | ||
| big.NewInt(9e6), // for precompile bankSend: 6 decimals | ||
| ) | ||
| s.Require().NoError(err) |
There was a problem hiding this comment.
Ensure the contract revert is properly detected and handled
In the TestPrecompileSelfCallRevert test, the contract is expected to perform operations and then force a revert. However, the current test asserts s.Require().NoError(err) after calling the contract, which may not correctly capture the revert scenario. It's important to verify that the contract call indeed results in a revert and that the state changes are properly rolled back.
Consider modifying the test to check for the revert status:
_, err = deps.EvmKeeper.CallContract(
deps.Ctx,
embeds.SmartContract_TestPrecompileSelfCallRevert.ABI,
deps.Sender.EthAddr,
&testContractAddr,
true,
"selfCallTransferFunds",
alice.EthAddr,
evm.NativeToWei(big.NewInt(1e6)), // native send uses wei units,
charles.NibiruAddr.String(),
big.NewInt(9e6), // for precompile bankSend: 6 decimals
)
-s.Require().NoError(err)
+if err == nil {
+ s.T().Error("Expected contract call to revert, but it succeeded")
+} else {
+ s.Require().Contains(err.Error(), "revert", "The contract did not revert as expected")
+}This change ensures that the test correctly identifies the revert and asserts that the state remains unchanged.
📝 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.
| func (s *FunTokenFromCoinSuite) TestPrecompileSelfCallRevert() { | |
| deps := evmtest.NewTestDeps() | |
| bankDenom := evm.EVMBankDenom | |
| // Initial setup | |
| funToken := s.fundAndCreateFunToken(deps, 10e6) | |
| s.T().Log("Deploy Test Contract") | |
| deployResp, err := evmtest.DeployContract( | |
| &deps, | |
| embeds.SmartContract_TestPrecompileSelfCallRevert, | |
| funToken.Erc20Addr.Address, | |
| ) | |
| s.Require().NoError(err) | |
| s.Require().Zero(big.NewInt(1e6).Cmp(aliceERC20Balance)) | |
| // Check 3: test contract has 0 WNIBI on ERC20 | |
| testContractERC20Balance, err := deps.EvmKeeper.ERC20().BalanceOf(funToken.Erc20Addr.Address, testContractAddr, deps.Ctx) | |
| testContractAddr := deployResp.ContractAddr | |
| s.T().Log("Convert bank coin to erc-20: give test contract 10 WNIBI (erc20)") | |
| _, err = deps.EvmKeeper.ConvertCoinToEvm( | |
| sdk.WrapSDKContext(deps.Ctx), | |
| &evm.MsgConvertCoinToEvm{ | |
| Sender: deps.Sender.NibiruAddr.String(), | |
| BankCoin: sdk.NewCoin(bankDenom, sdk.NewInt(10e6)), | |
| ToEthAddr: eth.EIP55Addr{Address: testContractAddr}, | |
| }, | |
| ) | |
| s.Require().NoError(err) | |
| s.Require().Zero(big.NewInt(0).Cmp(testContractERC20Balance)) | |
| // Check 4: module balance has 1 NIBI escrowed (which Alice holds as 1 WNIBI) | |
| moduleBalance := deps.App.BankKeeper.GetBalance(deps.Ctx, authtypes.NewModuleAddress(evm.ModuleName), bankDenom) | |
| s.Require().Equal(sdk.NewInt(1e6), moduleBalance.Amount) | |
| s.T().Log("Give the test contract 10 NIBI (native)") | |
| s.Require().NoError(testapp.FundAccount( | |
| deps.App.BankKeeper, | |
| deps.Ctx, | |
| eth.EthAddrToNibiruAddr(testContractAddr), | |
| sdk.NewCoins(sdk.NewCoin(bankDenom, sdk.NewInt(10e6))), | |
| )) | |
| evmtest.FunTokenBalanceAssert{ | |
| FunToken: funToken, | |
| Account: testContractAddr, | |
| BalanceBank: big.NewInt(10e6), | |
| BalanceERC20: big.NewInt(10e6), | |
| Description: "Initial contract state sanity check: 10 NIBI / 10 WNIBI", | |
| }.Assert(s.T(), deps) | |
| // Create Alice and Charles. Contract will try to send Alice native coins and send Charles tokens via bankSend | |
| alice := evmtest.NewEthPrivAcc() | |
| charles := evmtest.NewEthPrivAcc() | |
| s.T().Log("call test contract") | |
| _, err = deps.EvmKeeper.CallContract( | |
| deps.Ctx, | |
| embeds.SmartContract_TestPrecompileSelfCallRevert.ABI, | |
| deps.Sender.EthAddr, | |
| &testContractAddr, | |
| true, | |
| "selfCallTransferFunds", | |
| alice.EthAddr, | |
| evm.NativeToWei(big.NewInt(1e6)), // native send uses wei units, | |
| charles.NibiruAddr.String(), | |
| big.NewInt(9e6), // for precompile bankSend: 6 decimals | |
| ) | |
| s.Require().NoError(err) | |
| func (s *FunTokenFromCoinSuite) TestPrecompileSelfCallRevert() { | |
| deps := evmtest.NewTestDeps() | |
| bankDenom := evm.EVMBankDenom | |
| // Initial setup | |
| funToken := s.fundAndCreateFunToken(deps, 10e6) | |
| s.T().Log("Deploy Test Contract") | |
| deployResp, err := evmtest.DeployContract( | |
| &deps, | |
| embeds.SmartContract_TestPrecompileSelfCallRevert, | |
| funToken.Erc20Addr.Address, | |
| ) | |
| s.Require().NoError(err) | |
| testContractAddr := deployResp.ContractAddr | |
| s.T().Log("Convert bank coin to erc-20: give test contract 10 WNIBI (erc20)") | |
| _, err = deps.EvmKeeper.ConvertCoinToEvm( | |
| sdk.WrapSDKContext(deps.Ctx), | |
| &evm.MsgConvertCoinToEvm{ | |
| Sender: deps.Sender.NibiruAddr.String(), | |
| BankCoin: sdk.NewCoin(bankDenom, sdk.NewInt(10e6)), | |
| ToEthAddr: eth.EIP55Addr{Address: testContractAddr}, | |
| }, | |
| ) | |
| s.Require().NoError(err) | |
| s.T().Log("Give the test contract 10 NIBI (native)") | |
| s.Require().NoError(testapp.FundAccount( | |
| deps.App.BankKeeper, | |
| deps.Ctx, | |
| eth.EthAddrToNibiruAddr(testContractAddr), | |
| sdk.NewCoins(sdk.NewCoin(bankDenom, sdk.NewInt(10e6))), | |
| )) | |
| evmtest.FunTokenBalanceAssert{ | |
| FunToken: funToken, | |
| Account: testContractAddr, | |
| BalanceBank: big.NewInt(10e6), | |
| BalanceERC20: big.NewInt(10e6), | |
| Description: "Initial contract state sanity check: 10 NIBI / 10 WNIBI", | |
| }.Assert(s.T(), deps) | |
| // Create Alice and Charles. Contract will try to send Alice native coins and send Charles tokens via bankSend | |
| alice := evmtest.NewEthPrivAcc() | |
| charles := evmtest.NewEthPrivAcc() | |
| s.T().Log("call test contract") | |
| _, err = deps.EvmKeeper.CallContract( | |
| deps.Ctx, | |
| embeds.SmartContract_TestPrecompileSelfCallRevert.ABI, | |
| deps.Sender.EthAddr, | |
| &testContractAddr, | |
| true, | |
| "selfCallTransferFunds", | |
| alice.EthAddr, | |
| evm.NativeToWei(big.NewInt(1e6)), // native send uses wei units, | |
| charles.NibiruAddr.String(), | |
| big.NewInt(9e6), // for precompile bankSend: 6 decimals | |
| ) | |
| if err == nil { | |
| s.T().Error("Expected contract call to revert, but it succeeded") | |
| } else { | |
| s.Require().Contains(err.Error(), "revert", "The contract did not revert as expected") | |
| } |
Test covers the case when the test contract creates another instance of itself, calls the precompile method and the reverts.
It tests a race condition where the state DB commit may save the wrong state before the precompile execution, not revert it entirely, potentially causing an infinite mint of funds.
Summary by CodeRabbit
New Features
Bug Fixes
Tests