fix: SEP-10 verification, multi-sig fee control, matchmaking queue, ELO service#700
Conversation
, OpenKnight-Foundation#555, OpenKnight-Foundation#556 - OpenKnight-Foundation#529 Contract: SEP-10 challenge verification - Add issue_sep10_challenge / verify_sep10_challenge / is_sep10_verified - Nonce-based replay protection; challenge consumed on successful verify - Payload: SHA256(address_bytes || nonce_bytes || expiry_le8) - OpenKnight-Foundation#535 Contract: Multi-sig control for protocol fee parameters - Add configure_multisig (admin sets signers + M-of-N threshold) - Add propose_fee_change / approve_fee_proposal / cancel_fee_proposal - Fee change applied atomically when approvals reach threshold - Add get_fee_proposal / get_approval_count query helpers - OpenKnight-Foundation#555 Backend: Redis-based matchmaking queue (frontend integration) - Add frontend/lib/matchmakingService.ts — typed API client for the existing Rust/Redis matchmaking backend (join, status, leave, invite) - Add frontend/hook/useMatchmakingQueue.ts — React hook with polling, state machine (idle → searching → matched | error), and cleanup - OpenKnight-Foundation#556 Backend: ELO rating calculation service (frontend) - Add frontend/lib/eloService.ts — pure TS Elo implementation mirroring backend/modules/matchmaking/elo.rs (calculateMatchRatings, calculateDrawRatings, kFactor, expectedScore) - Add frontend/__tests__/eloService.test.ts — unit tests for all helpers Closes OpenKnight-Foundation#529, OpenKnight-Foundation#535, OpenKnight-Foundation#555, OpenKnight-Foundation#556
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR introduces three major feature areas: on-chain SEP-10 challenge verification with replay prevention via nonces, M-of-N multisig governance for fee parameter updates with threshold-based approvals, and frontend infrastructure for Elo rating calculations and Redis-backed matchmaking queue management with polling. Changes
Sequence DiagramssequenceDiagram
participant Admin as Admin/Issuer
participant Contract as Game Contract
participant Storage as Contract Storage
Admin->>Contract: issue_sep10_challenge(account)
Contract->>Storage: Store (nonce, expiry)
Contract-->>Admin: Return nonce
rect rgba(100, 150, 200, 0.5)
Note over Admin,Storage: Later: Verification
end
Admin->>Contract: verify_sep10_challenge(account, nonce, expiry, signature)
Contract->>Contract: Verify ED25519 signature over (account + nonce + expiry)
Contract->>Storage: Check nonce exists & not expired
alt Signature valid & nonce fresh
Contract->>Storage: Remove nonce (replay prevention)
Contract->>Storage: Record account in SEP10_VERIFIED
Contract-->>Admin: Success
else Invalid or expired
Contract-->>Admin: Error (InvalidSignature / NonceExpired)
end
sequenceDiagram
participant Signer1 as Signer 1
participant Signer2 as Signer 2
participant Contract as Game Contract
participant Storage as Contract Storage
Signer1->>Contract: configure_multisig(signers, threshold)
Contract->>Storage: Store signers & threshold config
Signer1->>Contract: propose_fee_change(new_fee, new_treasury)
Contract->>Storage: Create FeeProposal, auto-approve Signer1
Contract-->>Signer1: Proposal created
Signer2->>Contract: approve_fee_proposal(proposal_id)
Contract->>Storage: Increment approval count
Contract->>Storage: Check approval_count vs threshold
alt approval_count >= threshold
Contract->>Storage: Update FEE_BIPS & TREASURY_ADDR
Contract->>Storage: Clear pending proposal & approvals
Contract-->>Signer2: Success (fee updated)
else Approvals pending
Contract-->>Signer2: Proposal still pending
end
sequenceDiagram
participant UI as Frontend UI
participant Hook as useMatchmakingQueue Hook
participant Service as matchmakingService
participant API as Backend API
participant Queue as Redis Queue
UI->>Hook: join(matchRequest)
Hook->>Service: joinQueue(request)
Service->>API: POST /matchmaking/join
API->>Queue: Add requestId
API-->>Service: Return requestId & matchId
Service-->>Hook: Success
Hook->>Hook: Set state='searching'
rect rgba(100, 200, 150, 0.5)
Note over Hook,Queue: Polling Loop (every N ms)
end
loop Poll interval
Hook->>Service: getQueueStatus(requestId)
Service->>API: GET /matchmaking/status/:requestId
alt requestId still in queue
API-->>Service: QueueStatus
else Not found (matched/expired)
API-->>Service: 404 (null)
end
alt Not found
Hook->>Hook: Set state='matched', record matchId
loop Stop polling
end
end
end
UI->>Hook: leave()
Hook->>Service: leaveQueue(requestId)
Service->>API: DELETE /matchmaking/leave/:requestId
API->>Queue: Remove requestId
Hook->>Hook: Reset state='idle', clear identifiers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@Xuccessor Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (4)
contracts/game_contract/src/lib.rs (3)
1454-1465:expect("Not initialized")will panic instead of returning a typed error.
issue_sep10_challengeandconfigure_multisigbothexpect()onCONTRACT_ADMIN, which means callers receive a host-level panic ("Not initialized") rather than aContractError. Other admin-gated entry points in this file (e.g.set_max_stakeline 911) follow the same pattern, so this is consistent — but for new methods returningResult<…, ContractError>, prefer a typed error so SDK clients can handle it deterministically.- let current_admin: Address = env - .storage() - .instance() - .get(&CONTRACT_ADMIN) - .expect("Not initialized"); + let current_admin: Address = env + .storage() + .instance() + .get(&CONTRACT_ADMIN) + .ok_or(ContractError::Unauthorized)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/game_contract/src/lib.rs` around lines 1454 - 1465, The code currently calls .expect("Not initialized") when reading CONTRACT_ADMIN in functions like issue_sep10_challenge (and similarly configure_multisig / set_max_stake), which panics instead of returning a ContractError; change these to retrieve CONTRACT_ADMIN with .get(...).ok_or(...) and return a typed ContractError (e.g. Err(ContractError::NotInitialized) or a suitable existing variant) so issue_sep10_challenge and other admin-gated entry points return Err(...) instead of panicking; ensure you reference CONTRACT_ADMIN and the ContractError enum when making the change.
1539-1543: Truncation hazard ifaccount.to_string().len()ever exceeds 64 bytes.
copy_into_slice(&mut addr_buf[..str_len])will panic ifstr_len > 64. StellarG…/C…strkey addresses are 56 chars today, so this is fine in practice, but the same fixed-size buffer pattern is duplicated fromclaim_puzzle_rewardandclaim_win. Consider extracting anaddress_to_payload_bytes(&Env, &Address) -> Byteshelper to remove the magic64and prevent drift if Stellar adds longer address types in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/game_contract/src/lib.rs` around lines 1539 - 1543, The current code builds a fixed 64-byte buffer (addr_buf) and calls account_str.copy_into_slice(&mut addr_buf[..str_len]) which will panic if account_str.len() > 64; refactor by extracting a helper like address_to_payload_bytes(env: &Env, account: &Address) -> Bytes that allocates exactly the needed length (no fixed 64 magic), converts account.to_string() to bytes, and returns Bytes::from_slice(&env, &account_bytes), then replace the inline logic in this function and the duplicated sites claim_puzzle_reward and claim_win with calls to address_to_payload_bytes to avoid truncation hazards and remove duplicate code.
1771-1779: Auth check after storage read — minor ordering nit.
signer.require_auth()is invoked on line 1779 after both the signers-list lookup and the proposal-existence check. For consistency withpropose_fee_changeandapprove_fee_proposal(which callrequire_authafter the membership check but before the bulk of the work), this is fine; however callingrequire_authfirst would short-circuit unauthorized callers without doing extra storage reads. Optional — the current ordering does not produce incorrect behaviour becauserequire_authwill still fail before any state change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/game_contract/src/lib.rs` around lines 1771 - 1779, The auth check is done after reading storage for PENDING_FEE_PROPOSAL; move the authentication earlier so unauthorized callers are rejected before performing storage reads. Specifically, in the function that currently calls env.storage().instance().has(&PENDING_FEE_PROPOSAL) then signer.require_auth(), call signer.require_auth() first (matching the pattern used by propose_fee_change and approve_fee_proposal) and then perform the signers-list lookup and the proposal-existence check.frontend/hook/useMatchmakingQueue.ts (1)
128-132: Consider best-effortleaveQueueon unmount.If a user navigates away while
state === "searching", polling stops but the queue entry stays in Redis until it expires. A best-effortleaveQueue(requestIdRef.current)in the cleanup would avoid stale entries and ghost matches.♻️ Proposed change
useEffect(() => { return () => { stopPolling(); + const id = requestIdRef.current; + if (id) { + // Best-effort; ignore failures on unmount. + void leaveQueue(id).catch(() => {}); + } }; }, [stopPolling]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/hook/useMatchmakingQueue.ts` around lines 128 - 132, In the useEffect cleanup, add a best-effort call to leaveQueue(requestIdRef.current) before/after stopPolling to avoid leaving stale Redis entries when state === "searching"; check that requestIdRef.current exists and state === "searching", call leaveQueue in a fire-and-forget way (wrap in an immediately-invoked async function or use void leaveQueue(...)) and swallow/log errors (try/catch) so unmount is not blocked; keep stopPolling() as before and reference useEffect, stopPolling, leaveQueue, requestIdRef, and state to locate the code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/game_contract/src/lib.rs`:
- Around line 1688-1757: approve_fee_proposal currently counts approvals via
approvals.len() which may include approvals from signers removed via
configure_multisig; to fix, modify configure_multisig to clear any in-flight
proposal and approvals by removing PENDING_FEE_PROPOSAL and
FEE_PROPOSAL_APPROVALS from storage (env.storage().instance().remove(...))
whenever MULTISIG_SIGNERS is updated, ensuring that a signer rotation always
forces a fresh proposal and prevents stale approvals from counting in
approve_fee_proposal.
- Around line 1636-1683: propose_fee_change currently overwrites any existing
PENDING_FEE_PROPOSAL and resets FEE_PROPOSAL_APPROVALS, enabling a single signer
to grief in-flight proposals; change propose_fee_change to first check
env.storage().instance().get(&PENDING_FEE_PROPOSAL) and return an error (e.g.,
ContractError::PendingProposalExists) if a proposal is present, so new proposals
are rejected until the existing one is explicitly cancelled; keep the existing
proposer.require_auth() and leave cancel_fee_proposal to handle removal
(follow-up: tighten cancel rights), and update the event/behavior accordingly.
- Around line 1605-1630: The configure_multisig function accepts duplicated
addresses in signers which breaks approval semantics (FEE_PROPOSAL_APPROVALS is
keyed by Address); fix by validating uniqueness before persisting: build a
temporary set from signers and if set.len() != signers.len() return an
appropriate ContractError (e.g., DuplicateSigner or add InvalidSigners), perform
this check prior to setting MULTISIG_SIGNERS and MULTISIG_THRESHOLD, and only
persist after the uniqueness and threshold validations pass.
- Around line 1454-1496: Store the intended account alongside the nonce when
issuing SEP-10 challenges and enforce that the verifier is the same account:
modify issue_sep10_challenge to save a mapping from nonce (BytesN<32>) to a
tuple/value containing (account: Address, expiry: u64) instead of only expiry in
SEP10_CHALLENGES, and update verify_sep10_challenge to load that stored tuple
and reject if the supplied account does not match the stored account; also call
account.require_auth() (or otherwise require auth on the verifying account) in
verify_sep10_challenge to ensure only the intended account can complete
verification.
- Around line 1511-1524: The code incorrectly maps a missing nonce lookup to
ContractError::ChallengeAlreadyUsed; change this to a distinct error (e.g.,
ContractError::ChallengeNotFound or reuse an appropriate existing variant like
ContractError::Unauthorized) so a non-existent nonce is not treated as a replay.
Update the get() -> ok_or(...) call on the Map<BytesN<32>, u64> named challenges
(the lookup using nonce.clone()) to return the new/appropriate error variant,
and add the new enum variant to ContractError if needed, keeping
ContractError::ChallengeAlreadyUsed reserved for the actual replay path where a
nonce was previously consumed.
In `@contracts/game_contract/src/test.rs`:
- Around line 472-477: The comment is off-by-one: propose_fee_change (which
auto-approves the proposer) means there is already one approval before calling
approve_fee_proposal(&signer2), so after signer2 approves the proposal reaches
the threshold and is executed; update the inline comment above the
approve_fee_proposal(&signer2) call to reflect that the proposer was
auto-approved by propose_fee_change and that this call provides the second
approval (making it 2-of-3 and triggering execution) — reference
propose_fee_change and approve_fee_proposal in the comment so readers know why
the approval count is different.
- Around line 398-403: The test incorrectly assumes env.mock_all_auths()
bypasses ed25519_verify; update the test to either (A) generate a real Ed25519
signature and public key like tests::setup in lib.rs using
ed25519_dalek::SigningKey and pass that valid signature to
verify_sep10_challenge (ensure admin_pubkey and digest_bytes match the signer),
or (B) avoid calling verify_sep10_challenge at all and instead assert the
storage-only behavior you want (e.g., use init_contract helper only for storage
checks); apply the same fix to test_sep10_challenge_cannot_be_reused so no
ed25519_verify call receives all-zero keys/signatures.
In `@frontend/__tests__/eloService.test.ts`:
- Around line 68-71: The test uses parameters where delta is zero so the 100
floor isn't exercised; update the test that calls calculateNewRating to use
inputs that produce a negative delta large enough to push rating + delta below
100 (for example use rating like 110 with a very high opponentRating and result
"loss" and K=40) and then assert that the returned newRating is clamped to 100;
reference the calculateNewRating function and its returned newRating/delta
values when changing the test data so it actually hits the floor branch.
In `@frontend/hook/useMatchmakingQueue.ts`:
- Around line 55-79: startPolling uses setInterval which allows overlapping
getQueueStatus calls and stopPolling only clears the interval (doesn't cancel
inflight requests), causing races and possible state updates after unmount;
change to a re-arming setTimeout loop in startPolling so each poll awaits
getQueueStatus before scheduling the next, and add cancellation via an
AbortController or an "active" flag referenced by
requestIdRef/abortControllerRef that stopPolling sets (and aborts) to prevent
any subsequent setQueueStatus/setState/setError after leave/unmount; ensure
stopPolling clears pollRef, aborts the current controller, and startPolling
creates a fresh AbortController whose signal is passed into getQueueStatus (or
whose active flag is checked) before performing any state updates.
- Around line 65-69: The hook assumes a removed queue entry means a match but
doesn't set matchId; update the StatusResponse type to include an optional
match_id (string|null) populated by the backend when a request is paired, and
change the polling flow in useMatchmakingQueue/getQueueStatus so the response
carries match_id when the entry is removed; in useMatchmakingQueue, when the
polling response indicates removal (previously status === null) read
response.match_id, call setMatchId(response.match_id) and only
setState("matched") when a non-null match_id is present (otherwise treat as
expired), and ensure stopPolling is still called once you have handled match_id
or expiry.
In `@frontend/lib/eloService.ts`:
- Around line 55-66: The calculateNewRating function returns delta that doesn't
reflect the floor applied to newRating; compute the raw delta as Math.round(k *
(score - expected)), compute newRating = Math.max(100, rating + rawDelta), then
set the returned delta to appliedDelta = newRating - rating (i.e., clamp delta
to the actually-applied change) so callers see a consistent invariant (reference
calculateNewRating, delta, newRating).
In `@frontend/lib/matchmakingService.ts`:
- Around line 38-58: The POST body sent by the join queue function currently
nests player and includes join_time which doesn't match the backend
JoinQueueRequest shape; update the fetch call in the join function
(matchmakingService.ts) to send a flat JSON object with keys wallet_address:
request.walletAddress, elo: request.elo, match_type: request.matchType,
invite_address: request.inviteAddress ?? null, and max_elo_diff:
request.maxEloDiff ?? null (remove the player wrapper and omit join_time so the
server-side Utc::now() is used).
- Around line 61-75: The getQueueStatus function currently treats the response
as a bare QueueStatus; instead fetch and parse the JSON as the StatusResponse
shape ({ status: string, queue_status: QueueStatus | null }), then if
response.status === 404 return null, else if res.ok read the StatusResponse and
return the inner queue_status (or null) — also remap the snake_case fields of
that inner object to the frontend QueueStatus shape (e.g. queue_status.position
-> position, queue_status.estimated_wait_time -> estimatedWaitTime) so consumers
receive the correct typed object; update getQueueStatus to return
Promise<QueueStatus | null> accordingly and preserve existing error handling for
non-ok responses.
- Around line 90-115: The request body sent by acceptPrivateInvite does not
match the server's AcceptInviteRequest (it wraps fields in accepting_player and
sends join_time), causing deserialization failures; update acceptPrivateInvite
to send a flat JSON object with keys inviter_request_id, wallet_address, and elo
(omit join_time and the accepting_player wrapper) so the payload matches the
server's expected AcceptInviteRequest shape before calling POST
/matchmaking/accept-invite.
- Around line 18-29: The backend returns snake_case JSON and Duration objects,
so update the response handling in joinQueue, getQueueStatus, and
acceptPrivateInvite to parse the raw JSON (const raw = await res.json()) and map
fields to the TS interfaces: map raw.match_id -> matchId (use ?? undefined),
raw.request_id -> requestId, raw.match_type -> matchType, raw.position ->
position, and convert raw.estimated_wait_time to estimatedWaitSeconds using
raw.estimated_wait_time.secs; keep raw.status -> status. Ensure these
transformations replace the current direct casts (res.json() as Promise<...>) so
response.matchId and response.requestId are populated and polling works.
- Around line 78-87: The leaveQueue function currently calls a non-existent
DELETE /matchmaking/leave/:id and suppresses 404s; change leaveQueue to POST to
/matchmaking/cancel with a JSON body { request_id: requestId }, set the
Content-Type: application/json header, and remove the special-case 404
suppression so that non-OK responses throw an Error (include res.statusText).
Locate the function leaveQueue in matchmakingService.ts and replace the fetch
URL/method/body/headers accordingly and ensure errors are propagated.
---
Nitpick comments:
In `@contracts/game_contract/src/lib.rs`:
- Around line 1454-1465: The code currently calls .expect("Not initialized")
when reading CONTRACT_ADMIN in functions like issue_sep10_challenge (and
similarly configure_multisig / set_max_stake), which panics instead of returning
a ContractError; change these to retrieve CONTRACT_ADMIN with
.get(...).ok_or(...) and return a typed ContractError (e.g.
Err(ContractError::NotInitialized) or a suitable existing variant) so
issue_sep10_challenge and other admin-gated entry points return Err(...) instead
of panicking; ensure you reference CONTRACT_ADMIN and the ContractError enum
when making the change.
- Around line 1539-1543: The current code builds a fixed 64-byte buffer
(addr_buf) and calls account_str.copy_into_slice(&mut addr_buf[..str_len]) which
will panic if account_str.len() > 64; refactor by extracting a helper like
address_to_payload_bytes(env: &Env, account: &Address) -> Bytes that allocates
exactly the needed length (no fixed 64 magic), converts account.to_string() to
bytes, and returns Bytes::from_slice(&env, &account_bytes), then replace the
inline logic in this function and the duplicated sites claim_puzzle_reward and
claim_win with calls to address_to_payload_bytes to avoid truncation hazards and
remove duplicate code.
- Around line 1771-1779: The auth check is done after reading storage for
PENDING_FEE_PROPOSAL; move the authentication earlier so unauthorized callers
are rejected before performing storage reads. Specifically, in the function that
currently calls env.storage().instance().has(&PENDING_FEE_PROPOSAL) then
signer.require_auth(), call signer.require_auth() first (matching the pattern
used by propose_fee_change and approve_fee_proposal) and then perform the
signers-list lookup and the proposal-existence check.
In `@frontend/hook/useMatchmakingQueue.ts`:
- Around line 128-132: In the useEffect cleanup, add a best-effort call to
leaveQueue(requestIdRef.current) before/after stopPolling to avoid leaving stale
Redis entries when state === "searching"; check that requestIdRef.current exists
and state === "searching", call leaveQueue in a fire-and-forget way (wrap in an
immediately-invoked async function or use void leaveQueue(...)) and swallow/log
errors (try/catch) so unmount is not blocked; keep stopPolling() as before and
reference useEffect, stopPolling, leaveQueue, requestIdRef, and state to locate
the code to change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68da555b-ea0d-4745-96f0-94b4df9b080c
📒 Files selected for processing (6)
contracts/game_contract/src/lib.rscontracts/game_contract/src/test.rsfrontend/__tests__/eloService.test.tsfrontend/hook/useMatchmakingQueue.tsfrontend/lib/eloService.tsfrontend/lib/matchmakingService.ts
| // SEP-10 challenge verification (#529) | ||
| const SEP10_CHALLENGES: Symbol = symbol_short!("S10_CHAL"); // Map<BytesN<32>, u64> nonce → expiry | ||
| const SEP10_VERIFIED: Symbol = symbol_short!("S10_VER"); // Map<Address, bool> | ||
|
|
||
| // Multi-sig fee control (#535) | ||
| const MULTISIG_SIGNERS: Symbol = symbol_short!("MS_SIGN"); // Vec<Address> | ||
| const MULTISIG_THRESHOLD: Symbol = symbol_short!("MS_THRES"); // u32 | ||
| const PENDING_FEE_PROPOSAL: Symbol = symbol_short!("MS_PROP"); // Option<FeeProposal> | ||
| const FEE_PROPOSAL_APPROVALS: Symbol = symbol_short!("MS_APPR"); // Map<Address, bool> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Soroban best practices for unbounded collections: instance storage vs persistent storage with TTL
💡 Result:
For unbounded collections in Soroban (e.g., vectors or maps that can grow indefinitely like user lists or balances), use persistent storage with variable DataKeys, not instance storage or fixed-key persistent storage. Instance storage is unsuitable due to its ~64-100KB size limit per ledger entry and because all instance data loads with every contract invocation, increasing costs and enabling DoS as the collection grows. Single persistent keys for collections also risk DoS by hitting the 64KB entry limit. Best practice: Use persistent storage where each collection element uses a unique, variable key (e.g., DataKey::UserBalance(user_id) or indexed keys like DataKey::ListItem(counter)). Maintain a separate counter for iteration if needed. This spreads data across multiple ledger entries, avoiding size limits and DoS, while economic costs prevent abuse (e.g., ~0.044 XLM per entry, limiting attackers). All storage requires TTL management via extend_ttl to prevent expiration/archival; persistent data can be restored but needs proactive extension in contract logic. Temporary storage is unsuitable for unbounded persistent needs as it deletes on expiry.
Citations:
- 1: https://dev.to/soroswap/-costs-dos-risks-and-instance-vs-persistent-data-types-in-soroban-421o
- 2: https://www.mintlify.com/stellar/rs-soroban-sdk/api/storage/overview
- 3: https://docs.rs/soroban-sdk/latest/soroban_sdk/storage/struct.Storage.html
- 4: https://veridise.com/blog/learn-blockchain/how-to-develop-securely-on-soroban-storage-types-with-unbounded-data/
- 5: https://www.mintlify.com/stellar/rs-soroban-sdk/core/storage
- 6: https://mintlify.com/stellar/rs-soroban-sdk/api/storage/instance
- 7: https://mintlify.com/stellar/rs-soroban-sdk/api/storage/overview
🏁 Script executed:
# Search for usage of the symbols mentioned in the review
rg "SEP10_CHALLENGES|SEP10_VERIFIED|FEE_PROPOSAL_APPROVALS" contracts/game_contract/src/lib.rs -A 3 -B 1Repository: NOVUS-X/XLMate
Length of output: 2987
🏁 Script executed:
# Check the context around lines 1476-1488 and 1562-1568
sed -n '1470,1495p' contracts/game_contract/src/lib.rs
sed -n '1556,1575p' contracts/game_contract/src/lib.rsRepository: NOVUS-X/XLMate
Length of output: 1493
Move unbounded Maps from instance to persistent storage with per-item keys.
The three Maps—SEP10_CHALLENGES (nonce → expiry), SEP10_VERIFIED (address → bool), and FEE_PROPOSAL_APPROVALS (address → bool)—are stored as single values in instance storage. Since instance storage loads entirely on every invocation and is bounded by ~64-100KB, these maps will eventually exceed the contract entry size limit as they grow per nonce/user/signer, causing all contract calls to exceed budget and bricking the contract.
Use env.storage().persistent() with unique variable keys for each entry (e.g., (SEP10_CHALLENGES, nonce) and (SEP10_VERIFIED, address)) and set TTL on writes. This spreads entries across independent ledger entries with individual lifetimes, eliminating the read-entire-map-per-call cost. Apply the same pattern to FEE_PROPOSAL_APPROVALS; though it's smaller and cleared on execute/cancel, it remains vulnerable to the same issue.
| pub fn issue_sep10_challenge( | ||
| env: Env, | ||
| admin: Address, | ||
| account: Address, | ||
| nonce: BytesN<32>, | ||
| expiry: u64, | ||
| ) -> Result<(), ContractError> { | ||
| let current_admin: Address = env | ||
| .storage() | ||
| .instance() | ||
| .get(&CONTRACT_ADMIN) | ||
| .expect("Not initialized"); | ||
| current_admin.require_auth(); | ||
| if admin != current_admin { | ||
| return Err(ContractError::Unauthorized); | ||
| } | ||
|
|
||
| let current_ledger = env.ledger().sequence() as u64; | ||
| if expiry <= current_ledger { | ||
| return Err(ContractError::ChallengeExpired); | ||
| } | ||
|
|
||
| let mut challenges: Map<BytesN<32>, u64> = env | ||
| .storage() | ||
| .instance() | ||
| .get(&SEP10_CHALLENGES) | ||
| .unwrap_or(Map::new(&env)); | ||
|
|
||
| // Reject if nonce already exists (replay protection) | ||
| if challenges.get(nonce.clone()).is_some() { | ||
| return Err(ContractError::ChallengeAlreadyUsed); | ||
| } | ||
|
|
||
| challenges.set(nonce.clone(), expiry); | ||
| env.storage().instance().set(&SEP10_CHALLENGES, &challenges); | ||
|
|
||
| env.events().publish( | ||
| (symbol_short!("sep10"), symbol_short!("issued")), | ||
| (account, nonce), | ||
| ); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
SEP-10 challenge isn't bound to the issued account.
issue_sep10_challenge accepts an account argument but only emits it in the event — it stores nothing more than nonce → expiry in SEP10_CHALLENGES. verify_sep10_challenge later builds the signed payload from whatever account the caller supplies (line 1539–1543). That means a single admin-signed (account, nonce, expiry) tuple is the only thing that prevents a different address from being marked verified, and a leaked/observed signature can be replayed against the contract from any sender (the function has no require_auth for account).
Bind the nonce to the intended account in storage and require auth for the verifier:
🛡️ Suggested binding
- let mut challenges: Map<BytesN<32>, u64> = env
+ // Store (account, expiry) per nonce so verify can re-check the binding.
+ let mut challenges: Map<BytesN<32>, (Address, u64)> = env
.storage()
.instance()
.get(&SEP10_CHALLENGES)
.unwrap_or(Map::new(&env));
// Reject if nonce already exists (replay protection)
if challenges.get(nonce.clone()).is_some() {
return Err(ContractError::ChallengeAlreadyUsed);
}
- challenges.set(nonce.clone(), expiry);
+ challenges.set(nonce.clone(), (account.clone(), expiry));
env.storage().instance().set(&SEP10_CHALLENGES, &challenges);…and in verify_sep10_challenge:
- let expiry = challenges
+ let (issued_to, expiry) = challenges
.get(nonce.clone())
.ok_or(ContractError::ChallengeAlreadyUsed)?;
+ if issued_to != account {
+ return Err(ContractError::Unauthorized);
+ }
+ account.require_auth();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/game_contract/src/lib.rs` around lines 1454 - 1496, Store the
intended account alongside the nonce when issuing SEP-10 challenges and enforce
that the verifier is the same account: modify issue_sep10_challenge to save a
mapping from nonce (BytesN<32>) to a tuple/value containing (account: Address,
expiry: u64) instead of only expiry in SEP10_CHALLENGES, and update
verify_sep10_challenge to load that stored tuple and reject if the supplied
account does not match the stored account; also call account.require_auth() (or
otherwise require auth on the verifying account) in verify_sep10_challenge to
ensure only the intended account can complete verification.
| let mut challenges: Map<BytesN<32>, u64> = env | ||
| .storage() | ||
| .instance() | ||
| .get(&SEP10_CHALLENGES) | ||
| .unwrap_or(Map::new(&env)); | ||
|
|
||
| let expiry = challenges | ||
| .get(nonce.clone()) | ||
| .ok_or(ContractError::ChallengeAlreadyUsed)?; | ||
|
|
||
| let current_ledger = env.ledger().sequence() as u64; | ||
| if current_ledger > expiry { | ||
| return Err(ContractError::ChallengeExpired); | ||
| } |
There was a problem hiding this comment.
Wrong error variant when the nonce was never issued.
A missing nonce in SEP10_CHALLENGES is not "already used" — it could equally be a typo or a fabricated nonce. Returning ChallengeAlreadyUsed here is semantically incorrect and will confuse callers parsing error codes (and also shadows the genuine replay case which is indistinguishable). Add a dedicated error such as ChallengeNotFound (or reuse NotVerified/Unauthorized) and reserve ChallengeAlreadyUsed for the actual replay path.
🐛 Suggested change
let expiry = challenges
.get(nonce.clone())
- .ok_or(ContractError::ChallengeAlreadyUsed)?;
+ .ok_or(ContractError::Unauthorized)?; // or a new ChallengeNotFound variant🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/game_contract/src/lib.rs` around lines 1511 - 1524, The code
incorrectly maps a missing nonce lookup to ContractError::ChallengeAlreadyUsed;
change this to a distinct error (e.g., ContractError::ChallengeNotFound or reuse
an appropriate existing variant like ContractError::Unauthorized) so a
non-existent nonce is not treated as a replay. Update the get() -> ok_or(...)
call on the Map<BytesN<32>, u64> named challenges (the lookup using
nonce.clone()) to return the new/appropriate error variant, and add the new enum
variant to ContractError if needed, keeping ContractError::ChallengeAlreadyUsed
reserved for the actual replay path where a nonce was previously consumed.
| pub fn configure_multisig( | ||
| env: Env, | ||
| admin: Address, | ||
| signers: Vec<Address>, | ||
| threshold: u32, | ||
| ) -> Result<(), ContractError> { | ||
| let current_admin: Address = env | ||
| .storage() | ||
| .instance() | ||
| .get(&CONTRACT_ADMIN) | ||
| .expect("Not initialized"); | ||
| current_admin.require_auth(); | ||
| if admin != current_admin { | ||
| return Err(ContractError::Unauthorized); | ||
| } | ||
|
|
||
| let n = signers.len(); | ||
| if threshold == 0 || threshold > n { | ||
| return Err(ContractError::InvalidThreshold); | ||
| } | ||
|
|
||
| env.storage().instance().set(&MULTISIG_SIGNERS, &signers); | ||
| env.storage().instance().set(&MULTISIG_THRESHOLD, &threshold); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
configure_multisig doesn't validate signer uniqueness.
Duplicate addresses in signers are accepted and counted by signers.len(), but the approval map (FEE_PROPOSAL_APPROVALS) is keyed by Address and dedupes implicitly. Net effect:
- A threshold of
n(=signers.len()with duplicates) becomes unreachable, so legitimate proposals can never execute. - A misconfiguration like
[s1, s1, s2]with threshold2can be satisfied bys1alone (auto-approval as proposer) — exactly the situation multisig is supposed to prevent.
Reject duplicates at configure time:
🛡️ Suggested validation
let n = signers.len();
if threshold == 0 || threshold > n {
return Err(ContractError::InvalidThreshold);
}
+ // Reject duplicate signers
+ for i in 0..n {
+ for j in (i + 1)..n {
+ if signers.get(i).unwrap() == signers.get(j).unwrap() {
+ return Err(ContractError::InvalidThreshold);
+ }
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn configure_multisig( | |
| env: Env, | |
| admin: Address, | |
| signers: Vec<Address>, | |
| threshold: u32, | |
| ) -> Result<(), ContractError> { | |
| let current_admin: Address = env | |
| .storage() | |
| .instance() | |
| .get(&CONTRACT_ADMIN) | |
| .expect("Not initialized"); | |
| current_admin.require_auth(); | |
| if admin != current_admin { | |
| return Err(ContractError::Unauthorized); | |
| } | |
| let n = signers.len(); | |
| if threshold == 0 || threshold > n { | |
| return Err(ContractError::InvalidThreshold); | |
| } | |
| env.storage().instance().set(&MULTISIG_SIGNERS, &signers); | |
| env.storage().instance().set(&MULTISIG_THRESHOLD, &threshold); | |
| Ok(()) | |
| } | |
| pub fn configure_multisig( | |
| env: Env, | |
| admin: Address, | |
| signers: Vec<Address>, | |
| threshold: u32, | |
| ) -> Result<(), ContractError> { | |
| let current_admin: Address = env | |
| .storage() | |
| .instance() | |
| .get(&CONTRACT_ADMIN) | |
| .expect("Not initialized"); | |
| current_admin.require_auth(); | |
| if admin != current_admin { | |
| return Err(ContractError::Unauthorized); | |
| } | |
| let n = signers.len(); | |
| if threshold == 0 || threshold > n { | |
| return Err(ContractError::InvalidThreshold); | |
| } | |
| // Reject duplicate signers | |
| for i in 0..n { | |
| for j in (i + 1)..n { | |
| if signers.get(i).unwrap() == signers.get(j).unwrap() { | |
| return Err(ContractError::InvalidThreshold); | |
| } | |
| } | |
| } | |
| env.storage().instance().set(&MULTISIG_SIGNERS, &signers); | |
| env.storage().instance().set(&MULTISIG_THRESHOLD, &threshold); | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/game_contract/src/lib.rs` around lines 1605 - 1630, The
configure_multisig function accepts duplicated addresses in signers which breaks
approval semantics (FEE_PROPOSAL_APPROVALS is keyed by Address); fix by
validating uniqueness before persisting: build a temporary set from signers and
if set.len() != signers.len() return an appropriate ContractError (e.g.,
DuplicateSigner or add InvalidSigners), perform this check prior to setting
MULTISIG_SIGNERS and MULTISIG_THRESHOLD, and only persist after the uniqueness
and threshold validations pass.
| pub fn propose_fee_change( | ||
| env: Env, | ||
| proposer: Address, | ||
| new_fee_bips: u32, | ||
| new_treasury_address: Address, | ||
| ) -> Result<(), ContractError> { | ||
| if new_fee_bips > 1000 { | ||
| return Err(ContractError::InvalidAmount); | ||
| } | ||
|
|
||
| let signers: Vec<Address> = env | ||
| .storage() | ||
| .instance() | ||
| .get(&MULTISIG_SIGNERS) | ||
| .ok_or(ContractError::Unauthorized)?; | ||
|
|
||
| // Verify proposer is a signer | ||
| if !signers.contains(&proposer) { | ||
| return Err(ContractError::NotASigner); | ||
| } | ||
|
|
||
| proposer.require_auth(); | ||
|
|
||
| let proposal = FeeProposal { | ||
| new_fee_bips, | ||
| new_treasury_address, | ||
| proposed_at: env.ledger().sequence() as u64, | ||
| proposer: proposer.clone(), | ||
| }; | ||
|
|
||
| env.storage() | ||
| .instance() | ||
| .set(&PENDING_FEE_PROPOSAL, &proposal); | ||
|
|
||
| // Reset approvals and auto-approve for proposer | ||
| let mut approvals: Map<Address, bool> = Map::new(&env); | ||
| approvals.set(proposer.clone(), true); | ||
| env.storage() | ||
| .instance() | ||
| .set(&FEE_PROPOSAL_APPROVALS, &approvals); | ||
|
|
||
| env.events().publish( | ||
| (symbol_short!("multisig"), symbol_short!("proposed")), | ||
| proposer, | ||
| ); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
propose_fee_change silently overwrites an in-flight proposal — any signer can grief the queue.
A single signer (any one of N) can wipe an existing near-approved proposal by submitting a new one, resetting approval counts. The doc-comment ("Replaces any existing pending proposal") acknowledges this, but it weakens the M-of-N guarantee since one rogue or compromised signer can indefinitely block fee changes.
Consider rejecting if there's already a pending proposal and requiring an explicit cancel_fee_proposal first (which today is also any-signer, so consider tightening cancel to "only proposer or threshold-of-signers" as a follow-up).
🛡️ Suggested guard
if new_fee_bips > 1000 {
return Err(ContractError::InvalidAmount);
}
+ if env.storage().instance().has(&PENDING_FEE_PROPOSAL) {
+ return Err(ContractError::AlreadyApproved); // or a dedicated ProposalPending variant
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/game_contract/src/lib.rs` around lines 1636 - 1683,
propose_fee_change currently overwrites any existing PENDING_FEE_PROPOSAL and
resets FEE_PROPOSAL_APPROVALS, enabling a single signer to grief in-flight
proposals; change propose_fee_change to first check
env.storage().instance().get(&PENDING_FEE_PROPOSAL) and return an error (e.g.,
ContractError::PendingProposalExists) if a proposal is present, so new proposals
are rejected until the existing one is explicitly cancelled; keep the existing
proposer.require_auth() and leave cancel_fee_proposal to handle removal
(follow-up: tighten cancel rights), and update the event/behavior accordingly.
| export interface MatchmakingResponse { | ||
| status: string; | ||
| matchId?: string; | ||
| requestId: string; | ||
| } | ||
|
|
||
| export interface QueueStatus { | ||
| requestId: string; | ||
| position: number; | ||
| estimatedWaitSeconds: number; | ||
| matchType: MatchType; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm backend serde does not rename to camelCase for these models.
rg -nP -C2 'rename_all\s*=\s*"camelCase"' --type=rust
rg -nP -C5 'struct\s+(MatchmakingResponse|QueueStatus|JoinQueueRequest|AcceptInviteRequest)\b' --type=rustRepository: NOVUS-X/XLMate
Length of output: 2479
🏁 Script executed:
#!/bin/bash
# Check Duration serialization in the backend
rg -n "estimated_wait_time" backend/modules/matchmaking/ -A 2 -B 2
# Check if there's any custom serde configuration
rg -n "#\[serde" backend/modules/matchmaking/ --type=rust
# Look at the frontend matchmakingService.ts file
head -100 frontend/lib/matchmakingService.ts
# Check if there's a useMatchmakingQueue hook mentioned in the review
fd -e ts -e tsx | xargs rg -l "useMatchmakingQueue" 2>/dev/null | head -5Repository: NOVUS-X/XLMate
Length of output: 3996
🏁 Script executed:
#!/bin/bash
# Check the useMatchmakingQueue hook
cat -n frontend/hook/useMatchmakingQueue.ts | head -150
# Also search for access patterns of matchId and requestId
rg -n "matchId|requestId|match_id|request_id" frontend/hook/useMatchmakingQueue.tsRepository: NOVUS-X/XLMate
Length of output: 5144
🏁 Script executed:
#!/bin/bash
# Check how Rust serde serializes Duration by default
rg -n "use std::time::Duration\|use chrono::" backend/modules/matchmaking/models.rs
# Look for any Duration-specific serde configuration
rg -n "Duration" backend/modules/matchmaking/models.rs -B 2 -A 2Repository: NOVUS-X/XLMate
Length of output: 347
Type field casing and serialization format do not match backend JSON.
Backend structs in backend/modules/matchmaking/models.rs use snake_case field names without custom serde renaming: match_id, request_id, estimated_wait_time, match_type. The frontend TypeScript types declare camelCase equivalents, and the res.json() as Promise<X> cast does not perform any runtime mapping.
This breaks the hook at two critical points:
- Line 89 (
if (response.matchId)) will always be false — backend sendsmatch_id, somatchIdisundefined - Line 96–97 (
setRequestId(response.requestId)) storesundefined, blocking all polling logic (line 59–60)
Additionally, estimated_wait_time is a std::time::Duration, which serde serializes as { secs: number, nanos: number }, not a single number. The frontend QueueStatus.estimatedWaitSeconds: number cannot consume this format.
The request body correctly uses snake_case mapping (showing the pattern is known), but response handling needs the same treatment.
🔧 Solution: Add explicit mapping in each function
After calling res.json(), transform the response to match the frontend types:
// joinQueue
const raw = await res.json();
return {
status: raw.status,
matchId: raw.match_id ?? undefined,
requestId: raw.request_id,
};
// getQueueStatus
const raw = await res.json();
return {
requestId: raw.request_id,
position: raw.position,
estimatedWaitSeconds: raw.estimated_wait_time.secs,
matchType: raw.match_type,
};
// acceptPrivateInvite
const raw = await res.json();
return {
status: raw.status,
matchId: raw.match_id ?? undefined,
requestId: raw.request_id,
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/lib/matchmakingService.ts` around lines 18 - 29, The backend returns
snake_case JSON and Duration objects, so update the response handling in
joinQueue, getQueueStatus, and acceptPrivateInvite to parse the raw JSON (const
raw = await res.json()) and map fields to the TS interfaces: map raw.match_id ->
matchId (use ?? undefined), raw.request_id -> requestId, raw.match_type ->
matchType, raw.position -> position, and convert raw.estimated_wait_time to
estimatedWaitSeconds using raw.estimated_wait_time.secs; keep raw.status ->
status. Ensure these transformations replace the current direct casts
(res.json() as Promise<...>) so response.matchId and response.requestId are
populated and polling works.
| const res = await fetch(`${BASE_URL}/matchmaking/join`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| player: { | ||
| wallet_address: request.walletAddress, | ||
| elo: request.elo, | ||
| join_time: new Date().toISOString(), | ||
| }, | ||
| match_type: request.matchType, | ||
| invite_address: request.inviteAddress ?? null, | ||
| max_elo_diff: request.maxEloDiff ?? null, | ||
| }), | ||
| }); | ||
|
|
||
| if (!res.ok) { | ||
| throw new Error(`Failed to join queue: ${res.statusText}`); | ||
| } | ||
|
|
||
| return res.json() as Promise<MatchmakingResponse>; | ||
| } |
There was a problem hiding this comment.
Request body shape does not match JoinQueueRequest.
Per backend/modules/matchmaking/routes.rs (lines 9–16), JoinQueueRequest is flat with wallet_address, elo, match_type, invite_address, max_elo_diff — there is no player wrapper and join_time is set server-side via Utc::now() (lines 51–55). Posting the wrapped body here will fail JSON deserialization and the call will return 503, so the queue can never be joined.
🔧 Proposed fix
const res = await fetch(`${BASE_URL}/matchmaking/join`, {
method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({
- player: {
- wallet_address: request.walletAddress,
- elo: request.elo,
- join_time: new Date().toISOString(),
- },
+ wallet_address: request.walletAddress,
+ elo: request.elo,
match_type: request.matchType,
invite_address: request.inviteAddress ?? null,
max_elo_diff: request.maxEloDiff ?? null,
}),
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const res = await fetch(`${BASE_URL}/matchmaking/join`, { | |
| method: "POST", | |
| headers: { "Content-Type": "application/json" }, | |
| body: JSON.stringify({ | |
| player: { | |
| wallet_address: request.walletAddress, | |
| elo: request.elo, | |
| join_time: new Date().toISOString(), | |
| }, | |
| match_type: request.matchType, | |
| invite_address: request.inviteAddress ?? null, | |
| max_elo_diff: request.maxEloDiff ?? null, | |
| }), | |
| }); | |
| if (!res.ok) { | |
| throw new Error(`Failed to join queue: ${res.statusText}`); | |
| } | |
| return res.json() as Promise<MatchmakingResponse>; | |
| } | |
| const res = await fetch(`${BASE_URL}/matchmaking/join`, { | |
| method: "POST", | |
| headers: { "Content-Type": "application/json" }, | |
| body: JSON.stringify({ | |
| wallet_address: request.walletAddress, | |
| elo: request.elo, | |
| match_type: request.matchType, | |
| invite_address: request.inviteAddress ?? null, | |
| max_elo_diff: request.maxEloDiff ?? null, | |
| }), | |
| }); | |
| if (!res.ok) { | |
| throw new Error(`Failed to join queue: ${res.statusText}`); | |
| } | |
| return res.json() as Promise<MatchmakingResponse>; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/lib/matchmakingService.ts` around lines 38 - 58, The POST body sent
by the join queue function currently nests player and includes join_time which
doesn't match the backend JoinQueueRequest shape; update the fetch call in the
join function (matchmakingService.ts) to send a flat JSON object with keys
wallet_address: request.walletAddress, elo: request.elo, match_type:
request.matchType, invite_address: request.inviteAddress ?? null, and
max_elo_diff: request.maxEloDiff ?? null (remove the player wrapper and omit
join_time so the server-side Utc::now() is used).
| export async function getQueueStatus( | ||
| requestId: string, | ||
| ): Promise<QueueStatus | null> { | ||
| const res = await fetch( | ||
| `${BASE_URL}/matchmaking/status/${encodeURIComponent(requestId)}`, | ||
| ); | ||
|
|
||
| if (res.status === 404) return null; | ||
|
|
||
| if (!res.ok) { | ||
| throw new Error(`Failed to get queue status: ${res.statusText}`); | ||
| } | ||
|
|
||
| return res.json() as Promise<QueueStatus>; | ||
| } |
There was a problem hiding this comment.
Status endpoint returns StatusResponse, not bare QueueStatus.
StatusResponse is { status: String, queue_status: Option<QueueStatus> } (routes.rs lines 31–34). Returning the raw JSON typed as QueueStatus will leave every consumer reading position/estimated_wait_time as undefined. Also, when queue_status is None/null, the hook treats a successful response as "still searching" instead of "matched/expired".
🔧 Proposed fix
export async function getQueueStatus(
requestId: string,
): Promise<QueueStatus | null> {
const res = await fetch(
`${BASE_URL}/matchmaking/status/${encodeURIComponent(requestId)}`,
);
if (res.status === 404) return null;
if (!res.ok) {
throw new Error(`Failed to get queue status: ${res.statusText}`);
}
- return res.json() as Promise<QueueStatus>;
+ const body = (await res.json()) as { status: string; queue_status: QueueStatus | null };
+ return body.queue_status;
}(Field-name remap of the inner queue_status object is still needed — see the type-casing comment above.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/lib/matchmakingService.ts` around lines 61 - 75, The getQueueStatus
function currently treats the response as a bare QueueStatus; instead fetch and
parse the JSON as the StatusResponse shape ({ status: string, queue_status:
QueueStatus | null }), then if response.status === 404 return null, else if
res.ok read the StatusResponse and return the inner queue_status (or null) —
also remap the snake_case fields of that inner object to the frontend
QueueStatus shape (e.g. queue_status.position -> position,
queue_status.estimated_wait_time -> estimatedWaitTime) so consumers receive the
correct typed object; update getQueueStatus to return Promise<QueueStatus |
null> accordingly and preserve existing error handling for non-ok responses.
| export async function leaveQueue(requestId: string): Promise<void> { | ||
| const res = await fetch( | ||
| `${BASE_URL}/matchmaking/leave/${encodeURIComponent(requestId)}`, | ||
| { method: "DELETE" }, | ||
| ); | ||
|
|
||
| if (!res.ok && res.status !== 404) { | ||
| throw new Error(`Failed to leave queue: ${res.statusText}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm there is no `/leave` route and that `/cancel` is the canonical cancel endpoint.
rg -nP -C2 'matchmaking/(leave|cancel)' --type=rust
ast-grep --pattern 'web::scope("/matchmaking") $$$'Repository: NOVUS-X/XLMate
Length of output: 231
🏁 Script executed:
# Get the full routes.rs file content around the matchmaking scope
cat backend/modules/matchmaking/routes.rsRepository: NOVUS-X/XLMate
Length of output: 5015
🏁 Script executed:
# Search for leaveQueue usage in the frontend
rg -n "leaveQueue" frontend/ -t typescript -t javascriptRepository: NOVUS-X/XLMate
Length of output: 93
🏁 Script executed:
# Search for leaveQueue usage in the frontend without file type
rg -n "leaveQueue" frontend/Repository: NOVUS-X/XLMate
Length of output: 290
🏁 Script executed:
# Get the context around the leaveQueue call
sed -n '100,125p' frontend/hook/useMatchmakingQueue.tsRepository: NOVUS-X/XLMate
Length of output: 606
leaveQueue calls a non-existent route; users cannot leave the queue.
The backend exposes only /join, /status/{id}, /cancel, /accept-invite, and /match/{id}. There is no DELETE /matchmaking/leave/:id. The correct endpoint is POST /matchmaking/cancel expecting { request_id: Uuid }. Every call to leaveQueue receives a 404, which the res.status !== 404 guard silently suppresses, so queue entries are never removed and accumulate in Redis.
🔧 Fix
export async function leaveQueue(requestId: string): Promise<void> {
- const res = await fetch(
- `${BASE_URL}/matchmaking/leave/${encodeURIComponent(requestId)}`,
- { method: "DELETE" },
- );
-
- if (!res.ok && res.status !== 404) {
+ const res = await fetch(`${BASE_URL}/matchmaking/cancel`, {
+ method: "POST",
+ headers: { "Content-Type": "application/json" },
+ body: JSON.stringify({ request_id: requestId }),
+ });
+
+ if (!res.ok && res.status !== 404) {
throw new Error(`Failed to leave queue: ${res.statusText}`);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function leaveQueue(requestId: string): Promise<void> { | |
| const res = await fetch( | |
| `${BASE_URL}/matchmaking/leave/${encodeURIComponent(requestId)}`, | |
| { method: "DELETE" }, | |
| ); | |
| if (!res.ok && res.status !== 404) { | |
| throw new Error(`Failed to leave queue: ${res.statusText}`); | |
| } | |
| } | |
| export async function leaveQueue(requestId: string): Promise<void> { | |
| const res = await fetch(`${BASE_URL}/matchmaking/cancel`, { | |
| method: "POST", | |
| headers: { "Content-Type": "application/json" }, | |
| body: JSON.stringify({ request_id: requestId }), | |
| }); | |
| if (!res.ok && res.status !== 404) { | |
| throw new Error(`Failed to leave queue: ${res.statusText}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/lib/matchmakingService.ts` around lines 78 - 87, The leaveQueue
function currently calls a non-existent DELETE /matchmaking/leave/:id and
suppresses 404s; change leaveQueue to POST to /matchmaking/cancel with a JSON
body { request_id: requestId }, set the Content-Type: application/json header,
and remove the special-case 404 suppression so that non-OK responses throw an
Error (include res.statusText). Locate the function leaveQueue in
matchmakingService.ts and replace the fetch URL/method/body/headers accordingly
and ensure errors are propagated.
| export async function acceptPrivateInvite( | ||
| inviterRequestId: string, | ||
| walletAddress: string, | ||
| elo: number, | ||
| ): Promise<MatchmakingResponse | null> { | ||
| const res = await fetch(`${BASE_URL}/matchmaking/accept-invite`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| inviter_request_id: inviterRequestId, | ||
| accepting_player: { | ||
| wallet_address: walletAddress, | ||
| elo, | ||
| join_time: new Date().toISOString(), | ||
| }, | ||
| }), | ||
| }); | ||
|
|
||
| if (res.status === 404) return null; | ||
|
|
||
| if (!res.ok) { | ||
| throw new Error(`Failed to accept invite: ${res.statusText}`); | ||
| } | ||
|
|
||
| return res.json() as Promise<MatchmakingResponse>; | ||
| } |
There was a problem hiding this comment.
Request body shape does not match AcceptInviteRequest.
AcceptInviteRequest is flat: wallet_address, elo, inviter_request_id (routes.rs lines 19–23); join_time is filled server-side. The accepting_player wrapper here will fail to deserialize and the endpoint will return 503, so private invites can never be accepted.
🔧 Proposed fix
const res = await fetch(`${BASE_URL}/matchmaking/accept-invite`, {
method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({
inviter_request_id: inviterRequestId,
- accepting_player: {
- wallet_address: walletAddress,
- elo,
- join_time: new Date().toISOString(),
- },
+ wallet_address: walletAddress,
+ elo,
}),
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function acceptPrivateInvite( | |
| inviterRequestId: string, | |
| walletAddress: string, | |
| elo: number, | |
| ): Promise<MatchmakingResponse | null> { | |
| const res = await fetch(`${BASE_URL}/matchmaking/accept-invite`, { | |
| method: "POST", | |
| headers: { "Content-Type": "application/json" }, | |
| body: JSON.stringify({ | |
| inviter_request_id: inviterRequestId, | |
| accepting_player: { | |
| wallet_address: walletAddress, | |
| elo, | |
| join_time: new Date().toISOString(), | |
| }, | |
| }), | |
| }); | |
| if (res.status === 404) return null; | |
| if (!res.ok) { | |
| throw new Error(`Failed to accept invite: ${res.statusText}`); | |
| } | |
| return res.json() as Promise<MatchmakingResponse>; | |
| } | |
| export async function acceptPrivateInvite( | |
| inviterRequestId: string, | |
| walletAddress: string, | |
| elo: number, | |
| ): Promise<MatchmakingResponse | null> { | |
| const res = await fetch(`${BASE_URL}/matchmaking/accept-invite`, { | |
| method: "POST", | |
| headers: { "Content-Type": "application/json" }, | |
| body: JSON.stringify({ | |
| inviter_request_id: inviterRequestId, | |
| wallet_address: walletAddress, | |
| elo, | |
| }), | |
| }); | |
| if (res.status === 404) return null; | |
| if (!res.ok) { | |
| throw new Error(`Failed to accept invite: ${res.statusText}`); | |
| } | |
| return res.json() as Promise<MatchmakingResponse>; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/lib/matchmakingService.ts` around lines 90 - 115, The request body
sent by acceptPrivateInvite does not match the server's AcceptInviteRequest (it
wraps fields in accepting_player and sends join_time), causing deserialization
failures; update acceptPrivateInvite to send a flat JSON object with keys
inviter_request_id, wallet_address, and elo (omit join_time and the
accepting_player wrapper) so the payload matches the server's expected
AcceptInviteRequest shape before calling POST /matchmaking/accept-invite.
2 participants
Summary
Resolves all four issues assigned to Xuccessor in a single PR.
#529 — Contract: SEP-10 Challenge Verification
Implements the SEP-10 Web Authentication challenge/response flow on-chain.
New contract methods:
issue_sep10_challenge(admin, account, nonce, expiry)— admin stores a 32-byte nonce with an expiry ledger sequenceverify_sep10_challenge(account, nonce, signature)— verifies ED25519 sig overSHA256(address || nonce || expiry_le8), consumes the nonce, marks account verifiedis_sep10_verified(account)— query helperReplay protection: nonce is deleted on successful verification; duplicate nonces are rejected at issue time.
#535 — Contract: Multi-Sig Control for Protocol Fee Parameters
Adds M-of-N multi-signature governance for
fee_bipsandtreasury_addresschanges.New contract methods:
configure_multisig(admin, signers, threshold)— admin sets the signer set and approval thresholdpropose_fee_change(signer, new_fee_bips, new_treasury)— any signer proposes; proposer's approval is counted automaticallyapprove_fee_proposal(signer)— vote; fee change applied atomically when approvals ≥ threshold; returnstruewhen executedcancel_fee_proposal(signer)— any signer can cancel the pending proposalget_fee_proposal()/get_approval_count()— query helpers#555 — Backend: Redis-based Matchmaking Queue (frontend integration)
The Rust/Redis backend already implements the queue. This PR adds the frontend integration layer.
New files:
frontend/lib/matchmakingService.ts— typed fetch client for join/status/leave/accept-invite endpointsfrontend/hook/useMatchmakingQueue.ts— React hook with a state machine (idle → searching → matched | error), automatic polling every 3 s, and cleanup on unmount#556 — Backend: ELO Rating Calculation Service (frontend)
Pure TypeScript Elo implementation mirroring
backend/modules/matchmaking/elo.rs.New files:
frontend/lib/eloService.ts—calculateMatchRatings,calculateDrawRatings,kFactor,expectedScorefrontend/__tests__/eloService.test.ts— unit tests covering equal ratings, upsets, draws, floor clamping, and K-factor selectionTesting
contracts/game_contract/src/test.rscovering SEP-10 and multi-sig flowsfrontend/__tests__/eloService.test.ts— all TypeScript files verified withtsc --transpileModuleCloses #529
Closes #535
Closes #555
Closes #556
Summary by CodeRabbit
New Features
Tests