fix: use webhooks for discord notification #84

jahabeebs · 2024-11-04T23:15:28Z

🤖 Linear

Closes GRT-238

Description

Based off of this draft PR by @0xGorilla fix: changed bot token to webhook [WIP] #78

note: for the avatarURL in discordNotifier we are using an image hosted by a third party--can we deploy this icon somewhere instead so we don't have to rely on the alternative site? we could have a simple static site for Wonderland assets maybe?

note 2: in the unit tests for the discord notifier I give the operator the option of enabling useRealDiscordWebhook in which they can replace the mock with their own. Do we want to keep this there or better with just the mock? It would be nice to give the operator a chance to do the integration test but I don't want to add a .env to this package (which is not an app) just so we can add a VITE_DISCORD_WEBHOOK variable

integration test output:

# Conflicts: # packages/automated-dispute/src/exceptions/index.ts

linear · 2024-11-04T23:15:31Z

GRT-238 Use webhooks for Discord notification

0xyaco

Cool stuff over here! Left some comments related to design

0xyaco · 2024-11-05T10:44:26Z

packages/automated-dispute/src/config.ts

@@ -1,8 +1,7 @@
 import { z } from "zod";

 const ConfigSchema = z.object({
-    DISCORD_BOT_TOKEN: z.string().min(1),
-    DISCORD_CHANNEL_ID: z.string().min(1),
+    DISCORD_WEBHOOK: z.string().min(1),


Could using .url() result in a little bit more precise schema? [1]

[1] https://zod.dev/?id=strings

Suggested change

DISCORD_WEBHOOK: z.string().min(1),

DISCORD_WEBHOOK: z.string().url()

The only reason I didn't do this was because of integration testing (not sure if people ever test w/local webhooks?) but since we're not worrying about it for now I replaced this with url().optional() (I verified it is indeed optional)

0xyaco · 2024-11-05T11:13:17Z

packages/automated-dispute/tests/services/discordNotifier.spec.ts

+// Change this to `true` to test your Discord webhook URL set below rather than mocking it
+const useRealDiscordWebhook = false;


It's nice that you have thought of a way to actually send messages, getting closer to an integration test! And it's also nice that you got to see the DiscordNotifier in action.

Using the actual Discord webhook will probably help us during the E2E tests, although it is kinda complex to test stuff for this particular service, given that we should actually check in the proper Discord channel that the message was sent + it was nicely formatted + the content is ok + etc.

Let's keep this as a plain unit tests for the moment, fully mocking discord.js and later we can talk about testing Discord also E2E.

refactored to be plain unit tests

0xyaco · 2024-11-05T11:29:08Z

packages/automated-dispute/tests/services/discordNotifier.spec.ts

+        } else {
+            await notifier.send(message);
+
+            const WebhookClientMock = WebhookClient as unknown as vi.Mock;


Could it be that if you remove the if when mocking, you won't need this WebhookClientMock declaration and casting anymore?

🧠 indeed

0xyaco · 2024-11-05T11:30:33Z

packages/automated-dispute/tests/services/discordNotifier.spec.ts

-    it("sends an error message to the Discord channel", async () => {
-        const error = new Error("Test error");
-        const context = { key: "value" };
+    it("sends a message successfully", async () => {


Missing the describe("send", ...) block wrapping these tests

good catch ✅

0xyaco · 2024-11-05T11:43:14Z

packages/automated-dispute/src/services/discordNotifier.ts

        try {
-            await client.login(config.discordBotToken);
-
-            await new Promise<void>((resolve, reject) => {
-                client.once("ready", () => {
-                    logger.info("Discord bot is ready");
-
-                    resolve();
-                });
-
-                client.once("error", (error: Error) => {
-                    reject(error);
-                });
-            });
+            await this.client.send(webhookMessage);
+            console.error(this.client, this.client);
        } catch (error) {
-            logger.error(`Failed to initialize Discord notifier: ${error}`);
-
-            throw error;
+            throw new NotificationFailureException("An error occurred with the Discord client.");


Given that we want the agent to keep working even if there's an error during Discord notifications, we've got two options right now:

Not throwing anything and just logging if an error happens during Discord message sending actions

Keep this throw here, but handle every error with a try-catch block or similar

The option (1) seems like the safest as, at the moment, the agent has no place where a notification failure is the cause of the whole agent to fail. The option (2) seems like the more generalized option though, as it's throwing the error and letting the client handle it.

I suggest we do the following:

Let's keep this send method, but let's make it log the error if there's an error

Let's add a new sendOrThrow method that does what this method is doing at the moment

Wdyt?

separated the logic out into the send and sendOrThrow logic as you suggested 👍🏻

0xyaco · 2024-11-05T11:45:02Z

packages/automated-dispute/src/services/discordNotifier.ts

+        return {
+            username: message.username || "EBO Agent",
+            content: message.title,
+            avatarURL: message.avatarUrl || "https://cryptologos.cc/logos/the-graph-grt-logo.png",


We want the URL to be a new config param (new value in the config yml called notifier.discordDefaultAvatarUrl which is a string().url() + optional() if Discord allows you to specify no avatar URL) to be set during the class constructor:

Suggested change

avatarURL: message.avatarUrl || "https://cryptologos.cc/logos/the-graph-grt-logo.png",

avatarURL: message.avatarUrl || this.defaultAvatarUrl,

✅ it was indeed optional so I added that

0xyaco · 2024-11-05T11:52:05Z

packages/automated-dispute/src/interfaces/notificationService.ts

     */
-    notifyError(error: Error, context: any): Promise<void>;
+    createErrorMessage(err: unknown, context: unknown, defaultMessage: string): IMessage;


I feel like context might be empty (or {}), but the defaultMessage is always being used. Do you think we should probably swap the order of those two parameters and make the context's default value to be {} if not specified?

Also, I've seen that whenever you define the message inside the context that you are passing to this method, you are using the same value as the one used passed as defaultMessage. Maybe you can remove the message declaration from the context in those function calls?

good point, I swapped the order and have a default {} value for context if not specified now

indeed for some of these the context is redundant--in eboProcessor I removed the places where we were passing in the same error message as the context

0xnigir1 · 2024-11-05T13:08:35Z

apps/agent/README.md

we could add some link referring to Discord docs on how to create your Webhook

0xnigir1 · 2024-11-05T13:12:00Z

packages/automated-dispute/src/services/discordNotifier.ts

-                });
-            });
+            await this.client.send(webhookMessage);
+            console.error(this.client, this.client);


Suggested change

console.error(this.client, this.client);

i guess this log shouldn't be here right?

0xnigir1 · 2024-11-05T13:15:51Z

packages/automated-dispute/src/services/eboProcessor.ts

+                    if (err instanceof Error) {
+                        this.onActorError(requestId, err);
+                    } else {
+                        this.onActorError(requestId, new Error(String(err)));


does String(err) do the job? isn't it possible that it renders [object Object]? how is it different from JSON.stringify?

I originally needed this because of a type thing but I've removed this now

0xnigir1 · 2024-11-05T13:19:19Z

packages/automated-dispute/src/services/eboProcessor.ts

+                const errorMessage = this.notifier.createErrorMessage(
+                    new Error("Unsupported chain"),
+                    {
+                        message: `Chain ${chainId} not supported by the agent. Skipping...`,
+                        chainId,
+                        requestId,
+                    },
+                    `Chain ${chainId} not supported by the agent. Skipping...`,
+                );
+
+                this.notifier.send(errorMessage);


although this was the original way I implemented this, i'm thinking if one should create the error message and then send it or if creating the errorMessage should be internal to the implementation (this would mean modifying the Interface to not receive an IMessage but rather the signature from createErrorMessage)

cc @0xyaco @0xkenj1

I've done some refactoring of discordNotifier.ts to hopefully make things more concise--wdyt?

looks good to me, nice job 🙌

0xyaco · 2024-11-06T12:46:26Z

packages/automated-dispute/src/services/discordNotifier.ts

     */
-    constructor(url: string) {
+    constructor(url: string, defaultAvatarUrl?: string) {


Let's inject logger. And added some minor tweaks, just being consistent with the rest of our codebase:

Suggested change

constructor(url: string, defaultAvatarUrl?: string) {

constructor(

url: string,

private readonly logger: ILogger,

private readonly defaultAvatarUrl?: string

) {

0xyaco · 2024-11-06T12:50:59Z

packages/automated-dispute/src/services/eboProcessor.ts

-                        chainId,
-                        requestId,
-                    },
+                this.notifier.sendError(


Can we await these sendError calls? I'm afraid that if something unexpectedly raises an error within the promise, it might crash the whole agent.

Sure we can, I was just trying to avoid having to make a bunch of the eboProcessor functions async but I went ahead and changed them

0xyaco · 2024-11-06T12:51:07Z

packages/automated-dispute/src/services/eboProcessor.ts

-        );
-
-        this.notifier.send(errorMessage);
+        this.notifier.sendError(`Actor error for request ${requestId}`, { requestId }, error);


0xyaco · 2024-11-06T12:51:13Z

packages/automated-dispute/src/services/eboProcessor.ts

+                        `Could not create a request for epoch ${epoch} and chain ${chain}.`,
+                    );
+
+                    this.notifier.sendError(


0xyaco · 2024-11-06T12:51:18Z

packages/automated-dispute/src/services/eboProcessor.ts

-            );
-
-            await this.notifier.send(errorMessage);
+            this.notifier.sendError("Error creating missing requests", { epoch }, err);


0xyaco · 2024-11-06T12:51:22Z

packages/automated-dispute/src/services/eboProcessor.ts

-                    message: `Actor handling request ${requestId} was already terminated.`,
-                    requestId,
-                },
+            this.notifier.sendError(


0xyaco

Sweet!

0xnigir1

go go

0xGorilla and others added 6 commits October 29, 2024 17:15

fix: changed bot token to webhook

6c704b2

fix: added discord notifier

537ef50

fix: uncommented index

8807e3f

style: lint

b1757bd

Merge remote-tracking branch 'origin/dev' into fix/discord-webhook

139da81

# Conflicts: # packages/automated-dispute/src/exceptions/index.ts

feat: better notification logic

27c1f7d

jahabeebs requested review from 0xkenj1, 0xnigir1 and 0xyaco November 4, 2024 23:15

jahabeebs assigned jahabeebs and 0xGorilla Nov 4, 2024

jahabeebs changed the title ~~fix: use webhooks for Discord notification~~ fix: use webhooks for discord notification Nov 4, 2024

0xyaco reviewed Nov 5, 2024

View reviewed changes

0xnigir1 reviewed Nov 5, 2024

View reviewed changes

jahabeebs added 6 commits November 5, 2024 19:54

fix: pr comments

f6ceff2

fix: as unknown

84c0df8

fix: excess messages

85fb6cf

chore: docs

1aed46b

fix: stringify

b467c85

fix: this.actor and tests

1b1aab9

jahabeebs requested review from 0xnigir1 and 0xyaco November 6, 2024 01:19

0xyaco reviewed Nov 6, 2024

View reviewed changes

fix: pr comments

e281b9e

jahabeebs requested a review from 0xyaco November 6, 2024 17:29

0xyaco approved these changes Nov 6, 2024

View reviewed changes

0xnigir1 approved these changes Nov 6, 2024

View reviewed changes

jahabeebs merged commit 2c02271 into dev Nov 6, 2024
5 checks passed

jahabeebs deleted the fix/grt-238-discord-webhook branch November 6, 2024 19:54

jahabeebs mentioned this pull request Nov 6, 2024

fix: changed bot token to webhook [WIP] #78

Closed

	DISCORD_WEBHOOK: z.string().min(1),
	DISCORD_WEBHOOK: z.string().url()

		// Change this to `true` to test your Discord webhook URL set below rather than mocking it
		const useRealDiscordWebhook = false;

	avatarURL: message.avatarUrl \|\| "https://cryptologos.cc/logos/the-graph-grt-logo.png",
	avatarURL: message.avatarUrl \|\| this.defaultAvatarUrl,

fix: use webhooks for discord notification #84

fix: use webhooks for discord notification #84

Conversation

jahabeebs commented Nov 4, 2024 • edited Loading

🤖 Linear

Description

linear bot commented Nov 4, 2024

0xyaco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jahabeebs Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xyaco left a comment

Choose a reason for hiding this comment

0xnigir1 left a comment

Choose a reason for hiding this comment

jahabeebs commented Nov 4, 2024 •

edited

Loading

jahabeebs Nov 6, 2024 •

edited

Loading