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

staking/api/commission: fix possible panic in validation check #2763

Merged
merged 1 commit into from
Mar 13, 2020

Conversation

ptrus
Copy link
Member

@ptrus ptrus commented Mar 13, 2020

The validation check would panic whenever the number of bound steps was greater than rate_steps + 2.

@ptrus ptrus added c:bug Category: bug c:staking Category: staking labels Mar 13, 2020
@ptrus ptrus self-assigned this Mar 13, 2020
@ptrus ptrus force-pushed the ptrus/fix/comission-check branch from 083453c to f2dee24 Compare March 13, 2020 10:02
@Yawning
Copy link
Contributor

Yawning commented Mar 13, 2020

Add an error case test?

@ptrus
Copy link
Member Author

ptrus commented Mar 13, 2020

Add an error case test?

Not sure wym. The panic happened regardless if the bounds were vaild or not. The updated test does test this (e.g. it panics on old code & no error in new code).

Also the specific error is already tested here: https://github.com/oasislabs/oasis-core/blob/f2dee245b174ac0384908010a498e0fe9a9650a3/go/staking/api/commission_test.go#L521


Edit: i guess it would be nicer if i add a specific test case for this with an explicit comment on what it is testing. Done.

@codecov
Copy link

codecov bot commented Mar 13, 2020

Codecov Report

Merging #2763 into master will decrease coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2763      +/-   ##
==========================================
- Coverage   62.52%   62.41%   -0.12%     
==========================================
  Files         385      385              
  Lines       36406    36406              
==========================================
- Hits        22763    22721      -42     
- Misses      10748    10818      +70     
+ Partials     2895     2867      -28
Impacted Files Coverage Δ
go/staking/api/commission.go 98.66% <100%> (ø) ⬆️
go/worker/compute/executor/committee/state.go 74.07% <0%> (-11.12%) ⬇️
go/oasis-node/cmd/debug/byzantine/byzantine.go 35.01% <0%> (-5.04%) ⬇️
go/consensus/api/grpc.go 59.3% <0%> (-4.66%) ⬇️
go/worker/common/host/protocol/protocol.go 65.67% <0%> (-4.48%) ⬇️
go/worker/compute/txnscheduler/committee/node.go 60.18% <0%> (-4.02%) ⬇️
go/worker/common/p2p/p2p.go 67.56% <0%> (-3.61%) ⬇️
go/worker/compute/executor/committee/node.go 62% <0%> (-2.96%) ⬇️
go/oasis-node/cmd/debug/byzantine/merge.go 74.68% <0%> (-2.54%) ⬇️
go/runtime/client/client.go 66.28% <0%> (-2.3%) ⬇️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe064e6...ddcf8ee. Read the comment docs.

@ptrus ptrus force-pushed the ptrus/fix/comission-check branch 2 times, most recently from 1ad6444 to 9c5bbb7 Compare March 13, 2020 10:25
The validation check would panic whenever the number of bound steps was
greater than `rate_steps + 2`.
@ptrus ptrus force-pushed the ptrus/fix/comission-check branch from 9c5bbb7 to ddcf8ee Compare March 13, 2020 10:27
@ptrus ptrus merged commit 8001e4d into master Mar 13, 2020
@ptrus ptrus deleted the ptrus/fix/comission-check branch March 13, 2020 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c:bug Category: bug c:staking Category: staking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants