Vault

Vault

7M Downloads

Economy: withdrawPlayer() inconsistency

desht opened this issue ยท 3 comments

commented

I just noticed an inconsistency with withdrawPlayer(), depending on the actual economy plugin being used: passing a negative amount to withdrawPlayer() works fine when you're using iConomy6, but fails if you do the same thing with BOSEconomy 7 (looking at the Vault source, I can see an explicit check for negative amounts in Economy_BOSE7.java).

Thought about creating a PR, but decided it was probably to create an issue for now, since changing the BOSEconomy implementation to allow negative amounts is potentially a (minor) API breakage.

commented

You should never pass negative values to either of these. This is not the proper way to perform a withdraw. There is never such a thing as negative quantities of money.
I'm not sure why the Bose7 implementation has the check, but it should be known as a developer that you should use the appropriate method depending on if you need to withdraw or deposit.

commented

Fair enough. That still leaves the point of consistency - i.e. withdrawPlayer() is happy to take a negative value when using ico6 but not when using BOSEconomy. In which case the ico6 handler (and possibly other handlers) should probably be updated to disallow negative quantities.

Possibly also worth adding a note in the javadocs that negative quantities are not allowed?

commented

looks like it was there on most of them. I updated the docs and added the transaction failure to the ones that were missing it. Also added it to bank deposit/withdrawals.