feat: add webhooks - #30
Conversation
|
This is not how a ASP.NET Core webhook endpoint should look like. Prefer using the minimal API to create a route at build-time. This will let the internal ASP.NET core router handle all of the actual context. |
|
Will work on it. |
|
Done. Made it more similar to Node.js SDK' webhooks wrapper. Example usage: using DiscordBotsList.Api.Webhooks;
var builder = WebApplication.CreateBuilder(args);
var app = builder.Build();
app.MapGet("/", () => "Hello World!");
var webhooks = new Webhooks("my webhook secret");
app.MapPost("/webhooks", webhooks.Listener((context, vote) =>
{
Console.WriteLine($"A user with the ID of {vote.VoterId} has voted us on Top.gg!");
return Task.CompletedTask;
}));
app.Run(); |
b84b479 to
094a304
Compare
094a304 to
e98dc57
Compare
|
looks good to me |
velddev
left a comment
There was a problem hiding this comment.
Most of this wouldn't work with the actual api. Refer to the docs here for more information.
https://docs.top.gg/docs/API/v1/webhooks/
https://docs.top.gg/docs/API/v1/integrations
| var hmac = new HMACSHA256(authorization); | ||
|
|
||
| hmac.TransformFinalBlock(transformBuffer, 0, transformBuffer.Length); |
There was a problem hiding this comment.
This can be done without an allocation an hmac object every request by using
HMACSHA256.HashData(authorization, data);
| return; | ||
| } | ||
|
|
||
| var vote = JsonSerializer.Deserialize<Vote>(body, serializerOptions); |
There was a problem hiding this comment.
This would break on additional other values. See; https://docs.top.gg/docs/API/v1/webhooks/
There was a problem hiding this comment.
|
|
||
| namespace DiscordBotsList.Api.Webhooks | ||
| { | ||
| public class Vote |
There was a problem hiding this comment.
Wrong Type, this is the legacy payload
So sorry! I was too fixated on |
83bfe6b to
280c065
Compare
Added XML documentation for the Webhooks class.
velddev
left a comment
There was a problem hiding this comment.
LGTM, few small nits and naming recommendations
| using Microsoft.Extensions.Primitives; | ||
|
|
||
| namespace DiscordBotsList.Webhooks | ||
| { |
There was a problem hiding this comment.
Could you avoid adding scope for namespaces? This is no longer required
| /// <summary> | ||
| /// A Top.gg webhook manager. | ||
| /// </summary> | ||
| public abstract class Webhooks |
There was a problem hiding this comment.
nit: Could we call it something like WebhookEventListener. Webhooks feels like a constant class with readonly properties (e.g. Webhooks.VoteCreate)
| return; | ||
| } | ||
|
|
||
| context.Request.Headers.TryGetValue("x-topgg-trace", out var trace); |
There was a problem hiding this comment.
what happens if this returns false?
There was a problem hiding this comment.
Sorry! Forgot about that one. Should be resolved by now.
…espaces, remove redundant summary docstring indentations, prettier
61dd822 to
eb30b34
Compare
237a6c2 to
c6bb544
Compare
The following pull request is a toned down version of #29. This pull request focuses solely on adding a webhooks wrapper for ASP.NET Core/Blazor.