-
Notifications
You must be signed in to change notification settings - Fork 2
Add swarm leaderboard backend with DB #669
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
base: master
Are you sure you want to change the base?
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Makefile
Outdated
|
||
# all typescript packages, excluding docs | ||
TS_PKGS := $(ACCOUNT_PKGS),$(DEMOS_PKGS),${BACKEND_ONLY_PKGS},apps/submitter | ||
TS_PKGS := $(ACCOUNT_PKGS),$(DEMOS_PKGS),${BACKEND_ONLY_PKGS},apps/submitter,apps/swarm-leaderboard,apps/swarm-frontend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about leaderboard-backend
and leaderboard-frontend
? We can probably reuse for more than the swarm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! Will update this.
I planned to have both frontend and backend in the same app (not a good idea lol), updated this 🫡
apps/swarm-frontend/package.json
Outdated
"react": "^18.3.1", | ||
"react-dom": "^18.3.1", | ||
"sonner": "^1.7.2", | ||
"viem": "^2.21.53" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use all the sames deps here, but we won't need sonner and phosopor-icon at first (we can choose to add them later). If we use wagmi, we can also remove viem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed the toast notifs (sonner) and phosopor icon. Will remove viem and switch to wagmi later. (in TODO)
apps/swarm-leaderboard/README.md
Outdated
|
||
## Quickstart | ||
|
||
- `bun install` — Install dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be make setup
apps/swarm-leaderboard/README.md
Outdated
- `make migrate` — Run database migrations (customize as needed) | ||
- `make migrate-fresh` — Reset and re-run migrations | ||
- `make migrate-rollback` — Roll back the last migration | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can skip everything below, it's just a repeat of the list.
(If you copied this from somewhere, feel free to make that change there as well.)
export type User = Selectable<UserTable> | ||
export type NewUser = Insertable<UserTable> | ||
export type UserUpdate = Updateable<UserTable> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about UpdateGame
for consistency with NewGame
?
|
||
export interface UserTable { | ||
id: Generated<number> | ||
happy_wallet: string // primary login wallet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably should be typed as Address
(import from common
) unless the type is used for some kind of autogen?
export interface GuildTable { | ||
id: Generated<number> | ||
name: string | ||
code: string // unique code for joining |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this leaks, it's a bit of an issue. Maybe have a guild admin that has the right to add people to the guild instead?
export interface SessionTable { | ||
id: Generated<number> | ||
user_id: number // FK to users | ||
session_uuid: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea to read up on uuid & session best practices
user_id: number // FK to users | ||
game_id: number // FK to games | ||
score: number | ||
played_at: ColumnType<Date, string, never> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this meant to be the latest time the score was updated? (Then maybe last_updated_at
)
if (criteria.id) query = query.where("id", "=", criteria.id) | ||
if (criteria.name) query = query.where("name", "=", criteria.name) | ||
if (criteria.icon_url) query = query.where("icon_url", "=", criteria.icon_url) | ||
if (criteria.created_at) query = query.where("created_at", "=", criteria.created_at) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless this file is autogen, I don't think searching by icon_url or created_at is useful 😅
Tbh, probably name neither, to be useful needs to be fuzzy search. Was curious, and sqlite apparently supports this via
https://www.sqlite.org/spellfix1.html
https://www.sqlite.org/fts3.html
Not repeating comment for other tables bu tof course applies as well.
apps/swarm-leaderboard/src/server.ts
Outdated
const userRepo = new UserRepository(db) | ||
|
||
// GET /users | ||
app.get("/users", async (c) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will probably want some zod validation on these routes, check the submitter to see how that's done.
- using the openAPI middleware for route description
- needs the
cors
middleware (can be applied a single time on theapp
initially I think) otherwise we won't be able to call the API from browsers
apps/swarm-leaderboard/src/server.ts
Outdated
if (query.happy_wallet !== undefined) criteria.happy_wallet = String(query.happy_wallet) | ||
if (query.name !== undefined) criteria.name = String(query.name) | ||
if (query.guild_id !== undefined) criteria.guild_id = Number(query.guild_id) | ||
if (query.created_at !== undefined) criteria.created_at = new Date(String(query.created_at)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that by default this allows an empty query to return ALL users, which we probably don't want to allow. zod validation as an union can fix this.
I also think zod can be used for parsing so there would be no need for the entire logic here (would be replaced by the zod logic) and query
could then be used as the criteria directly.
Makefile
Outdated
|
||
# all typescript packages, excluding docs | ||
TS_PKGS := $(ACCOUNT_PKGS),$(DEMOS_PKGS),${BACKEND_ONLY_PKGS},apps/submitter | ||
TS_PKGS := $(ACCOUNT_PKGS),$(DEMOS_PKGS),${BACKEND_ONLY_PKGS},apps/submitter,apps/swarm-leaderboard,apps/swarm-frontend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add a SWARM_PKGS
export async function startServer(port: number) { | ||
await initDb() | ||
console.log(`Server running on port ${port}`) | ||
return Bun.serve({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious why using Bun.serve
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be the alternative approach right?
import { initDb } from "./initDB";
import { app } from "./server";
const port = Number(process.env.PORT) || 3000;
// Start DB initialization as soon as possible
const dbReady = initDb().then(() => {
console.log(`Database initialized, server ready on port ${port}`);
}).catch((err) => {
console.error("Failed to initialize database:", err);
process.exit(1);
});
const fetch: typeof app.fetch = async (request, ...args) => {
await dbReady; // Ensure DB is ready before handling requests
return app.fetch(request, ...args);
};
export default {
port,
fetch,
};
Just wanted to ask, I want to initDB
first, before launching the server, so that it sets up the tables. What's the correct way to do this (Please ping me on slack if it's non trivial 😅 ),
- Do I create a table if it doesn't exist on the first API request I get on that table's endpoint?
- Or is there a way to check if the table exists or not, and set all of them up when spinning up the server?
Or another way altogether, what's the proper approach to handle this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: This was just bad code, I fixed the approach to use proper migrations
}) | ||
|
||
// POST /users | ||
app.post("/users", async (c) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of separate app.__
calls, if you chain the routes, you can get a typesafe rpc sdk for your front end :)
const routes = app.get('/', () => {...})
.get('/users', () => {...})
.get('/post', () => {...})
export type RoutesType = typeof route
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have kind of implemented it, but unsure if I did that correctly (having trouble integrating the openapi part of the code, so I will try that in the end, having trouble wrapping my head around it 😓 )
"@vitejs/plugin-react-swc": "^3.7.0", | ||
"autoprefixer": "^10.4.20", | ||
"postcss": "^8.4.41", | ||
"tailwindcss": "^3.4.10", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should use tailwind 4 (if its not a painful change at this point) :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will start from scratch on this later 😅
tbh I don't even know the basics of tailwind or CSS, so I'm even afraid of touching this part 😓
apps/leaderboard-frontend/README.md
Outdated
## Running from the Monorepo Root | ||
|
||
To run the frontend (with iframe support, if needed), from the repo root: | ||
|
||
- `make leaderboard-frontend.dev` — Dev mode (hot reload, with iframe) | ||
- `make leaderboard-frontend.prod` — Production mode (with iframe) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably this info doesn't live here, as root make help
is documented 🤔 maybe we can clean that up (not part of this pr, just thinking)
let query = this.db.selectFrom("users") | ||
if (criteria.id) query = query.where("id", "=", criteria.id) | ||
if (criteria.happy_wallet) query = query.where("happy_wallet", "=", criteria.happy_wallet) | ||
if (criteria.name) query = query.where("name", "=", criteria.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think kysely prefers .$if
conditionals https://kysely.dev/docs/recipes/conditional-selects
import type { Kysely } from "kysely" | ||
import type { Database, NewSession, Session, UpdateSession } from "../db/types" | ||
|
||
export class SessionRepository { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably no point in a session repository i think? (since you are using a middleware to handle this)
@@ -0,0 +1,69 @@ | |||
import { db } from "./db/driver" | |||
|
|||
export async function initDb() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is interesting, why not use migrations? (and how will you handle future changes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temporary for now, will handle migrations when I get there.
It's all new stuff with Kysely, so this is just enough to get it to work for now 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: using migrations now
user_id: number // FK to users | ||
game_id: number // FK to games |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you want to get real cool 😎 use branded types
export type UserTableId = Generated<number> & { _brand: 'users_id' }
export interface UserTable {
id: UserTableId
//...
}
export interface UserGameScoreTable {
id: Generated<number>
user_id: UserTableId
game_id: GameTableId
//...
then you can never accidently pass a game_id
to user_id
target: "node", | ||
external: ["better-sqlite3"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just as a question/comment (dont need to change anything if its working fine)... why better-sqlite3 & node here?
also this would be production, where as in dev, you are using bun --bun
which is explicitly not node (thats what --bun
does)
seems a bit weird to me to have such different targets... and in your db you are using bun:sqlite
not better-sqlite3
so 🤔 how does that work 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this, it's an artifact from the randomness service I think, when I was trying to get the new package to compile with the existing Makefile structure 😅
Fixed this now 🫡
d0c752c
to
d013d49
Compare
…ts manually [skip ci]
… comments [skip ci]
…and related records [skip ci]
…rd routes [skip ci]
5e98b69
to
74cfd9a
Compare
Linked Issues
Description
This PR adds the Leaderboard backend (
apps/leaderboard-leaderboard
) built with:The backend includes database models and repositories for users, guilds, games, scores, and leaderboards.
Updates to the Makefile add build, development, and production targets for the app.
To run:
cp apps/leaderboard/.env.example apps/leaderboard/.env
(the values are already filled in, but just to confirm, they should be :-make migrate-fresh
(and latermake migrate
if reqd) to setup the sqlite dbTo run the server :-
From the project root:
make leaderboard-backend.dev
(to run server in dev mode)Or, from the project root:
make leaderboard-backend.prod
Else,
cd apps/leaderboard-backend
And run
make setup
->make build
->make dev
ormake prod
Toggle Checklist
Checklist
Basics
norswap/build-system-caching
).Reminder: PR review guidelines
Correctness
C1. Builds and passes tests.
C2. The code is properly parameterized & compatible with different environments (e.g. local,
testnet, mainnet, standalone wallet, ...).
C3. I have manually tested my changes & connected features.
C4. I have performed a thorough self-review of my code after submitting the PR,
and have updated the code & comments accordingly.
Architecture & Documentation
(2) commenting these boundaries correctly, (3) adding inline comments for context when needed.
Public APIS and meaningful (non-local) internal APIs are properly documented in code comments.
in a Markdown document.
make changeset
forbreaking and meaningful changes in packages (not required for cleanups & refactors).