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

x/stake: Add another function for UpdateValidator if cliff validator doesn't exist #2293

Closed
ValarDragon opened this issue Sep 10, 2018 · 7 comments
Assignees

Comments

@ValarDragon
Copy link
Contributor

Summary

I feel like we can significantly improve the code clarity for UpdateValidator if we separate concerns there, and have a different method if the cliff validator already exists, and doesn't already exist. This is evidenced by there being 3 "if cliff val exists" calls in the function.

/cc @rigelrozanski @cwgoes @alexanderbez

@alexanderbez
Copy link
Contributor

Agreed this could be improved a bit. ref: #1923

@rigelrozanski
Copy link
Contributor

good idea!

@cwgoes
Copy link
Contributor

cwgoes commented Sep 11, 2018

Concept ACK. Maybe there are other places in staking we could similarly simplify.

@rigelrozanski
Copy link
Contributor

rigelrozanski commented Sep 13, 2018

Possibly superseded issue if we remove cliff validator all-together....

@cwgoes
Copy link
Contributor

cwgoes commented Sep 13, 2018

Possibly superseded issue if we remove cliff validator all-together....

Maybe, but I think the cliff validator logic is a substantive performance improvement for the majority of likely real calls of UpdateValidator.

@rigelrozanski
Copy link
Contributor

@cwgoes let's discuss this more - of course we want the performance - however I think there may be other ways which we could get (nearly) the same performance increase without having all the headache and code complexity of the cliff validator - see thoughts here: #2312

Let's talk about it in today's call however

@ValarDragon
Copy link
Contributor Author

ValarDragon commented Sep 14, 2018

Closing in favor of #2312, this becomes kind of irrelevant since we no longer have a reserved "cliff validator" seat, its just the 100th element of the arr, and we're switching to the end-block stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants