Spongeapi: 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.

[Erigitic]

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.

[MarkL4YG]

@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.

1. 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.

[MarkL4YG]

I would build upon @ryantheleach s suggestion. Each account having a U(U)ID and a Set<UUID> named owners would solve the problem of differentiating between virtual and player accounts. Player accounts can use the UUID of the player as their ID while virtual accounts have their own.
This also allows ANY account to have multiple owner allowing account sharing for friends and groups without having to create virtual accounts. Also I would suggest to associate a default-account property to players in order to determine which account to use for sign-shops e.g.

[ryantheleach]

@MarkL4YG "Player accounts can use the UUID of the player as their ID while virtual accounts have their own."

The usecases need to have different methods in order to disambiguate the account type. I'm tempted that account type should be a catalogue type, with 2 defaults.

This would allow the economy provider to know the difference between accounts, make them tax exempt, or whatever.

E.g. some types that come off the top of my head are Player, Shared, Virtual, Town, Shop, Faction,

[MarkL4YG]

@ryantheleach As I said: Having the account have a players UUID would uniquely specify the account as a player account. While the opposite clearly makes it a virtual one.

Another method would be making a Account interface with multiple subinterfaces like PlayerAccount, SharedPlayerAccount, ShopAccount, BusinessAccount and/or FactionAccount

Oh and just fyi: We have fancy quotes in markdown ^^ use ">"

"Player accounts can use the UUID of the player as their ID while virtual accounts have their own."

[ryantheleach]

A UUID is a UUID. We can't accurately predict the difference between a 'player' uuid and a uuid of something else.

Yes there are some oddities with the way that Mojang have their UUID scheme that makes it more likely to _guess_ correctly, but that would still mean relying on the cache, relying on a user being online, or having logged into the server previously.

The whole mess can be avoided by having seperate methods, so we may as well do so as it's good design, rather then relying upon economy implementations to guess correctly!

[Erigitic]

@ryantheleach How inaccurate is it to use getPlayer(UUID)? I've gotten nothing but accurate results from using it. Yes it relies on a user "having logged into the server previously", but the question is why would they have an account if they were not logged into the server before? I don't think they would, so I don't see that being an issue.

I do agree there may be an issue with not being able to differentiate between a unique and virtual account with the way the first option is currently described above. But I still think the first option is the way to go, it just needs a few changes that will allow for easier distinction between a player owned account and virtual accounts.

[ST-DDT]

@Erigitic well you have to consider the situation where there are to distinct servers (pvp & build) that only share the economy database.
User A only plays pvp thus he never joined the build server. However User B might want to send User A some funds while he is on the build server.

In that case the server would wrongly report the account as virtual although the player exists. Similar issues might occur with fake players that are used for trading/AI.

[pie-flavor]

@Erigitic The problem with that is that it requires that the implementation be able to distinguish between different types of UUIDs. And simply saying that getPlayer can be used makes the big assumption that that's appropriate for all economy plugins, while keeping there from being an alternative that needs a different system. An important thing to remember is that the API must cover as many use cases as possible, because plugins can only ever implement what the API allows.

[ST-DDT]

How about we just use String(16) ids for the accounts? If we start using UUIDs the users might just end up mixing them up with player UUIDs. In the following example I explicitly use Strings to separate it from player UUIDs,

Using String ids has the benefit, that you might use some meaningful account ids: "Bank_of_Minecraft" should be as valid as "012345678-9abcd-ef01-23456". (Or using P as prefix for non-virtual player accounts and V for virtual non-player accounts). The id of the account is always decided by the economy plugin, it might use the displayname to generate the id though. "Bank_of_Minecraft" -> "Bank_of_Minecr714".

As for virtual or none virtual accounts: Is that really important? Why would you change your (code's) behaviour based on that? There are one to two things economy plugins needs to do.

• Get the preferred account for a player (the thing that is used to simply pay money from and to by default)
• Get all accounts the player has access to (so that plugin can use that in an command argument, aka pay with the guild's account)

---

We just have to specify one thing. If there is an Economy plugin every player has an account if he at least joined once (and his account has not been deleted since). If he did not join the server it is possible to (automatically) create one for him. It is up to the economy plugin to auto-remove player accounts that match the default.

• Account getOrCreateAccountFor(UUID player)
• Account getOrCreateAccountFor(Player player)
• Optional<Account> getAccountFor(UUID player)
• Optional<Account> getAccountFor(Player player)
• Collection<Account> getAccessibleAccounts(UUID/Player, Set<Context> contexts)

If plugins need (virtual) accounts they can request them from the economy service

• Optional<Account> getAccountById(String id)
• Account createAccount() <-- Id is chosen automatically maybe adding a optional displayname and requesting plugin is possible here

It is NOT possible to create an account with a specific id yourself

---

(This is used to accomplish the AccountHolder @Aaron1011 suggested)

Also it is possible to register AccessDecisionManagers in the economy plugin.
The AccessDecisionManagers operate either on a accessor-category or the entire accessor-multimap (to be discussed)

• registerADM(ADM adm) <--preferred or
• registerADM(String category, ADM adm)

the ADM interface would look like this:

• Tristate hasAccess(Player/CommandSender accessor, MultiMap<String,String> accessRestrictions, Set<Context> contexts) <--preferred or
• boolean hasAccess(Player/CommandSender accessor, Set<String> accessRestrictions, Set<Context> contexts)

An example impl would look like this: (This would usually be provided by the economy plugin itself, others will be provided by the plugins themselves.)

````java
public static final String TYPE_PLAYER = "players";

Tristate hasAccess(Player accessor, MultiMap accessRestrictions, Set contexts) {
boolean access = accessRestrictions.get(TYPE_PLAYER).contains(accessor.getUUID().toString());
if (access) {
return Tristate.TRUE;
} else {
return Tristate.UNDEFINED;
}
}
````

Player have access to an Account if at least one ADM grants them access AND no ADM denies them access.
Plugins are not restricted to access any accounts.

---

(This is used to accomplish the AccountHolder @Aaron1011 suggested)

On accounts there is the option to grant access for different people/things.

• void grantAccess(String category, String key)
• boolean hasAccess(Player/CommandSender player)

Example usages:

• grantAccess(TYPE_PLAYER, player.getUUID().toString())
• grantAccess("guild", "TheStrongGuys")
• grantAccess("permission", "foo.bar.access")
• grantAccess("password", "123456") (/unlockAccount Bank_Of_Minecr714 123456 (for current session only))

The account data structure would look like this:
yaml 'Bank_Of_Minecr714': displayname: 'Bank_Of_Minecraft' owner: 'server-management-tools' money: 1337 access: permission: [ 'myserver.admin.account', 'myserver.accounts.Bank_Of_Minecraft' ] players: [ 'phpMyAdmin' ] group: [ 'admin' ]

This access list/ower could also be used to detect to which plugin/town/thing the account belongs, as requested by @ryantheleach .

The granted access values are stored along with the account (by the economy plugin) and will be used for economyService.getAccessibleAccounts(player).

---

Did I miss something? If yes maybe we should start with a list of requirements the economy API has to fulfil.

• get accounts by id / uuid (not the player one)
• get account for player
• get accessible accounts for player

[pie-flavor]

As for virtual or none virtual accounts: Is that really important? Why would you change your (code's) behaviour based on that?

What do you mean here? Depending on the plugin, virtual and player accounts could be treated vastly differently.

We just have to specify one thing. If there is an Economy plugin every player has an account if he at least joined once (and his account has not been deleted since).

Why is there a stipulation that every player has an account?

Also, these ADMs should involve Contexts.

[ST-DDT]

As for virtual or none virtual accounts: Is that really important? Why would you change your (code's) behaviour based on that?

What do you mean here? Depending on the plugin, virtual and player accounts could be treated vastly differently.

Could you give me an example?

We just have to specify one thing. If there is an Economy plugin every player has an account if he at least joined once (and his account has not been deleted since).

Why is there a stipulation that every player has an account?

Maybe I phrased that wrong. Every player that needs an account will automatically get one created, if needed.

Also, these ADMs should involve Contexts.

Yeah, thats also an option.

[pie-flavor]

Could you give me an example?

https://github.com/pie-flavor/Pieconomy

Maybe I phrased that wrong. Every player that needs an account will automatically get one created, if needed.

Again, though, that should be left up to the implementation. What if a player needs a permission to have an account? What if there are no player accounts? You're thinking of what is convenient for the most common use-case, but if we want to prevent Vault-like plugins from existing, we need to cover as many use-cases as possible.

---

Another general suggestion would be the concept of a suspended payment. ResultType.ACCOUNT_NO_SPACE exists for a reason, but it's not possible in _every_ circumstance to check that early and cancel the entire process if it fails, e.g. refunding an auction bid when the seller cancels the auction. If something like that becomes the case, then it becomes the using plugin's responsibility to suspend the payment and pay it at a later date, likely by command. I think that it should be the implementation's responsibility, especially given that this lets it do all sorts of things like tax overinput, or allow the player to choose what happens.

[ST-DDT]

Sorry to be blunt but your example is not usefull at all. It doesnt even become clear what is a virtual or none virtual account in your plugin.

If i assume the inventory to be a player account and the servers account to be a virtual account, then under which circumstances is it important that i can differentiate between them being virtual or not?

Is the server not charged any taxes? What about guild accounts? IMO its better to differentiate it by the owner instead of the account type.

Can i go to negative balance for only virtual accounts? IMO its up to the eco plugin to decide/config that and not something using plugins should care about.

If there is no space in the target account then the transaction will fail regardless of being a virtual account or player inventory.

---

As for players always having an account.
If the player has no permission to create an account then an always fail on transaction account could be returned.

This saves the using plugin the hassle to check does the user habe an account? Should it create (a virtual) one automatically? If it is not possible to create one or it isnt even attempted what is the using plugin supposed to tell the user what to do?

Well i can imagine plugins also handling that case, so as long as it is properly documented how plugins are supposed to deal in such situations, then i can also agree to players not having accounts.

---

While having more reasons for fail results is a good thing. It also places a burden on using plugins that they have to handle all those cases and display approproate messages to the user.
Maybe there should be an option to get the corresponding text for those reasons via API. This would also support similar messages across plugins.

[pie-flavor]

As for virtual or none virtual accounts: Is that really important? Why would you change your (code's) behaviour based on that?

I'm sorry, by "your" I thought you meant "the implementing plugin's", not "the using plugin's". However the implementing plugin, in that case, not only needs to know the difference upon use, but upon creation as well.

If the player has no permission to create an account then an always fail on transaction account could be returned

But this conveys nothing meaningful and plugins will be unable to handle the case on its own. What is the advantage over returning an Optional?

Maybe there should be an option to get the corresponding text for those reasons via API. This would also support similar messages across plugins.

Could be fiddly, but you could embed it in the TransactionResult.

[dualspiral]

Could be fiddly, but you could embed it in the TransactionResult.

Just as a thought, you could make the result enum a catalog type that implements a few methods:

• boolean isSuccessful()
• default Optional<Text> getErrorMessage() { return Optional.empty() }

Plugins that care about the specific error can check to see if the specific CatalogType was returned, either through an ID check or just the object, and plugins can add their own catalog types if they want to add implementation specific errors. Plugins that only care for success or failiure can check the relevant method - there potentially could be other methods on here, but I'm not overly sure what they might be right now!

[ST-DDT]

As for virtual or none virtual accounts: Is that really important? Why would you change your (code's) behaviour based on that?

I'm sorry, by "your" I thought you meant "the implementing plugin's", not "the using plugin's".

Oh. I see that my choice of words was confusing as well. I'm sorry.

However the implementing plugin, in that case, not only needs to know the difference upon use, but upon creation as well.

Thats why i have the getOrCreateFor(Player) methods as well.
AFAICT there is no need for more than one pure/physical player account (creating more accounts and granting the player access is still possible).

[pie-flavor]

Another note on the API - something like this would be nice to put in the API contract. Very useful for plugins which use commands, and applicable to almost all use cases of the API.

[Zerthick]

So one use case that I commonly run into (particularly in shop-type plugins) is I want to exchange an item in the player's inventory for funds in their balance. Currently, the only way to do this is to first withdraw the funds from the player's account, check to see if it succeeds. If the transaction is a success, offer the items to the player's inventory, then check to see if that succeeds (for example the player could be out of space), if it does succeed all is well, however if it does not you have to go back and manually reverse the transaction. It would be nice to simplify the process and instead have something like ''don't complete this transaction unless process X also succeeds". Or at the very least some way to easily reverse a transaction once it has occurred.

[pie-flavor]

@Zerthick I think that's something to fix in inventories rather than economies, tbh. Reversible transactions on inventories have much wider application.

[RysingDragon]

I'm all for UniqueAccounts and VirtualAccounts becoming one. While I was implementing the EconomyService I saw no need for them to have different behaviors in handling transactions.
It's also confusing (at least to me) on which one you should be using. It's definitely clear that VirtualAccounts would be Accounts which is shared amongst multiple players. In the javadocs it shows an example of a VirtualAccount being for a non-player Entity. My first impressions were that Entities should be able to have their own UniqueAccounts but after reading up more on the Economy API, I wasn't so sure.

[ryantheleach]

@Aaron1011 There seems to be quite a bit of discussion surrounding design.

This is slated for Milestone API7, Do you think it will be ready in time for inclusion?

Or should it be removed now considering this issue is still at the design phase and doesn't yet seem to have an associated PR?

[turtledude01]

I have a small request on the topic of economy. Currently I know of no way for a transaction to be canceled in any way. I know it might be a small case where this is usable, but flexibility is best. An example where this would be beneficial is where you have 3 plugins, the eco plugin, the plugin calling a transaction, and a 3rd plugin that checks each eco transaction by itself to make sure that some other thing is able to happen (In the case of my ServerEco plugin this would be checking that another account has enough funds to go through with the transaction) I'm aware that this could be implemented by the eco plugin itself, but I believe it would be better for the API to allow multiple eco plugins to handle if a transaction should go through or not.
I am also for option 2 of the original post as I see no reason that virtual accounts should matter in most cases, maybe just an optional setting where an account can be flagged as player owned or not.

[pie-flavor]

More thoughts: @Eufranio has given me another good example of missing functionality. The economy best practices make clear that 'ability to withdraw' is not as simple as balance - price >= 0. So what do you do if you want to visually indicate purchasability to the player? Your only option is to compare balance to price. I vote a canWithdraw and canDeposit method is included, so you don't have to do this check yourself.

[liach]

Can we add a loan api?

[parlough]

@liach Can you do a little mock-up of how you think that would look?

[liach]

So first, we will have a new interface, for example, LeaseAccount. It can have different methods, such as getInterestRate (unit, such as day, specified in javadocs) getInterestDue getLoanedAmount, etc.

[ST-DDT]

IMO loans are too dynamic for an API.
Especially the interest rate can be very server specific. Lets say every day a player is connected he has only half the interest.

IMO canWithdraw/diposit is sufficient.
Where the money comes from is irrelevant for the other plugins.

[ST-DDT]

Also there might be more than one CreditProvider ...

Lets start the other way round. What would be your usecase?

[liach]

It's okay if this idea is not incorporated into the API. Yeah, not too many people will use it,

[Flibio]

I agree, I believe loans should be implemented into their own plugins, not the SpongeAPI.

[MarkL4YG]

I just wanted to add the wish to remove business logic from the account interfaces.
While stuff like #getDefaultBalance should definitively managed by the currency the accounts itself shouldn't contain methods like deposit, withdraw, setBalanace and so on simply because I believe that's the job of the EconomyService and not the account representations.

On top of that most specifications explicitly state ___not to put business logic into the model tier___ but in its own.
Especially when working with different supported data storages this drives stuff increasingly complex.