Split EconomyProvider interface or remove hasXSupport methods

[Jikoo]

Edit: After the discussion here, I feel like the hasXSupport methods are a bad idea and I would actually like to see Treasury drop the idea of not implementing the entirety of its interfaces.

It really doesn't make sense to me to have 50% of the methods be designed to not be implemented. Anyone who does offer Treasury support should be able to offer bank support with minimal extra work due to how similar the accounts are.

If we do truly want a method to check for support that should likely just be rolled into the API version system, not the provider interface(s). The javadocs for each method state the API version it was added in and the provider states the API version it was programmed against.

Original writeup: I would like to see EconomyProvider separated into several interfaces.

As a developer, it makes it a lot easier to work with player/bank accounts if I know exactly what is available to me rather than constantly re-checking if banks are supported and whatnot.

As a server owner, the increased granularity makes it possible to invent a solution that more exactly suits my needs by selecting the exact bank and player account implementations I want.

• TreasuryApiProvider
  • Provide Treasury API version implemented, owning plugin
• CurrencyProvider extends TreasuryApiProvider
  • Provides currency definitions and handles parsing
  • Should define currency management (creation, deletion, modification, etc.)
  • Treasury should probably implement and provide at ServicePriority.Lowest
  • See #40
  • Definitely needs improvement, but outside the scope of this issue
• AccountProvider extends TreasuryApiProvider
  • Provides a requestCurrenciesInUse or similar method
  • Allows CurrencyProvider to support undefined currencies, making migration easier.
• PlayerAccountProvider extends AccountProvider
  • Provides PlayerAccounts
• BankAccountProvider extends AccountProvider
  • Provides BankAccounts
  • Methods should not clash with PlayerAccountProvider
  • Makes it possible to create a generic MyEconomyImpl implements PlayerAccountProvider, BankAccountProvider to mimic Vault style interface usage

[MrIvanPlays]

this is overengineering like why do you need this when everything is put in a single class? That's like adding 1 more axle to a bicycle and saying "it makes it handle better" although it works perfectly fine without it.

[Jikoo]

Just because Vault puts everything into one class doesn't mean it's correct to do so. The goal is to be the most flexible and easy to use API, not the most Vault-like.

For currencies in particular it doesn't make sense to have each economy rewrite currency handling. Treasury can provide a generic that doesn't store data but does handle parsing and handoffs. Maybe rather than an interface it should just be a registry system though, that part definitely needs some brainstorming.

For economies it's just flat out easier. If an economy doesn't implement a major feature set, the feature set should fall through to the first available option, not hard fail. Say I'm running a big Bungee network. I probably have a system backed by a database for player accounts, however, banks for server-specific factions and whatnot don't need (and maybe need to actively avoid) that persistence. The increased granularity allows me to install a bank plugin with local storage. I can run my player account and bank account plugins side by side with no trouble. Alternately, as a developer I want to offer a feature that requires bank support. Unfortunately, users tend to install economies that don't have bank support. Instead of iterating over all available economies to find the first that offers banks I can just directly attempt to obtain the active BankProvider.

[MrIvanPlays]

Vault puts everything into one class

I never mentioned vault in my reply. Stop throwing it everywhere. When I'm talking about such stuff I'm talking from API design perspective.

We need changes about currencies sure, not every economy provider should rewrite it.

If an economy doesn't implement a major feature set

You have a return value to play with. Have it return a response object with an enum value saying to the plugin that obtains and uses the API that the feature they try to use is not supported. Simple.

I can run my player account and bank account plugins side by side with no trouble.

No one said that you need service modularization to create 2 plugins that depend on the same API but use different features of that API. You can go without it. OR EVEN BETTER: have a 1 plugin which does both of these things.

Alternately, as a developer I want to offer a feature that requires bank support.

If bank support is not baked in, either bake it yourself or don't enable that feature, announce that it can't be enabled in the console, make the server owner to change the economy provider to a provider which has bank support.

[Jikoo]

I never mentioned vault in my reply. Stop throwing it everywhere. When I'm talking about such stuff I'm talking from API design perspective.

This API is trying to supersede Vault. Shortcomings in Vault are always relevant.

You have a return value to play with. Have it return a response object with an enum value saying to the plugin that obtains and uses the API that the feature they try to use is not supported. Simple.

Providing a bunch of implemented methods that specifically are not implemented is completely illogical. An API should not be designed to be partially implemented. That's bad design.

[MrIvanPlays]

Providing a bunch of implemented methods that specifically are not implemented is completely illogical. An API should not be designed to be partially implemented. That's bad design.

Then force all providers to implement them all by not giving them the option to not implement them. Or even better: have a economy provider by default in treasury or in a child plugin of treasury which implements it all and has a lot of features in order of storing data. Problem solved.

This API is trying to supersede Vault.

Trying to supersede any project doesn't mean you have to completely not take anything from it.

Shortcomings in Vault are always relevant.

Not really. We're particularly discussing API design here and not "vault does x, we shouldn't".

[Jikoo]

Then force all providers to implement them all by not giving them the option to not implement them.

That's a pretty good option. Unfortunately, even though banks aren't very different from player accounts (a couple extra fields and maybe a different table/storage location) if the goal is ease of adoption that will bar the vast majority of economy plugins initially. Ease of adoption vs having the absolute cleanest API design possible.

Or even better: have a economy provider by default in treasury or in a child plugin of treasury which implements it all and has a lot of features in order of storing data. Problem solved.

The whole point of APIs like this is that it offers choice to developers and users. Shipping a full default implementation negates that choice.

This API is trying to supersede Vault.

Trying to supersede any project doesn't mean you have to completely not take anything from it.

Shortcomings in Vault are always relevant.

Not really. We're particularly discussing API design here and not "vault does x, we shouldn't".

Not sure what your angle is here. The whole point of this API is that Vault isn't good enough, it's even in the readme. Also, why would you not use a competitor as a point of reference? This isn't a new concept, we should learn from past mistakes.

[lokka30]

I'm against bloating Treasury with an economy plugin inside it. Leaving the economy out of Treasury will keep it without bloat. That's a benefit of Vault and Reserve.

There are a few paths we have here:

• Don't have any economy in Treasury.
• Have an economy in Treasury that has full API support but zero commands (apart from /treasury reload also reloading the economy's data)
• Have a basic economy in Treasury

[MrNemo64]

I don't think that Treasury should have an economy implementation, make another separate project that implements treasury yes but not having them both in the same. I do like the idea of separating the economy provider into multiples interfaces

[lokka30]

@Geolykt and @NoahvdAa, do you have any thoughts on this?

[MrIvanPlays]

I will make my opinion clearer, don't.

[MrNemo64]

Maybe we can have a Bukkit like class. We split the Economy Provider into interfaces and then have a class with delegated methods for all the diferent interfaces. This way you can work with all of them at ones or with the interfaces

[Jikoo]

The problem with a multi-provider is that the developer then has to remember to register all of the provider options. If they do it themselves by combining the interfaces they're much less likely to forget to do so. Treasury could technically handle that for them, but then you'd run into issues where maybe the economy allows the user more granular control and intentionally didn't register the bank service or something.

After thinking about it I really would prefer that Treasury just not have a concept of "no bank support" because it really doesn't make sense to me to have 50% of the methods be designed to not be implemented. Anyone who does offer Treasury support should be able to offer bank support with minimal extra work due to how similar the accounts are.

I'm torn on the currency front, but I feel like that's a separate whole issue.

[Geolykt]

I am neutral on the splitting part. Bank support is indeed not much more of a difference to regular player-based accounts.

Jikoo's last comment raises a fair question - does it make sense to even not support any feature? Later on, as the impl becomes outdated compared to the API it may make sense, but at release it should be the best to assume that the implementation supports all features that are dictated by the API. This would not be much of an issue if the plugins were to be written from the ground up with 0% vault compat in mind.

[lokka30]

I agree that every economy provider should have bank support. @MrIvanPlays, what do you think on this specifically?

[MrIvanPlays]

I think of yes. I mean is it a proper economy provider if it doesn't implement everything?

[MrIvanPlays]

An idea I've just got in my mind. Whenever a economy provider tries to register into treasury, make a test of all features. If it fails at any feature, do not register it.

[NoahvdAa]

An idea I've just got in my mind. Whenever a economy provider tries to register into treasury, make a test of all features. If it fails at any feature, do not register it.

-1 on this, I don't like the idea of creating unnecessary calls to the provider, especially when the has____Support methods already exists.

[MrIvanPlays]

I mean we can clean it up after these calls. Its just an idea I am not forcing it to go into the implementation.

[Jikoo]

I've edited OP to reflect my current stance on this for clarity.

An idea I've just got in my mind. Whenever a economy provider tries to register into treasury, make a test of all features. If it fails at any feature, do not register it.

I don't hate it, but what do we do about things like a misconfigured SQL connection? Generally those plugins may reconnect on a custom reload command, but if we've unregistered their economy they may not realize they need to re-register it. It may be more trouble than it's worth.

Re: unnecessary calls to the provider: A couple queries to test functionality would be a drop in the bucket compared to regular player usage. I don't think a single account creation (and then ideally deletion - probably should add deletion API) and maybe a balance modification would break the server's back.

[Geolykt]

Re: unnecessary calls to the provider: A couple queries to test functionality would be a drop in the bucket compared to regular player usage. I don't think a single account creation (and then ideally deletion - probably should add deletion API) and maybe a balance modification would break the server's back.

Should this ever be done it should be important that the accounts are deleted after the tests as otherwise functionalities such as baltop or a global balance view would be broken

[MrIvanPlays]

I don't hate it, but what do we do about things like a misconfigured SQL connection?

We can force the provider to call a method with an input whether it is ready for a feature test or not. If it doesn't call it after x amount of time, throw an exception.

[lokka30]

What do you all think about implementing #43 to solve this issue? @Jikoo @MrIvanPlays @Geolykt @MrNemo64

[MrIvanPlays]

It would be better to have it as an enum instead of splitting the economyprovider since it would be more difficult for implementing.

[MrNemo64]

What do you all think about implementing #43 to solve this issue? @Jikoo @MrIvanPlays @Geolykt @MrNemo64

I like the idea. Would like to develop the idea in that issue.

About splitting the provider into several providers, could we have all providers splited as described by Jikoo or something like that and then have the economy provider extend all those interfaces? This way if you want to use the separated parts them you just cast, if you want all use the EconomyProvider. This would allow also for the implementation to have its logic separated by default

[Jikoo]

What do you all think about implementing #43 to solve this issue?

Overall this would be closed by any decision on it, ignoring, a removal, a split of the interfaces, or a refactor to a new location.

Personally would prefer to see Treasury drop the idea of not supporting methods and not split the interface - banks are so similar to regular accounts that if you support one you may as well do both.

Having an inbuilt "is method available in version" method could be handy, but hopefully the dependent plugin would just check API version and warn instead (or Treasury could handle it if API version required > current version). I feel like the API version system where version is easily accessible by implementations should be sufficient instead of adding additional back and forth of feature checks.

[lokka30]

What do you all think about implementing #43 to solve this issue?

Overall this would be closed by any decision on it, ignoring, a removal, a split of the interfaces, or a refactor to a new location.

Personally would prefer to see Treasury drop the idea of not supporting methods and not split the interface - banks are so similar to regular accounts that if you support one you may as well do both.

Having an inbuilt "is method available in version" method could be handy, but hopefully the dependent plugin would just check API version and warn instead (or Treasury could handle it if API version required > current version). I feel like the API version system where version is easily accessible by implementations should be sufficient instead of adding additional back and forth of feature checks.

I agree that a majority of Treasury's features should not be optional anymore, especially bank accounts. Although I am still concerned that there could be things added into the API that are considered 'optional' for plugins to implement. How would we manage this? Go with the 'has ... support enum' route? Best of both worlds I think.

[lokka30]

Merging this issue into #43.