-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Bank] Add event dispatch on account events #5141
Comments
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. |
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) |
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 |
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
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 Now onto other events:
To finish this off, all the event names here should also be prefixed with |
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
That is reasonable and probably something worth handling
Hmm, I could see the 3 calls being useful, so I don't think that is an issue.
Sort of like
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.
Potentially containing what info? Like a dict by GuildID that contains the UserID and potentially something else (maybe account balance)?
:eeveesad: (I was hoping to squeeze it in, no worries)
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. |
Not exactly, as there d.py passes you with So in the case of 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):
...
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. 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. |
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? |
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 |
There are plans to support usage of Bank API with IDs so usage of |
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
The text was updated successfully, but these errors were encountered: