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

fix(export-genesis): fix export genesis command for missing values #1800

Merged
merged 8 commits into from
Oct 24, 2022

Conversation

Pantani
Copy link
Contributor

@Pantani Pantani commented Oct 11, 2022

part of #1384

Description

Some values are missing in the genesis JSON when we are exporting it. So we can't validate the genesis after export.

Most of the values should be upgraded based on default genesis parameters, but maybe we should also add the liquidityPoolRecords parameter from the liquidity module

@codecov
Copy link

codecov bot commented Oct 11, 2022

Codecov Report

Merging #1800 (cbd1656) into main (5e569a1) will not change coverage.
The diff coverage is n/a.

❗ Current head cbd1656 differs from pull request most recent head 40b2b8e. Consider uploading reports for the commit 40b2b8e to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1800   +/-   ##
=======================================
  Coverage   43.13%   43.13%           
=======================================
  Files          15       15           
  Lines        1340     1340           
=======================================
  Hits          578      578           
  Misses        742      742           
  Partials       20       20           

@Pantani Pantani marked this pull request as draft October 11, 2022 04:33
@Pantani Pantani self-assigned this Oct 11, 2022
@Pantani Pantani marked this pull request as ready for review October 11, 2022 05:40
@mmulji-ic
Copy link
Contributor

@Pantani @glnro this is not tested via the current tests that we have right? If not, is it practical to add something?

return nil, errors.New("atom denom not found")
}
atomMetaData.Name = "Cosmos Hub Atom"
atomMetaData.Symbol = "ATOM"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, when i query the node in public net, denom-metadata is already there.

{"metadatas":[{"description":"The native staking token of the Cosmos Hub.","denom_units":[{"denom":"uatom","exponent":0,"aliases":["microatom"]},{"denom":"matom","exponent":3,"aliases":["milliatom"]},{"denom":"atom","exponent":6,"aliases":[]}],"base":"uatom","display":"atom","name":"","symbol":""}],"pagination":{"next_key":null,"total":"1"}}

also, the denom is uatom.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is different than the actual denom

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@Pantani Pantani Oct 11, 2022

Choose a reason for hiding this comment

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

We can confirm the parameter through mainnet API (http://<rpc...>:1317/cosmos/bank/v1beta1/denoms_metadata) . The parameter the SDK use as a key index is base, and we are using uatom:

$ curl http://<RPC_ADDRESS>:1317/cosmos/bank/v1beta1/denoms_metadata
{
  "metadatas": [
    {
      "description": "The native staking token of the Cosmos Hub.",
      "denom_units": [
        {
          "denom": "uatom",
          "exponent": 0,
          "aliases": [
            "microatom"
          ]
        },
        {
          "denom": "matom",
          "exponent": 3,
          "aliases": [
            "milliatom"
          ]
        },
        {
          "denom": "atom",
          "exponent": 6,
          "aliases": [
          ]
        }
      ],
      "base": "uatom",
      "display": "atom",
      "name": "",
      "symbol": ""
    }
  ],
  "pagination": {
    "next_key": null,
    "total": "1"
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@glnro glnro requested a review from yaruwangway October 11, 2022 16:10
@okwme
Copy link
Contributor

okwme commented Oct 12, 2022

It's strange that Name and Symbol were left empty when the rest was populated.
We should ensure whatever it is it matches https://github.com/cosmos/chain-registry/blob/master/cosmoshub/assetlist.json

They have us listed as

        "name": "Cosmos",
        "symbol": "ATOM",

Either we update the chain registry or update here so that name = Cosmos instead of Cosmos Hub Atom.
I prefer the former since there may be wallets, exchanges or explorers using the chain-registry already.

Copy link
Contributor

@okwme okwme left a comment

Choose a reason for hiding this comment

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

change "Name" to "Cosmos" or change registry

@Pantani Pantani requested a review from okwme October 13, 2022 18:35
app/upgrades/v8/upgrades.go Outdated Show resolved Hide resolved
Copy link
Contributor

@okwme okwme left a comment

Choose a reason for hiding this comment

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

tests are timing out but I checked them locally and they pass e2e : )

@okwme okwme merged commit ad1ffce into main Oct 24, 2022
@okwme okwme deleted the fix/export-genesis branch October 24, 2022 16:24
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.

5 participants