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.

Sign up for GitHub

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

Jump to bottom

Add generic actions for governance #633

Merged
merged 32 commits into from
May 20, 2019
Merged

Add generic actions for governance #633

merged 32 commits into from
May 20, 2019

Conversation

ethanfrey
Copy link
Contributor

@ethanfrey ethanfrey commented May 16, 2019 •
edited
Loading

Addresses #586

This will finalize the x/gov functionality

All changes for the dynamic Decoder/Executor/Options are completed as prototyped in the spec.
All code compiles and tests pass.

Things to do (maybe not as part of this PR):

And of course #589 when this is merged

@ethanfrey ethanfrey force-pushed the governance-actions branch from 71786d9 to 90db0e3 Compare May 16, 2019 18:14
@ethanfrey ethanfrey requested review from husio and ruseinov and removed request for husio May 16, 2019 18:15
@ethanfrey ethanfrey mentioned this pull request May 16, 2019
Closed
husio
husio reviewed May 17, 2019
View reviewed changes
Copy link
Contributor

@husio husio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When documenting public entities, please finish your sentence with a period (.) 💅

What I have noticed is that we have no rule for which error to use when. It is fully random which error instance a validator failure will return. For example, if a field value is required but empty, we have places where ErrEmpty, ErrInput, ErrState, ErrModel or ErrMsg is returned. At least 5 different types for the same case!! We must do something about it because it does not look professional.

Code looks good. I will be persistent about good enough comments though 👮‍♀️😉 I am looking forward to the tests to pass.

x/gov/codec.proto Show resolved Hide resolved
x/gov/codec.proto Outdated Show resolved Hide resolved
x/gov/codec.proto Show resolved Hide resolved
x/gov/context.go Outdated Show resolved Hide resolved
x/gov/context.go Outdated Show resolved Hide resolved
x/gov/context.go Outdated Show resolved Hide resolved
x/gov/init.go Show resolved Hide resolved
x/gov/interfaces.go Outdated Show resolved Hide resolved
x/gov/interfaces.go Outdated Show resolved Hide resolved
x/gov/msg.go Outdated Show resolved Hide resolved
ethanfrey added 11 commits May 17, 2019 18:55
@ethanfrey
Add generic interfaces for external logic: Executor / OptionLoader
edae92a
@ethanfrey
Updated governance proposal types to use new spike design
6ac9a2a
@ethanfrey
Started big refactoring, addressing build errors for new message format
5528767
@ethanfrey
Fix all build errors
fc86251
@ethanfrey
Use loader and executor in handlers, and cleanup
d9023fc
@ethanfrey
Updated protobuf for better electorate-election rule refs, fixed nami…
8eca033 
…ngs and numbers
@ethanfrey
Fixed up code to match new models
351b60d
@ethanfrey
Add election rule context when executing successful vote
813b5c5
@ethanfrey
Addressed most PR comments
c71803b
@ethanfrey
Remove use of weavetest
89c1f7f
@ethanfrey
Removed switch abuse, where it stood in for a series of if statements
5bcd30e 
There are some valid switches, where it just takes on value (enum)
and matches, and I left that. Note that these were used heavily
in Validate (to save a bit of typing) but would have to change anyway
for multi-error as many of these could be true and we would want to
match them all.

Sorry @alpe
@ethanfrey ethanfrey force-pushed the governance-actions branch from acdc427 to 5bcd30e Compare May 17, 2019 16:55
@ethanfrey ethanfrey marked this pull request as ready for review May 20, 2019 06:47
@codecov-io
Copy link

codecov-io commented May 20, 2019 •
edited
Loading

Codecov Report

Merging #633 into master will decrease coverage by 0.3%.
The diff coverage is 79.4%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #633     +/-   ##
========================================
- Coverage      69%   68.7%   -0.4%     
========================================
  Files         172     174      +2     
  Lines        9345    9330     -15     
========================================
- Hits         6452    6411     -41     
- Misses       2146    2163     +17     
- Partials      747     756      +9
Impacted Files Coverage Δ
x/gov/model_helper.go 100% <100%> (ø) ⬆️
x/gov/bucket.go 69.4% <100%> (ø) ⬆️
x/gov/interfaces.go 60% <60%> (ø)
x/gov/init.go 82% <73.9%> (ø) ⬆️
x/gov/msg.go 80% <73.9%> (-2.1%) ⬇️
x/gov/handler.go 73.8% <77.9%> (-4%) ⬇️
x/gov/model.go 84.4% <88.3%> (-6.2%) ⬇️
x/gov/context.go 92.3% <92.3%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea74448...9356e7c. Read the comment docs.

husio
husio approved these changes May 20, 2019
View reviewed changes
x/gov/app_helpers_test.go Outdated Show resolved Hide resolved
x/gov/bucket_test.go Show resolved Hide resolved
x/gov/codec.proto Show resolved Hide resolved
x/gov/handler.go Outdated Show resolved Hide resolved
x/gov/handler.go Outdated Show resolved Hide resolved
x/gov/handler.go
}

func NewTextResolutionHandler(auth x.Authenticator) *TextResolutionHandler {
// TODO: actually add a bucket to store resolutions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO should not be merged. Please either complete it or remove it as we try to remove all TODO from weave codebase.

There are more TODO comments in this file 👮‍♀️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is referenced in a comment in the PR.
I don't know whether to do it now, or just a quick follow-up PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you promise to address it this week, than 👍

x/gov/helpers_test.go Outdated Show resolved Hide resolved
x/gov/init.go Show resolved Hide resolved
x/gov/init_test.go Outdated Show resolved Hide resolved
x/gov/interfaces.go Show resolved Hide resolved
husio
husio reviewed May 20, 2019
View reviewed changes
Copy link
Contributor

@husio husio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where all those extra dependencies are coming from?

husio
husio approved these changes May 20, 2019
View reviewed changes
Copy link
Contributor

@husio husio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥇

@ethanfrey
Copy link
Contributor Author

Where all those extra dependencies are coming from?

They come from prototool. When I ran make lint.
Shall I remove that?

@husio
Copy link
Contributor

husio commented May 20, 2019

Yes, please. You can run go mod tidy

@ethanfrey ethanfrey merged commit a956183 into master May 20, 2019
@ethanfrey ethanfrey deleted the governance-actions branch May 20, 2019 10:01
This was referenced May 20, 2019
Merged
Closed
@alpe alpe mentioned this pull request May 23, 2019
Closed
7 tasks
@ethanfrey ethanfrey mentioned this pull request May 27, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@husio husio husio approved these changes

@ruseinov ruseinov Awaiting requested review from ruseinov

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ethanfrey @codecov-io @husio