feat: add precompile for calling bank to evm from evm #2135
Conversation
WalkthroughThis pull request introduces two new methods to the Changes
Sequence DiagramsequenceDiagram
participant Sender
participant EVM
participant BankModule
participant FunToken
Sender->>EVM: Call sendToEvm()
EVM->>FunToken: Validate transfer
FunToken->>BankModule: Deduct bank coins
FunToken->>EVM: Mint/Transfer ERC20 tokens
EVM-->>Sender: Return sent amount
Sender->>EVM: Call bankMsgSend()
EVM->>FunToken: Validate send parameters
FunToken->>BankModule: Execute bank transfer
BankModule-->>Sender: Return success status
Assessment against linked issues
Possibly related issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
There was a problem hiding this comment.
Copilot reviewed 4 out of 11 changed files in this pull request and generated 1 comment.
Files not reviewed (7)
- x/evm/embeds/artifacts/contracts/IFunToken.sol/IFunToken.json: Language not supported
- x/evm/embeds/artifacts/contracts/TestERC20TransferThenPrecompileSend.sol/TestERC20TransferThenPrecompileSend.json: Language not supported
- x/evm/embeds/artifacts/contracts/TestFunTokenPrecompileLocalGas.sol/TestFunTokenPrecompileLocalGas.json: Language not supported
- x/evm/embeds/artifacts/contracts/TestInfiniteRecursionERC20.sol/TestInfiniteRecursionERC20.json: Language not supported
- x/evm/embeds/artifacts/contracts/TestNativeSendThenPrecompileSend.sol/TestNativeSendThenPrecompileSend.json: Language not supported
- x/evm/embeds/artifacts/contracts/TestPrecompileSelfCallRevert.sol/TestPrecompileSelfCallRevert.json: Language not supported
- x/evm/embeds/contracts/IFunToken.sol: Language not supported
Comments suppressed due to low confidence (2)
x/evm/precompile/funtoken.go:551
- [nitpick] The error message 'no funtoken found for bank denom "%s"' could be more descriptive to help users understand the context better.
return nil, fmt.Errorf("no funtoken found for bank denom \"%s\"", bankDenom)
x/evm/precompile/funtoken.go:731
- The error message for argument type validation in 'parseArgsBankMsgSend' should be more explicit to indicate the expected and received types.
err = ErrArgTypeValidation("string to", args[0])
| // 0) bankDenom | ||
| bankDenom, ok = args[0].(string) | ||
| if !ok { | ||
| err = ErrArgTypeValidation("string bankDenom", args[0]) |
There was a problem hiding this comment.
The error message for argument type validation in 'parseArgsSendToEvm' should be more explicit to indicate the expected and received types.
| err = ErrArgTypeValidation("string bankDenom", args[0]) | |
| err = fmt.Errorf("expected string for bankDenom, got %T", args[0]) |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2135 +/- ##
==========================================
- Coverage 65.10% 65.03% -0.07%
==========================================
Files 274 274
Lines 21732 21896 +164
==========================================
+ Hits 14148 14240 +92
- Misses 6610 6657 +47
- Partials 974 999 +25
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
x/evm/precompile/funtoken.go (2)
529-587: Emit EVM events when transferring tokens
While this function neatly handles the logic for transferring bank coins to ERC20, consider adding an event emission at the end for success/failure. An event can help frontends track or confirm the completion of cross-chain transfers more easily.
677-713: Optionally emit events for bankMsgSend
The function successfully transfers coins to the recipient and returns a boolean indicating success. Consider emitting additional EVM events to provide clients with immediate feedback on send operations, parallel to the standard approach in most EVM-based contracts.x/evm/precompile/funtoken_test.go (2)
432-490: Comprehensive test coverage for sendToEvm
This test scenario covers minting and transferring flows thoroughly. As an additional check, consider adding a negative test case for sending zero or insufficient funds to enhance robustness.
492-559: Validations in bankMsgSend test
The test ensures proper final balances and a success return value. Adding a failing test where the sender lacks enough balance or uses an invalid recipient address could provide valuable edge-case coverage.x/evm/embeds/artifacts/contracts/TestInfiniteRecursionERC20.sol/TestInfiniteRecursionERC20.json (1)
Line range hint
1-313: Ensure test coverage for recursive precompile calls.Given this contract tests infinite recursion scenarios and the PR implements bank-to-EVM precompile functionality, we should verify that recursive precompile calls are properly tested.
Consider adding test cases to specifically verify:
- Maximum recursion depth for bank-to-EVM calls
- Error handling for recursive precompile calls
- Gas consumption patterns in recursive scenarios
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
CHANGELOG.md(4 hunks)x/evm/embeds/artifacts/contracts/IFunToken.sol/IFunToken.json(2 hunks)x/evm/embeds/artifacts/contracts/TestERC20TransferThenPrecompileSend.sol/TestERC20TransferThenPrecompileSend.json(1 hunks)x/evm/embeds/artifacts/contracts/TestFunTokenPrecompileLocalGas.sol/TestFunTokenPrecompileLocalGas.json(1 hunks)x/evm/embeds/artifacts/contracts/TestInfiniteRecursionERC20.sol/TestInfiniteRecursionERC20.json(1 hunks)x/evm/embeds/artifacts/contracts/TestNativeSendThenPrecompileSend.sol/TestNativeSendThenPrecompileSend.json(1 hunks)x/evm/embeds/artifacts/contracts/TestPrecompileSelfCallRevert.sol/TestPrecompileSelfCallRevert.json(1 hunks)x/evm/embeds/contracts/IFunToken.sol(1 hunks)x/evm/precompile/funtoken.go(3 hunks)x/evm/precompile/funtoken_test.go(1 hunks)x/evm/precompile/precompile.go(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- x/evm/embeds/artifacts/contracts/TestERC20TransferThenPrecompileSend.sol/TestERC20TransferThenPrecompileSend.json
- x/evm/embeds/artifacts/contracts/TestPrecompileSelfCallRevert.sol/TestPrecompileSelfCallRevert.json
- x/evm/embeds/artifacts/contracts/TestNativeSendThenPrecompileSend.sol/TestNativeSendThenPrecompileSend.json
- CHANGELOG.md
- x/evm/embeds/artifacts/contracts/TestFunTokenPrecompileLocalGas.sol/TestFunTokenPrecompileLocalGas.json
🔇 Additional comments (15)
x/evm/precompile/funtoken.go (8)
45-46: New methods recognized
The introduction of two new precompile method constants (FunTokenMethod_sendToEvm and FunTokenMethod_bankMsgSend) is well-named and consistent with the existing naming convention.
77-80: Switch cases for new methods
These new switch cases neatly integrate sendToEvm and bankMsgSend into the existing precompile flow. The logic for routing to the correct methods appears consistent with the rest of the code.
589-633: Mint or unescrow logic seems correct
This helper function correctly distinguishes between owner-minted and escrow-based ERC20 tokens. Thorough checks for balances before/after unescrow ensure correctness.
635-640: Argument parsing structure is consistent
The top-level argument parsing for sendToEvm follows the same pattern used in other parse functions. The number of arguments is validated before accessing them.
642-645: Improve error message for argument type
This is similar to a past comment () requesting that the type mismatch error include both the expected and received type. For example:
- err = ErrArgTypeValidation("string bankDenom", args[0])
+ err = fmt.Errorf("expected string for bankDenom, got %T", args[0])646-660: Remainder of parse logic
The subsequent checks for the amount and to fields look correct. Proper early returns on type validation errors are in place.
662-675: Address parsing logic
The logic for detecting Ethereum hex vs. Bech32 addresses is robust. The clear error messages facilitate troubleshooting invalid addresses.
715-744: Argument parsing for bankMsgSend
The function’s argument checks (type validation, positive amount requirement) are consistent. There are no obvious gaps in error handling.
x/evm/embeds/contracts/IFunToken.sol (2)
56-73: Definition of sendToEvm
This added interface method accurately reflects the usage in the precompile code, ensuring that the receiver can be either an EVM hex address or a Bech32 address. The documentation comment is comprehensive.
74-87: Definition of bankMsgSend
The newly introduced interface method aligns with the Cosmos bank module's send semantics. The return value being a boolean for success is a straightforward design choice.
x/evm/precompile/precompile.go (1)
230-231: Marking sendToEvm and bankMsgSend as mutations
Classifying these methods as state-mutating in isMutation is correct, given they both transfer tokens. This ensures proper gas calculation and event handling.
x/evm/embeds/artifacts/contracts/IFunToken.sol/IFunToken.json (2)
129-157: bankMsgSend ABI entry
The ABI entry correctly mirrors the contract function’s signature and aligns with the implementation in the precompile.
187-215: sendToEvm ABI entry
This ABI addition accurately matches the newly introduced contract function, ensuring the JSON artifact remains up-to-date with the code.
x/evm/embeds/artifacts/contracts/TestInfiniteRecursionERC20.sol/TestInfiniteRecursionERC20.json (2)
Line range hint 1-311: Security test contract implementation looks good.
The contract implements necessary ERC20 functions and includes specific test methods for security testing. The presence of attackBalance and attackTransfer methods indicates proper test coverage for potential recursive call vulnerabilities.
312-313: Verify bytecode changes against source modifications.
The bytecode has been updated, which could indicate changes in the contract's implementation. Let's verify these changes match the source.
✅ Verification successful
Let me check the contract source to verify the bytecode changes.
Bytecode changes are consistent with the contract source
The bytecode changes in the JSON artifact file correspond to the compiled version of the TestInfiniteRecursionERC20 contract. The contract is designed to test infinite recursion scenarios through precompile interactions, specifically:
balanceOf- Implements recursive behavior through staticcall to FUNTOKEN_PRECOMPILE_ADDRESStransfer- Implements recursive behavior through FUNTOKEN_PRECOMPILE.sendToBank- Attack functions that trigger the recursive behaviors
The bytecode reflects these implementations and the contract's inheritance from OpenZeppelin's ERC20.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify contract source matches bytecode
# Look for the source file and any recent changes
# Find the source file
fd -t f "TestInfiniteRecursionERC20.sol"
# Check for any imports or dependencies in the contract
rg -A 5 "^import.*TestInfiniteRecursionERC20"
# Look for any recent changes to the contract
rg "contract TestInfiniteRecursionERC20"
Length of output: 286
Script:
#!/bin/bash
# Get the contract source code
cat x/evm/embeds/contracts/TestInfiniteRecursionERC20.sol
Length of output: 1281
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
x/evm/precompile/funtoken_test.go (4)
432-490: Consider adding edge cases to TestSendToEvm.The test thoroughly covers the happy path, but would benefit from additional test cases:
- Invalid amounts (zero or negative)
- Non-existent denominations
- Insufficient balance scenarios
492-559: Add failure scenarios to TestBankMsgSend.While the success path is well-tested, consider adding test cases for:
- Invalid recipient addresses
- Zero or negative amounts
- Non-existent denominations
- Insufficient balance scenarios
This would help ensure proper error handling and validation.
566-653: Improve test readability and maintainability.Consider the following improvements:
- Define constants for commonly used values (500, 100 tokens)
- Extract the ERC20 deployment and funding setup into a helper function
- Create a helper function for the repeated balance assertion pattern
Example refactor:
+const ( + INITIAL_SUPPLY = 1000000 + INITIAL_TRANSFER = 500 + TEST_TRANSFER = 100 +) +func setupTestERC20WithFunding(deps *evmtest.TestDeps, recipient gethcommon.Address) (gethcommon.Address, error) { + // Deploy ERC20 + erc20Resp, err := evmtest.DeployContract(deps, embeds.SmartContract_TestERC20) + if err != nil { + return gethcommon.Address{}, err + } + + // Transfer initial amount + deployerAddr := gethcommon.HexToAddress(erc20Resp.EthTxMsg.From) + _, err = deps.EvmKeeper.CallContract( + deps.Ctx, + embeds.SmartContract_TestERC20.ABI, + deployerAddr, + &erc20Resp.ContractAddr, + true, + keeper.Erc20GasLimitExecute, + "transfer", + recipient, + bigTokens(INITIAL_TRANSFER), + ) + return erc20Resp.ContractAddr, err +}
561-564: Improve helper function documentation and constants.Consider these improvements:
- Define TOKEN_DECIMALS as a named constant
- Add a more descriptive comment explaining the purpose and usage
+const TOKEN_DECIMALS_MULTIPLIER = 1_000_000_000_000_000_000 // 1e18: Standard ERC20 decimals +// bigTokens converts a human-readable token amount to its smallest unit representation +// by multiplying it with 1e18 (standard ERC20 decimals) func bigTokens(n int64) *big.Int { - e18 := big.NewInt(1_000_000_000_000_000_000) // 1e18 + e18 := big.NewInt(TOKEN_DECIMALS_MULTIPLIER) return new(big.Int).Mul(big.NewInt(n), e18) }
| ); err != nil { | ||
| return nil, fmt.Errorf("failed to send coins to module: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
I think you're missing the burn call here, in the case the funtoken mapping was created from the EVM side.
| wantBank := big.NewInt(234) // 1234 - 1000 => 234 | ||
| bankBal := deps.App.BankKeeper.GetBalance(deps.Ctx, deps.Sender.NibiruAddr, bankDenom).Amount.BigInt() | ||
| s.EqualValues(wantBank, bankBal, "did user lose 1000 unibi from bank?") | ||
|
|
There was a problem hiding this comment.
Can we also check the EVM module account's balance.
| } | ||
|
|
||
| func bigTokens(n int64) *big.Int { | ||
| e18 := big.NewInt(1_000_000_000_000_000_000) // 1e18 |
There was a problem hiding this comment.
(nit) could just do big.NewInt(1e18) to not have the reader manually count the number of zeroes.
| // no bank side left | ||
| balAfter := deps.App.BankKeeper.GetBalance(deps.Ctx, alice.NibiruAddr, bankBal.Denom).Amount.BigInt() | ||
| s.Require().EqualValues(bigTokens(0), balAfter) | ||
|
|
There was a problem hiding this comment.
Can you also check the EVM module account's balance? It should be zero as well here.
This pull request includes several changes to the
CHANGELOG.mdfile and theIFunToken.jsonfile to fix bugs, add new features, and simplify the code. The most important changes include fixing a missing error check, adding new precompile functions, and updating the changelog with the latest changes.Bug Fixes and Improvements:
CHANGELOG.md: Fixed a bug where theerr != nilcheck was missing in thebankBalanceprecompile method. This ensures that errors are properly handled in the precompile function.New Features:
CHANGELOG.md: Added a new precompile for calling the bank from the EVM, allowing for more seamless integration between the EVM and the banking system.x/evm/embeds/artifacts/contracts/IFunToken.sol/IFunToken.json: Added thebankMsgSendfunction, which allows sending messages to the bank with specified parameters such asto,bankDenom, andamount. This function returns a boolean indicating success.x/evm/embeds/artifacts/contracts/IFunToken.sol/IFunToken.json: Added thesendToEvmfunction, which allows sending tokens to the EVM with specified parameters such asbankDenom,amount, andto. This function returns the amount sent.# Purpose / AbstractSummary by CodeRabbit
New Features
sendToEvmandbankMsgSendChores
Tests
sendToEvmandbankMsgSendmethods