ArcanePlugins / Treasury

🏦 A powerful multi-platform library for next-level plugin integrations.
https://hangar.papermc.io/ArcanePlugins/Treasury
Other
57 stars 13 forks source link

Wrap Treasury's Economy API to Sponge's Economy API inside treasury-sponge #266

Closed MrIvanPlays closed 1 year ago

MrIvanPlays commented 1 year ago

@Jikoo can you do a review once more please?

MrIvanPlays commented 1 year ago

Thanks. Waiting on @lokka30

MrIvanPlays commented 1 year ago

I've spent the last 2 hours (or more) adding support into AnnotationConfig to be able to extend config classes and add more config options by such method. That said, I've added an option to disable the sync.

A248 commented 1 year ago

I don't mean to be too much of a contrarian. However, I want to know about how this PR interacts with economy plugins providing their own implementation of the Sponge economy API.

Let's say I write a multi-platform economy plugin implementing the Treasury API. I support Sponge. On Sponge, moreover, I want to provide my custom implementation of the Sponge Economy API that handles matters slightly differently from the Treasury adapter implementation (contained in this PR). For example, maybe I want to provide a customizable or more sophisticated way to handle Sponge requests that would otherwise cause me to use CompletableFuture.join().

This is not a theoretical concern, at least I do not think so. The reason is because of lessons learned maintaining my plugin LibertyBans. In LibertyBans, I support some Sponge ban management-related APIs in a way that is slightly different than merely wrapping my own API. Therefore, I imagine someone writing an economy plugin might have similar analogous reasons for wanting to implement the Sponge Economy API separately.

MrIvanPlays commented 1 year ago

@A248 There's a config option to disable Treasury wrapping the Treasury Economy API onto Sponge's Economy API. I don't know how will Sponge react to 2 ProvideServiceEvent listeners that provide an economy implementation, so I don't know what will happen if both this wrap is enabled whilst another plugin is providing.

MrIvanPlays commented 1 year ago

Upon a glance over the code, things seem excellent. Before I grant this a seal of approval, I would like to make sure that Treasury's player and non player account types are comfortably interchangeable with Sponge's unique and virtual account types. Thank you to Ivan and others who have contributed to this PR for the fantastic work, I hope the Sponge community will be able to more easily adopt and incorporate Treasury with this adapter layer.

Yes they are. A wrapped by Treasury VirutalAccount's String identifier can be easily converted back into a NamespacedKey using the fromString method, from which you can retrieve a NonPlayerAccount. As for UniqueAccounts and PlayerAccounts, the identifier is a UUID, so it's the same business there.

lokka30 commented 1 year ago

Lovely docs over at Sponge here, yep it'll be perfect. Thanks so much Ivan, A2 and Jikoo. Squash and merge when you're ready 👍🏻