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

Bucky/gov spec #968

Merged
merged 3 commits into from
May 8, 2018
Merged

Bucky/gov spec #968

merged 3 commits into from
May 8, 2018

Conversation

ebuchman
Copy link
Member

@ebuchman ebuchman commented May 8, 2018

  • some typo fixes
  • use "enum" types instead of strings where possible
  • simplify some naming
  • update notation for mappings
  • reduce indenting by remove else statements after the if throws

Will require careful review against the original as it's hard to see everything clearly in the diff.

Also, some thoughts:

  • do we need to store the whole Procedure in every proposal? Might be better to store it once and reference by name or hash so we don't duplicate the storage so often
  • can we change the name of Options to Votes ?
  • we need to add explicit max size checks for all strings. we have correctlyFormatted but we should be explicit about what it does
  • we should rename Tx to Msg
  • The Options map says the key is <proposalID | voterAddress | validatorAddress>. Do we really need all that ? I think this relates to the fact that a delegator needs to submit a vote for each validator it's delegated too to vote with its full bonded power. Would be awesome if this wasn't required!!

@ebuchman
Copy link
Member Author

ebuchman commented May 8, 2018

Note we should try to wrap this up re #457

@ebuchman
Copy link
Member Author

ebuchman commented May 8, 2018

We should also consider/incorporate #926 ?

@gamarin2
Copy link
Contributor

gamarin2 commented May 8, 2018

do we need to store the whole Procedure in every proposal? Might be better to store it once and reference by name or hash so we don't duplicate the storage so often

Original spec was exactly as you suggested, but @sunnya97 moved away from it during implementation. He'll probably be better placed to explain why.

can we change the name of Options to Votes ?

I think that's already the case in the implementation @sunnya97? I personally like Options, because it's more explicit IMO.

we need to add explicit max size checks for all strings. we have correctlyFormatted but we should be explicit about what it does

Yes, the purpose of the spec was to be abstract enough to accommodate the implementer. Once module is implemented, spec will be modified accordingly.

we should rename Tx to Msg

Yes

The Options map says the key is <proposalID | voterAddress | validatorAddress>. Do we really need all that ? I think this relates to the fact that a delegator needs to submit a vote for each validator it's delegated too to vote with its full bonded power. Would be awesome if this wasn't required!!

@sunnya97 plans to implement a custom Map type to work around that. But even without it we won't need all this since we're moving to a more 'automatic' way of voting. Voters loose their ability to cherry pick their votes for each validator they delegate to because of this though.

@ebuchman
Copy link
Member Author

ebuchman commented May 8, 2018

Ok thanks. I'll merge this and sync with Sunny on my questions and reconciliation

@ebuchman ebuchman mentioned this pull request May 8, 2018
@ebuchman ebuchman merged commit 8bfc5e1 into develop May 8, 2018
@ebuchman
Copy link
Member Author

ebuchman commented May 8, 2018

Follow up in #457 (comment)

@ebuchman ebuchman deleted the bucky/gov-spec branch May 8, 2018 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants