Initial Commit

[chrlschwb]

Adding two discord bots

• Old Version of Joystream Discord Bot
• New JOY transfer Discord Bot

[bwhm]

Hey @chrlschwb,

For the Discord-Tip-Bot, I have a few suggestions:

1. Replace balances.transfer with balances.transferKeepAlive, so we avoid the account (and balances) being reaped.
2. Use bn. Although probably unlikely the account, much less a user, exceeds this balance, it's still better.
3. It doesn't look like fees are considered. If correct, that will cause trouble down the road...

Now, on the more serious side:
4. It looks like the only way to deposit is to provide a seed to the bot? If that is correct, please find another approach to this. For example:

• Require a deposit when setting the wallet, and/or request a signature, or add a random, small amount of HAPI on top of the amount the user wants to deposit. Then, have the user tell the bot when "ready", and listen for balances.Transfer events matching amount/address for 5 blocks or so...

5. Maintain some "maximum" balance, so we avoid the bot holding more funds than we want some relatively insecure server to control. If exceeded, the bot automatically withdraws some amount back to the user.

Going to assign @DzhideX for the "full" review, as the DB parts would take me longer than it should :D

For the "old" discord bot: should we assume that this is the same version as PR #1? If so, I don't really want to merge it, unless it's clearly listed as deprecated or smth.

[DzhideX]

As far as I understood, we're only reviewing the tip-bot for now so that is what I will be focusing on, let me know if that's wrong @bwhm!

Hi @chrlschwb,

First off, I'd like to say that I’m mostly going to focus on the functionality here as the code generally looks good. It’s simple, clean, and very readable. The setup is easy thanks to the (discord-bot) readme and I was able to get it running very quickly with MongoDB Atlas. Great work there!

Specific problems:

• Register:
  • If I set my address to something that is not a correct address (e.g., ") and try to withdraw the bot completely crashes.
• Deposit:
  • If I set the seed to an invalid raw seed (e.g., a really big string like “HHHHH…”) polkadot throws an error -> crash.
  • If I set the value to a value greater than Number.MAX_SAFE_INTEGER polkadot again throws an error -> crash.
  • The bot allows me to deposit more than I have in my account (input: correct seed, value larger than what I own).
• Withdraw:
  • Due to the deposit bug, the bot also allows me to withdraw more than I should be able to. The transaction fails but the bot doesn't report it, rather just prints: You have withdrawn X JOY.
  • If I input a non-number or greater than Number.MAX_SAFE_INTEGER it results in an error -> crash.
• These you are free to ignore (very minor):
  • [NIT] There are a couple of spelling mistakes like recieve instead of receive, Trasnfer instead of Transfer and similar.
  • [NIT] Although the structure is generally fine, maybe running a formatter (like Prettier) might be a good idea.

General thoughts and recommendations:

• The bot needs better error handling. It is inevitable that some edge cases are not going to be handled but that shouldn't be the reason a bot completely crashes and stops working for everyone.
• The bot should implement more input validation. I see you've sprinkled some around the codebase (like isNan in Transfer.ts and other similar checks) but this should be more extensive. However, I don't expect the devs to think to try and think of every possible problem that might happen which is why I would recommend:
• Thorough and meticulous testing. Considering this bot is to deal with people's money there is no room for error. Maybe we can have a couple community members setup a test server and really try every possible edge case and functionality.

To conclude, the bot works as long as you follow the confines of what I would say would be mostly normal transactions for everyday users. That being said, I was a bit more strict in my review because I think we can't allow bad actors to so easily be able to crash or manipulate the system (responsible for transferring people's money).

[DzhideX]

Why do we fetch the same users twice in this function?

[chrlschwb]

That is necessary. The code is verifying whether the sender address and receiver address are valid. If this verification process is skipped and the receiver address is invalid, the sender would lose JOY.

[DzhideX]

Sorry if I'm missing something obvious but didn't we already perform this same check at the top of the function (for both the sender and receiver)? What is stopping us from just using tx and rx instead of sendJoy and receiveJoy 🤔

[chrlschwb]

You are right. Just fixed the code. :)

[bedeho]

This PR is too big, and we are not fixing old problems, and feature set is not appropriate, here is a better way forward in my opinion

#7