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

Module accounts #4252

Closed
wants to merge 9 commits into from
Closed

Module accounts #4252

wants to merge 9 commits into from

Conversation

fedekunze
Copy link
Collaborator

@fedekunze fedekunze commented May 2, 2019

Implement modules accounts to hold coins on escrow. Originally part of #4210

Closes #3628


  • 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 marked this pull request as ready for review May 2, 2019 09:52
@fedekunze
Copy link
Collaborator Author

which other modules should implement a module account ? @rigelrozanski @alexanderbez @alessio

x/auth/account.go Outdated Show resolved Hide resolved
x/auth/account.go Outdated Show resolved Hide resolved
@alexanderbez alexanderbez requested a review from sunnya97 May 2, 2019 13:45
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.

Left a few bits of feedback.

.pending/improvements/sdk/3628-Add-modules-acc Outdated Show resolved Hide resolved
x/auth/account.go Outdated Show resolved Hide resolved
x/auth/account.go Outdated Show resolved Hide resolved
x/auth/account.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 2, 2019

Codecov Report

Merging #4252 into master will decrease coverage by 0.09%.
The diff coverage is 38.09%.

@@            Coverage Diff            @@
##           master    #4252     +/-   ##
=========================================
- Coverage   58.91%   58.81%   -0.1%     
=========================================
  Files         215      215             
  Lines       14482    14533     +51     
=========================================
+ Hits         8532     8548     +16     
- Misses       5310     5345     +35     
  Partials      640      640

fedekunze and others added 2 commits May 2, 2019 16:09
Co-Authored-By: fedekunze <31522760+fedekunze@users.noreply.github.com>
@@ -88,8 +88,12 @@ type GenesisAccount struct {
DelegatedVesting sdk.Coins `json:"delegated_vesting"` // delegated vesting coins at time of delegation
StartTime int64 `json:"start_time"` // vesting start time (UNIX Epoch time)
EndTime int64 `json:"end_time"` // vesting end time (UNIX Epoch time)

// module account fields
Module string `json:"module"` // name of the module
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation seems funky. Running gofmt -w -s cmd/gaia/app/genesis.go should do it

cmd/gaia/app/genesis.go Outdated Show resolved Hide resolved
cmd/gaia/app/genesis.go Outdated Show resolved Hide resolved
x/auth/account.go Outdated Show resolved Hide resolved
x/auth/account.go Outdated Show resolved Hide resolved
x/auth/account.go Outdated Show resolved Hide resolved
x/auth/account.go Outdated Show resolved Hide resolved
Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

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

Excellent job! I'll test this shortly, when @alexanderbez comments are addressed this is good to go for me.

alessio and others added 2 commits May 5, 2019 11:45
Co-Authored-By: fedekunze <31522760+fedekunze@users.noreply.github.com>
Co-Authored-By: fedekunze <31522760+fedekunze@users.noreply.github.com>
Co-Authored-By: fedekunze <31522760+fedekunze@users.noreply.github.com>
@fedekunze
Copy link
Collaborator Author

moved to #4255 as per our discussion

@fedekunze fedekunze closed this May 5, 2019
@fedekunze fedekunze deleted the fedekunze/module-accounts branch May 5, 2019 16:19
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.

Remove accounts to store deposits within Governance
3 participants