-
Notifications
You must be signed in to change notification settings - Fork 4
44 block redis #48
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: main
Are you sure you want to change the base?
44 block redis #48
Conversation
BREAKING_CHANGE: api to message broker, keep old route for testing and connect the form to a redis publisher for adding a tournament Makefile docs/block-local-test.md package-lock.json srcs/blockchain/.env.test.blockchain srcs/blockchain/src/SmartContract/dump.rdb srcs/blockchain/src/SmartContract/ignition/deployments/chain-31337/deployed_addresses.json srcs/blockchain/src/SmartContract/ignition/deployments/chain-31337/journal.jsonl srcs/blockchain/src/module/block.consumers.ts srcs/blockchain/src/module/block.controller.ts srcs/blockchain/src/module/block.routes.ts srcs/blockchain/src/module/block.schema.ts srcs/blockchain/src/module/block.service.ts srcs/blockchain/src/plugins/fastify-ioredis.ts srcs/blockchain/src/views/index.ejs srcs/nginx/src/html/app.js
test(blockchain): convert test to match subscriber receipt data Makefile srcs/blockchain/Dockerfile.dev srcs/blockchain/src/SmartContract/dump.rdb srcs/blockchain/src/SmartContract/ignition/deployments/chain-31337/journal.jsonl srcs/blockchain/src/config/env.ts srcs/blockchain/src/core/GameStorage.client.ts srcs/blockchain/src/core/database.ts srcs/blockchain/src/module/block.consumers.ts srcs/blockchain/src/module/block.controller.ts srcs/blockchain/src/module/block.service.ts srcs/blockchain/src/plugins/fastify-ioredis.ts srcs/blockchain/src/server.ts srcs/blockchain/src/test/app.test.ts
fix(block): redis type in route srcs/blockchain/.env.test.blockchain srcs/blockchain/src/SmartContract/ignition/deployments/chain-31337/deployed_addresses.json srcs/blockchain/src/SmartContract/ignition/deployments/chain-31337/journal.jsonl srcs/blockchain/src/config/env.ts srcs/blockchain/src/module/block.routes.ts srcs/blockchain/src/types/fastify.d.ts
feature(nginx): add route to access to DAPP /blockchain package-lock.json srcs/blockchain/nodemon.json srcs/blockchain/package.json srcs/blockchain/src/module/block.routes.ts srcs/blockchain/src/views/header.ejs srcs/dev-docker-compose.yml srcs/nginx/conf.d/default.conf srcs/nginx/src/html/app.js
Ilia1177
left a comment
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.
J'ai pas tout compris... Mais ca a l'air plutot cool !
| proxy_read_timeout 3600s; # 1h - temps max sans réception de données | ||
| } | ||
| location /blockchain { | ||
| proxy_pass http://api-gateway:3000/api/block/blockchain; |
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.
block/blockchain c'est un peu redondant comme chemin ? J'ai pas bien compris l'utilité de faire des proxy_pass sur les services...
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
| id: string, | ||
| fields: string[], | ||
| ): Promise<void> { | ||
| const payload = JSON.parse(fields[1]); |
Copilot
AI
Jan 14, 2026
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.
The parsing of stream data JSON.parse(fields[1]) is unsafe and could throw if the data is malformed. Consider adding try-catch error handling around this operation to prevent the consumer from crashing, and log the problematic message ID for debugging.
| const payload = JSON.parse(fields[1]); | |
| let payload: any; | |
| try { | |
| payload = JSON.parse(fields[1]); | |
| } catch (err) { | |
| app.log.error( | |
| { | |
| event: 'tournament.payload_parse_error', | |
| streamId: id, | |
| rawPayload: fields[1], | |
| err, | |
| }, | |
| 'Failed to parse tournament payload from stream', | |
| ); | |
| // Acknowledge malformed message to avoid poisoning the consumer with unrecoverable data | |
| try { | |
| await redis.xack(STREAM, GROUP, id); | |
| } catch (ackErr) { | |
| app.log.error( | |
| { | |
| event: 'tournament.malformed_ack_error', | |
| streamId: id, | |
| err: ackErr, | |
| }, | |
| 'Failed to acknowledge malformed tournament message', | |
| ); | |
| } | |
| return; | |
| } |
| <meta http-equiv="X-UA-Compatible" content="IE=edge"> | ||
| <title><%= title %></title> | ||
| <link rel="stylesheet" href="/output.css"> | ||
| <link rel="stylesheet" href="http://localhost:3005/output.css"> |
Copilot
AI
Jan 14, 2026
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.
The hardcoded URL http://localhost:3005/output.css will fail in containerized environments where the blockchain service is not accessible on localhost. This should use a relative path like /output.css or reference an environment variable for the base URL.
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.
Effectivement pas direct dans le code mais env
| export const env = cleanEnv(process.env, { | ||
| NODE_ENV: str({ | ||
| choices: ['development', 'test', 'production', 'staging'], | ||
| default: 'developement', |
Copilot
AI
Jan 14, 2026
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.
The spelling of 'development' in the NODE_ENV default value is incorrect ('developement' instead of 'development'). This will cause the environment validation to fail since 'developement' is not in the choices array.
| default: 'developement', | |
| default: 'development', |
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
| consumeLoop(app, redis).catch((err) => app.log.error({ err }, 'Tournament consumer fatal error')); | ||
| } | ||
|
|
||
| async function consumeLoop(app: FastifyInstance, redis: any): Promise<void> { |
Copilot
AI
Jan 14, 2026
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.
The type annotation redis: any loses TypeScript's type safety. Since you've already imported Redis from ioredis (line 11) and used it in processMessage, you should use redis: Redis here for consistency and type safety.
rom98759
left a comment
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.
Enlevé juste les fichiers inutiles en trop et c'est parfait
| # COPY src ./src | ||
| COPY .env.test ./ | ||
| COPY .env.test.blockchain ./ |
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.
Plus de copie de src ?
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.
Fichier nécessaire ?
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.
Ce fichier est généré auto avec hardhat donc pas nécessaire, surtout qu'il change à chaque fois
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.
Ce fichier est généré auto avec hardhat donc pas nécessaire, surtout qu'il change à chaque fois
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.
Ce fichier est généré auto avec hardhat donc pas nécessaire, surtout qu'il change à chaque fois
| export const env = cleanEnv(process.env, { | ||
| NODE_ENV: str({ | ||
| choices: ['development', 'test', 'production', 'staging'], | ||
| default: 'developement', |
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
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.
Superbe animation
| <meta http-equiv="X-UA-Compatible" content="IE=edge"> | ||
| <title><%= title %></title> | ||
| <link rel="stylesheet" href="/output.css"> | ||
| <link rel="stylesheet" href="http://localhost:3005/output.css"> |
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.
Effectivement pas direct dans le code mais env
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.
C'est le fichier compiler de index.ts ? A ne pas mettre dedans normalement
codastream
left a comment
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.
La PR est très intéressante et le readme aide à saisir les modifications (un peu de doc globale sur les méthodes de Redis comme broker de message aiderait aussi à bien comprendre). Les cas d'erreur ou de message en attente sont anticipés et beaucoup d'options de Redis utilisées.
Quelques modifs possibles:
- les types pour eslint
- ajouts au gitignore
- makefile colima peut il être simplifié ?
- Peut-on mettre à jour la doc dans le wiki sur l'API finale
et éventuellement plus tard
- logger et gestion d'erreurs
| proxy_send_timeout 3600s; # 1h - temps max sans envoi de données | ||
| proxy_read_timeout 3600s; # 1h - temps max sans réception de données | ||
| } | ||
| location /blockchain { |
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.
Je ne suis pas sûr d'avoir bien saisi l'architecture finale de nos API
Définie ainsi, la route blockchain est publique : est-ce nécessaire ? est-ce temporaire ? Est-ce pour l'API ou pour accéder à l'app décentralisée (Dapp) ?
Je pensais que la route pour stocker les scores, par exemple POST /tournaments ne serait accessible qu'en interne, et qu'il n'y aurait même pas besoin de passer pour cela par la gateway.
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.
Est-ce qu'on peut mettre à jour https://github.com/codastream/transcendence/wiki/API-Documentation en incluant au besoin les messages internes via Redis ?
| ifeq ($(OS),Darwin) | ||
| ifneq ($(CHIP), arm64) | ||
| @echo "Checking Colima status and mounts..." | ||
| @if ! colima list 2>/dev/null | grep -q "Running"; then \ | ||
| echo "Starting Colima with mount $(PROJECT_PATH)"; \ | ||
| colima start --mount "$(PROJECT_PATH):w" --vm-type vz; \ | ||
| else \ | ||
| echo "Colima is running, checking mounts..."; \ | ||
| if ! colima status 2>/dev/null | grep -q "$(PROJECT_PATH)"; then \ | ||
| echo "Mount missing, restarting Colima with correct mount..."; \ | ||
| colima stop; \ | ||
| colima start --mount "$(PROJECT_PATH):w" --vm-type vz; \ | ||
| else \ | ||
| echo "Mount already configured: $(PROJECT_PATH)"; \ | ||
| fi; \ | ||
| fi | ||
| else | ||
| @echo "Skipping Colima start: Chip is '$(OS)' (Not x386)" | ||
| endif | ||
| else | ||
| @echo "Skipping Colima start: OS is '$(OS)' (Not Darwin)" | ||
| endif |
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.
On a une vérif du processeur dans la vérif de l'OS, et apparemment il faut utiliser Colima seulement si Apple && puce Intel (donc pas arm64) ? En partant du même double check que colima
| ifeq ($(OS),Darwin) | |
| ifneq ($(CHIP),arm64) | |
| @echo "Target detected: Mac Intel ($(CHIP)). Checking Colima..." | |
| @if ! colima status 2>/dev/null | grep -q "Running"; then \ | |
| echo "Starting Colima for Intel Mac..."; \ | |
| colima start --mount "$(PROJECT_PATH):w" --vm-type vz; \ | |
| else \ | |
| echo "Colima is already running on $(CHIP)."; \ | |
| fi | |
| endif | |
| endif |
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 gonna change in the next PR.
Je crois que je suis le seul a utiliser colima (?)
Pour demarrer le projet avec COLIMA, le mieux est de declarer une varaible dans le .env COLIMA=true afin de lancer le projet avec COLIMA.
| @@ -0,0 +1,25 @@ | |||
| import { bool, cleanEnv, port, str } from 'envalid'; | |||
|
|
|||
| export const env = cleanEnv(process.env, { | |||
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.
👍
| @@ -1,5 +1,6 @@ | |||
| export interface AppLogger { | |||
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.
le service utilise déjà a priori le logger de Fastify. Il n'est a priori pas nécessaire de redéfinir une interface et surtout de le passer en paramètre des fonctions du service.
une solution plus serait de l'importer dans les fichiers après en avoir exposé une instance globale (soit dans app.ts, soit dans un fichier à part), et d'avoir un fichier de config (pour définir les niveaux, le mode de sérialisation, si possible de manière harmonisée entre les services)
cf #41
|
|
||
| const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); | ||
|
|
||
| while (!app.closing) { |
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.
après avoir demandé à Gemini, il semble qu'on puisse s'appuyer sur le hook onClose de Fastify pour fermer la connexion Redis. Dans l'état actuel, le lecteur reste bloqué pendant 5000ms.
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.
je ne premds pas le temps de regarder les diffs. c'est un peu long, et ça va bouger.
| restart: unless-stopped | ||
| healthcheck: | ||
| <<: *healthcheck-template | ||
| test: ["CMD", "wget", "--no-verbose", "--tries=1", "--spider", "http://localhost:${GAME_SERVICE_PORT}/health"] |
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.
pour la cohérence ?
| test: ["CMD", "wget", "--no-verbose", "--tries=1", "--spider", "http://localhost:${GAME_SERVICE_PORT}/health"] | |
| test: ["CMD", "wget", "--no-verbose", "--tries=1", "--spider", "http://127.0.0.1:${GAME_SERVICE_PORT}/health"] |
| healthcheck: | ||
| <<: *healthcheck-template | ||
| test: ["CMD", "wget", "--no-verbose", "--tries=1", "--spider", "http://localhost:${UM_SERVICE_PORT}/health"] | ||
| test: ['CMD', 'wget', '--no-verbose', '--tries=1', '--spider', '--no-check-certificate', 'https://localhost/health'] |
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.
| test: ['CMD', 'wget', '--no-verbose', '--tries=1', '--spider', '--no-check-certificate', 'https://localhost/health'] | |
| test: ['CMD', 'wget', '--no-verbose', '--tries=1', '--spider', '--no-check-certificate', 'https://127.0.0.1/health'] |
| **/*.js | ||
| *.pyc | ||
| __pycache__ | ||
| __pycache__ |
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.
| __pycache__ | |
| __pycache__ | |
| src/public/output.css | |
| src/SmartContract/ignition/deployments/*/artifacts/ | |
| src/SmartContract/ignition/deployments/*/journal.jsonl | |
| src/SmartContract/ignition/deployments/*/build-info/ | |
| src/SmartContract/artifacts | |
| src/SmartContract/build-info | |
| src/SmartContract/journal.json | |
| src/SmartContract/dump.rdb |
| test-coverage-user: build-core | ||
| cd srcs/users && npx vitest run --coverage --config vite.config.mjs | ||
|
|
||
| test-block: |
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.
on ajoute test-block à test pour le lancer aussi dans la CI ?
il faudrait apparemment adapter certaines commandes (@gnome-terminal ne fonctionnerait pas)
🚀 Pull Request: Blockchain Consumer Service & Redis Integration
📝 Description
Cette PR introduit la couche de consommation asynchrone pour le module Blockchain. L'objectif est de découpler la logique métier des tournois de leur enregistrement sur la blockchain (Avalanche/Solidity) en utilisant une architecture Event-Driven basée sur Redis Streams.
Changements majeurs :
npm run devpour assurer un environnement de développement stable.ioredispour le framework Fastify.startTournamentConsumer, un consommateur robuste pour le streamtournament.results.⚙️ Architecture du Consommateur
La fonction
startTournamentConsumerimplémente un pattern de consommation fiable (at-least-once) pour garantir qu'aucun résultat de tournoi ne soit perdu avant son enregistrement on-chain.1. Finalité Globale
Le service écoute les événements
tournament.results, les transforme enBlockTournamentInput, et déclenche le traitement viaaddSubTournament(enregistrement DB + Blockchain).Ce mécanisme assure le découplage entre :
2. Concepts Redis Streams utilisés
tournament.results) : Un journal d'événements persistant, ordonné et adressé par ID unique.blockchain-group) : Permet une répartition automatique des messages entre plusieurs instances et un suivi des messages non-acquittés via la PEL (Pending Entries List).🛠️ Détails Techniques (Opérations Redis)
L'implémentation repose sur trois commandes critiques pour la fiabilité du système :
XREADGROUPUtilisé dans la boucle de consommation principale (
consumeLoop) pour récupérer les nouveaux messages :>: Indique que nous voulons les messages qui n'ont jamais été délivrés à d'autres consommateurs.BLOCK 5000: Commande non-bloquante pour le thread principal, attendant jusqu'à 5s l'arrivée d'un message.XPENDINGUtilisé dans
recoverPendingpour inspecter la PEL (Pending Entries List).XCLAIMEssentiel pour la tolérance aux pannes :
✅ Checklist
Envoi de données au
blockchain-servicecommande Redis
Spécifications du format :
tour_id(string) : L'identifiant unique du tournoi.player1àplayer4(string) : Id des participants.Exemple de fonction d'envoi de données