search

ericgrandt / TotalEconomy

All in one economy plugin for Minecraft.
https://ericgrandt.github.io/TotalEconomy/
31 stars 33 forks source link

Economy withdraw and deposit do not follow behaviour of Vault JavaDoc #414

Closed Sytm closed 3 weeks ago

Sytm commented 2 months ago

According to the Vault JavaDocs it seems to be fine to withdraw (and deposit) an amount of zero, but this plugin rejects that transaction here

Vault JavaDocs: grafik

An example for zero to be a valid amount would be if the account of a player is frozen, so the transaction could still be denied, even if the player has sufficient funds.

ericgrandt commented 2 months ago

Hi @Sytm, this was done on purpose as I didn't see a need for Total Economy to handle running all the withdraw logic when the amount was zero. I'd be curious to know more about the use case you brought up and if this is indeed something we should look into supporting for the withdraw command.

Currently I don't envision Total Economy supporting frozen accounts, though if we did there'd be a way to check that state without having to use withdraw. Thanks.

Sytm commented 2 months ago

For some context: My plugin allows admins providing a custom formula, so they can customize the price increase for teleportations (like constant price, linear growth, exponential growth, etc). My plugin uses the Vault API to query and modify the amount of money a player has.

One user reported an issue that they cannot teleport when using a formula that evaluates to zero for the first teleport and then increases with each successive one, when using this economy plugin.

My issue here is just based on the technically correct behavior according the Vault API that it's implementations should follow (I would at least expect them to do), because some economy plugins might support freezing and have different behavior, while this one doesn't but shouldn't disallow zero withdrawals.

(You could also short circuit the execution of the deposit / withdraw methods in your Vault implementation on zero amounts with a successful result, without altering the behavior of the rest of your plugin, although that would be a somewhat dirty solution)

ericgrandt commented 2 months ago

Thanks for the context @Sytm. This makes sense. I have an idea that'll allow for the Total Economy implementation of Vault to allow zero as a value while also preventing it from other areas in the plugin where zero shouldn't be allowed (e.g. Total Economy's pay command). Best of both worlds I think. I'll let you know when that's in place. Thanks again!

And just so I remember when I'm making changes for this, this change should also be made to the deposit functionality as well.

ericgrandt commented 2 months ago

Hi @Sytm, give this new release a shot: https://modrinth.com/plugin/total-economy/version/0.12.1. Thanks

ericgrandt commented 3 weeks ago

Hi @Sytm, following up again to see if this is resolved. If there's no response in the next couple of days I'll close this issue to avoid it getting too stale, though feel free to reopen if it is still an issue. Thanks.

Sytm commented 3 weeks ago

Sorry for not answering, but spinning up a MySQL server for testing is always a bit annoying. I have tested the edge case and it works now without any problems.

I also noticed that the released version string and the version in your plugin.yml do not match