Vault2: Economy learns about UUIDs & BigDecimals. #138
Conversation
This commit adds UUID support to Vault, allowing plugins to bypass the
OfflinePlayer methods which result in Bukkit trying to resolve a player
to associate with the OfflinePlayer (via the server playercache and if
that player doesn't exist via Mojang.)
This is incredibly useful for any plugin which wants to have an Economy
account that isn't associated with a player. This includes Towny,
Factions, Shops plugins and others.
Most importantly: having UUID methods will give these plugins an avenue
to update from using the String accountName methods deprecated since
Vault 1.4, which doesn't result in slow OfflinePlayer creation.
AbstractEconomy has been updated so that the various Economy plugins
supported internally by Vault will have support for the new methods in
the same manner as when the OfflinePlayer methods were added.
Small javadoc typos have also been fixed up (extra {'s, an additional
{@link, etc.)
These methods are meant for players, non-players and anything with a UUID.
To match the PR I have opened at the Vault repo, which has had the native economy plugin support removed, the VaultAPI plugin no longer requires the AbstractEconomy class. Removal means that this Pull Request no longer calls Bukkit.getOfflinePlayer(uuid), making this much safer.
Add jetbrains annotations & remove mention of specifc economy plugins.
All classes are the same as their v1 selves.
New methods include: - UUID-focused methods replacing the Name and OfflinePlayer methods. - getUUIDNameMap() which makes the economy plugin able to supply a Map of UUIDs and last-known-names on request. This will be used to replace the code in Vault which converts between economy plugins (and is currently only able to convert accounts belonging to players which have logged in.) The @nullable annotation is used here in order to declare that the last-known-name of the account is allowed to be null. - getAccountName(UUID) which will return the last-known-name of an account or null. Other Changes: - Minor changes to javadocs.
|
Thanks for the work @LlmDl, on the surface (from my phone) this looks great. I imagine much like the past PR that there will be plenty of discussion to follow. I very much appreciate the time and work on this! Given the size I'll probably review it relatively slowly but it's on my radar. |
|
The Vault2 Chat, Permission and EconomyResponse classes are identical to their originals, to make it easier. With the deprecated methods in Vault2 Economy gone, that class has actually become smaller than Vault1 Economy. Vault1 Economy has a bunch of javadoc fixes to go along with the @deprecated message added to Economy, Chat, Permission and EconomyResponse. Looking forward to any feedback your team has. |
|
Definitely think this would add some much-needed updates to Vault. |
| @@ -0,0 +1,995 @@ | |||
| /* This file is part of Vault. | |||
There was a problem hiding this comment.
I know this was mentioned within the PR itself, but we definitely should be updating both Chat and Permission in the same fashion as Economy. We can do this now or we can do so in another PR, either is fine by me.
There was a problem hiding this comment.
I would like to keep this PR concise and limited to just Economy. I think the code related to Chat and Permissions should be improved by chat and permission plugin authors.
Co-authored-by: Morgan <cerealcable@users.noreply.github.com>
Co-authored-by: Morgan <cerealcable@users.noreply.github.com>
- Added renameAccount(UUID, String). - Remove @nullable annotation and remove repo/dependency from pom.xml. - Fixed typo in javadoc in Economy.
|
@cerealcable It's been a couple months. How's your free time looking these days? |
|
@cerealcable it's been 7 months, how are things looking? If the team needs some new blood when it comes to maintaining Vault and shuffling development along, it might be time to consider adding a team member. I'm still running into a constant stream of new economy plugins that do not use Vault properly, and would benefit from a UUID-based system. |
|
A much needed change |
|
I would also like to express my interest in this, just to give this a little more visibility. |
|
I believe that the project is kinda dead, someone else needs to take it and continue it or create a fork (like NuVotifier for Votifier for example) |
I literally told @LlmDl exactly that several days ago but he insisted on making it happen on this repo. I think just like you and don't see vault ever merging this, so please someone just make Vault2 or call it whatever you want. As long as it's a drop in replacement for Vault, it's only gonna be beneficial. |
|
A similar UUID support has been part of my Vault-"fork" Tresor for a while now. (E.g. see its Economy class). While I like some approaches that are done in these "vault2" classes here (like dropping the "player") I don't believe that they are the way to go. They still fall short in certain places (e.g. missing BigDecimal support which is the industry standard for financial transactions, also not async-capabilities. Tresor's "ModernEconomy" tries to address that as well as expose supported Features like banks, UUIDs etc)
I see similar issues (and had them when I tried to address the UUID-issue here in the past) and have invested some time recently in getting Tresor up and running and did some first live tests with it recently so I personally would say that's a possible future alternative. If anyone has input on the services API design for it then now is the time ;) |
|
I guess there was never any update posted here. I did end up emailing cerealcable, and he did make a commitment to getting this merged. He did join the Towny discord's #plugin-development channel where we would discuss things further. Things have petered out again though. re: Tresor's fork: BigDecimal support is another thing I've had mentioned to me as well. Definitely worth having that. What Tresor is working on looks nice but it is missing the ability to get the entire map of accounts for converting between eco plugins, and to lookup a name associated with a UUID. It is also missing the ability to call for the name behind a UUID being changed. I would also argue that using |
Yeah, that's another one of those interesting approaches in this PR (although getting all accounts should be considered an extremely expensive operation which should probably be a (potential async) stream) but I personally don't think that mapping should be part of the economy at all (I plan to add a separate UUID mapper service in Tresor) but conversion between different plugins offering the same services is indeed an issue I have spent some time thinking about before, gonna add that to my todo. Ideally that wouldn't be part of a single service but a more general concept...
That's a good point, it was designed under the assumption of this mainly being players but of course a plugin might want to represent other types of accounts like that. I guess technically the whole bank concept is a bit weird, I might need to look into that more for the new Tresor economy service, maybe that can just be unified into one system but I feel like this goes a bit far for this issue/repo ;) |
|
Yes, one of the pitfalls of the current Vault is that it can only convert OfflinePlayer accounts (making it basically useless for Towny servers where the majority of the money is in Town/Nation banks.) |
|
Conflict resolved. There's still much interest in this from economy plugin developers. Can this be merged in the new year @cerealcable ? |
I second this. |
|
Imo before moving forward with this the vault2 Economy interface should be adjusted to use BigDecimal instead of double for balances. Doubles will cause rounding errors (and with that potential exploits) hence why it's pretty standard to use BigDecimal for any kind of financial data in Java. Seeing as this aims to establish a completely new standard Economy provider it feels like the right point to make such a change. |
|
How about, if this can get merged I will make it BigDecimal before any release is made to the public? It feels like a separate PR to me. Original PR was opened Apr 4 2021. |
|
I don't think that's the right approach. Because if this is merged then it will be public and people will to start use it, snapshot or not. People tend to run dev builds in this space after all and not doing all breaking changes at once just results in a mess imo. |
I do think this is a great approach. BigDecimal is the baseline for monetary values. |
|
Alright this week when I have time I will bigdecimal-ify this PR. |
future. Major clean up of javadocs in the Economy class.
|
@Phoenix616 Ready for review again. |
LlmDl
changed the title
|
Please merge this. It is very much needed |
|
Please merge this, much needed change |
|
as an economy dev, i can confirm, that we need urgently uuid support ! |
|
Hello, we're fast coming up on the 3 year anniversary of this PR coming about. Can I get a review @Phoenix616? |
|
We definitely need asynchronous functions with CompletableFutures as well. Due to potential transaction floods, this has become a crucial requirement in contemporary environments. Can you maybe include this in your pull request as well? @LlmDl |
Well while my review doesn't really mean much regarding whether or not this PR gets merged (as I have no part in Vault development) I definitely like the move to BigDecimal. (I have to agree with lightPlugins though that futures would be nice too but that would open up a whole other can of worms that I'm unsure people/this project are ready for but I guess doing it now could avoid a breaking change later when it becomes apparent that it would be a good addition to the API) One issue I see in general is that moving to a whole different interface could hinder adoption, maybe an automatic conversion from the original interface could help here? (I experimented with automatic registration of bridge service providers but I'm still unsure if that's the best route to go in general) |
|
Thanks for the input. @cerealcable @Sleaker can we get this merged before we hit 3 years? |
This is the second incarnation of my efforts to introduce Vault's Economy to UUIDs, and replace the OfflinePlayer used within Vault1's Economy.
For a breakdown of the previous PR:
Economy has had a bit of an overhaul. It should still be familiar to anyone updating their Vault1 plugin though.
Most plugins already have something in them that will be using an OfflinePlayers UUID to store and read to/from their DB.
Aside from replacing their OfflinePlayer methods with UUID methods, they will see three new methods missing from my previous PR.
getUUIDNameMap()is a method that they probably already have somewhere in their plugin. When Vault is asked, their plugin will supply a Map keyed to UUIDs of every account they have in their Economy. This will be used in Vault to convert between economies. More details in this commit message.getAccountName(UUID)is a method that will be indispensable for error messaging and other feedbacks, allowing plugins to avoid using the UUID to get a OfflinePlayer to get the OP's Name.renameAccount(UUID uuid, String name)is a method that allows a plugin to alter a name associated with an account.December 2023 edit:
The PR now replaces the usage of doubles with BigDecimal, to better support fool proof math/prevent rounding errors.
The javadocs in the Economy class have had a complete overhaul.
Banks have had their methods UUID-ified and their amounts BigDecimal'd.