fix(dice-duel): prevent predictable outcomes with commit-reveal

[safal207]

Closes #1

Summary

• require both session-bound 256-bit commitments before the Hub locks stakes
• verify each reveal and derive four dice from both secrets, independent of session_id and reveal order
• add inclusive deadlines, both forfeiture directions, and neutral zero-reveal cancellation
• make the mock Hub stateful with exact terms, auth, exactly-once settlement, refund accounting, and compatible existing event shapes
• add canonical-XDR golden vectors, rollback/auth/event regressions, and optimized-WASM resource coverage

Verification

• cargo fmt -p dice-duel -p mock-game-hub -- --check
• cargo clippy -p dice-duel -p mock-game-hub --all-targets --no-deps -- -D warnings
• cargo test --workspace --locked -j 2 — 93 passed, 0 failed, 1 expected ignored resource test
• cargo build --locked --release --target wasm32v1-none -p dice-duel -p mock-game-hub
• cargo test -p dice-duel --test wasm_resource --locked -- --ignored --nocapture — passed

Peak observed optimized-WASM lifecycle budget: 1,397,614 CPU instructions / 2,693,428 bytes memory.

Readiness gates

This remains a draft until all deployment and client gates are resolved:

1. The production Hub must confirm the exact cancel_game(session_id) neutral-settlement ABI and real refund semantics.
2. The deployment flow must not reuse a legacy mock Hub that lacks the new stateful cancellation behavior.
3. The incompatible game storage schema requires a new Dice contract ID or proof that all legacy sessions are drained before upgrade.
4. The Studio still targets the legacy testnet contract and ABI. Regenerate bindings only after the new deployment, then implement a two-round commitment exchange with secure local secret retention and explicit timeout/forfeit/cancel UI states.

No production deployment or Studio contract-ID change is included here.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[gitar-bot]

💡 Quality: resolve_timeout returns GameAlreadyEnded for a Ready game

In resolve_timeout, a game in the Ready phase (both players validly revealed but reveal_winner not yet called) falls through the phase != Phase::Revealing guard and returns Error::GameAlreadyEnded. That is misleading: the game has not ended — it simply must be settled via reveal_winner rather than timed out. Callers/clients may interpret GameAlreadyEnded as a terminal state and stop, when the correct action is to call reveal_winner. Consider a distinct error (e.g. reuse BothPlayersNotRolled's counterpart or add a ReadyForSettlement code) so the timeout path signals "use reveal_winner" instead of "ended".

_{Was this helpful? React with 👍 / 👎}

[gitar-bot]

Code Review 👍 Approved with suggestions 0 resolved / 1 findings

Implements commit-reveal to prevent predictable dice-duel outcomes with session-bound 256-bit commitments, independent reveals, inclusive deadlines, and neutral cancellation. The core logic is sound and well-tested across unit, integration, and optimized-WASM resource coverage. Consider clarifying the resolve_timeout error for games in the Ready phase—returning GameAlreadyEnded is misleading since the game hasn't ended but must be settled via reveal_winner instead.

💡 Quality: resolve_timeout returns GameAlreadyEnded for a Ready game

📄 contracts/dice-duel/src/lib.rs:474-476

In resolve_timeout, a game in the Ready phase (both players validly revealed but reveal_winner not yet called) falls through the phase != Phase::Revealing guard and returns Error::GameAlreadyEnded. That is misleading: the game has not ended — it simply must be settled via reveal_winner rather than timed out. Callers/clients may interpret GameAlreadyEnded as a terminal state and stop, when the correct action is to call reveal_winner. Consider a distinct error (e.g. reuse BothPlayersNotRolled's counterpart or add a ReadyForSettlement code) so the timeout path signals "use reveal_winner" instead of "ended".

🤖 Prompt for agents

Code Review: Implements commit-reveal to prevent predictable dice-duel outcomes with session-bound 256-bit commitments, independent reveals, inclusive deadlines, and neutral cancellation. The core logic is sound and well-tested across unit, integration, and optimized-WASM resource coverage. Consider clarifying the `resolve_timeout` error for games in the `Ready` phase—returning `GameAlreadyEnded` is misleading since the game hasn't ended but must be settled via `reveal_winner` instead.

1. 💡 Quality: resolve_timeout returns GameAlreadyEnded for a Ready game
   Files: contracts/dice-duel/src/lib.rs:474-476

   In resolve_timeout, a game in the `Ready` phase (both players validly revealed but reveal_winner not yet called) falls through the `phase != Phase::Revealing` guard and returns `Error::GameAlreadyEnded`. That is misleading: the game has not ended — it simply must be settled via reveal_winner rather than timed out. Callers/clients may interpret GameAlreadyEnded as a terminal state and stop, when the correct action is to call reveal_winner. Consider a distinct error (e.g. reuse `BothPlayersNotRolled`'s counterpart or add a `ReadyForSettlement` code) so the timeout path signals "use reveal_winner" instead of "ended".

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

---

Important

Your trial ends in 4 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more.

_{Was this helpful? React with 👍 / 👎 | Gitar}

[leocagli]

Mergeado. Verifiqué la rama entera en local antes:

cargo build --locked --release --target wasm32v1-none    rc=0
cargo test --workspace --locked                          93 tests, 0 fallos

Coincide con lo que declarás en la descripción.

Lo que cierra

Este PR resuelve la #1, que era el problema más grave del repositorio: el
resultado del dado se podía calcular antes de apostar, así que quien creaba la
partida elegía si ganaba.

Y hace algo que yo había marcado como riesgo aparte: el mock-game-hub ahora
tiene estado. Antes aceptaba cualquier cosa y no movía nada, así que los tres
juegos pasaban sus suites en verde contra un hub que no existía. Una suite
entera podía estar verde con un juego apostando puntos que el jugador no tenía.
Ahora hay términos exactos, autorización y liquidación exactamente una vez.

Una corrección que me toca a mí

Cuando abrí este PR pensé que el problema era que reimplementaste el commit y
revelación en dice-duel en vez de usar game-hub-core, que ya está en main
desde ayer con exactamente esas funciones.

Comparé las dos y la tuya es mejor que la mía en tres puntos concretos:

1. Preimage canónico. Vos armás un CommitmentPreimage tipado y hasheás
to_xdr(). Yo concateno bytes a mano:

buf.append(&contract.to_string().to_bytes());
buf.append(&Bytes::from_array(env, &game.to_val().get_payload().to_be_bytes()));
buf.append(&Bytes::from_array(env, &session_id.to_be_bytes()));
buf.append(&player.to_string().to_bytes());

Eso funciona porque las direcciones en string son de ancho fijo, pero es una
suposición que tu versión no necesita. XDR lleva tipo y longitud.

2. Separación de dominio. Tenés COMMITMENT_DOMAIN en el preimage. Yo no
tengo nada equivalente, así que un hash de este esquema podría colisionar con
uno de otro esquema del mismo sistema.

3. Qué ata el compromiso. Vos atás los dos jugadores y las dos apuestas. Mi
commitment() recibe un solo stake y ata sólo el lado de quien se
compromete. En mi plantilla no es explotable porque las apuestas se fijan al
crear y no cambian, pero el primitivo es más débil que el tuyo.

Aparte, mi truco de identificar el juego con
game.to_val().get_payload().to_be_bytes() depende de la representación interna
de Val en Soroban. Tu Bytes plano no depende de nada.

Así que el problema no es que no hayas usado el crate compartido: es que el
crate compartido debería adoptar tu enfoque. Abro una issue para eso y te la
ofrezco si te interesa, porque la escribiste vos.

Un detalle que tu PR destapó, y es mío

Compilando tu rama apareció esto:

warning: type `__GAME_STRUCT__` should have an upper camel case name
  --> contracts/_template/src/lib.rs:71:12

Es el marcador de la plantilla que agregué ayer. Está bien que sea un marcador,
pero deja un warning permanente en el workspace y rompería un gate de
-D warnings como el que usás. Lo arreglo yo, no es tuyo.

Gracias por el laburo, y por los vectores golden de XDR, que es la clase de test
que nadie escribe hasta que algo se rompe en producción.

[leocagli]

Aclaración: está en borrador, así que no puedo mergearla todavía. De mi lado ya está aprobada y verificada, con el detalle en el comentario de arriba.

Cuando la marques como lista la mergeo. Si le falta algo, sin apuro.

[safal207]

Gracias, Leo, por verificar la rama y por la revisión detallada.

Voy a mantenerla en borrador por ahora. Tu ejecución valida el cambio de contrato en el head "88c6a069dbe2088d7c83fb319ffebf12a3a9c1ce", pero no cierra los cuatro gates de despliegue y cliente:

• no hay una confirmación que vincule el Hub desplegado con "cancel_game(session_id)" y los reembolsos exactos;
• "deploy.ts" todavía puede reutilizar el Hub legado comprobando únicamente que existe;
• el estado cambia de "temporary" a "persistent" y no hay migración, nuevo Dice contract ID ni prueba de sesiones antiguas drenadas;
• Studio conserva el ABI, bindings y flujo anteriores, sin commitments, retención segura de secretos ni estados de timeout/forfeit/cancel.

No afirmo que el Hub desplegado carezca de "cancel_game"; la afirmación exacta es que esa compatibilidad todavía no está demostrada.

Además, "main" avanzó a "bc60a285727ad35a3c0c99062811d0bf2e6b31ff", por lo que el resultado combinado exacto aún no fue probado.

Si preferís fusionarlo como un cambio contract-only y dejar esos cuatro puntos como gates de despliegue en follow-ups separados, confirmámelo. Primero ajustaré la descripción, actualizaré la rama contra "main" y volveré a ejecutar la suite sobre el nuevo SHA. Mientras tanto, mantengo el borrador.

[safal207]

Thanks again for verifying the build and all 93 tests.

To avoid blocking the GrantFox review while keeping deployment safety explicit, can we treat #1 as the contract-layer deliverable, merge the verified implementation, and track deployment reuse, storage migration, Hub ABI integration, and frontend migration as separate follow-ups?

If that boundary works for you, I’ll update the branch, resolve the remaining review note, and mark PR #14 ready for review.

[safal207]

Hi @leocagli, following up on the scope question from 2 September.

Can you confirm that #1 can be accepted as a contract-layer deliverable, with Hub compatibility, deployment reuse checks, storage migration/new deployment, and frontend migration tracked as explicit prerequisites for deployment in separate follow-ups?

Your successful build and 93-test run covered 88c6a069dbe2088d7c83fb319ffebf12a3a9c1ce. The branch is now behind main. Once that delivery boundary is confirmed, I will update the branch and PR description, address the remaining timeout-error review note, rerun validation on the resulting commit, and mark the PR ready after those checks pass.

If accepting the contract layer separately does not work for this project, please identify which integration requirement must be completed within this PR. That will let me proceed against a clear acceptance boundary. Thanks!

[safal207]

Hi @leocagli, could you give a scope decision by 16 September 2026 on the question in my 2 and 7 September comments?

Can #1 be accepted as the contract-layer deliverable, with Hub compatibility, deployment reuse, storage migration/new deployment, and frontend integration remaining explicit deployment prerequisites in separate follow-ups? Or must any of those be completed in PR #14 before acceptance? If so, please identify the required items and dependencies.

Your earlier build and 93-test verification are appreciated. Once the acceptance scope is confirmed, I can update the branch, address the remaining review note, and validate the resulting commit before marking the PR ready. The existing verification does not cover that future updated commit or deployment readiness.

If you cannot decide by that date, please let me know who owns the decision and when it can be made. Until the scope is clarified, I am keeping the PR in draft and additional implementation paused. Thanks!