General Economy API Improvement Proposals

[lokka30]

EconomyFailureReason

• [x] Complete removal of this enum
  • Ivan and I believe this enum should be removed. See here for reasoning.
  • [x] Approvals: @lokka30, @MrIvanPlays
  • [x] Resolved

Currency

• [x] supportsNegativeBalances

  • This method should be removed.
  • We discussed earlier that Treasury does not care about negative balances, that is all handled by the economy provider. If the economy provider thinks that a given account is not capable of receiving a given transaction, then they will just prevent the transaction from going through.
  • [x] Approvals: @lokka30, @MrIvanPlays
  • [x] Resolved

• [x] getStartingBalance(UUID):

  • I am thinking of moving this to Account and instead making it getStartingBalance().
  • This means that each account can be given a different starting balance if the economy provider wants that.
  • The current implementation allows only players to have custom starting balances.
  • It also seems quite weird to have a context-centric method (dealing with player UUIDs) in an environment which does not really directly with any economy accounts.
  • [x] Approvals: @lokka30, @MrIvanPlays
  • [x] Resolved

EconomyProvider

• [x] retrieveAllAccountsPlayerIsMemberOf and retrieveAllAccountsPlayerHasPermission
  • These need to return account objects, not IDs.
  • theoretically, the economy provider can override these methods and have more optimised approach since they have direct access to their database, but in practice, nobody is going to do this
  • people want the accounts - they are going to retrieve those anyways - so we need to just return those
  • as an API, Treasury should not need to care how this method is implemented so long it gets the job done as the method contracts specify
  • [x] Approvals: @lokka30, @MrIvanPlays
  • [x] Resolved

Miscellaneous

• [x] #250
  • [x] Approvals: @lokka30, @MrIvanPlays
  • [ ] Resolved

---

Miscellaneous To-do's

• [x] In EconomyProvider#unregisterCurrency's javadoc, I need to fix a spelling mistake, "IF" instead of "If".

[lokka30]

I'd like to hear feedback from @MrIvanPlays before I implement my proposed improvements.

[MrIvanPlays]

Currency#supportNegativeBalances and Currency#parse shall stay. As for the results, the Response class is designed around that philosophy you say. Please tell me when you're ready with all edits so I can state opinion on the other stuff too.

[lokka30]

Wow, you were quick to reply - thank you! I'm all done with my edits.

Currency#supportNegativeBalances and Currency#parse shall stay.

Why do you believe each are beneficial? I've outlined my reasoning against their persistence.

As for the results, the Response class is designed around that philosophy you say

Great - so I'll need to scan through the javadocs and note down which ones infringe on that. It's probable that we forgot to update some of the javadocs when we made updates to the Response class.

[MrIvanPlays]

parse iirc is for human friendly formatting of the big decimal so it can be sent as /balance for example.

As for the supportsNegativeBalances, plugins which utilize the api shall know whether the currency supports them or not.

As for all the failure reasons, i think everything that is not used by treasury shall be removed and delegated to the api implementations.

[lokka30]

parse iirc is for human friendly formatting of the big decimal so it can be sent as /balance for example.

this was format(BigDecimal, Locale).

parse is meant to take that formatted string back into a BigDecimal.

As for the supportsNegativeBalances, plugins which utilize the api shall know whether the currency supports them or not.

Does it matter for plugins to know if the provider supports negative balances or not? My non-final opinion is that plugins will create transactions and try to send them through, the provider will refuse them if they are not applicable for an account (e.g. they withdraw too much, causing a negative balance).

As for all the failure reasons, i think everything that is not used by treasury shall be removed and delegated to the api implementations.

I see the merit in this, although I am still lenient to expand this enum with constants which would be used by almost all economy providers. These small things can contribute to making it easier to implement an economy provider.

[lokka30]

Update: Added a suggestion to remove the OTHER_FAILURE constant in the original post.

[MrIvanPlays]

Expanding it makes bloat and sets some standarts about specific failure reasons, which I dont want to happen. Even better idea just popped in my head - remove this enum altogether from the api.

[lokka30]

Update: I've discussed an idea regarding getStartingBalance(UUID).

Expanding it makes bloat and sets some standarts about specific failure reasons, which I dont want to happen. Even better idea just popped in my head - remove this enum altogether from the api.

I think that's a great idea. Failure reasons can also be more descriptive if people are not inclined to use predefined ones. I'll add that idea to the top and state that we're both happy with that

[MrIvanPlays]

I agree with the proposed changes about getStartingBalance

[lokka30]

@Jikoo and anyone else reading, I hope you're doing well 🙂

If you would like to have a vote on any of these proposals (and also make ones of your own), please leave a comment. If Ivan and I are the only ones active on this issue, then I'll assume that we can move forward on whichever is agreed upon.

[MrIvanPlays]

More problems I've found: EconomyProvider#retrieveAllAccountsPlayerIsMemberOf and EconomyProvider#retrieveAllAccountsPlayerHasPermission: why are the account objects mapped to their keys instead of directly returning the account objects? @MrNemo64 had proposed these methods awhile ago, why is that how it is? Imo these methods should either be removed or rewritten.

[MrNemo64]

More problems I've found: EconomyProvider#retrieveAllAccountsPlayerIsMemberOf and EconomyProvider#retrieveAllAccountsPlayerHasPermission: why are the account objects mapped to their keys instead of directly returning the account objects? @MrNemo64 had proposed these methods awhile ago, why is that how it is? Imo these methods should either be removed or rewritten.

The why of this methods was to allow a consumer to know all the accounts where a player could, for exameple, retrieve money. This way any consumer can allow the player to choose between its avilable accounts to do whatever the player wants.

The why of returning the key instead of the account is not forcing the provider to load the accounts. We don't know what a consumer will do this the returned accounts, maybe it just saves the keys somewhere, why should we assume that if these methods are called, the consumer will use the accounts? They may only want the keys to save them somewhere so why force the provider with the overhead of loading the account? The consumer can just then retrieve the accounts if they need them.

[MrIvanPlays]

The why of returning the key instead of the account is not forcing the provider to load the accounts.

They are already loaded because there's no other way of doing these checks. Imo these 2 methods are pretty much bloat and we don't need them in the general api. If any plugin wants to do these checks, they shall do them manually by themselves.

[MrNemo64]

The why of returning the key instead of the account is not forcing the provider to load the accounts.

They are already loaded because there's no other way of doing these checks.

Why would the accounts be loaded? We don't know how the provider will do the checks? Lets say the provider has an sql database and the permissions are stored in a table PlayerCanDo with the columns:

• player: UUID of the player
• account: key of the account
• permission: a permission that a player has on this account, lets say it's an enum
• state: the tristate as a number (1 = TRUE 0 = FALSE -1 = UNSPECIFIED) The provider may load the whole account and then check the permissions, or just do a querry to this table and just retrieve the account, something like

  select account
  from PlayerCanDo
  where state = 1 and player = ? and permission = ?

  The provider here does not worry about loading the whole account and everything, just checks the permissions

This was my logic when proposing to have these methods implemented this way back then, since I haven't been able to keep up with the API changes I don't know if this reasoning is still valid for the case

[MrIvanPlays]

They've been always implemented like that:

• retrieve account
• isMember/hasPermission
• store all ids into a list
• when everything is done, return the list with ids

it's a little bit more complicated because of the mapping to ids and waiting for all the accounts to respond but that's basically what happens when you call one of the two methods in the default implementation. Some economy providers may choose to override them and make them more efficient, however my opinion here is that 99% of the economy providers won't implement them, simply because they're default methods.

[MrNemo64]

That's true but like, why would you like, revoke the posibility for optimization here. Wouldn't make more sense having a mothod to retrieve all accounts in a given collection of keys? And allow the provider to do as they please. What's the reason as to why these methods should return the account and not the key? Is it just comodity or am I missing something?

[MrIvanPlays]

That's true but like, why would you like, revoke the possibility for optimization here.

Where's the optimization where you retrieve all non player account ids, then request the accounts, then check whether a specific unique id is member/has permission, then map the accounts back to ids? Combine that with a plugin which needs the accounts and then you have id -> account -> id -> account! What optimization are we talking about?

What's the reason as to why these methods should return the account and not the key?

No need for id -> account -> id mapping, just id -> account.

OR even better - remove these methods from the general API altogether and make the one who needs this information do it by themselves.

[MrNemo64]

Where's the optimization where you retrieve all non player account ids, then request the accounts, then check whether a specific unique id is member/has permission, then map the accounts back to ids? Combine that with a plugin which needs the accounts and then you have id -> account -> id -> account! What optimization are we talking about?

I said revoke the possibilite for optimization, not removing the acctual optimization, at the moment there is no optimization, just a default implementation that works as good as a default implementation can work

OR even better - remove these methods from the general API altogether and make the one who needs this information do it by themselves.

That means that the consumer has to, basically, retrieve all accounts and check one by one, right?

[MrIvanPlays]

That means that the consumer has to, basically, retrieve all accounts and check one by one, right?

That means that the consumer can do it however they please.

[MrNemo64]

That means that the consumer has to, basically, retrieve all accounts and check one by one, right?

That means that the consumer can do it however they please.

What other ways are? At the moment I can only think of that one. How would you do it?

[MrIvanPlays]

That means that the consumer has to, basically, retrieve all accounts and check one by one, right?

That means that the consumer can do it however they please.

What other ways are? At the moment I can only think of that one. How would you do it?

First of all I can't think of any reason why do I need this information. Second, if I really need it, I'd do it on a selected few non player accounts, rather than ALL the non player accounts.

[MrNemo64]

First of all I can't think of any reason why do I need this information.

Lets say you are a shop plugin and the player is paying, you want to display the accounts that the player has permission to retrieve from and show them to the player so they can select the one to use

Second, if I really need it, I'd do it on a selected few non player accounts, rather than ALL the non player accounts.

By a selected few do you mean that you, somehow, preselect from all the existing non player accounts a group and check permissions on them? If so, how would you preselect them?

[MrIvanPlays]

Lets say you are a shop plugin and the player is paying, you want to display the accounts that the player has permission to retrieve from and show them to the player so they can select the one to use

Yeah I guess then querying all the accounts might be needed. In that case, the methods have to be delegated to the economy implementation, where there are countless ways to improve the current implementation.

[MrNemo64]

Lets say you are a shop plugin and the player is paying, you want to display the accounts that the player has permission to retrieve from and show them to the player so they can select the one to use

Yeah I guess then querying all the accounts might be needed. In that case, the methods have to be delegated to the economy implementation, where there are countless ways to improve the current implementation.

So now, returning key or account. I personally would return the keys, like with the provided default implementation, and maybe add a (maybe default) method that given a collection of keys, retrieves the accounts. Let the provider optimize as much as they want

[MrIvanPlays]

DO YOU EVEN READ WHAT I'M SAYING? THE CURRENT METHODS ARE INEFFICIENT BECAUSE OF ALL THIS CONVERSION. Returning the accounts would make much more sense since we've already queried them, and then let the plugin do whatever they want to do. In any case, I think making these methods non-default would be much more viable option.

[MrNemo64]

DO YOU EVEN READ WHAT I'M SAYING? THE CURRENT METHODS ARE INEFFICIENT BECAUSE OF ALL THIS CONVERSION. Returning the accounts would make much more sense since we've already queried them, and then let the plugin do whatever they want to do.

Yes I read you, current methods are default implementations that are as good as they can be. With the default implementation we just helped the provider with the implementation of the API. This does not mean that we forbid them from using a better suited for their case implemenation. And if the provider does want to load everuthing -> check -> get keys, the accounts will, most likely, be cached so a call to retirve them latter would not take that much anyway

[MrIvanPlays]

current methods are default implementations that are as good as they can be.

Nope. We can just return the accounts and they'd become even better.

[lokka30]

I don't mind which way we go on that particular issue. However, one thing I have to support Nemo's side (keeping it returning keys instead of Accounts) is that an optimisation-happy implementation can override the method and return a list of namespaced keys from a single database query rather than using Treasury's default implementation for that method which requests a bunch of accounts.

On another note which kind of sits on Ivan's side (switching it to return Account objects), why would someone need to only query the keys when they will need to turn them into account objects eventually anyways in order to do anything with given accounts? The only thing I can think of is that a plugin can grab the list of keys returned from this method, filter them by their namespace (so they can grab the accounts that they've created using their namespace), and then retrieve a list of account objects in a more optimised way.

To resolve both sides, what if the method returned Account objects, but there was also another method which returned just the keys?

[MrIvanPlays]

This is the current default method. It's the same for has permission, just that we don't call isMember but hasPermission. See the issue now clearly? Where's the optimization you both talking about? If you want to keep the methods let's delegate them to the implementation. That way we can keep it return ids instead of accounts.

[MrNemo64]

I don't mind which way we go on that particular issue. However, one thing I have to support Nemo's side (keeping it returning keys instead of Accounts) is that an optimisation-happy implementation can override the method and return a list of namespaced keys from a single database query rather than using Treasury's default implementation for that method which requests a bunch of accounts.

On another note which kind of sits on Ivan's side (switching it to return Account objects), why would someone need to only query the keys when they will need to turn them into account objects eventually anyways in order to do anything with given accounts? The only thing I can think of is that a plugin can grab the list of keys returned from this method, filter them by their namespace (so they can grab the accounts that they've created using their namespace), and then retrieve a list of account objects in a more optimised way.

To resolve both sides, what if the method returned Account objects, but there was also another method which returned just the keys?

I'm ok with that

[MrNemo64]

This is the current default method. It's the same for has permission, just that we don't call isMember but hasPermission. See the issue now clearly? Where's the optimization you both talking about? If you want to keep the methods let's delegate them to the implementation. That way we can keep it return ids instead of accounts.

Again, we do not have an optimised method, just a default implementation that it's as good as it can be The optimization is potential, a potencial implementation that providers can do. As you said right now, the optimization relies on delegating to the provider but we provide a default implementation in case the provider doesn't really care about that

[lokka30]

This is the current default method. It's the same for has permission, just that we don't call isMember but hasPermission. See the issue now clearly? Where's the optimization you both talking about? If you want to keep the methods let's delegate them to the implementation. That way we can keep it return ids instead of accounts.

Treasury's default impl is not optimised, but if the economy provider wants to (I myself don't care about these micro-optimisations yet for my own economy provider), they are fully able to override Treasury's default impl with their own method which can do everything in a single fast database query.

[MrIvanPlays]

No it's not as good as it can be. We can return the account objects directly and remove the unnecessary mapping back to id.

[lokka30]

Note, we have a few options on the table -

1. Remove these methods
2. Don't change - keep them returning IDs
3. Change - make them return Account objects
4. Rename the current one to clearly return IDs, and make another method which returns Accounts

I'm inclined to go for number 4. The account one provides a lot of convenience for people using the API, and the ID one allows plugins to do things in a more optimised way if they happen to call this method a lot.

[MrIvanPlays]

and the ID one allows plugins to do things in a more optimised way if they happen to call this method a lot.

HOW MANY TIMES DO I HAVE TO TELL YOU BOTH THERE IS NO OPTIMIZATION WHEN YOU'RE DOING ID -> ACCOUNT -> ID. The optimization is one and only and that is to make it return Accounts. Another option I can do is via using the java services to make an implementation inside the treasury plugin itself which would use my process api to further optimize this and make them all calls properly parallel.

[lokka30]

Treasury is doing ID -> Account -> ID. The economy provider is not forced to do that conversion whatsoever. It can make a pretty simple SQL query to return a list of account IDs which happens to have the given player as a member. The only reason Treasury even does the ID -> Account conversion is because it needs to check the members of each account and what permissions they have. The economy provider can do that without using the Account object at all.

[MrIvanPlays]

Treasury is doing ID -> Account -> ID. The economy provider is not forced to do that conversion whatsoever. It can make a pretty simple SQL query to return a list of account IDs which happens to have the given player as a member. The only reason Treasury even does the ID -> Account conversion is because it needs to check the members of each account and what permissions they have. The economy provider can do that without using the Account object at all.

YES HELLO FINALLY What I've been saying in the last HOUR is that we can remove the default implementation and force the economy provider to implement it.

[lokka30]

Treasury is doing ID -> Account -> ID. The economy provider is not forced to do that conversion whatsoever. It can make a pretty simple SQL query to return a list of account IDs which happens to have the given player as a member. The only reason Treasury even does the ID -> Account conversion is because it needs to check the members of each account and what permissions they have. The economy provider can do that without using the Account object at all.

YES HELLO FINALLY What I've been saying in the last HOUR is that we can remove the default implementation and force the economy provider to implement it.

I didn't say that - I suggested that the method was fine, since Treasury provides the default way to do it which works great but is less efficient than what can be done if economy providers want to override that with their own logic where they have direct access to their database.

[MrIvanPlays]

But the problem is that 99% of the economy providers won't, and eventually, some kid installs some plugin which calls this multiple times with some of the common treasury economy providers, which don't reimplement these methods, and that kid sees it's coming from that plugin and that plugin developer says "go to treasury", and suddenly, it's our problem now. Treasury should not deal with plugin/economy provider issues, treasury should deal with issues directly related to the treasury plugin and to issues with the API design. THAT'S IT.

[lokka30]

let's go for your idea Ivan - returning accounts without converting them IDs as you say, nobody is going to override and optimise any of our methods and people are looking to get the account object via that method anyways ~~However, I still want to remove the parse method.~~

[lokka30]

Resolved by #247. I will resolve the javadoc issues in another PR.