[Bank] Add event dispatch on account events

[madebylydia]

What component of Red (cog, command, API) would you like to see improvements on?

Bank's API

Describe the enhancement you're suggesting.

It would be nice if cogs can take advantage of listeners for the Bank cog. Cogs could know when an user's account get changed, pruned, spend money, and so on...

Anything else?

No response

[Jackenmen]

I think it wouldn't be a bad idea.

Needs design decision before PR can be made, if someone is interested in pursuing this, let us know.

[yamikaitou]

I'm putting together a list of possible events to trigger and when, give me a day or so and I'll share it here. This is a feature I'm interested in, so I'd like to see it happen (yes, I'm volunteering if you want to take it as such)

[yamikaitou]

Here is my initial, quick thought on events to add, mostly went off a mental list of items that would be useful to me.

bank_deposit_credits(member, amount)
bank_transfer_credits(from_member, to_member, amount)
bank_withdraw_credits(member, amount)
bank_set_global(state) # True for Global, will not send bank_wipe
bank_prune_accounts(guild, users) # Provides all UserIDs that were pruned
bank_wipe(guild)
bank_claim_payday(member, amount)

I considered doing one for set_balance but it would result in 2 events being sent since deposit/transfer/withdraw all call it

[Jackenmen]

Looks like I'm a forgetful person, thanks for another reminder in discord, Yami.

I think that we should have one dispatch responsible for any call to bank.set_balance() and not have separate dispatches for bank_deposit_credits and bank_withdraw_credits. This means we would instead have bank_set_balance(member, amount_before, amount_after).

bank_transfer_credits event can probably remain as described here, though I do wonder whether the information here is enough - maybe we should consider including new balances of both members as well. This can technically be queried easily but I'm kind of wondering whether this information could already get changed before the listener gets called therefore causing some race conditions. If we were to dispatch new balances as well, we should probably wrap it all in a single object and make it into a simple: bank_transfer_credits(transfer_data).

It might even make some sense to apply this idea of the wrapper objects to the other events. I'm a bit unclear on that but I think it's worth considering.

As for how this would work - during bank.transfer_credits() call, bank_set_balance event would be dispatched twice (due to calls to bank.set_balance()) and after that, bank_transfer_credits event would be called.

Now onto other events:

• bank_set_global - is there a reason we shouldn't dispatch wipe here?
• bank_prune_accounts seems fine, though using a wrapper object here might make some sense in case we decide to expose more stuff in the future
• bank_wipe seems fine
• bank_claim_payday is an economy-related event, I think it's a bit out of scope for this issue and could be discussed separately

To finish this off, all the event names here should also be prefixed with red_.

[yamikaitou]

I think that we should have one dispatch responsible for any call to bank.set_balance() and not have separate dispatches for bank_deposit_credits and bank_withdraw_credits. This means we would instead have bank_set_balance(member, amount_before, amount_after).

As mentioned on Discord, that seems reasonable. Since deposit and withdraw both call set internally, it makes sense (and would cover the people who call set_balance directly)

If we were to dispatch new balances as well, we should probably wrap it all in a single object and make it into a simple: bank_transfer_credits(transfer_data).

That is reasonable and probably something worth handling

As for how this would work - during bank.transfer_credits() call, bank_set_balance event would be dispatched twice (due to calls to bank.set_balance()) and after that, bank_transfer_credits event would be called.

Hmm, I could see the 3 calls being useful, so I don't think that is an issue.

It might even make some sense to apply this idea of the wrapper objects to the other events. I'm a bit unclear on that but I think it's worth considering.

Sort of like on_member_update but send back a bank.Account-like object? Here I was originally thinking that it would be as simple as just dispatching events but now you have us thinking about smart things.

bank_set_global - is there a reason we shouldn't dispatch wipe here?

My thought was that the assumption would be that a wipe occurred but thinking about it again, it does indeed make sense to dispatch wipe here as well since someone may only be listening to the wipe.

bank_prune_accounts seems fine, though using a wrapper object here might make some sense in case we decide to expose more stuff in the future

Potentially containing what info? Like a dict by GuildID that contains the UserID and potentially something else (maybe account balance)?

bank_claim_payday is an economy-related event, I think it's a bit out of scope for this issue and could be discussed separately

:eeveesad: (I was hoping to squeeze it in, no worries)

To finish this off, all the event names here should also be prefixed with red_.

Agreed

Also, unless someone says otherwise, I'm going to start drafting a PR for this some time soon. So if anyone has feedback, do let me know either here or in Discord. Also, other improvements to Bank/Economy I should consider for future expansion (I don't know why, but I sort of like writing cogs/games to interact with the Bank), let me know in Discord.

[Jackenmen]

Sort of like on_member_update but send back a bank.Account-like object? Here I was originally thinking that it would be as simple as just dispatching events but now you have us thinking about smart things.

Not exactly, as there d.py passes you with before and after discord.Member objects. If on_member_update did something like what I'm proposing, it would have a signature like on_member_update(event: MemberUpdateEvent) and MemberUpdateEvent object would probably have the member objects under event.before and event.after.

So in the case of on_red_bank_transfer_credits, it could potentially look like this (I'm not too sold on the names used here):

class BankTransferInformation:
    transfer_amount: int

    sender_id: int
    recipient_id: int

    sender_new_balance: int
    recipient_new_balance: int

And then the listener:

async def on_red_bank_transfer_credits(transfer_information: BankTransferInformation):
    ...

Potentially containing what info? Like a dict by GuildID that contains the UserID and potentially something else (maybe account balance)?

Thinking more of future-proofing here, maybe the API will allow doing different prune actions in the future (one thing that comes to mind is allowing bot owner to prune all local banks for example) and while it would probably still be breaking nonetheless, I feel 3rd-parties would have an easier time updating to support this case if we had a wrapper object. But yes, it is also possible that someone would ask us in the future to give more data about the pruned bank accounts and while I don't think we should do it now, I wouldn't necessarily mind doing it in the future if a use case presents itself.

Either way, I think that just having wrapper objects for all events won't hurt the usability and seems like it could benefit us in general.

[madebylydia]

I'm fine with everything said so far (More like lost, but shhh :v), but I got a little... "problem" with the class.

IDs are fine to me, but ideally, and assuming bank has access to it, they could return discord.User objects instead of simple ID. Like so:

class BankTransferInformation:
    transfer_amount: int

    sender: discord.User
    recipient: discord.User

    sender_new_balance: int
    recipient_new_balance: int

What if the developer making advantage of this event need to get access to the User object? Of course it can use bot.get_user, but honestly, it's boring to do that where it could be done by default by the event. If the bot get an ID instead of the discord.User object, then of course it would return the ID, but that may get into confusing to have to check if it's an int or instance of discord.User, but I guess that shouldn't be the case.

[yamikaitou]

My plan was to use a Member/User object since the bank could be local and an ID wouldn't give that indication unless I also sent back a GuildID, which seems pointless at that point

[Jackenmen]

There are plans to support usage of Bank API with IDs so usage of discord.User and discord.Member objects is not going to work for us. We could make it a bit easier for the user by adding properties that would get the member/user object for you. With that in mind, we will need to ensure guild id is part of all the event data as well.