[v2] Petition to remove Account#canAfford

[A248]

User Story

The economy API has transitioned to use CompletableFuture. This is a welcome change. However, an asynchronous API entails a reconsideration of assumptions. Some approaches which may seem standard course for a synchronous API become ill-suited and undesirable where futures are concerned.

Particularly, it is a common code pattern to "check, then act." In an asynchronous context, however, "check, then act" is heavily flawed. It is a two-step operation, which entails race conditions and/or redundant operations. Sometimes these redundancies imply a heavy penalty, such as an additional database query. Ideally, we want the Treasury API to be structured to discourage massively wasteful method calls.

I want to consider the case of withdrawing a player's balance. A beginner might write code to the effect of "check if the player can afford the withdrawal, then perform the withdrawal." That is, call Account#canAfford, and if that succeeds, proceed with Account#withdrawBalance.

However, this procedure is incorrect and inefficient. The efficiency is obvious: two method calls implies two CompletableFutures and thus two database queries (for simplicity, I'm assuming a database backend). Moreover and more importantly, this approach is incorrect. We stipulated that an EconomyProvider can allow negative balances. Some EconomyProviders do.

As far as I can tell, canAfford is a convenience method for checking that the player has enough balance. I acknowledge there are some use cases for checking affordability without performing a withdrawal. However, checking affordability can be accomplished by using requestBalance directly and inspecting the value. Because developers are human, the method canAfford creates much opportunity for error.

Is your feature request related to a problem?

Account#canAfford is highly susceptible to misuse. It adds nothing of value to the API but presents a tempting pitfall for new developers. This method should be removed in favor of proper API use.

Describe the solution you'd like.

Remove the method. In my opinion, the method is too dangerous to exist on its own. Clients who wish to check affordability should use requestBalance.

Describe alternatives you've considered.

Heavily document the specific purpose of the method. You'd need to clarify that it should strictly be used for checking affordability, and not as a precondition before performing a transaction.

Additional information

In the SpongeAPI, something similar happened. The API's ban service originally contained methods isBanned and getBanFor. This made it easy for API users to accidentally call both methods to find a ban. Even the documentation itself made this mistake.

When the SpongeAPI's ban service was switched to use CompletableFuture, I argued that isBanned should be removed in favor of getBanFor, since isBanned is liable to misuse. https://github.com/SpongePowered/SpongeAPI/issues/2186

[lokka30]

Thank you very much for your post – I agree.

[MrIvanPlays]

I agree too. @Jikoo thoughts?

[Jikoo]

Agreed, probably far better off without it.

Even in a sync system "check, then request" has never made much sense to me - the economy is still going to make those checks prior to completing the transaction. It also seemed odd in Vault to check balance - amount >= 0 because that breaks support for economies with negative balances. In my mind the only use case for a pre-check is a bulk purchase option where multiple smaller transactions are aggregated by the requester (i.e. "buy as many dirt as possible") to optimize economy calls. Even then it gets messy.