[Enhancement] Hook into ForgeEssentials Economy Module/Wallet

[spannerman79]

Just like what you have at https://github.com/notabadminer/UniversalCoinsVaultPlugin is there a way that you can hook the mod into the inbuilt Vault thats part of ForgeEssentials? https://github.com/ForgeEssentials/ForgeEssentials/wiki/Economy%20Module

[spannerman79]

I've been "talking" with the devs on an issue I created and this can be done.

The exact comment is https://github.com/ForgeEssentials/ForgeEssentials/issues/1863#issuecomment-156801049

[notabadminer]

I will work on this when I have time. It might take a while.

[spannerman79]

Thank you for actually considering this, it is very much appreciated.

[olee]

@notabadminer you can contact me (preferably on IRC in #forgeessentials) if you have questions.

[notabadminer]

@olee I made an attempt at this. https://github.com/notabadminer/UniversalCoinsMod/blob/master/java/universalcoins/util/FEWallet.java

I'm not sure if I did this right, but it appears to be working. Please let me know if I should be doing things differently.

[spannerman79]

If @olee doesn't respond in some time, if you like you could release an early alpha/test build and I'll be more then willing to test it out @notabadminer.

[notabadminer]

I published a pre-release with the current implementation. It probably has bugs. I'll try to get some testing done this week and fine-tune things as needed.

[olee]

Ok I don't know about implementing the Wallet and Economy interface in the same class, but in general this should work (though I really cannot advise this). It would be better to make this two separate classes.

Economy is a manager class (singleton). Wallet is an object created by economy for each player if a wallet is requested in get.

Also UserIdent allows you to directly retrieve the UUID - please remove that extremely ugly thing there:

String playerUID = arg0.toString().replace("(", "").substring(0, arg0.toString().indexOf("|") - 1);

instead you probably want

String playerUID = ident.getUUID().toString();

[olee]

Ok screw that - this implementation is seriously bugged in multiplayer!

    @Override
    public Wallet getWallet(UserIdent arg0) {
        String playerUID = arg0.toString().replace("(", "").substring(0, arg0.toString().indexOf("|") - 1);
        accountNumber = UniversalAccounts.getInstance().getOrCreatePlayerAccount(playerUID);
        return this;
    }

    @Override
    public Wallet getWallet(EntityPlayerMP arg0) {
        accountNumber = UniversalAccounts.getInstance().getOrCreatePlayerAccount(arg0.getUniqueID().toString());
        return this;
    }

These two functions will always return the same object! You need to return a wallet for each player:

public class FEEconomy implements Economy {
    @Override
    public Wallet getWallet(UserIdent ident) {
                return new FEWallet(ident.getUUID());
    }
    @Override
    public Wallet getWallet(EntityPlayerMP arg0) {
                return new FEWallet(arg0.getUniqueID());
    }
        // And all the other stuff of Economy interface
}

public class FEWallet implements Wallet {
        protected UUID uuid;
    public FEWallet(UUID uuid) {
                this.uuid = uuid;
    }
        // And all the other stuff of Wallet interface like the functions that return / change the money of ONE player with uuid.toString() as ID
}

[olee]

Here is a reference implementation on the wallet: https://github.com/ForgeEssentials/ForgeEssentials/blob/develop/src/main/java/com/forgeessentials/economy/PlayerWallet.java Yours would look pretty much the same. The only difference is that our data class PlayerWallet is not only the one implementing the Wallet interface, but also directly the storage class.

PS: You should also think about updating your wallet from int to long - we had cases of players getting more money than int can store.

[notabadminer]

I thought it would be OK to always return the same object as the value is refreshed by UniveralAccounts. UniversalAccounts and UCWorldData are Singleton On Nov 30, 2015 6:15 AM, "Björn Zeutzheim" notifications@github.com wrote:

Here is a reference implementation on the wallet: https://github.com/ForgeEssentials/ForgeEssentials/blob/develop/src/main/java/com/forgeessentials/economy/PlayerWallet.java Yours would look pretty much the same.

— Reply to this email directly or view it on GitHub https://github.com/notabadminer/UniversalCoinsMod/issues/32#issuecomment-160642075 .

[olee]

But the stored accountNumber woudl be overwritten each time! :laughing: Sometimes multiple wallets are used at the same time, so you also need to create individual objects. Basically you can do it pretty much like you did - just split it in two separate classes.

[notabadminer]

That is intended behavior. Calls to FEWallet update the account number and then run whatever function is required. Maybe this is a bad idea. With the current implementation, is it possible that the account number could change during the call to add/remove coins? On Nov 30, 2015 6:34 AM, "Björn Zeutzheim" notifications@github.com wrote:

But the stored accountNumber woudl be overwritten each time! [image: :laughing:]

— Reply to this email directly or view it on GitHub https://github.com/notabadminer/UniversalCoinsMod/issues/32#issuecomment-160646720 .

[olee]

Yes - the wallet returned in getWallet can stay around for quite a while. If another wallet is requested in between, it will make the first one invalid (equal to the second). Just make it two classes and create a new instance on each getWallet.

[spannerman79]

I just hand a thought, would this interfere with a player having a secondary account that can be done with Universal Coins via the ATM? Or it will be kept as a completely "isolated" account away from FE Wallet @notabadminer ?

[olee]

If this is finished, FE would not store any wallet data any more but instead UCM would store it. FE would just be able to access the coins stored by UCM, so all FE economy features would become usable with UCM.

[notabadminer]

Correct. But FE will only interact with the primary account. The secondary UC account will only be accessed with cards. This is true for the Vault plugin too. On Nov 30, 2015 2:09 PM, "Björn Zeutzheim" notifications@github.com wrote:

If this is finished, FE would not store any wallet data any more but instead UCM would store it. FE would just be able to access the coins stored by UCM, so all FE economy features would become usable with UCM.

— Reply to this email directly or view it on GitHub https://github.com/notabadminer/UniversalCoinsMod/issues/32#issuecomment-160777646 .

[spannerman79]

The secondary UC account will only be accessed with cards. This is true for the Vault plugin too.

Thank you for clearing that up. Makes it easier to explain that the secondary accounts are like a private vault/term deposit style account exclusive to UC.

[notabadminer]

I reworked the economy stuff. It should be much better now. Sorry. Late night programming and inexperience makes bad things happen.

I've also update the pre-release jars.

[olee]

Still failed. Wallet looks better, but you should remove the no-argument constructor.

As for economy it still fails. You should not use the FE data api there - it try to load data that has nothing to do with your mod. Just keep it like the sample I sent you before:

    @Override
    public Wallet getWallet(UserIdent ident) {
        return new FEWallet(UniversalAccounts.getInstance().getOrCreatePlayerAccount(ident.getUuid().toString()));
    }
    @Override
    public Wallet getWallet(EntityPlayerMP arg0) {
        return new FEWallet(UniversalAccounts.getInstance().getOrCreatePlayerAccount(arg0.getUniqueID().toString()));
    }

[notabadminer]

I made the changes. Thank you for your help!

[olee]

Now this looks at least kinda acceptable :wink: @spannerman79 you should do some testing.

[spannerman79]

:smile:

@notabadminer now that @olee has made that comment I would assume that the build at https://github.com/notabadminer/UniversalCoinsMod/releases/tag/1.7.x-1.6.39b has the changes made that was in the previous comments?

[notabadminer]

Yes. I deleted the old files and uploaded them again On Dec 1, 2015 9:09 AM, "Spanner_Man" notifications@github.com wrote:

[image: :smile:]

@notabadminer https://github.com/notabadminer now that @olee https://github.com/olee has made that comment I would assume that the build at https://github.com/notabadminer/UniversalCoinsMod/releases/tag/1.7.x-1.6.39b has the changes made that was in the previous comments?

— Reply to this email directly or view it on GitHub https://github.com/notabadminer/UniversalCoinsMod/issues/32#issuecomment-161035196 .

[olee]

Any new information on this issue? @spannerman79 how did your test go?

PS: Can someone please fix the title of this issue? I always read "echantment" instead of "enhancement" - just a simple "[Request]" would be better :laughing:

[spannerman79]

It's actually an enhancement - not my fault you mix up your letters :P

I've been a bit busy IRL to test this. Either this weekend or next I'd have the actual time to sit with some others on the server and push the limits so to speak

[olee]

@spannerman79 sorry, but "Enchancement" and "Enhancement" are two different things for me. Where the first one doesn't even exist :laughing: PS: You should check your browser's spellchecking :wink:

[spannerman79]

Ok I've done some testing and so far - even though I have a small server and mostly only friends and some family member play it looks like there are no issues so far.

It will need more users (obvious) to move this out of alpha though.

Edit: I won't make another reply I'll just add to this.

My focus was just looking at an easier way to hook FE Wallet into UniversalCoins. The people that play on my small little private forge server aren't a large testing base.

The only way that UCM can get out of being in early alpha support for FE is that perhaps people from FE are given a little "nudge" to help test this mod @olee. I know it may be asking for much but as you know the larger the test base the quicker possible issues can be found and pin pointed.

[olee]

There is no need for more people to test this. Instead I would like you to test, if you can use FE shops or /wallet without any problems. If it works for you, it will work for the rest, too.