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

Redelegate and Unbond never check denomination in transaction #4598

Closed
4 tasks
kwunyeung opened this issue Jun 20, 2019 · 3 comments · Fixed by #4600
Closed
4 tasks

Redelegate and Unbond never check denomination in transaction #4598

kwunyeung opened this issue Jun 20, 2019 · 3 comments · Fixed by #4600
Assignees

Comments

@kwunyeung
Copy link
Contributor

Summary of Bug

Both redelegation and unbond tx can be processed even if the amount is specified with denom other than uatom.

e.g.

Redelegation
https://cosmos.bigdipper.live/transactions/7D153586C7FC707708F954DD5F7C795745DC99AF11C7CFAE0458A87F2A5388AA

Unbonding
https://cosmos.bigdipper.live/transactions/83D4D5CFDE8AC2F5B340A5BF275FDF8C9FDAB5604C5B488D44F7E1F65A548913

The number of shares did updated and the redelegation/unbonding have been created. This is rather an inconsistency in the message and the operation. If it only take shares, then the message should not require submitting in coin.

On the other hand, the doc in LCD show shares in the example

https://cosmos.network/rpc/#/ICS21/post_staking_delegators__delegatorAddr__redelegations

and it will give error if number of shares is specified in the request. It takes coin as well.

Version

v0.34.7

Steps to Reproduce


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@fedekunze fedekunze self-assigned this Jun 20, 2019
@rigelrozanski
Copy link
Contributor

good catch

@alexanderbez
Copy link
Contributor

alexanderbez commented Jun 20, 2019

Thanks @kwunyeung! The concept of shares has been removed from both the messages and the outputs of unbondings/redelegations. Users provide an amount (currently Coin). So we can either:

  1. Check the denom against k.Params().BondDenom
  2. Change the Amount to an sdk.Int

I'm more partial to (1) but both should work.

@fedekunze fedekunze mentioned this issue Jun 20, 2019
5 tasks
@fedekunze
Copy link
Collaborator

fedekunze commented Jun 20, 2019

Note: this is an UX bug, it does not affect the state machine as the tokens are converted to shares (worth of params.BondDenom) on the redelegation and unbond funcs within the staking Keeper.

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

Successfully merging a pull request may close this issue.

4 participants