feat: compartir el protocolo binario entre cliente y servidor - #79
Conversation
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| const decodedPacket = pkg.decodeClientPacket(data as PacketPayload); | ||
| pkg.setData(data as PacketPayload); | ||
| const packageID = pkg.getPackageID(); | ||
|
|
||
| if (decodedPacket.id !== packageID) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
⚠️ Performance: Redundant full packet decode added to WS hot path
Every inbound packet is now fully decoded twice: pkg.decodeClientPacket() parses the entire payload (including JSON.parse for marketAction/retosAction and an O(n) Object.entries(SERVER_PACKET_ID).find() reverse lookup per packet in getServerPacketName), and then protocol.handleData re-reads the same bytes through the real handlers. The only use of the decoded result is the decodedPacket.id !== packageID guard, but decodedPacket.id is derived from the same first byte that getPackageID() reads, so the check can never be true — it is dead code. This doubles per-packet parsing cost in the server's hottest path (movement/attack/ping traffic) for no functional benefit.
Remove the redundant decodeClientPacket pre-pass and its always-false id guard; keep validation inside the individual handlers.:
pkg.setData(data as PacketPayload);
const packageID = pkg.getPackageID();
trackClientActivity(ws, packageID);
protocol.handleData(ws, packageID);
Was this helpful? React with 👍 / 👎
| export function decodeClientPacket(input: BinaryInput): DecodedClientPacket { | ||
| const reader = new PacketReader(input); | ||
| const id = reader.getByte(); | ||
| const type = getServerPacketName(id); | ||
| let payload: ClientPacketPayloads[ServerPacketName]; | ||
|
|
||
| switch (type) { | ||
| case "connectCharacter": | ||
| payload = { ticket: reader.getString(), typeGame: reader.getByte(), idChar: reader.getByte() }; | ||
| break; | ||
| case "position": | ||
| payload = { heading: reader.getByte(), moveId: reader.getInt() }; | ||
| break; | ||
| case "click": | ||
| payload = { x: reader.getByte(), y: reader.getByte(), button: reader.getByte() }; |
There was a problem hiding this comment.
⚠️ Security: Malformed packets bypass flood tracking and spam error log
decodeClientPacket runs before trackClientActivity and throws a RangeError on any unknown opcode, truncated packet, or trailing bytes. Because the throw is caught and routed to dumpError before tracking happens, a connected client can flood the server with malformed packets that (a) completely bypass the per-connection PPS/interval anti-flood accounting in trackClientActivity, and (b) trigger a dumpError call for every bad packet, enabling log/telegram spam. Previously an unknown/garbage packet was still counted by trackClientActivity and then dropped as a no-op. If the pre-decode is kept, move trackClientActivity before it (or track inside the catch) so malformed traffic is still rate-limited and not logged per-packet.
Drop the pre-decode so malformed packets follow the same tracked, silent no-op path as before instead of throwing and skipping rate limiting.:
pkg.setData(data as PacketPayload);
const packageID = pkg.getPackageID();
trackClientActivity(ws, packageID);
protocol.handleData(ws, packageID);
Was this helpful? React with 👍 / 👎
|
hi could you re reun the workflows so i can know if theres something to change thank you |
| "build": "pnpm run clean && tsc && node scripts/copy-assets.cjs", | ||
| "predev": "pnpm run protocol:build", | ||
| "dev": "NODE_ENV=development tsx watch src/server.ts", | ||
| "test": "tsx --test tests/**/*.test.ts", |
There was a problem hiding this comment.
💡 Quality: client-activity policy test not wired into CI
The new test:client-activity script (testClientActivityPolicy.ts) guards the intentional ping-vs-activity invariant, but CI only runs pnpm test which globs tests/**/*.test.ts; the policy script lives under src/scripts/ and is never executed in CI. The invariant it protects (pings feed liveness but not AFK metrics) can silently regress. Add it to CI, e.g. run pnpm run test:client-activity alongside pnpm test, or move it into tests/.
Was this helpful? React with 👍 / 👎
|
Revisé esta contra el Los docker-compose dejan de construirReescribiste COPY packages/protocol/...
COPY server .y actualizaste Pero los seis build:
context: .Para Compose, ese Reproducido con docker 29.1.3 sobre tu rama: La misma imagen con contexto raíz sí construye: O sea que lo único roto es el contexto que pasan los compose. Por qué esto importa más de lo que parece: el CI queda verde porque su job El arreglo es en los seis archivos: build:
context: ../
dockerfile: server/Dockerfile(y el equivalente para frontend). Ajustá también los Lo otro, que no bloquea pero conviene mirarEn No es bloqueante y puede que sea intencional. Si lo es, decilo en el cuerpo de la Sobre lo demásMiré el resto con lente de seguridad porque tocás Arreglá los compose y la mergeamos. Aviso aparte: los PRs de fork quedan en |
ok perfecto muchas gracias por el feedback empezare a trabajar en esto!!! |
Code Review
|
| Auto-apply | Compact |
|
|
Important
Your trial ends in 5 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more.
Was this helpful? React with 👍 / 👎 | Gitar
2574dc0 to
2a3da42
Compare
|
Hola @leocagli , ya apliqué la corrección solicitada en los seis archivos docker-compose: todos usan ahora la raíz del repositorio como contexto y declaran explícitamente el Dockerfile correspondiente. También verifiqué que los volumes del servidor continúan resolviéndose correctamente. |
|
Verificado y mergeado. Gracias por el arreglo rápido. Comprobé que el problema que había señalado quedó resuelto corriendo el build de verdad, que es como lo había reproducido: Antes eso moría con Sobre lo otro que había mencionado, el decode estricto en |
2 participants
Resumen
Extrae el contrato binario compartido a
packages/protocolpara que frontend y servidor consuman una única fuente de verdad. Esta entrega migra completamente el flujo cliente→servidor y deja centralizados los opcodes de ambas direcciones, las primitivas binarias, constantes y modelos compartidos.También actualiza CI y Docker para construir el paquete antes que sus consumidores, y agrega una guía de pruebas manuales y automáticas.
Criterios de aceptación
Existe un paquete compartido con la definición del protocolo
@openao/protocolenpackages/protocol.PacketReader,PacketWritery codecs cliente→servidor.Cliente y servidor lo importan, sin copias locales de lo migrado
@openao/protocoly delega en él la creación de los 30 paquetes cliente→servidor.bytebuffery las copias locales migradas.Cambiar un opcode en el paquete compartido rompe la compilación de ambos lados
Hay tests de serialización y deserialización
decode(encode(payload))para cada uno de los 30 paquetes cliente→servidor migrados.El juego sigue funcionando igual
200.200.Validación automática
packages/protocol: typecheck, 34/34 tests y build.server: typecheck, lint y build.frontend: typecheck, lint y build de producción.git diff --check: sin errores.Documentación
Se agregó
docs/protocol-testing.mdcon instrucciones para:Closes #28