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

Decouple auth/vesting #4486

Closed
4 tasks
alessio opened this issue Jun 5, 2019 · 11 comments · Fixed by #5040
Closed
4 tasks

Decouple auth/vesting #4486

alessio opened this issue Jun 5, 2019 · 11 comments · Fixed by #5040
Milestone

Comments

@alessio
Copy link
Contributor

alessio commented Jun 5, 2019

Decouple the vesting functionality from x/auth and create a new x/vesting module which extends x/auth functionality.


For Admin Use

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

What are you plans here @alessio? A sub-package under x/auth?

@alessio alessio added this to the v1.0 LTS milestone Jun 5, 2019
@alessio
Copy link
Contributor Author

alessio commented Jun 6, 2019

Vesting types and functions should live either in a subpackage within x/auth or in a standalone x/vesting package. I have a slight preference for the latter - in my view if we manage to decouple completely it won't make much sense anymore to have in x/auth.

@alexanderbez
Copy link
Contributor

I'd actually argue for the former is possible. I don't see that clear of a separation...they're nearly identical types apart from a few extra fields and methods.

@rigelrozanski
Copy link
Contributor

Yeah this is purely out of style what we do or don't group sub-modules - I would argue because vesting relies heavily on the auth infrastructure, that it should be a subpackage of auth - but also there isn't really a problem with having it as a standalone module. kinda indifferent tbh

@alessio
Copy link
Contributor Author

alessio commented Jun 6, 2019

I'm fine with either really

@fedekunze
Copy link
Collaborator

I like the idea of a x/vesting module in case we want to extend the current vesting functionality. This could allow us to say create new vesting accounts on a live chain instead of defining them just on genesis

@alexanderbez
Copy link
Contributor

It really depends on on the common ground between it and auth. Even with extending it, having it a sub-package could make more sense. Alternatively, it could just import x/auth and be a standalone package. Maybe that does make more sense.

@rigelrozanski
Copy link
Contributor

So in that case - vesting-auth would effectively be extending auth and vesting-auth would be the module imported by the application

@alexanderbez
Copy link
Contributor

Correct @rigelrozanski

@fedekunze
Copy link
Collaborator

@rhuairahrighairidh @karzak is the Kava team working on this issue?

@karzak
Copy link
Contributor

karzak commented Sep 11, 2019

Yes, I have a PR that's about ready. I will rebase it on #5017 and submit today/tomorrow.

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 a pull request may close this issue.

5 participants