-
Notifications
You must be signed in to change notification settings - Fork 47
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 scenario #287
Validator scenario #287
Conversation
alpe
commented
Jan 22, 2019
•
edited
edited
- Add/ Remove validator
- Prevent empty validator updates crashing tendermint
- Fixes validator initializer not called
Codecov Report
@@ Coverage Diff @@
## master #287 +/- ##
=======================================
+ Coverage 70.8% 71% +0.1%
=======================================
Files 143 144 +1
Lines 6281 6307 +26
=======================================
+ Hits 4451 4482 +31
+ Misses 1392 1384 -8
- Partials 438 441 +3
Continue to review full report at Codecov.
|
cmd/bnsd/client/client.go
Outdated
return b.conn.Validators(nil) | ||
} | ||
|
||
func (b *BnsClient) GetValidators(height int64) (*ctypes.ResultValidators, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a sentence of an explanation would make this method perfect.
💅 What do you think about using -1
as a height
value to get the current validators list (instead of GetCurrentValidators
method).
I think it would be much better if this package would not return ctypes
structures. Current API leaks out it's dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add docs 👍
The server functionality for this is built into Tendermint. Unlike the other client functions I would need to substitute all the nested ctypes, too. Our use case is very much limited to admin operations with the cluster. I will extract this code into an admin client to separate this from "normal" client operations.
cmd/bnsd/scenarios/main_test.go
Outdated
} | ||
`, addr) | ||
`, addr, multiSigContractAddr, addr) | ||
println(appState) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👮♂️
x/validators/errors.go
Outdated
@@ -10,13 +10,17 @@ import ( | |||
// ABCI Response Codes | |||
// x/update_validators reserves 40 ~ 49. | |||
const ( | |||
CodeEmptyDiff uint32 = 40 | |||
CodeWrongType = 41 | |||
CodeEmptyDiff uint32 = 40 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💅 They are const
, so it might be better to leave them without a type. Compiler can do an extra work for us then.
x/validators/msg.go
Outdated
strings.ToLower(m.Pubkey.Type) != "ed25519" { | ||
return errors.WithCode(errInvalidPubKey, CodeInvalidPubKey) | ||
} | ||
// can power be negative? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't think it would make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me neither. Added a condition to prevent this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯