Implement withdrawIfSufficient method and CurrenciesAPI for scheduling

[1robie]

This pull request introduces a major enhancement to the currency API by adding atomic, safe purchase operations to prevent double-spending and improve cross-server consistency. It also introduces new utility classes for argument checking and locking, updates the documentation to guide users on these new features, and standardizes default parameter handling. The changes are aimed at making currency operations safer, more robust, and easier to extend for custom implementations.

New atomic purchase operations and supporting utilities:

• Added withdrawIfSufficient and withdrawIfSufficientAsync methods to Currencies, enabling atomic withdrawal only if the player can afford it, and ensuring nothing is debited unless the operation succeeds. Also added hasNativeConditionalWithdraw to check backend atomicity.
• Introduced CurrencyArgumentChecks for consistent validation of arguments in conditional currency operations, ensuring all providers reject invalid input the same way.
• Added CurrencyLocks utility for per-player, per-provider locking, preventing race conditions during emulated atomic operations.

Threading and plugin lifecycle improvements:

• Added CurrenciesAPI to manage plugin instance registration and main-thread checks, supporting safe asynchronous operations for main-thread-bound currencies.

Documentation updates:

• Expanded the readme.md with comprehensive guidance on safe purchases, backend atomicity, custom economy integration, and asynchronous usage patterns, including code samples and backend capability table. [1] [2]

API consistency and maintainability:

• Standardized the use of default currency name and reason via constants in Currencies, simplifying method signatures and ensuring consistent default behavior throughout the API. [1] [2] [3] [4] [5] [6]

Versioning:

• Bumped the project version from 1.0.14 to 1.0.15.…d add CurrenciesAPI for main thread scheduling

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 212 complexity · 24 duplication

Metric	Results
Complexity	212
Duplication	24

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[Traqueur-dev]

Merci pour cette PR 🙏 — l'approche est la bonne et le travail est sérieux. withdrawIfSufficient en default method sur CurrencyProvider permet d'ajouter la fonctionnalité sans aucune rupture binaire ni source pour les providers existants, le fallback émulé sous verrou est un choix raisonnable, et la doc du readme est de très bonne qualité — le tableau d'atomicité par backend est exactement ce dont les utilisateurs ont besoin.

Quelques points à reprendre avant merge, dont un bug de débit avéré.

Ce que j'ai vérifié

• develop ne compile plus actuellement : com.bencodez:votingplugin tire org.mozilla:rhino:1.9.1, incompatible avec la cible Java 8 du projet. L'exclusion ajoutée dans build.gradle.kts est donc un correctif nécessaire, pas du bruit — cf. commentaire dédié.
• Avec cette exclusion, tout le nouveau code compile en --release 8. Les seules erreurs restantes sont dans ExcellentEconomyProvider, et je les reproduis à l'identique sur develop : ExcellentEconomy 2.8.0 est compilé en JDK 25 et je n'ai qu'un JDK 21 ici. Pré-existant, hors périmètre de la PR — mais c'est aussi le seul fichier que je n'ai pas pu valider.
• Les signatures utilisées par les autres providers sont bonnes : EconomyManager.withdraw(...) et PlayerPointsAPI.take(...) renvoient bien un boolean, VotingPluginUser.removePoints(int) et les surcharges RedisEconomy compilent.

Bloquants

1. ExperienceProvider débite plus que le montant demandé sur un montant fractionnaire — seul provider "entier" sans la garde stripTrailingZeros().scale() > 0.
2. CurrencyLocks est un verrou strippé présenté comme un verrou par joueur : deux achats sans rapport peuvent se bloquer et renvoyer un FAILED dont le message affirme une cause fausse.
3. isBackendGuaranteed() sur-promet pour Vault : la colonne du readme dit « yes » et le code renvoie nativeSuccess, alors que la note de la même ligne dit « as atomic as the underlying economy plugin ». C'est le cas d'usage cross-serveur que la PR vise, donc ça compte.
4. ExcellentEconomyProvider : appels Bukkit hors du thread principal alors que requiresMainThread() renvoie false, deux getBalance() bloquants inutiles, et toute cause d'échec écrasée en INSUFFICIENT_FUNDS.
5. CurrenciesAPI.init() lève au second appel — casse le démarrage du 2ᵉ plugin quand la lib est shadée sans relocation, ce que le readme ne fait que recommander.

Autres points

Détaillés en commentaires inline : common ForkJoinPool pour l'async, contrat « rien n'est débité sauf en SUCCESS » intenable en l'état, assert désactivé par défaut dans ZEssentialsProvider, import inutilisé, bump de version qui entre en conflit avec le flux de release du repo, CurrencyRegistry qui introduit une API publique parallèle non annoncée, nullabilité de TransactionResult.

Deux remarques de forme :

• Beaucoup de refactoring cosmétique this. non lié est mélangé au diff, ce qui alourdit nettement la relecture. Un commit séparé serait plus lisible.
• Aucun test. src/test n'existe pas et le test-plugin/ n'est pas touché. Pour une fonctionnalité dont l'argument central est « pas de double-spend », CurrencyArgumentChecks, TransactionResult et le withdrawIfSufficient émulé (avec un provider bidon en mémoire) sont du Java pur, testables en quelques dizaines de lignes sans serveur. C'est le meilleur endroit pour démontrer que la garantie tient.

Enfin, la CI n'a jamais tourné sur cette PR (0 check) — à approuver avant merge, ne serait-ce que pour valider ExcellentEconomyProvider sous JDK 25.

---

Generated by Claude Code

[Traqueur-dev]

Le joueur peut être débité de plus que le montant demandé.

C'est le seul provider "entier" qui n'a pas la garde amount.stripTrailingZeros().scale() > 0 — LevelProvider, ItemProvider, PlayerPointsProvider et VotingProvider la font tous.

Scénario : current = 100, amount = 10.5

• remaining = 89.5
• setTotalExperience(player, remaining.intValue()) → tronque à 89
• le joueur perd donc 11 XP au lieu de 10.5
• et le TransactionResult renvoyé annonce balance = 89.5, qui ne correspond pas à l'état réel du joueur

Proposition, alignée sur LevelProvider :

if (amount.stripTrailingZeros().scale() > 0) {
    return TransactionResult.failed(amount, "Experience only supports whole amounts, got " + amount + ".");
}

---

Generated by Claude Code

[Traqueur-dev]

Verrou strippé présenté comme un verrou par joueur → faux positifs.

Avec 64 stripes, deux couples (provider, joueur) totalement indépendants peuvent tomber sur la même ReentrantLock. Un achat parfaitement légitime peut alors attendre 250 ms puis repartir en FAILED avec le message :

"Timed out waiting for a concurrent operation on the same balance."

…alors qu'il n'y avait aucune opération concurrente sur ce solde. Le message est factuellement faux dans ce cas, et le joueur voit un échec sans raison.

Deux points à traiter :

1. La collision elle-même — un ConcurrentHashMap<Key, ReentrantLock> avec une clé (identityHashCode(provider), playerId) supprime le problème à la racine. Si tu tiens au striping pour éviter la croissance de la map, il faut beaucoup plus de stripes et un message d'erreur qui n'affirme pas la cause.

2. Le timeout de 250 ms — si l'attente se produit sur le thread principal, c'est 5 ticks de freeze. Le cas est rare (pour les providers requiresMainThread(), l'async repasse sur le main thread, donc les deux ne se croisent pas), mais il devient réel dès qu'un appelant utilise la méthode synchrone hors du thread principal.

---

Generated by Claude Code

[Traqueur-dev]

isBackendGuaranteed() promet plus que ce que Vault peut tenir.

La colonne dit « yes », mais la note de la même ligne dit « As atomic as the underlying economy plugin » — les deux se contredisent. Vault est une abstraction : withdrawPlayer délègue au plugin d'économie derrière, et la majorité (EssentialsX, CMI…) fait un read-modify-write non atomique.

Le problème n'est pas la doc, c'est ce que le code renvoie : VaultProvider.withdrawIfSufficient construit un TransactionResult.nativeSuccess(...), donc backendGuaranteed = true. Or le readme apprend juste au-dessus à l'utilisateur que isBackendGuaranteed() == true signifie qu'il est protégé contre le double-spend cross-serveur. Sur un réseau multi-serveurs avec EssentialsX derrière Vault, c'est faux — et c'est exactement le cas d'usage que la PR cherche à sécuriser.

Même remarque pour ZESSENTIALS : economyManager.withdraw(...) renvoie bien un booléen, mais rien ne dit que le check et le débit sont indivisibles côté zEssentials.

Deux options :

• un troisième état (DELEGATED / inconnu) pour les backends qui rapportent un résultat sans garantir l'atomicité, distinct de REDISECONOMY qui, lui, valide côté Redis ;
• ou redescendre VAULT et ZESSENTIALS dans la colonne et renvoyer emulatedSuccess(...).

---

Generated by Claude Code

[Traqueur-dev]

Trois points sur cette méthode synchrone, tous liés au fait que ce provider déclare requiresMainThread() = false un peu plus bas.

1. Appels Bukkit hors du thread principal. Comme le provider s'annonce thread-safe, cette méthode va légitimement être appelée depuis un autre thread (CurrencyRegistry, ou un appelant qui a lu le contrat). Or elle fait Bukkit.getPlayer(playerId) puis api.withdraw(player, ...) avec une entité Player vivante. Le contrat requiresMainThread() porte sur le backend, mais l'implémentation, elle, touche à l'API Bukkit.

2. getBalance() bloquant. Sur les deux branches (succès et échec) on rappelle this.getBalance(playerId), qui pour un joueur hors-ligne fait getBalanceAsync(...).join() — un aller-retour DB bloquant, en plus du retrait qu'on vient de faire. Deux appels réseau là où il en faudrait zéro : OperationResult porte déjà l'info, et nativeSuccess/nativeInsufficientFunds acceptent null comme solde (c'est ce que fait la variante async juste en dessous).

3. Cause d'échec écrasée. success == false est systématiquement traduit en nativeInsufficientFunds, alors que ça peut être un tout autre problème (devise introuvable, données joueur non chargées…). L'appelant affiche « tu n'as pas assez d'argent » à un joueur qui a largement de quoi payer. TransactionResult.failed(...) existe pour ça.

À noter : c'est le seul fichier de la PR que je n'ai pas pu compiler ici — ExcellentEconomy 2.8.0 est compilé en JDK 25 et je n'ai qu'un JDK 21 (les mêmes erreurs apparaissent sur develop, donc rien à voir avec la PR). Les signatures OperationResult.success() et api.withdraw(Player, ...) restent à valider par la CI.

---

Generated by Claude Code

[Traqueur-dev]

init() qui lève au second appel va casser des démarrages de plugin.

Le readme dit que la relocation est recommandée, pas obligatoire. En pratique, beaucoup shadent la lib sans relocaliser. Dans ce cas la classe CurrenciesAPI est chargée une seule fois et partagée : le premier plugin appelle init(this), le second se prend un IllegalStateException en plein onEnable et ne démarre pas. Le second plugin n'a rien fait de mal et n'a aucun moyen de s'en prémunir.

Je rendrais l'appel idempotent — ignorer silencieusement (ou logger) plutôt que lever :

if (plugin != null) {
    return;
}

Dans la foulée, un fallback éviterait à beaucoup de monde de découvrir en production que l'async renvoie FAILED parce qu'ils ont oublié init() :

public static Plugin getPlugin() {
    if (plugin == null) {
        try {
            plugin = JavaPlugin.getProvidingPlugin(CurrenciesAPI.class);
        } catch (Throwable ignored) {
        }
    }
    return plugin;
}

Ça fonctionne justement quand la lib est shadée, ce qui est le cas d'usage principal.

---

Generated by Claude Code

[Traqueur-dev]

Import inutilisé — OfflinePlayer n'apparaît nulle part dans le fichier.

---

Generated by Claude Code

[Traqueur-dev]

Je retirerais le bump de version de cette PR.

Le repo suit git-flow : les versions sont posées par des commits :memo: Version sur les branches de release (Merge branch 'release/1.0.14'), pas depuis les PR de feature. Bumper ici risque surtout de créer un conflit au moment de la release.

Et si le bump est conservé, le readme est à mettre à jour aussi — les snippets Maven (ligne 50) et Gradle (ligne 67) annoncent toujours 1.0.14.

---

Generated by Claude Code

[Traqueur-dev]

👍 Cette exclusion n'est pas cosmétique, c'est un correctif nécessaire : develop ne résout plus ses dépendances sans elle.

Vérifié en local sur develop seul :

> Could not resolve org.mozilla:rhino:1.9.1.
  Required by: root project 'CurrenciesAPI' > com.bencodez:votingplugin:6.17.2 > com.bencodez:advancedcore:3.8.2-SNAPSHOT
  > Dependency resolution is looking for a library compatible with JVM runtime version 8,
    but 'org.mozilla:rhino:1.9.1' is only compatible with JVM runtime version 11 or newer.

(advancedcore est un SNAPSHOT, la casse est donc arrivée toute seule côté amont.)

Ça mériterait d'être extrait dans sa propre PR et mergé tout de suite, indépendamment du reste : la branche est cassée pour tout le monde en attendant.

---

Generated by Claude Code

[Traqueur-dev]

Cette classe est une seconde API publique, parallèle à Currencies, et elle n'est mentionnée ni dans le titre ni dans la description de la PR.

Il n'y a aucun pont entre les deux : rien dans Currencies ne permet d'atteindre un provider enregistré ici, et inversement. Un utilisateur se retrouve avec deux façons de faire la même chose selon l'origine de sa devise. Pour une lib publiée sur JitPack, c'est un choix d'architecture qui mérite sa propre discussion — je la sortirais dans une PR séparée, ou je la câblerais explicitement à Currencies.

Deux détails si elle reste :

• getRegisteredNames() renvoie les clés normalisées (trim + lowercase). Celui qui enregistre "MyGems" récupère "mygems", ce que rien n'annonce. Soit on stocke le nom d'origine à côté de la clé, soit on le documente.
• register(...) appelle normalize(name) avant le null-check du provider, donc un register(null, null) remonte « The currency name cannot be null or blank » — anodin, mais l'ordre inverse serait plus logique.

---

Generated by Claude Code

[Traqueur-dev]

getBalance() et getErrorMessage() peuvent renvoyer null — c'est documenté en javadoc, mais pas annoté, alors que CurrencyRegistry utilise déjà org.jetbrains.annotations dans cette même PR. Un @Nullable ici ferait remonter l'avertissement dans l'IDE de l'utilisateur au lieu d'un NPE à l'exécution, et @NotNull sur getStatus() / getAmount() clarifierait le reste.

---

Generated by Claude Code

[Traqueur-dev]

Correction du dernier paragraphe de ma review : la CI a tourné et le build passe (Build and Publish CurrenciesAPI / build ✅). J'avais interrogé la mauvaise API. ExcellentEconomyProvider est donc bien validé sous JDK 25 — seule reste la remarque sur le fond de ce fichier, pas sur sa compilation.

(Il reste Codacy Static Code Analysis en action_required.)

---

Generated by Claude Code