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

erc20 keeper delete function have to delete related map #366

Closed
4 tasks
hea9549 opened this issue Mar 7, 2022 · 2 comments · Fixed by #368
Closed
4 tasks

erc20 keeper delete function have to delete related map #366

hea9549 opened this issue Mar 7, 2022 · 2 comments · Fixed by #368
Assignees
Labels
bug Something isn't working

Comments

@hea9549
Copy link

hea9549 commented Mar 7, 2022

Summary of Bug

I'm looking at the evmos code. I think that func (k Keeper) DeleteTokenPair(ctx sdk.Context, tokenPair types.TokenPair), a function that deletes TokenPair, should delete not only id but also erc20Map and denomMap.

func (k Keeper) UpdateTokenPairERC20(ctx sdk.Context, erc20Addr, newERC20Addr common.Address) proceeds as follows:

  1. deletes pair using DeleteTokenPair function,
  2. overwrites denomMap
  3. deletes ERC20Map and creates it again.

There is no problem with the current code progress, but if DeleteTokenPair is used elsewhere, there is a possibility that a bug may occur due to misunderstanding.(To be precise, only the id may be deleted and the mapping may remain)

So I suggest to delete all related mappings in the DeleteTokenPair function.

If this proposal is accepted, I will submit the modified code PR.

Version

all

Steps to Reproduce

Screenshots

Additional context


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@danburck
Copy link
Contributor

danburck commented Mar 7, 2022

@hea9549 Thanks for reporting this, this is great. We have no objections to this improvement. Please feel free to open a PR with the suggested changes and we'll review it.

@fedekunze fedekunze added the bug Something isn't working label Mar 7, 2022
@fedekunze fedekunze self-assigned this Mar 7, 2022
@hea9549
Copy link
Author

hea9549 commented Mar 7, 2022

oops @fedekunze fix this issue 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants