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 integration tests for the ERC20 extension #2044

Closed
wants to merge 89 commits into from

Conversation

MalteHerrmann
Copy link
Contributor

Description

This PR adds integration tests to the ERC20 EVM extension.


Closes ENG-2319

Copy link

linear bot commented Nov 15, 2023

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.

Added the classic ERC20 methods as of yet, will add the metadata and increase/decrease allowance ones later today.

I've left some remarks though, find them below 👇

precompiles/erc20/utils_test.go Outdated Show resolved Hide resolved
precompiles/erc20/utils_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
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.

Great work @MalteHerrmann, we need to test that the contract and direct calls match the behaviour with the normal ERC-20 contracts that we have now deployed on Evmos for the registered coins. I suggest adding it to the DescribeTables and comparing the call to the ERC-20 vs EVM Extension

Copy link

codecov bot commented Nov 24, 2023

Codecov Report

Merging #2044 (cbc0414) into main (8a46ed8) will increase coverage by 0.16%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2044      +/-   ##
==========================================
+ Coverage   70.17%   70.34%   +0.16%     
==========================================
  Files         353      353              
  Lines       25948    25948              
==========================================
+ Hits        18210    18253      +43     
+ Misses       6792     6748      -44     
- Partials      946      947       +1     

see 3 files with indirect coverage changes

@MalteHerrmann MalteHerrmann marked this pull request as ready for review November 27, 2023 07:14
@MalteHerrmann MalteHerrmann requested a review from a team as a code owner November 27, 2023 07:14
@MalteHerrmann MalteHerrmann requested review from Vvaradinov and facs95 and removed request for a team November 27, 2023 07:14
Comment on lines +370 to +373
// FIXME: The gas used on the precompile is much higher than on the EVM
Entry(" - direct call", directCall, int64(3_021_572)),
Entry(" - through erc20 contract", erc20Call, int64(54_381)),
Entry(" - through erc20 v5 contract", erc20V5Call, int64(52_122)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll have to address the gas usage still, as it is right now, the call the to EVM extension is using way more gas than the call to the Solidity contract.

@MalteHerrmann
Copy link
Contributor Author

As per discussion with @0xstepit this PR will be split into smaller PRs ⛏️

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.

None yet

4 participants