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

Standarize deposit representation in governance proposal files #5783

Closed
4 tasks
sunnya97 opened this issue Mar 10, 2020 · 4 comments · Fixed by #5790
Closed
4 tasks

Standarize deposit representation in governance proposal files #5783

sunnya97 opened this issue Mar 10, 2020 · 4 comments · Fixed by #5790
Assignees
Labels
C:CLI C:x/gov good first issue help wanted T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).

Comments

@sunnya97
Copy link
Member

Different Proposal type files represent deposits differently.

For example, Text Proposals have use the string version of an sdk.Coin (i.e. 10uatom) while ParamChange Proposals use the JSON version (i.e. { "denom": "stake", "amount": "10000"})

https://github.com/cosmos/cosmos-sdk/blob/master/x/gov/client/cli/tx.go#L92

https://github.com/cosmos/cosmos-sdk/blob/master/x/params/client/cli/tx.go#L44


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@sunnya97 sunnya97 added C:x/gov T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). labels Mar 10, 2020
@jgimeno jgimeno self-assigned this Mar 11, 2020
@jgimeno
Copy link
Contributor

jgimeno commented Mar 11, 2020

I think makes more sense the second version, more accurate.

{ "denom": "stake", "amount": "10000"}

The only problem I see is in cases like this:


{
  "title": "Test Proposal",
  "description": "My awesome proposal",
  "type": "Text",
  "deposit": "10test"
}

Which is equivalent to:

$ %s tx gov submit-proposal --title="Test Proposal" --description="My awesome proposal" --type="Text" --deposit="10test" --from mykey

Where the equivalent flag is --deposit="10test", in that case looks like the denom+amount would make the alternate version different.

So maybe is easier to use the short version. Any thoughts on this?

@sunnya97
Copy link
Member Author

I have a slight preference towards the first one (the string version). Most of the places in our user flow that users have to use sdk.Coins, they use the string version.

But on the other hand, we typically use the JSON version, in JSONs (like the genesis file for example).

I guess it depends what the expected way of creating proposal files is. Do we expect people to write them by hand, or are they supposed to be the output of some Proposal Generation tool?

@jgimeno
Copy link
Contributor

jgimeno commented Mar 11, 2020

What about on every JSON always:

{ "denom": "stake", "amount": "10000"}

but on command line the short one, and convert to json version behind the scenes when needed.

@alexanderbez
Copy link
Contributor

I recommend to take string which is comma delimited and call ParseCoins instead of JSON. This makes client logic and parsing easier.

@jgimeno jgimeno changed the title Standardize deposit representation in governance proposal files Standarize deposit representation in governance proposal files Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:CLI C:x/gov good first issue help wanted T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants