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

Validator groups #22

Merged
merged 19 commits into from
Apr 6, 2021
Merged

Validator groups #22

merged 19 commits into from
Apr 6, 2021

Conversation

ethanfrey
Copy link
Contributor

Closes #5

@ethanfrey ethanfrey requested a review from maurolacy March 30, 2021 19:42
@ethanfrey ethanfrey marked this pull request as ready for review March 30, 2021 19:42
@ethanfrey
Copy link
Contributor Author

Happy for a first review when you can. I need to implement the validator set calculation logic, but all the basics should be wired together

Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

I noticed in passing that, according to RegisterValidatorKey comments, No two operators may have the same consensus_key. That is not currently being enforced in execute_register_validator_key AFAIK.

contracts/tgrade-valset/src/contract.rs Show resolved Hide resolved
contracts/tgrade-valset/src/msg.rs Show resolved Hide resolved
contracts/tgrade-valset/src/contract.rs Outdated Show resolved Hide resolved
contracts/tgrade-valset/src/contract.rs Outdated Show resolved Hide resolved
contracts/tgrade-valset/src/contract.rs Show resolved Hide resolved
contracts/tgrade-valset/src/contract.rs Show resolved Hide resolved
@ethanfrey
Copy link
Contributor Author

I noticed in passing that, according to RegisterValidatorKey comments, No two operators may have the same consensus_key. That is not currently being enforced in execute_register_validator_key AFAIK.

Very good point, I will add that check - duplicates there could cause issues (attack vectors) in other code.

Also, when you can review the last 2 commits, please do so, I am sure you can find some issues there as well. Then I will do a push with all the suggested improvements. Very helpful review.

Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

Still reviewing calculate_diff.

contracts/tgrade-valset/src/contract.rs Show resolved Hide resolved
contracts/tgrade-valset/src/contract.rs Outdated Show resolved Hide resolved
contracts/tgrade-valset/src/contract.rs Outdated Show resolved Hide resolved
contracts/tgrade-valset/src/contract.rs Show resolved Hide resolved
Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

lgtm. Some tests are missing, but I agree we can add them in another iteration.

@ethanfrey ethanfrey merged commit 9cda168 into main Apr 6, 2021
@ethanfrey ethanfrey deleted the validator-groups branch April 6, 2021 11:57
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.

Control validator set with group contract
2 participants