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

EconomyTransactionResult #268

Closed MrIvanPlays closed 1 year ago

MrIvanPlays commented 1 year ago

I don't think returning the new balance is a good indication of whether a transaction succeeded or failed. Since we don't have Account#canAfford anymore, there's no clear indication of whether a transaction succeeded. Hence, I propose adding a EconomyTransactionResult.

MrIvanPlays commented 1 year ago

@Jikoo @lokka30 thoughts?

lokka30 commented 1 year ago

An exception will be thrown if it fails, right?

MrIvanPlays commented 1 year ago

I think just for that method it's gonna be friendlier if the initiator knows whether the transaction failed because of insufficient balance or something else. I don't think the exception is a proper way in that specific case as an indication.

lokka30 commented 1 year ago

I'm leaning far towards using exceptions for that. We keep consistency, and we reduce the code size and complexity for us + implementers. I don't see any negative of using an exception to state that a transaction failed due to an insufficient balance or something - that's entirely what the treasury exception is good at facilitating.

I'd like to hear what anyone else thinks - @Jikoo @yannicklamprecht hope you don't mind the tag, in case you're interested 🙂

Jikoo commented 1 year ago

I also think an exception would be fine for that. If it's not building the full trace, it's generally the same as any other data container performance-wise, and it's consistent with Treasury's behavior elsewhere.

MrIvanPlays commented 1 year ago

ok