Bugfix and typofix #4212
Bugfix and typofix #4212
Conversation
Non-bug; only effect that the other instanceof is always evaluated.
This omission is currently not disruptive, since the next case clause only contains a break. Also converted a few `return false` to breaks, to align with the rest of the switch statement.
Without the break statement, the execution would continue through the subsequent case clauses until it encountered a break, executing `checkArgument` calls meant for `REIMBURSEMENT_MAX_AMOUNT`. More specifically, the bug would cause a failed check in the case where `inputValueAsCoin.value <= 200000000` is false.
|
@ManfredKarrer I merged too soon. I didn't look at the two commits touching the DAO until after merging. I think it's correct but sensitive stuff. |
| @@ -144,6 +144,7 @@ private void validateBsqValue(Coin currentParamValueAsCoin, Coin inputValueAsCoi | |||
| case REIMBURSEMENT_MIN_AMOUNT: | |||
| checkArgument(inputValueAsCoin.value >= Restrictions.getMinNonDustOutput().value, | |||
| Res.get("validation.amountBelowDust", Restrictions.getMinNonDustOutput().value)); | |||
| break; | |||
There was a problem hiding this comment.
I think the correct version would be:
case REIMBURSEMENT_MIN_AMOUNT:
case COMPENSATION_REQUEST_MAX_AMOUNT:
case REIMBURSEMENT_MAX_AMOUNT:
checkArgument(inputValueAsCoin.value >= Restrictions.getMinNonDustOutput().value,
Res.get("validation.amountBelowDust", Restrictions.getMinNonDustOutput().value));
checkArgument(inputValueAsCoin.value <= 200000000,
Res.get("validation.inputTooLarge", "200 000 BSQ"));
break;
Also MinAmount must not exceed the 200 000 BSQ.
| @@ -285,14 +285,15 @@ private boolean isBtcOutputOfBurnFeeTx(TempTxOutput tempTxOutput) { | |||
| // We always have the BSQ change before the burned BSQ output if both are present. | |||
| checkArgument(optionalOpReturnIndex.isPresent()); | |||
| if (index != optionalOpReturnIndex.get() - 1) { | |||
| return false; | |||
| break; | |||
There was a problem hiding this comment.
I don't see any problem with that change but as it is a pure "cosmetic" change I prefer to not touch the code. Some could argue the early return is more readable. I don't have a strong opinion and the break is more consistent but evaluating benefit / risk just does not justify the change IMO even it seems a trivial and very low risk change.
| } | ||
|
|
||
| // Without checking the fee we would not be able to distinguish between 2 structurally same transactions, one | ||
| // where the output is burned BSQ and one where it is a BSQ change output. | ||
| long blindVoteFee = daoStateService.getParamValueAsCoin(Param.BLIND_VOTE_FEE, tempTxOutput.getBlockHeight()).value; | ||
| return availableInputValue == blindVoteFee; | ||
| } | ||
| break; |
There was a problem hiding this comment.
If the semantic changes in this file are reverted, take care not to revert adding this break.
There was a problem hiding this comment.
I'm doing a revert of the whole PR. There was an easy to use button to do the revert.
Best make a new PR with the good changes.
Two omitted breaks in switch statements and one typo. One of the omitted breaks is serious-ish, info in commit messages.