-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Bugfixes related to switch statements #4250
Bugfixes related to switch statements #4250
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.
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.
Manfred [0] pointed out that COMPENSATION_REQUEST_MIN_AMOUNT and REIMBURSEMENT_MIN_AMOUNT should be checked for not exceeding 200k BSQ too. [0] https://github.com/bisq-network/bisq/pull/4212/files/4ec6bac658ca5c8e3cd4d4a9e0ac7207bac8799b..fb76fd65481e882ae0e232c3d8c1814e29a9e878#r420249962
For the Also adding a test for validator is (I believe) technically easy (in contrast to some UI changes). |
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.
utACK
@ManfredKarrer I still need your review on this to be able to merge, but I think this is a reasonable and safe change.
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.
utACK
This is a second go at #4212, which was reverted. The previous PR was fixing some problems with switch statements and a typo where a short-circuit boolean operator was used without need. This PR is the same as the last one, but the semantic/cosmetic changes (why the original PR was reverted) are removed and it also implements a suggestion by @ManfredKarrer to add a (forgotten) check.