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

bugfix: Update utils.go #530

Merged
merged 6 commits into from Apr 28, 2022
Merged

bugfix: Update utils.go #530

merged 6 commits into from Apr 28, 2022

Conversation

lizisky
Copy link
Contributor

@lizisky lizisky commented Apr 26, 2022

bugfix: compare 2 DenomUnit variable wrong because they are pointers

Description

Closes: #XXX


All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

PR review checkboxes:

I have...

  • added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • included the correct type prefix in the PR title
  • targeted the correct branch (see PR Targeting)
  • provided a link in the PR description to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all required CI checks have passed

Code maintenance:

I have...

  • written unit and integration tests
  • added relevant godoc and code comments.
  • updated relevant documentation (docs/) or specification (x/<module>/spec/)

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • confirmed that this PR does not change production code

bugfix: compare 2 DenomUnit variable wrong because they are pointers
Copy link
Contributor

@danburck danburck left a comment

Choose a reason for hiding this comment

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

Thank you for contributing! This looks good to me.

Can you write some tests in x/erc20/types/utils_test.go?

lizisky and others added 2 commits April 27, 2022 09:56
decimals := uint8(coinMetadata.DenomUnits[0].Exponent)

The original code maybe OK for evmos itself. However, it may causes incorrect decimal value

1. in the metadata validation func: cosmos-sdk/x/bank/types/metadata.go: Metadata.Validate() 
it makes sure Denomination units are sorted in ascending order.

2. https://docs.evmos.org/modules/erc20/01_concepts.html
"Coin Metadata to ERC20 details" informaiton
@lizisky
Copy link
Contributor Author

lizisky commented Apr 27, 2022

@danburck thanks for your review, I have added the test code in x/erc20/types/utils_test.go

I have submit another commit about fetch "decimal" value from cosmos coin metadata during deploy ERC20 contract.
I think fetch "decimal" value from the first denomUnit is incorrect, because it's ALWAYS zero according to the metadata's validate algorithm.

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.

Thanks @liziblockchain! Code changes look great. Left a few minor comments on the tests

x/erc20/types/utils_test.go Show resolved Hide resolved
x/erc20/types/utils_test.go Outdated Show resolved Hide resolved
x/erc20/types/utils.go Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 27, 2022

Codecov Report

Merging #530 (9d1724e) into main (9a4eb7a) will increase coverage by 0.10%.
The diff coverage is 82.35%.

❗ Current head 9d1724e differs from pull request most recent head 776bba1. Consider uploading reports for the commit 776bba1 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #530      +/-   ##
==========================================
+ Coverage   81.46%   81.56%   +0.10%     
==========================================
  Files         115      115              
  Lines        6360     6375      +15     
==========================================
+ Hits         5181     5200      +19     
+ Misses       1024     1018       -6     
- Partials      155      157       +2     
Impacted Files Coverage Δ
x/erc20/types/utils.go 75.86% <75.00%> (+36.97%) ⬆️
x/erc20/keeper/proposals.go 87.08% <100.00%> (+0.25%) ⬆️

@fedekunze fedekunze enabled auto-merge (squash) April 27, 2022 15:05
@fedekunze fedekunze merged commit 110d182 into evmos:main Apr 28, 2022
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

3 participants