Economy API refactor

[Aaron1011]

There's been some discussion recently about refactoring the Economy API. The following changes have been proposed:

• Combine VirtualAccount and UniqueAccount into one. All accounts would have:
• A unique UUIDs (not the same as a player's UUID).
• A list of UUIDs of owners of the account. These would usually be player UUIDS, but could also be provider-supplied UUIDS with provider-defined semantics
• An optional 'friendly name'

Another idea is to leave the system as-is, but make the following changes:

• A new AccountHolder interface will be introduced. This would represent a player or some-plugin defined account holder (e.g. a GriefPrevention town).
• Virtual accounts now have a list of owning AccountHolder UUIDs

Additionally, we're planning on greatly expanding EconomyTransactionEvent. Instead of being read-only, the event should be able to be modified by plugins. This could take the form of adding on a 'tax', or even completely changing the transaction amount.

We'd welcome feedback from anyone interested in the Economy API - especially economy plugin developers such as @Flibio and @Erigitic

[Zerthick]

So one thing that I've always been curious about is that the Economy API currently provides the means for getting and creating accounts but not deleting them. This can lead to cases of "orphaned" accounts that no longer have an owning entity associated with them. For example, in my playershop plugin each shop is given its own account to store its funds, however if a shop is deleted I have no way of deleting the account associated with it. Perhaps this could be garbage collected by the plugin implementing the API itself i.e. if an account has been inactive for X amount of time it will automatically be deleted, but it seems like plugin developers should be given the ability to delete accounts they know will never be used. Thoughts?

[ryantheleach]

I don't think deleting accounts that have a balance other then zero is a good idea. server admins may want to view them afterwards to fix things up.

[Zerthick]

True, but that seems like something that would be enforced by the implementing plugin, not the API itself.

[ericgrandt]

I'm 100% for unique accounts and virtual accounts becoming one. Currently both the unique and virtual account classes are doing the same exact thing for the most part, they are just identified differently (UUID or String identifier). So why not combine them? I think this would simplify the handling and storage of accounts, not that's it too terribly complicated now, but I do think this would be a great change regardless. The friendly name, in my opinion, is a must for the virtual accounts. It would make it a lot easier to see what the account is associated with whether it be the name of a town or shop, just to name a few things. Lastly, I'm all for more control over the EconomyTransactionEvent.

A question regarding the "Unique UUID". Would this be auto generated or would the plugin developer have to set it themselves?

[Aaron1011]

A question regarding the "Unique UUID". Would this be auto generated or would the plugin developer have to set it themselves?

My idea is that we could have two overloads of the creation method - one which takes a UUID, and one which auto-generates one.

[Flibio]

I agree with Erigitic, everything mentioned sounds good to me. It will require modifying data storage (at least in EconomyLite), but overall I think it will still work out for the better.

[MarkTwoFive]

@Erigitic I would go as far as forcing all accounts to have a friendly name for administrative purposes.

[Zerthick]

I also support merging unique and virtual accounts into one class. My shop plugin for example generates a unique UUID for each shop, it seems silly to not utilize that for a unique account, even though it seems the "correct" way to handle it would be to create a virtual account for the shop.

[ItsDoot]

I'd like to also propose adding versions of the methods in EconomyService and Account that return CompletableFutures, similar to #1524.

[ryantheleach]

https://github.com/SpongePowered/SpongeAPI/blob/871fcd1b9cddc82fefce9fdecc1ac690ae6762dd/src/main/java/org/spongepowered/api/service/economy/transaction/TransactionResult.java

and

https://github.com/SpongePowered/SpongeAPI/blob/b1aa5e04aae06a86d50b646daad06bab697d1707/src/main/java/org/spongepowered/api/service/economy/transaction/TransferResult.java

are due for a rename, since Transactions are now used elsewhere in the API, the names are now really confusing.

Why are there 2 different results? was the event originally designed to be mutable / routable for changing the destination of a transfer?

Essentially the usecases that arn't covered perfectly by the API are:

1. Disambiguating virtual accounts creation in the API, vs accounts that are defaulted for players.

This needs to happen, as a common usecase is player balances having a default value, where as creating unlimited virtual accounts could result in default funds being applied.

2. Creating town accounts, that are managed by one or more players, and being able to fetch the owners back. Towns have UUID's but need to be virtual. at the same rate, it could be useful to find all accounts that are flagged as towns that are associated with a given player? Unless that's considered plugin specific API.

[pie-flavor]

I support option 2 of @Aaron1011's original suggestion. I would also say that a use case not covered by the current API, which I ran into in Pieconomy, is that TransferResult does not indicate which player the transfer failed on.