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

Governance API: The Proposal JSON response does not include who submitted the proposal #3027

Closed
4 tasks
jmdfm opened this issue Dec 7, 2018 · 21 comments · Fixed by #3184
Closed
4 tasks
Assignees

Comments

@jmdfm
Copy link
Contributor

jmdfm commented Dec 7, 2018

Summary of Bug

The API response for calling proposal does not include who submitted the proposal. That would be useful information to know.

Steps to Reproduce

  1. Create proposal.
  2. Call API:
cosmos-sdk HEAD % curl -k https://0.0.0.0:1317/gov/proposals/4
{
  "type": "gov/TextProposal",
  "value": {
    "proposal_id": "4",
    "title": "Stupid Test Proposal",
    "description": "This is a description of my proposal to move to the Yukon and disappear.",
    "proposal_type": "Text",
    "proposal_status": "VotingPeriod",
    "tally_result": {
      "yes": "0.0000000000",
      "abstain": "0.0000000000",
      "no": "0.0000000000",
      "no_with_veto": "0.0000000000"
    },
    "submit_time": "2018-12-06T02:53:14.9623162Z",
    "deposit_end_time": "2018-12-08T02:53:14.9623162Z",
    "total_deposit": [
      {
        "denom": "STAKE",
        "amount": "10"
      },
      {
        "denom": "node0Token",
        "amount": "500"
      }
    ],
    "voting_start_time": "2018-12-06T02:53:14.9623162Z",
    "voting_end_time": "2018-12-08T02:53:14.9623162Z"
  }
}%

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@jbibla
Copy link
Contributor

jbibla commented Dec 7, 2018

strongly agree with @johnmcdowall here.

also on his desire to move to the Yukon.

@alexanderbez
Copy link
Contributor

/cc @sunnya97 is there a reason we don't include the proposer?

@sunnya97
Copy link
Member

We decided not to store the proposer primarily because it's the substance of a proposal that is important, not the identity of the proposer 😉.

However, the proposer will usually put down a portion of the deposit themselves, and we should highlight anyone who wants to put monetary stake (deposit) behind a proposal, not just the proposer of it.

@fedekunze
Copy link
Collaborator

However, the proposer will usually put down a portion of the deposit themselves, and we should highlight anyone who wants to put monetary stake (deposit) behind a proposal, not just the proposer of it.

So technically if you query the the first proposal deposit you can get the proposer with the depositor address, right @sunnya97 ?

@jbibla
Copy link
Contributor

jbibla commented Dec 10, 2018

it's the substance of a proposal that is important, not the identity of the proposer

i'm not sure i understand why we need to make this kind of decision at the SDK level. both the proposal and it's associated data are important and may be relevant to some users in some use cases.

some UIs might agree with you and make the choice not expose the proposers information, while others may decide for their use case it would be best to expose this information.

@sunnya97
Copy link
Member

So technically if you query the the first proposal deposit you can get the proposer with the depositor address

Not quite, because the deposits in state are just put lexigraphically by depositer address, not by time. If you really wanted to know who submitted the proposal, you could see who submitted the tx by doing a tx query rather than a state query.

i'm not sure i understand why we need to make this kind of decision at the SDK level...some UIs might agree with you and make the choice not expose the proposers information, while others may decide for their use case it would be best to expose this information.

That's a fair point. The reason I made the decision at the SDK level to not include the proposer is that it isn't used anywhere in the state machine logic, so it didn't make sense to store it in state, as that is additional storage data that isn't necessary. Do you think it's really necessary enough to burden all nodes with storing that data, or would it be sufficient for UIs that want to show the proposer to get it through a tx query.

Tbh, I prefer not to store it in state, but don't care too strongly about this.

@jaekwon @cwgoes @gamarin2 what are your thoughts?

@cwgoes
Copy link
Contributor

cwgoes commented Dec 10, 2018

Tbh, I prefer not to store it in state, but don't care too strongly about this.

I would second this.

I do think that in our UIs where we can choose whether or not to show the proposer address, we should elect not to show it. Proposers who wish to self-identify can do so in the proposal text.

@jbibla
Copy link
Contributor

jbibla commented Dec 10, 2018

thanks for the explanation @sunnya97!

Proposers who wish to self-identify can do so in the proposal text.

fair point @cwgoes.

@jmdfm
Copy link
Contributor Author

jmdfm commented Dec 10, 2018

@cwgoes A lot of users want to know who has submitted a given proposal. You're not protecting anything by not displaying it, it's just bad UX and pointless obfuscation.

@sunnya97 It comes down to what is the point of LCD and its API? This is basic information, that does not cost a lot to store, or at the very least if it's not stored LCD should take care of finding and assembling the information for the proper API response. Several times now you've given me the answer of 'just query the TX' without acknowledging that LCD is supposed to reduce those kinds of scenarios, or even in the case of the deposit API returning nil after voting period has closed HOW to perform such a query when the block height isn't even returned as part of the response.

If this is not the role that LCD is supposed to perform then I think the team needs to have a clear statement about what IS the purpose of LCD, because right now a lot of the APIs seem to give half responses that can't be semantically linked together as one would expect with any proper API.

@faboweb
Copy link
Contributor

faboweb commented Dec 11, 2018

@johnmcdowall love your statement! I think we need a proper description of purpose for the LCD. I will take the feedback and bring it up with the maintaining teams.
I agree with all of your points, but to be fair: The teams are working hard and the LCD is just not the very top priority right now.

@cwgoes
Copy link
Contributor

cwgoes commented Dec 11, 2018

You're not protecting anything by not displaying it, it's just bad UX and pointless obfuscation.

The choice of what information to show in a UI definitely influences what users will do with the interface, and in particular which information they will consider in voting on proposals. Personally, I think that we want to minimize the importance of the identity of the proposer, and maximize the importance of the content of the proposal. Other UI implementers may make different decisions (and the Voyager team may well disagree with me!).

On the SDK side, since it's certainly possible to query the transaction submitting the proposal and return the proposer address, I agree that not implementing an endpoint exposing it would amount to "pointless obfuscation" - so let's do that.

@alexanderbez alexanderbez self-assigned this Dec 11, 2018
@jmdfm
Copy link
Contributor Author

jmdfm commented Dec 11, 2018

@faboweb Totally understand things are chaotic right now. I just feel it's important to challenge these tacit decisions before things become concrete and we end up paying the cost of needing an Infura to do basic things ;)

@alexanderbez
Copy link
Contributor

alexanderbez commented Dec 12, 2018

So what is the exact consensus on the actionable item(s) here? PR #3091 fixes/adds some tags, namely you can get the tx where the proposal was created via 'action=proposal-created&proposal-id=N'.

Are we looking to implement another endpoint here?

@jmdfm
Copy link
Contributor Author

jmdfm commented Dec 12, 2018

A new endpoint that wraps the tags process, say /proposal/proposer would be ideal, yeah.

@jmdfm
Copy link
Contributor Author

jmdfm commented Dec 12, 2018

Also, @alexanderbez if you are willing to add that API endpoint I will also be willing to use your work as the foundation for adding APIs to get the historical votes and deposits per #2880, unless you want to tackle that at the same time.

EDIT: Oops, missed #3091. Nice!

@alexanderbez
Copy link
Contributor

alexanderbez commented Dec 12, 2018

I would be more than happy to implement that API endpoint (/proposals/{id}/proposer)!

Would like further thoughts if this is OK from @fedekunze, @faboweb or @jackzampolin.

@jackzampolin
Copy link
Member

jackzampolin commented Dec 13, 2018

@alexanderbez Lets do that, but ideally it just gets added to the original endpoint. If that requires extensive mods to the exiting endpoint then I don't have an issue exposing it like you mention there.

@alexanderbez
Copy link
Contributor

Well, we'd have to modify the proposal structs I imagine. At that point we might as well keep it in state...

@alexanderbez
Copy link
Contributor

Ok, I'm going to implement this endpoint using a tx query under the hood.

@sunnya97
Copy link
Member

@alexanderbez Why would we have to modify the original proposal structs to do a tx query?

@alexanderbez
Copy link
Contributor

@sunnya97 sorry, misunderstanding. We'd have to modify the proposal structs in order to get the existing endpoint to display this data. I'm suggesting we don't do that ;-)

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.

9 participants