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

update x/genutil to match module spec #4757

Merged
merged 8 commits into from
Jul 24, 2019
Merged

Conversation

fedekunze
Copy link
Collaborator

@fedekunze fedekunze commented Jul 22, 2019

closes #4751

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added a relevant changelog entry: clog add [section] [stanza] [message]

  • rereviewed Files changed in the github PR explorer


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@fedekunze fedekunze added Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity. WIP labels Jul 22, 2019
@fedekunze fedekunze changed the title reestructure genutil module update x/genutil to match module spec Jul 22, 2019
@fedekunze fedekunze mentioned this pull request Jul 22, 2019
4 tasks
@codecov
Copy link

codecov bot commented Jul 23, 2019

Codecov Report

Merging #4757 into master will increase coverage by 0.01%.
The diff coverage is 68.42%.

@@            Coverage Diff             @@
##           master    #4757      +/-   ##
==========================================
+ Coverage   53.87%   53.89%   +0.01%     
==========================================
  Files         270      270              
  Lines       17230    17229       -1     
==========================================
+ Hits         9283     9285       +2     
+ Misses       7263     7260       -3     
  Partials      684      684

@fedekunze fedekunze marked this pull request as ready for review July 23, 2019 08:56
@fedekunze fedekunze added R4R and removed WIP labels Jul 23, 2019
Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

Left some minor nit picks. Thanks for doing this! Will be a lot easier to work with genutils with the updated structure

x/genutil/client/cli/gentx.go Outdated Show resolved Hide resolved
x/genutil/client/cli/collect.go Outdated Show resolved Hide resolved
x/genutil/alias.go Outdated Show resolved Hide resolved
x/genutil/types/codec.go Show resolved Hide resolved
x/genutil/gentx.go Outdated Show resolved Hide resolved
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

ACK

// ModuleCdc defines a generic sealed codec to be used throughout this module
var ModuleCdc *codec.Codec

// TODO: abstract genesis transactions registration back to staking
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make an issue for this if one does not already exist :)

@@ -0,0 +1,41 @@
package types
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this worthy of having to be under a specific directory?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this file? or the types package as as a whole

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is, these are type definitions here, might as well add them to remain consistent with our file organization

Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

ACK

@rigelrozanski rigelrozanski merged commit bfb6445 into master Jul 24, 2019
@rigelrozanski rigelrozanski deleted the fedekunze/4751-genutils branch July 24, 2019 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update genutils to match module spec
4 participants