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.

Sign up for GitHub

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

Jump to bottom

imp(erc20): ERC-20 registration #1997

Merged
merged 32 commits into from Nov 13, 2023
Merged

imp(erc20): ERC-20 registration #1997

merged 32 commits into from Nov 13, 2023

Conversation

fedekunze
Copy link
Contributor

Description

Closes #XXX

Vvaradinov
Vvaradinov approved these changes Nov 10, 2023
View reviewed changes
Copy link
Contributor

@Vvaradinov Vvaradinov left a comment •
edited

Choose a reason for hiding this comment

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

LGTM! Pending linter and unit tests CI. Left some questions.

x/evm/keeper/precompiles.go Outdated Show resolved Hide resolved
x/evm/types/params.go Outdated Show resolved Hide resolved
x/erc20/keeper/precompile.go Outdated Show resolved Hide resolved
MalteHerrmann
MalteHerrmann reviewed Nov 10, 2023
View reviewed changes
Copy link
Contributor

@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.

Looks good for now, will address some of the broken tests on this PR 👍

x/evm/types/params.go Outdated Show resolved Hide resolved
x/evm/types/params.go Show resolved Hide resolved
x/erc20/types/interfaces.go Show resolved Hide resolved
x/erc20/keeper/precompile.go Outdated Show resolved Hide resolved
@codecov Codecov
Copy link

codecov bot commented Nov 10, 2023 •
edited

Codecov Report

Merging #1997 (a2149c6) into main (0add6e8) will increase coverage by 0.08%.
The diff coverage is 89.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1997      +/-   ##
==========================================
+ Coverage   70.30%   70.38%   +0.08%     
==========================================
  Files         334      335       +1     
  Lines       24990    25089      +99     
==========================================
+ Hits        17568    17659      +91     
- Misses       6518     6524       +6     
- Partials      904      906       +2
Files Coverage Δ
app/app.go 81.00% <100.00%> (+0.02%) ⬆️
precompiles/outposts/osmosis/osmosis.go 36.92% <ø> (ø)
x/erc20/keeper/keeper.go 88.88% <100.00%> (+1.38%) ⬆️
x/evm/keeper/params.go 35.41% <100.00%> (+12.68%) ⬆️
x/evm/keeper/precompiles.go 81.39% <100.00%> (+16.95%) ⬆️
x/evm/types/params.go 74.33% <92.30%> (+1.95%) ⬆️
x/erc20/keeper/precompiles.go 74.41% <74.41%> (ø)

MalteHerrmann
MalteHerrmann reviewed Nov 10, 2023
View reviewed changes
Copy link
Contributor

@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.

Left some comments on adjustments I've made so far 👇

x/erc20/keeper/mock_test.go Outdated Show resolved Hide resolved
x/erc20/types/mocks/README.md Outdated Show resolved Hide resolved
x/evm/keeper/params.go Show resolved Hide resolved
x/evm/types/params.go Show resolved Hide resolved
github-advanced-security[bot]
github-advanced-security bot found potential problems Nov 12, 2023
View reviewed changes
x/evm/keeper/precompiles.go
Comment on lines +167 to +170
for address := range k.precompiles {
addresses[i] = address
i++
}

Check warning

Code scanning / CodeQL

Iteration over map Warning

Iteration over map may be a possible source of non-determinism
MalteHerrmann
MalteHerrmann approved these changes Nov 13, 2023
View reviewed changes
Copy link
Contributor

@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.

This should be finished now - added some last comments on missing coverage for specific lines. 👇

x/erc20/keeper/precompiles.go Show resolved Hide resolved
x/erc20/keeper/precompiles.go Show resolved Hide resolved
x/erc20/keeper/precompiles.go Show resolved Hide resolved
@MalteHerrmann MalteHerrmann marked this pull request as ready for review November 13, 2023 12:09
@MalteHerrmann MalteHerrmann requested a review from a team as a code owner November 13, 2023 12:09
@MalteHerrmann MalteHerrmann requested review from facs95 and 0xstepit and removed request for a team November 13, 2023 12:09
fedekunze
fedekunze commented Nov 13, 2023
View reviewed changes
Copy link
Contributor Author

@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.

LGTM

@fedekunze fedekunze merged commit 9c5b3eb into main Nov 13, 2023
32 of 33 checks passed
@fedekunze fedekunze deleted the fedekunze/erc20-registration branch November 13, 2023 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Vvaradinov Vvaradinov Vvaradinov approved these changes

@MalteHerrmann MalteHerrmann MalteHerrmann approved these changes

@facs95 facs95 Awaiting requested review from facs95 facs95 is a code owner automatically assigned from evmos/core-engineering

@0xstepit 0xstepit Awaiting requested review from 0xstepit 0xstepit is a code owner automatically assigned from evmos/core-engineering

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@fedekunze @Vvaradinov @MalteHerrmann