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

[Bug]: Type cosmossdk_io_math.LegacyDec (from package cosmossdk.io/math) not show correctly if the cli generated using autocli #19975

Closed
1 task done
QuocThi opened this issue Apr 8, 2024 · 13 comments · Fixed by #19976
Assignees
Labels

Comments

@QuocThi
Copy link

QuocThi commented Apr 8, 2024

Is there an existing issue for this?

  • I have searched the existing issues

What happened?

I am facing an issue with the client cli command generated using autocli. Any Msg response from server that has field with type DecCoin (underlying type is cosmossdk_io_math.LegacyDec) will be shown incorrectly. Here are some examples:
I have a custom module that will return a DecCoin type when query balances of account:
message QueryBalancesResponse { repeated cosmos.base.v1beta1.DecCoin coins = 1; }

NOTE: the correct balances is: 4000000000ntoken OR 4token

When using autocli to generate the client query cmd it show incorrect balances:

binaryd q gm balances address1q9mhu97ju0qckqgqf5fnhdqe5el63kzq33phdc                                                                                                                                                           
coins:
- amount: "4000000000000000000"
  denom: token

when using the cosmos's bank module to show, it shows correctly (because cosmos's bank using: cosmos.base.v1beta1.Coin):

binaryd q bank balances address1q9mhu97ju0qckqgqf5fnhdqe5el63kzq33phdc                                                                                                                                                         
balances:
- amount: "4000000000"
  denom: ntoken

If I use custom command (which uses client.PrintProto() from package "github.com/cosmos/cosmos-sdk/client") it also print output correctly.

binaryd q gm balances address1q9mhu97ju0qckqgqf5fnhdqe5el63kzq33phdc                                                                                                                                                            
coins:
- amount: "4.000000000000000000"
  denom: token

The gov module also show wrong number if the Params have type DecCoin.
cosmos.base.v1beta1.DecCoin minimum_gas_prices
proposal.json:

cat draft_proposal.json
{
  "messages": [
    {
      "@type": "/chain.custom_module.v1.MsgUpdateParams",
      "authority": "address10d07y265gmmuvt4z0w9aw880jnsr700jzx3uhc",
      "params": {
        "minimum_gas_prices": [
          {
            "denom": "ntoken",
            "amount": "10000.000000000000000000"
          }
        ]
      }
    }
  ],
  "metadata": "ipfs://CID",
  "deposit": "10000token",
  "title": "asd",
  "summary": "asd",
  "expedited": false
}

gov query output:

binaryd q gov proposals
pagination:
  total: "1"
proposals:
- deposit_end_time: "2024-04-10T04:14:28.10353Z"
  final_tally_result:
    abstain_count: "0"
    no_count: "0"
    no_with_veto_count: "0"
    yes_count: "0"
  id: "1"
  messages:
  - type: cosmos-sdk/custom_module/MsgUpdateParams
    value:
      authority: address10d07y265gmmuvt4z0w9aw880jnsr700jzx3uhc
      params:
        minimum_gas_prices:
        - amount: "10000000000000000000000"
          denom: ntoken
     ....

My previous solution is using EnhanceCustomCommand: true and keep our custom client cli to use the client.PrintProto() for our custom modules. But problem still happen with cosmos's core module like staking, gov. Here is the list of comos's module still use DecCoin type.

Cosmos SDK Version

0.50.3

How to reproduce?

No response

@QuocThi QuocThi added the T:Bug label Apr 8, 2024
@julienrbrt
Copy link
Member

julienrbrt commented Apr 8, 2024

Hi!

What's your x/tx version?
EDIT: Never mind, I am confusing this with another issue.

@julienrbrt
Copy link
Member

julienrbrt commented Apr 8, 2024

Thank you for finding this, I can reproduce.

https://v050.simapp.zone:1317/cosmos/distribution/v1beta1/community_pool

{
  "pool": [
    {
      "denom": "stake",
      "amount": "435835.220000000000000000"
    }
  ]
}

While simd q distribution community-pool isn't properly marshalled.

We need to define another encoder like this: https://github.com/cosmos/cosmos-sdk/blob/main/client/v2/autocli/query.go#L161-L186, as apparently, x/tx marshals cosmos.base.v1beta1.DecCoin differently.

@julienrbrt julienrbrt self-assigned this Apr 8, 2024
@QuocThi
Copy link
Author

QuocThi commented Apr 8, 2024

Hi @julienrbrt ,
so this should be fixed in autocli package from upstream repository and included in the next release right?
Also here is the list of cosmos's core module Msg that uses DecCoin type, I attached but it don't show up.
cosmos_types_use_DecCoins.txt

@julienrbrt
Copy link
Member

We'll release a client/v2 v2.0.0-beta.2 this week (#19530) and it will be usable in any v0.50.x release.
We have our monthly patch planned for this week as well.

@QuocThi
Copy link
Author

QuocThi commented Apr 16, 2024

Hi @julienrbrt,
Seem the v2.0.0-beta.2 still not released yet? I'm checking from this page and see the latest version still :v2.0.0-beta.1: https://pkg.go.dev/cosmossdk.io/client/v2.
Do I check on the right site?

Thanks

@julienrbrt
Copy link
Member

Correct, we've been backporting things, and the one PR that we want merged before beta.2 is still wip: #19530

Feel free to follow the issue above to be alerted.

@QuocThi
Copy link
Author

QuocThi commented Apr 17, 2024

Hi @julienrbrt,
One more type I missed is cosmossdk.io/math.LegacyDec also need the same fix as DecCoin, you can reproduce by using below queries:

binaryd q staking validators ==> the commission.delegator_shares is wrong.
binaryd q staking delegations <delegator_address> ==> the delegation.shares is wrong.

@julienrbrt julienrbrt reopened this Apr 17, 2024
@QuocThi
Copy link
Author

QuocThi commented May 13, 2024

Hi @julienrbrt,
Any update on which release will contain the fix of these two bugs?

Thanks

@julienrbrt
Copy link
Member

Hi, this release: #19530

@tac0turtle
Copy link
Member

@julienrbrt is this able to be closed?

@julienrbrt
Copy link
Member

julienrbrt commented Jun 3, 2024

@julienrbrt is this able to be closed?

No, it's done for one of the two (DecCoin but not yet LegacyDec)

@QuocThi
Copy link
Author

QuocThi commented Jun 19, 2024

Hi @julienrbrt, do you have any plan to fix the issue? Still waiting for the release.

@julienrbrt
Copy link
Member

julienrbrt commented Jun 19, 2024

Yep, client/v2 now has my full attention. Expect a release this week!

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

Successfully merging a pull request may close this issue.

3 participants