-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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: Make a denom of --min-self-delegation CLI flag required #9548
fix: Make a denom of --min-self-delegation CLI flag required #9548
Conversation
602bd1a
to
6b36f19
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution. Can you please format the changes?
@@ -25,7 +25,7 @@ var ( | |||
defaultCommissionRate = "0.1" | |||
defaultCommissionMaxRate = "0.2" | |||
defaultCommissionMaxChangeRate = "0.01" | |||
defaultMinSelfDelegation = "1" | |||
defaultMinSelfDelegation = "1" + sdk.DefaultBondDenom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to make stake
the default flag which I don't think we should do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Make sense.
But, I've referred to the defaultAmount
which contains sdk.DefaultBondDenom
as below:
https://github.com/youngjoon-lee/cosmos-sdk/blob/af995cd5ee5c30dccbb989cd6892e142d5dde818/x/staking/client/cli/tx.go#L24
Because I made a denom required, I think the defaultMinSelfDelegation
has to have a denom.
Of course, I added a logic to check whether the denom is the same as the expected one, or not.
What do you think about this strategy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I thought this is a uncompleted improvement, as long as we change MsgCreateValidator
to contains MinSelfDelegation
as sdk.Coin
(not sdk.Int
): https://github.com/youngjoon-lee/cosmos-sdk/blob/09f9a5573e4132fbedfaab6d948df1cc93e6e896/x/staking/types/tx.pb.go#L43
While I wrote this PR, I've felt that the original author chose sdk.Int
rather than sdk.Coin
because he wanted to define MinSelfDelegation
as "the minimum value regardless of what its denom is".
If so, I think it's okay to close this PR without merging (It would be better to make consistency between MsgCreateValidator
and CLI options).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah honestly, I don't think having a denom in the CLI UX will add that much benefit to be honest. I'm not sure what others think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. As the Gaia document has been updated: cosmos/gaia#867, I think it's clear that the unit of --min-self-delegation
is stake
or uatom
.
So, I'm closing this PR. Please feel free to open this PR if you think it's necessary. @ryanchristo
Description
Closes: #9386
As we discussed, I've put a denom to the
--min-self-delegation
CLI flag.But, I didn't change the msg types (
MsgCreateValidator
and so on) which containMinSelfDelegation
assdk.Int
in order not to break the API backward compatibility. If you think that msg types also need to be changed, please let me know :)Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change