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

imp(erc20): support registering multiple assets #914

Merged
merged 27 commits into from
Oct 27, 2022

Conversation

fedekunze
Copy link
Contributor

@fedekunze fedekunze commented Sep 21, 2022

Description

Closes: ENG-774

Context

Projects such as IBC bridges (Axelar, Gravity) or projects that have multiple coins (Osmosis, Regen eco-credits, etc) would like to register multiple tokens at once either as ERC20s or SDK Coins. Unfortunately, that is not supported at the moment and they need to create a single proposal for each denomination

@linear
Copy link

linear bot commented Sep 21, 2022

ENG-774 Support registering multiple denominations for ERC20s and Coins

Projects such as IBC bridges (Axelar, Gravity) or projects that have multiple coins (Osmosis, Regen eco-credits, etc) would like to register multiple tokens at once either as ERC20s or SDK Coins. Unfortunately, that is not supported at the moment and they need to create a single proposal for each denomination.

Scope:

  • Use slices / arrays to register multiple coins or ERC20 tokens

@fedekunze fedekunze marked this pull request as ready for review September 21, 2022 07:58
@codecov
Copy link

codecov bot commented Sep 21, 2022

Codecov Report

Merging #914 (0285ad3) into main (65c05a5) will decrease coverage by 4.70%.
The diff coverage is 27.83%.

❗ Current head 0285ad3 differs from pull request most recent head ba0c4b1. Consider uploading reports for the commit ba0c4b1 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #914      +/-   ##
==========================================
- Coverage   81.32%   76.62%   -4.71%     
==========================================
  Files         126      131       +5     
  Lines        7188     7751     +563     
==========================================
+ Hits         5846     5939      +93     
- Misses       1196     1665     +469     
- Partials      146      147       +1     
Impacted Files Coverage Δ
x/erc20/client/cli/tx.go 0.00% <0.00%> (ø)
x/erc20/keeper/proposals.go 92.24% <ø> (-0.91%) ⬇️
x/erc20/proposal_handler.go 1.66% <0.00%> (-0.23%) ⬇️
x/erc20/types/proposal.go 92.13% <87.50%> (+0.67%) ⬆️
x/erc20/client/cli/utils.go 100.00% <100.00%> (ø)
app/upgrades/v9/upgrades.go 39.39% <0.00%> (-0.08%) ⬇️
app/forks.go 0.00% <0.00%> (ø)
app/upgrades/v9_1/upgrades.go
app/upgrades/v5/upgrades.go 56.73% <0.00%> (ø)
... and 8 more

@fedekunze fedekunze marked this pull request as draft September 21, 2022 09:24
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.

@fedekunze nice implementation.

Will need to try the cli still and waiting for the full tests with several including batch registrations.

Can you also adjust the docs?

x/erc20/proposal_handler.go Outdated Show resolved Hide resolved
x/erc20/client/cli/tx.go Show resolved Hide resolved
@danburck danburck marked this pull request as ready for review October 26, 2022 15:30
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.

Thanks for taking over this @danburck!

CHANGELOG.md Outdated Show resolved Hide resolved
docs/developers/guides/cosmos_coin_registration.md Outdated Show resolved Hide resolved
testutil/integration.go Outdated Show resolved Hide resolved
x/erc20/keeper/keeper_test.go Show resolved Hide resolved
x/erc20/keeper/keeper_test.go Show resolved Hide resolved
x/erc20/proposal_handler.go Outdated Show resolved Hide resolved
x/erc20/proposal_handler.go Outdated Show resolved Hide resolved
Copy link
Contributor

@GAtom22 GAtom22 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 guys!! Left a few comments! 🚀

testutil/integration.go Outdated Show resolved Hide resolved
testutil/integration.go Outdated Show resolved Hide resolved
testutil/integration.go Outdated Show resolved Hide resolved
x/erc20/spec/03_state_transitions.md Outdated Show resolved Hide resolved
x/erc20/spec/08_clients.md Outdated Show resolved Hide resolved
x/recovery/keeper/ibc_callbacks_test.go Outdated Show resolved Hide resolved
x/recovery/keeper/ibc_callbacks_test.go Outdated Show resolved Hide resolved
x/vesting/keeper/integration_test.go Show resolved Hide resolved
x/vesting/keeper/msg_server_test.go Show resolved Hide resolved
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. Approved with minor comments

CHANGELOG.md Show resolved Hide resolved
testutil/integration.go Outdated Show resolved Hide resolved
@danburck danburck enabled auto-merge (squash) October 27, 2022 09:03
@danburck danburck merged commit acd8ba3 into main Oct 27, 2022
@danburck danburck deleted the fedekunze/erc20-multi-register branch October 27, 2022 09:10
@danburck
Copy link
Contributor

@fedekunze I separated the test suite refactor into this ticket

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants