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

Add display,name,symbol to tokenfactory denom creation #861

Merged
merged 8 commits into from
Jun 13, 2023

Conversation

philipsu522
Copy link
Contributor

@philipsu522 philipsu522 commented Jun 8, 2023

Describe your changes and provide context

x/bank requires that display, name and symbol for all denoms are valid. However, when exporting genesis we found that this is not the case.
This is an initial PoC. We need to backport (migrate all existing denoms) if this looks good.

Testing performed to validate your change

  • Unit test
  • Created denom locally, ran validate-genesis, ensure it passes
  • Created denom on 3.0.3, ran upgrade, ensure new genesis is valid
      "denom_metadata": [
        {
          "base": "factory/sei1099gwedy7ds2ysc8xjegfl9720qmczgfwdzdk7/test",
          "denom_units": [
            {
              "aliases": [],
              "denom": "factory/sei1099gwedy7ds2ysc8xjegfl9720qmczgfwdzdk7/test",
              "exponent": 0
            }
          ],
          "description": "",
          "display": "factory/sei1099gwedy7ds2ysc8xjegfl9720qmczgfwdzdk7/test",
          "name": "factory/sei1099gwedy7ds2ysc8xjegfl9720qmczgfwdzdk7/test",
          "symbol": "factory/sei1099gwedy7ds2ysc8xjegfl9720qmczgfwdzdk7/test"
        },

@philipsu522 philipsu522 requested review from Kbhat1 June 8, 2023 16:27
@philipsu522 philipsu522 enabled auto-merge (squash) June 8, 2023 16:27
@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Merging #861 (db2076a) into master (b897550) will increase coverage by 0.08%.
The diff coverage is 46.15%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #861      +/-   ##
==========================================
+ Coverage   63.53%   63.61%   +0.08%     
==========================================
  Files         250      250              
  Lines       15507    15533      +26     
==========================================
+ Hits         9852     9882      +30     
+ Misses       5187     5184       -3     
+ Partials      468      467       -1     
Impacted Files Coverage Δ
x/tokenfactory/keeper/migrations.go 65.00% <36.36%> (-35.00%) ⬇️
x/tokenfactory/keeper/createdenom.go 84.78% <100.00%> (+1.44%) ⬆️

... and 1 file with indirect coverage changes

@philipsu522 philipsu522 changed the title [WIP] Add display,name,symbol to tokenfactory denom creation Add display,name,symbol to tokenfactory denom creation Jun 12, 2023
@yzang2019
Copy link
Contributor

yzang2019 commented Jun 13, 2023

Can you add an integration test for this? Instruction are here: https://github.com/sei-protocol/sei-chain/tree/master/integration_test

@philipsu522 philipsu522 force-pushed the tokenfactory-metadata-fixes branch from 882b17f to 762dd98 Compare June 13, 2023 04:05
Copy link
Contributor

@yzang2019 yzang2019 left a comment

Choose a reason for hiding this comment

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

Nice

@yzang2019
Copy link
Contributor

@philipsu522 We also need to add the new integration test to workflow so that CI can pick it up

env: ADMIN_ADDR
# Create denom
- cmd: seid tx tokenfactory create-denom test --from admin --fees 2000usei
env: PARAMS
Copy link
Contributor

Choose a reason for hiding this comment

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

This command is not PARAMS right?

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.

3 participants