Skip to content
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

tests(erc20): Add ERC20 transfer integration tests #2085

Merged
merged 9 commits into from
Nov 28, 2023

Conversation

MalteHerrmann
Copy link
Contributor

Description

This PR adds integration tests for ERC20 transfers in the process of merging #2044 in smaller chunks.

@MalteHerrmann MalteHerrmann requested a review from a team as a code owner November 27, 2023 16:50
@MalteHerrmann MalteHerrmann requested review from Vvaradinov and facs95 and removed request for a team November 27, 2023 16:50
@MalteHerrmann
Copy link
Contributor Author

The failing tests are fixed by #2088 through emitting the additional approval event in case of a transferFrom transaction.

Copy link
Contributor

@Vvaradinov Vvaradinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

precompiles/erc20/integration_test.go Show resolved Hide resolved
Copy link
Contributor

@0xstepit 0xstepit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great Job @MalteHerrmann, very clean and well organized tests. Just left some nit.

precompiles/erc20/integration_test.go Show resolved Hide resolved
precompiles/erc20/integration_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work! I found a few instances where the result boolean value is not checked

_, _, err = is.factory.CallContractAndCheckLogs

precompiles/erc20/integration_test.go Outdated Show resolved Hide resolved
precompiles/erc20/integration_test.go Outdated Show resolved Hide resolved
precompiles/erc20/integration_test.go Show resolved Hide resolved
precompiles/erc20/integration_test.go Outdated Show resolved Hide resolved
precompiles/erc20/integration_test.go Outdated Show resolved Hide resolved
precompiles/erc20/integration_test.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@MalteHerrmann MalteHerrmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fedekunze good points, I've addressed your suggestion to check the bool values and will apply those changes to the other PRs too.

It would be good to merge this one first before adjust the other PRs, so the changes to the ERC20AllowanceCaller.sol don't have to be repeated

precompiles/erc20/integration_test.go Show resolved Hide resolved
precompiles/erc20/integration_test.go Outdated Show resolved Hide resolved
precompiles/erc20/integration_test.go Outdated Show resolved Hide resolved
@fedekunze
Copy link
Contributor

tests will be fixed in a separate PR

@fedekunze fedekunze merged commit e495da2 into main Nov 28, 2023
28 of 31 checks passed
@fedekunze fedekunze deleted the malte/add-erc20-transfer-integration-tests branch November 28, 2023 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants