-
Notifications
You must be signed in to change notification settings - Fork 201
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
fix(evm): transfer for native to erc20 #2090
base: main
Are you sure you want to change the base?
Changes from 9 commits
9d59336
a0afa5a
476ffbc
eb72b31
f356315
43ae3e1
80d5353
3ad846b
e4b932d
b2c863d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,11 +54,11 @@ func (s *Suite) TestExportInitGenesis() { | |
s.Require().NoError(err) | ||
|
||
// Transfer ERC-20 tokens to user A | ||
_, err = deps.EvmKeeper.ERC20().Transfer(erc20Addr, fromUser, toUserA, amountToSendA, deps.Ctx) | ||
_, err, _ = deps.EvmKeeper.ERC20().Transfer(erc20Addr, fromUser, toUserA, amountToSendA, deps.Ctx) | ||
s.Require().NoError(err) | ||
|
||
// Transfer ERC-20 tokens to user B | ||
_, err = deps.EvmKeeper.ERC20().Transfer(erc20Addr, fromUser, toUserB, amountToSendB, deps.Ctx) | ||
_, err, _ = deps.EvmKeeper.ERC20().Transfer(erc20Addr, fromUser, toUserB, amountToSendB, deps.Ctx) | ||
s.Require().NoError(err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification 🛠️ Refactor suggestion Based on the code review, I can now generate the final response: Add test cases for non-standard ERC20 tokens The current tests only use standard OpenZeppelin ERC20 implementations (
Example approach:
The reason for this categorization is that the codebase only tests standard ERC20 implementations (OpenZeppelin's ERC20), which doesn't cover the non-standard token scenarios mentioned in the PR objectives. This gap in test coverage could lead to issues when handling non-standard tokens in production. 🔗 Analysis chainAdd test cases for non-standard ERC20 tokens. The test only covers the happy path with a standard ERC20 token. Given the PR objectives to handle non-standard tokens, additional test cases are needed. Let's verify if there are any test cases for non-standard tokens: Consider adding test cases for:
Let me try a different approach to verify the test coverage for non-standard ERC20 tokens. This will help us:
Let me check the actual ERC20 contract implementation and test setup to understand what kind of tokens are being tested. This will help us understand:
Let me check the ERC20 contract implementation that's being used in the tests. This will help us understand:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for test cases involving non-standard ERC20 tokens
rg -l "TestERC20" | xargs rg -l "fee.*transfer|non.*standard"
Length of output: 61 Script: #!/bin/bash
# First, let's find all ERC20-related test files
fd ".*test.*" | rg -l "ERC20"
# Then, let's examine the test implementations to understand the coverage
rg -A 10 "func.*Test.*ERC20.*"
Length of output: 2233 Script: #!/bin/bash
# Let's examine the ERC20 contract implementation and test setup
rg -l "CreateFunToken" | xargs rg -A 10 "CreateFunToken"
# Also check for any ERC20 contract deployments or mocks
fd ".*\.sol" | xargs cat
# Check test utilities and helpers
rg -A 5 "ERC20" x/evm/evmtest/
Length of output: 66063 Script: #!/bin/bash
# Check the ERC20 contract implementation
cat x/evm/embeds/contracts/ERC20Minter.sol
cat x/evm/embeds/contracts/TestERC20.sol
# Look for any test files that might test non-standard behavior
rg -A 10 "func.*Test.*Transfer" x/evm/
Length of output: 3136 |
||
|
||
// Create fungible token from bank coin | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,23 +73,53 @@ | |
func (e erc20Calls) Transfer( | ||
contract, from, to gethcommon.Address, amount *big.Int, | ||
ctx sdk.Context, | ||
) (out bool, err error) { | ||
) (success bool, err error, received *big.Int) { | ||
received = big.NewInt(0) | ||
|
||
recipientBalanceBefore, err := e.BalanceOf(contract, to, ctx) | ||
if err != nil { | ||
return false, errors.Wrap(err, "failed to retrieve recipient balance"), received | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Based on the search results, I can now generate the final response: Test coverage needed for fee-on-transfer token handling The balance tracking implementation in
Suggested test cases to add:
🔗 Analysis chainBalance tracking implementation looks good but needs test coverage. The implementation correctly handles fee-on-transfer tokens by:
However, several critical paths lack test coverage:
Let's verify the test coverage: Also applies to: 93-96, 98-104 Let me check the actual test implementations to verify the coverage. Let me search for test files in a broader context to find balance-related tests. Let me check the test implementations in the funtoken test files since they might contain the balance verification tests. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for existing test cases in the codebase
# Look for test files that might contain relevant test cases
fd -e go -e _test.go | xargs rg -l "TestERC20.*Transfer|TestFeeOnTransfer"
Length of output: 168 Script: #!/bin/bash
# Check the test implementations for transfer-related tests
rg -A 10 "TestERC20.*Transfer|TestFeeOnTransfer" x/evm/keeper/funtoken_from_erc20_test.go x/evm/embeds/embeds_test.go
# Also search for any test cases that verify balances
rg -B 2 -A 5 "BalanceOf.*ctx" x/evm/keeper/funtoken_from_erc20_test.go x/evm/embeds/embeds_test.go
Length of output: 1147 Script: #!/bin/bash
# Search for test files related to ERC20 keeper
fd -e go test.go keeper | xargs rg -l "erc20"
# Search for balance verification patterns in test files
rg -B 2 -A 5 "balance.*Before|balance.*After|received.*amount" x/evm/keeper/
Length of output: 1676 Script: #!/bin/bash
# Check funtoken test files for balance verification tests
rg -B 5 -A 10 "Transfer.*\(" x/evm/keeper/funtoken_from_erc20_test.go x/evm/keeper/funtoken_state_test.go
# Also check for any balance verification in the precompile tests
rg -B 5 -A 10 "Transfer.*\(" x/evm/precompile/funtoken_test.go
Length of output: 1409 🧰 Tools🪛 GitHub Check: codecov/patch
|
||
|
||
input, err := e.ABI.Pack("transfer", to, amount) | ||
if err != nil { | ||
return false, fmt.Errorf("failed to pack ABI args: %w", err) | ||
return false, fmt.Errorf("failed to pack ABI args: %w", err), received | ||
} | ||
resp, _, err := e.CallContractWithInput(ctx, from, &contract, true, input) | ||
if err != nil { | ||
return false, err | ||
return false, err, received | ||
} | ||
|
||
recipientBalanceAfter, err := e.BalanceOf(contract, to, ctx) | ||
if err != nil { | ||
return false, errors.Wrap(err, "failed to retrieve recipient balance"), received | ||
} | ||
|
||
received = new(big.Int).Sub(recipientBalanceAfter, recipientBalanceBefore) | ||
|
||
// we can't check that received = amount because the recipient could have | ||
// a transfer fee or other deductions. We can only check that the recipient | ||
// received some tokens | ||
if received.Sign() <= 0 { | ||
return false, fmt.Errorf("no (or negative) ERC20 tokens were received by the recipient"), received | ||
} | ||
|
||
var erc20Bool ERC20Bool | ||
err = e.ABI.UnpackIntoInterface(&erc20Bool, "transfer", resp.Ret) | ||
if err != nil { | ||
return false, err | ||
|
||
// per erc20 standard, the transfer function should return a boolean value | ||
// indicating whether the operation succeeded. If the unpacking failed, we | ||
// need to check the recipient balance to determine if the transfer was successful. | ||
if err == nil { | ||
// should be true if the transfer was successful but we do it anyway | ||
// to respect the contract's return value | ||
success = erc20Bool.Value | ||
|
||
return success, nil, received | ||
} | ||
|
||
return erc20Bool.Value, nil | ||
success = true | ||
return | ||
} | ||
|
||
// BalanceOf retrieves the balance of an ERC20 token for a specific account. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR only editing msg_server.go this much is not addressing the issue. The change needed is to use the success boolean returned by Ref: ERC20 Specification
The following implementation could work, for example. Then you can freely use the function and depend on its error value. func (e erc20Calls) Transfer(
contract, from, to gethcommon.Address, amount *big.Int,
ctx sdk.Context,
) (success bool, err error) {
input, err := e.ABI.Pack("transfer", to, amount)
if err != nil {
return false, fmt.Errorf("failed to pack ABI args: %w", err)
}
resp, err := e.CallContractWithInput(ctx, from, &contract, true, input)
if err != nil {
return false, err
}
var erc20Bool ERC20Bool
err = e.ABI.UnpackIntoInterface(&erc20Bool, "transfer", resp.Ret)
// ➡️ This check guarantees that a boolean is parsed out
if err != nil {
return false, err
}
// ➡️ This check fixes the issue
success = erc20Bool.Value
if !success {
return false, fmt.Errorf("called executed, but returned success=false")
}
return success, nil
} We should also comment on why we treat success=false as an error. Edit: On second thought, if you write it like this, it removes the need for the boolean return value entirely. You could rewrite the function to only return an error. @matthiasmatt |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -554,14 +554,6 @@ | |||||||||||||||||||
) (*evm.MsgConvertCoinToEvmResponse, error) { | ||||||||||||||||||||
erc20Addr := funTokenMapping.Erc20Addr.Address | ||||||||||||||||||||
|
||||||||||||||||||||
recipientBalanceBefore, err := k.ERC20().BalanceOf(erc20Addr, recipient, ctx) | ||||||||||||||||||||
if err != nil { | ||||||||||||||||||||
return nil, errors.Wrap(err, "failed to retrieve balance") | ||||||||||||||||||||
} | ||||||||||||||||||||
if recipientBalanceBefore == nil { | ||||||||||||||||||||
return nil, fmt.Errorf("failed to retrieve balance, balance is nil") | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
// Escrow Coins on module account | ||||||||||||||||||||
if err := k.bankKeeper.SendCoinsFromAccountToModule( | ||||||||||||||||||||
ctx, | ||||||||||||||||||||
|
@@ -583,52 +575,36 @@ | |||||||||||||||||||
return nil, errors.Wrap(err, "failed to retrieve balance") | ||||||||||||||||||||
} | ||||||||||||||||||||
if evmModuleBalance == nil { | ||||||||||||||||||||
return nil, fmt.Errorf("failed to retrieve balance, balance is nil") | ||||||||||||||||||||
return nil, fmt.Errorf("failed to retrieve EVM module account balance, balance is nil") | ||||||||||||||||||||
} | ||||||||||||||||||||
if evmModuleBalance.Cmp(coin.Amount.BigInt()) < 0 { | ||||||||||||||||||||
return nil, fmt.Errorf("insufficient balance in EVM module account") | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
// unescrow ERC-20 tokens from EVM module address | ||||||||||||||||||||
res, err := k.ERC20().Transfer( | ||||||||||||||||||||
success, err, actualReceivedAmount := k.ERC20().Transfer( | ||||||||||||||||||||
erc20Addr, | ||||||||||||||||||||
evm.EVM_MODULE_ADDRESS, | ||||||||||||||||||||
recipient, | ||||||||||||||||||||
coin.Amount.BigInt(), | ||||||||||||||||||||
ctx, | ||||||||||||||||||||
) | ||||||||||||||||||||
if err != nil { | ||||||||||||||||||||
return nil, errors.Wrap(err, "failed to transfer ERC20 tokens") | ||||||||||||||||||||
} | ||||||||||||||||||||
if !res { | ||||||||||||||||||||
return nil, fmt.Errorf("failed to transfer ERC20 tokens") | ||||||||||||||||||||
if err != nil || !success { | ||||||||||||||||||||
return nil, errors.Wrap(err, "failed to transfer ERC-20 tokens") | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle nil error correctly when transfer fails If Apply this diff to fix the error handling: if err != nil || !success {
- return nil, errors.Wrap(err, "failed to transfer ERC-20 tokens")
+ if err != nil {
+ return nil, errors.Wrap(err, "failed to transfer ERC-20 tokens")
+ } else {
+ return nil, fmt.Errorf("failed to transfer ERC-20 tokens")
+ }
} 📝 Committable suggestion
Suggested change
🧰 Tools🪛 GitHub Check: codecov/patch
|
||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
// Check expected Receiver balance after transfer execution | ||||||||||||||||||||
recipientBalanceAfter, err := k.ERC20().BalanceOf(erc20Addr, recipient, ctx) | ||||||||||||||||||||
if err != nil { | ||||||||||||||||||||
return nil, errors.Wrap(err, "failed to retrieve balance") | ||||||||||||||||||||
} | ||||||||||||||||||||
if recipientBalanceAfter == nil { | ||||||||||||||||||||
return nil, fmt.Errorf("failed to retrieve balance, balance is nil") | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
expectedFinalBalance := big.NewInt(0).Add(recipientBalanceBefore, coin.Amount.BigInt()) | ||||||||||||||||||||
if r := recipientBalanceAfter.Cmp(expectedFinalBalance); r != 0 { | ||||||||||||||||||||
return nil, fmt.Errorf("expected balance after transfer to be %s, got %s", expectedFinalBalance, recipientBalanceAfter) | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
// Burn escrowed Coins | ||||||||||||||||||||
err = k.bankKeeper.BurnCoins(ctx, evm.ModuleName, sdk.NewCoins(coin)) | ||||||||||||||||||||
burnCoin := sdk.NewCoin(coin.Denom, sdk.NewIntFromBigInt(actualReceivedAmount)) | ||||||||||||||||||||
err = k.bankKeeper.BurnCoins(ctx, evm.ModuleName, sdk.NewCoins(burnCoin)) | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure Before creating |
||||||||||||||||||||
if err != nil { | ||||||||||||||||||||
return nil, errors.Wrap(err, "failed to burn coins") | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
// Emit event with the actual amount received | ||||||||||||||||||||
_ = ctx.EventManager().EmitTypedEvent(&evm.EventConvertCoinToEvm{ | ||||||||||||||||||||
Sender: sender.String(), | ||||||||||||||||||||
Erc20ContractAddress: funTokenMapping.Erc20Addr.String(), | ||||||||||||||||||||
ToEthAddr: recipient.String(), | ||||||||||||||||||||
BankCoin: coin, | ||||||||||||||||||||
BankCoin: burnCoin, | ||||||||||||||||||||
}) | ||||||||||||||||||||
|
||||||||||||||||||||
return &evm.MsgConvertCoinToEvmResponse{}, nil | ||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -143,7 +143,7 @@ func (p precompileFunToken) bankSend( | |
|
||
// Caller transfers ERC20 to the EVM account | ||
transferTo := evm.EVM_MODULE_ADDRESS | ||
_, err = p.evmKeeper.ERC20().Transfer(erc20, caller, transferTo, amount, ctx) | ||
_, err, _ = p.evmKeeper.ERC20().Transfer(erc20, caller, transferTo, amount, ctx) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle the Received Amount from At line 162: _, err, _ = p.evmKeeper.ERC20().Transfer(erc20, caller, transferTo, amount, ctx) The Consider capturing and utilizing the -receivedAmount, err, _ := p.evmKeeper.ERC20().Transfer(erc20, caller, transferTo, amount, ctx)
+receivedAmount, err, _ := p.evmKeeper.ERC20().Transfer(erc20, caller, transferTo, amount, ctx)
if err != nil {
return nil, fmt.Errorf("failed to send from caller to the EVM account: %w", err)
}
+// Update the amount to reflect the actual received amount
+amount = receivedAmount By handling |
||
if err != nil { | ||
return nil, fmt.Errorf("failed to send from caller to the EVM account: %w", err) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update transfer result handling for fee-on-transfer tokens.
The test ignores the actual received amount (third return value) from
Transfer()
. For fee-on-transfer tokens, the received amount could be less than the sent amount, which this test wouldn't catch.Consider updating the test to handle fee-on-transfer scenarios:
Also applies to: 61-61