Skip to content

feat: contract visibility accessors and platform UI components (#473, #474, #477, #479)#490

Open
Ejere wants to merge 1 commit intotheblockcade:mainfrom
Ejere:feat/contract-accessors-and-ux-components
Open

feat: contract visibility accessors and platform UI components (#473, #474, #477, #479)#490
Ejere wants to merge 1 commit intotheblockcade:mainfrom
Ejere:feat/contract-accessors-and-ux-components

Conversation

@Ejere
Copy link
Copy Markdown

@Ejere Ejere commented Mar 30, 2026

Closes #473
Closes #474
Closes #477
Closes #479

Summary

Resolves four issues across contracts and frontend:


#473 — oracle-integration: source config snapshot and update policy summary

  • Added OracleSourceSnapshot and UpdatePolicySummary contract types
  • source_config_snapshot() returns whitelisted oracle addresses + count; returns None before init (missing-source behavior documented)
  • update_policy_summary() returns deterministic stale-threshold (20 ledgers) and "on_request" cadence — safe to cache by clients
  • 7 new tests: snapshot reads, None-before-init, count consistency, policy determinism across calls
  • docs/contracts/oracle-integration.md updated with method descriptions, return types, and missing-source behavior

#474 — tournament-system: elimination-path summary and remaining-match count

  • Added EliminationPath type: rounds_played, last_round_active, is_active
  • remaining_match_count(id) — ceiling-div on current-round participants; odd participants get a bye
  • elimination_path(id, player) — walks RoundParticipants per round to build a compact path summary without off-chain bracket reconstruction
  • Returns TournamentNotFound / PlayerNotJoined for missing entities (documented)
  • 8 new tests: even/odd participants, single-participant bye, missing tournament/player, active vs. eliminated state after advance_round
  • docs/contracts/tournament-system.md updated

#477 — GameLobby: first-time user checklist card

  • ONBOARDING_CHECKLIST_DISMISSED_FLAG constant exported from global-state-store.ts
  • Dismissible FirstTimeChecklist component (3 actionable items: connect wallet, browse games, place wager)
  • Dismissal dispatched via FLAGS_SET → persisted to localStorage via existing flags mechanism
  • Returning users (flag already set in storage) never see the checklist; core lobby actions remain immediately reachable
  • 6 new tests: first-run render, dismissal persistence, returning-user skip, item toggle, aria-label accessibility

#479 — Reusable Timeline component

  • New Timeline + TimelineEntry component in components/v1/Timeline.tsx
  • API: items: TimelineItemData[], orientation: horizontal | vertical, compact, status, timestamp, metadata slots
  • TxStatusPanel refactored to use <Timeline orientation="horizontal"> for its Submitted → Pending → Confirmed step view
  • ContractEventFeed event list (<ol>) carries sc-timeline sc-timeline--vertical for timeline composition
  • eventToTimelineItem() adapter exported for mapping ContractEvent → TimelineItemData
  • Exported from components/v1/index.ts
  • sc-timeline + .onboarding-checklist CSS added to index.css
  • 20+ new tests: item rendering, order preservation, status classes, horizontal/vertical layouts, compact mode, eventToTimelineItem adapter edge cases

Test plan

  • cd contracts/oracle-integration && cargo test — all tests green
  • cd contracts/tournament-system && cargo test — all tests green
  • cd frontend && npm test — GameLobby checklist, ContractEventFeed timeline, and Timeline unit tests pass
  • Existing tests for TxStatusPanel, ContractEventFeed, and GameLobby remain unaffected

Summary by CodeRabbit

New Features

  • Query oracle source configuration and update policies via new contract methods.
  • Track tournament player progression with new query methods for remaining matches and elimination history.
  • Reusable Timeline component for displaying sequential steps and progress.
  • First-time user onboarding checklist with persistent dismissal state.

Documentation

  • Added contract method documentation for oracle and tournament query endpoints.

…heblockcade#473, theblockcade#474, theblockcade#477, theblockcade#479)

oracle-integration (theblockcade#473):
- Add OracleSourceSnapshot and UpdatePolicySummary contract types
- Add source_config_snapshot() — returns configured oracle addresses or None before init
- Add update_policy_summary() — deterministic stale-threshold and cadence summary
- Document missing-source behavior in oracle-integration.md
- Add 7 tests covering snapshot reads, None-before-init, policy determinism

tournament-system (theblockcade#474):
- Add EliminationPath contract type (rounds_played, last_round_active, is_active)
- Add remaining_match_count() — ceiling-div match count for current round
- Add elimination_path() — walks per-round participant lists to build path summary
- Document missing-tournament and missing-participant error behavior
- Add 8 tests covering even/odd participants, missing entities, elimination tracking

GameLobby (theblockcade#477):
- Export ONBOARDING_CHECKLIST_DISMISSED_FLAG constant from global-state-store
- Add dismissible FirstTimeChecklist component with three actionable items
- Track dismissal via FLAGS_SET action so state persists across reloads via localStorage
- Returning users (flag already set) skip the checklist immediately
- Add 6 tests: first-run rendering, dismissal persistence, returning-user skip, item toggle

Timeline component (theblockcade#479):
- Add reusable Timeline / TimelineEntry component (horizontal + vertical layouts)
- Support status, timestamp, and metadata slots; compact modifier for tight contexts
- TxStatusPanel refactored to use Timeline for its Submitted->Pending->Confirmed steps
- ContractEventFeed event list carries sc-timeline--vertical for timeline composition
- Add eventToTimelineItem() adapter for mapping ContractEvent to TimelineItemData
- Export Timeline from components/v1/index.ts
- Add sc-timeline and onboarding-checklist CSS to index.css
- Add 20+ tests: item rendering, ordering, status classes, both use-case adaptations
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 30, 2026

📝 Walkthrough

Walkthrough

This PR implements four features: new read-only oracle and tournament contract accessors, a reusable timeline component for visualizing sequential events, and a dismissible first-time user onboarding checklist with persistent state. Changes span contract logic, frontend components, styling, and test coverage with no modifications to existing method logic.

Changes

Cohort / File(s) Summary
Oracle Integration Contract
contracts/oracle-integration/src/lib.rs, contracts/oracle-integration/src/test.rs
Added OracleSourceSnapshot and UpdatePolicySummary types; introduced source_config_snapshot() (returns configured oracle sources or None if uninitialized) and update_policy_summary() (returns deterministic policy with hardcoded stale threshold and cadence symbol). Comprehensive test coverage for both methods including initialization-dependent behavior.
Oracle Integration Documentation
docs/contracts/oracle-integration.md
Documented two new accessor methods with field definitions, initialization-dependent return behavior for snapshot, and deterministic policy summary semantics.
Tournament System Contract
contracts/tournament-system/src/lib.rs
Added EliminationPath type with rounds_played, last_round_active, and is_active fields. Implemented remaining_match_count() (calculates ceiling of participants/2 per round) and elimination_path() (tracks player participation across rounds from 1 to current). Extensive unit tests cover active/eliminated scenarios and error cases (TournamentNotFound, PlayerNotJoined).
Tournament System Documentation
docs/contracts/tournament-system.md
Documented two new query methods, match-counting logic (ceiling division for byes), elimination-path field semantics, and explicit missing-entity error behaviors.
Timeline Component Infrastructure
frontend/src/components/v1/Timeline.tsx, frontend/src/components/v1/index.ts
New reusable timeline component with configurable orientation (horizontal/vertical), compact mode, and support for item-level status, timestamp, and metadata. Exports types TimelineItemStatus, TimelineItemData, TimelineProps.
Timeline Styling
frontend/src/index.css
Added .sc-timeline system with horizontal/vertical layout variants, per-item status modifiers (active, completed, error), dot/label styling, and connector pseudo-elements. Separately added .onboarding-checklist component styles.
ContractEventFeed Integration
frontend/src/components/v1/ContractEventFeed.tsx, frontend/tests/components/v1/ContractEventFeed.test.tsx
Exported eventToTimelineItem() adapter that converts ContractEvent to TimelineItemData; updated list className to include timeline classes. Tests verify adapter behavior (id/label mapping, timestamp normalization, contractId truncation) and timeline integration.
Transaction Status Timeline
frontend/src/components/v1/TxStatusPanel.tsx
Refactored to use reusable Timeline component; replaced inline step rendering with data-driven txTimelineItems and step-status resolver function; changed test ID structure to container-level identifier.
First-Time User Checklist
frontend/src/pages/GameLobby.tsx, frontend/src/services/global-state-store.ts, frontend/tests/components/GameLobby.test.tsx
Added ONBOARDING_CHECKLIST_DISMISSED_FLAG persistent state key. Implemented local FirstTimeChecklist component in GameLobby with dismissal handler that persists flag via FLAGS_SET action. Tests verify checklist visibility on first run, persistence across reloads, and correct behavior for returning users.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

Poem

🐰 Fresh snapshots and paths for tournament sages,
Timelines unfold through the frontend's grand stages,
New checklists greet users with wisdom so bright,
While oracles speak truth in deterministic light.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately summarizes the main changes: adding contract visibility accessors (oracle-integration and tournament-system) and platform UI components (timeline, onboarding checklist).
Linked Issues check ✅ Passed All four linked issues (#473, #474, #477, #479) have their core requirements met: oracle accessors added with tests and docs [#473], tournament accessors with tests and docs [#474], dismissible onboarding checklist with persistence and tests [#477], reusable timeline component with orientation/status/slots and integration into TxStatusPanel/ContractEventFeed [#479].
Out of Scope Changes check ✅ Passed All changes align with the four linked issues. Contract additions (oracle snapshots, tournament accessors) and frontend components (timeline, onboarding) are directly scoped to the stated objectives; CSS styling supports the new UI components and is not extraneous.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch feat/contract-accessors-and-ux-components

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (4)
contracts/oracle-integration/src/test.rs (1)

498-516: Add one pre-init assertion for update_policy_summary.

Docs/state say this accessor is init-independent, but current tests only call it after setup. Adding a pre-init case would lock that guarantee.

✅ Suggested test addition
+#[test]
+fn update_policy_summary_is_available_before_init() {
+    let env = Env::default();
+    let contract_id = env.register(OracleIntegration, ());
+    let client = OracleIntegrationClient::new(&env, &contract_id);
+
+    let summary = client.update_policy_summary();
+    assert_eq!(summary.stale_threshold_ledgers, 20);
+    assert_eq!(summary.cadence, soroban_sdk::Symbol::new(&env, "on_request"));
+}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@contracts/oracle-integration/src/test.rs` around lines 498 - 516, Add a new
test similar to the existing ones that calls update_policy_summary on a client
before running setup_initialized to assert the accessor works pre-init: create
Env::default(), obtain the contract client without performing initialization
(use the same test helper but the variant that returns an uninitialized client
or otherwise construct the client prior to calling setup_initialized), call
client.update_policy_summary() and assert summary.cadence ==
soroban_sdk::Symbol::new(&env, "on_request") (and optionally compare
stale_threshold_ledgers if desired); reference update_policy_summary and
setup_initialized to locate where to add the test.
frontend/src/components/v1/ContractEventFeed.tsx (1)

67-84: The feed still bypasses the shared timeline entry markup.

Lines 67-84 add the adapter, but Line 813 still wraps bespoke EventRow nodes instead of rendering the shared timeline structure. Because those rows never emit .sc-timeline__item / .sc-timeline__dot / .sc-timeline__label elements, the shared timeline visuals and future fixes will not apply here.

Also applies to: 811-845

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/src/components/v1/ContractEventFeed.tsx` around lines 67 - 84, The
events feed is still rendering bespoke EventRow nodes instead of the shared
timeline markup, so timeline visuals and future fixes don't apply; locate the
rendering loop in ContractEventFeed where EventRow is used and replace it with
the shared timeline structure (or the shared TimelineItem component) so each
event emits the required elements/classes: .sc-timeline__item,
.sc-timeline__dot, and .sc-timeline__label. Ensure the adapter function
eventToTimelineItem is used to map ContractEvent -> TimelineItemData and that
the rendered DOM for each timeline entry matches the shared timeline API
(labels, dot, and container) instead of custom wrappers so CSS/behavior applies
uniformly.
frontend/src/components/v1/Timeline.tsx (2)

33-43: Avoid hardcoding aria labels in a reusable component.

Right now, the label is coupled to specific product contexts. Exposing ariaLabel keeps this component reusable and localization-friendly.

Proposed refinement
 export interface TimelineProps {
   items: TimelineItemData[];
   /** Visual orientation of the timeline. Defaults to `'vertical'`. */
   orientation?: 'horizontal' | 'vertical';
   /** Reduces padding and font size for space-constrained contexts. */
   compact?: boolean;
   /** Additional CSS class names applied to the root element. */
   className?: string;
+  /** Accessible label for the ordered list. */
+  ariaLabel?: string;
   /** `data-testid` prefix forwarded to child elements. */
   testId?: string;
 }
@@
 export const Timeline: React.FC<TimelineProps> = ({
   items,
   orientation = 'vertical',
   compact = false,
   className = '',
+  ariaLabel,
   testId = 'sc-timeline',
 }) => {
@@
       className={rootClasses}
       data-testid={testId}
-      aria-label={
-        orientation === 'horizontal' ? 'Transaction steps' : 'Event history'
-      }
+      aria-label={ariaLabel ?? 'Timeline'}
       role="list"
     >

Also applies to: 134-136

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/src/components/v1/Timeline.tsx` around lines 33 - 43, The Timeline
component currently hardcodes ARIA labels; update the TimelineProps interface to
include an optional ariaLabel?: string (or ariaLabelledBy?: string if preferred)
and thread that prop through the Timeline component (and any child elements that
currently use the hardcoded label), defaulting to a sensible localized fallback
when ariaLabel is not provided; locate the props definition (TimelineProps) and
the render usage in the Timeline component (references around the Timeline
component and the usage at the other occurrence noted at lines 134-136) and
replace the hardcoded string with the passed-in ariaLabel prop to keep the
component reusable and localization-friendly.

81-87: Use semantic <time> for timestamps.

This improves accessibility/semantics with no behavioral change.

Proposed refinement
-      {item.timestamp && (
-        <span
-          className="sc-timeline__timestamp"
-          data-testid={`${entryId}-timestamp`}
-        >
-          {item.timestamp}
-        </span>
-      )}
+      {item.timestamp && (
+        <time
+          className="sc-timeline__timestamp"
+          data-testid={`${entryId}-timestamp`}
+        >
+          {item.timestamp}
+        </time>
+      )}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/src/components/v1/Timeline.tsx` around lines 81 - 87, The timestamp
is rendered with a non-semantic <span>; update the JSX in Timeline (the block
that renders item.timestamp, className "sc-timeline__timestamp" and data-testid
`${entryId}-timestamp`) to use a semantic <time> element instead of <span>, keep
the same className and data-testid, and set the time element's dateTime
attribute to an ISO/parsable value derived from item.timestamp (e.g.,
item.timestamp or a normalized ISO string) so the visual output is unchanged but
accessibility/semantics are improved.
🤖 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/tournament-system/src/lib.rs`:
- Around line 456-470: The manual increment "round += 1" can overflow; replace
it with a checked addition on the round cursor and propagate Error::Overflow on
failure (same pattern used for rounds_played). Specifically, within the loop
that iterates while round <= current_round (using variables round and
current_round and accessing DataKey::RoundParticipants and updating
rounds_played), call round.checked_add(1).ok_or(Error::Overflow)? and assign the
result back to round so any overflow returns the contract Overflow error instead
of wrapping.
- Around line 425-427: Replace the raw arithmetic on `count` with checked
operations: use `count.checked_add(1).ok_or(Error::Overflow)?` and then
`checked_div(2).ok_or(Error::Overflow)?` (or equivalent chained checked calls)
so the ceiling-division on `participants.len()` returns an error on overflow
instead of wrapping; ensure you still return the result as `Ok(...)` and
reference `Error::Overflow`, `participants`, and `count` when making the change.
- Around line 776-777: The assertions are using Result wrappers and .expect() on
non-try_ client methods: update assertions that call remaining_match_count(...)
and similar non-try_ methods to compare against the raw value (remove the
Ok(...) wrapper) and remove .expect() calls on elimination_path(...) results
(use the returned value directly), while leaving try_ methods unchanged;
specifically edit usages of remaining_match_count, elimination_path, and other
non-try_ methods in the tests to assert against their direct return types and
drop any .expect() wrapping so the comparisons use the unwrapped success values.

In `@frontend/src/components/v1/TxStatusPanel.tsx`:
- Around line 199-214: currentStepIndex and resolveStepStatus are mis-mapped for
terminal states; compute currentStepIndex directly from phase (TxPhase.IDLE->0,
SUBMITTED->1, PENDING->2, CONFIRMED->3) and do not force a blanket index for
failures—keep the phase-derived index even when isFailed so failures show at the
correct step. Then simplify resolveStepStatus: if isFailed and stepIndex ===
currentStepIndex return 'error'; else if stepIndex < currentStepIndex return
'completed'; else if stepIndex === currentStepIndex return 'active'; otherwise
return 'idle'. Update the functions/variables currentStepIndex,
resolveStepStatus, TxPhase and isFailed accordingly.

In `@frontend/src/index.css`:
- Around line 604-611: The grid for the vertical timeline items currently
defines only three columns on the .sc-timeline--vertical .sc-timeline__item
selector, causing the metadata cell to wrap to a new row when both label and
timestamp are present; update the grid-template-columns for
.sc-timeline--vertical .sc-timeline__item to add a fourth column reserved for
metadata (so dot, label, timestamp, metadata each have their own column) and
ensure the metadata cell (the element/slot that renders metadata) is placed in
that fourth column rather than allowed to span or flow to a new row.

In `@frontend/src/pages/GameLobby.tsx`:
- Around line 25-30: The checklist’s checked state is not persisted so
FirstTimeChecklist (state managed by useState and toggled by toggle) always
starts empty; initialize checked from a persisted store (e.g. localStorage) when
the component mounts, update that store whenever toggle runs, and persist a
derived “completed” flag (or persist all per-item booleans) so the initial
visibility check (the same logic that uses onDismiss) reads that persisted
completed/dismissed value and hides the card on reload; update the code paths
around setChecked, toggle, and onDismiss to read/write the same storage key and
to call onDismiss (or set dismissed) when the persisted completed flag becomes
true.

---

Nitpick comments:
In `@contracts/oracle-integration/src/test.rs`:
- Around line 498-516: Add a new test similar to the existing ones that calls
update_policy_summary on a client before running setup_initialized to assert the
accessor works pre-init: create Env::default(), obtain the contract client
without performing initialization (use the same test helper but the variant that
returns an uninitialized client or otherwise construct the client prior to
calling setup_initialized), call client.update_policy_summary() and assert
summary.cadence == soroban_sdk::Symbol::new(&env, "on_request") (and optionally
compare stale_threshold_ledgers if desired); reference update_policy_summary and
setup_initialized to locate where to add the test.

In `@frontend/src/components/v1/ContractEventFeed.tsx`:
- Around line 67-84: The events feed is still rendering bespoke EventRow nodes
instead of the shared timeline markup, so timeline visuals and future fixes
don't apply; locate the rendering loop in ContractEventFeed where EventRow is
used and replace it with the shared timeline structure (or the shared
TimelineItem component) so each event emits the required elements/classes:
.sc-timeline__item, .sc-timeline__dot, and .sc-timeline__label. Ensure the
adapter function eventToTimelineItem is used to map ContractEvent ->
TimelineItemData and that the rendered DOM for each timeline entry matches the
shared timeline API (labels, dot, and container) instead of custom wrappers so
CSS/behavior applies uniformly.

In `@frontend/src/components/v1/Timeline.tsx`:
- Around line 33-43: The Timeline component currently hardcodes ARIA labels;
update the TimelineProps interface to include an optional ariaLabel?: string (or
ariaLabelledBy?: string if preferred) and thread that prop through the Timeline
component (and any child elements that currently use the hardcoded label),
defaulting to a sensible localized fallback when ariaLabel is not provided;
locate the props definition (TimelineProps) and the render usage in the Timeline
component (references around the Timeline component and the usage at the other
occurrence noted at lines 134-136) and replace the hardcoded string with the
passed-in ariaLabel prop to keep the component reusable and
localization-friendly.
- Around line 81-87: The timestamp is rendered with a non-semantic <span>;
update the JSX in Timeline (the block that renders item.timestamp, className
"sc-timeline__timestamp" and data-testid `${entryId}-timestamp`) to use a
semantic <time> element instead of <span>, keep the same className and
data-testid, and set the time element's dateTime attribute to an ISO/parsable
value derived from item.timestamp (e.g., item.timestamp or a normalized ISO
string) so the visual output is unchanged but accessibility/semantics are
improved.
🪄 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: dd4b0c5a-a00b-4c16-9e05-cab1156b13f5

📥 Commits

Reviewing files that changed from the base of the PR and between 5bade99 and 94e0789.

📒 Files selected for processing (14)
  • contracts/oracle-integration/src/lib.rs
  • contracts/oracle-integration/src/test.rs
  • contracts/tournament-system/src/lib.rs
  • docs/contracts/oracle-integration.md
  • docs/contracts/tournament-system.md
  • frontend/src/components/v1/ContractEventFeed.tsx
  • frontend/src/components/v1/Timeline.tsx
  • frontend/src/components/v1/TxStatusPanel.tsx
  • frontend/src/components/v1/index.ts
  • frontend/src/index.css
  • frontend/src/pages/GameLobby.tsx
  • frontend/src/services/global-state-store.ts
  • frontend/tests/components/GameLobby.test.tsx
  • frontend/tests/components/v1/ContractEventFeed.test.tsx

Comment on lines +425 to +427
// Ceiling division: an odd participant gets a bye and counts as one match.
let count = participants.len();
Ok((count + 1) / 2)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Check if the file exists and read the relevant section
wc -l contracts/tournament-system/src/lib.rs

Repository: theblockcade/stellarcade

Length of output: 110


🏁 Script executed:

# Read the file to check for no_std attribute and the code in question
head -50 contracts/tournament-system/src/lib.rs

Repository: theblockcade/stellarcade

Length of output: 1686


🏁 Script executed:

# Read around lines 425-427 to see the actual code
sed -n '420,430p' contracts/tournament-system/src/lib.rs

Repository: theblockcade/stellarcade

Length of output: 457


🏁 Script executed:

# Search for Error enum definition to verify Overflow variant exists
rg -A 20 "enum Error" contracts/tournament-system/src/lib.rs

Repository: theblockcade/stellarcade

Length of output: 720


🏁 Script executed:

# Check context around the function to understand what it does
sed -n '410,440p' contracts/tournament-system/src/lib.rs

Repository: theblockcade/stellarcade

Length of output: 1335


Use checked arithmetic for the ceiling-division operation.

The code uses raw arithmetic count + 1 instead of safe checked operations. Even though Error::Overflow already exists in the contract's error enum, the operation should use checked arithmetic per the Soroban contract guidelines: "Use safe arithmetic operations (checked_add, checked_div) with explicit Overflow error variants in Soroban contracts". This ensures the getter never wraps on large participant counts.

Suggested fix
         // Ceiling division: an odd participant gets a bye and counts as one match.
         let count = participants.len();
-        Ok((count + 1) / 2)
+        let rounded = count.checked_add(1).ok_or(Error::Overflow)?;
+        Ok(rounded.checked_div(2u32).ok_or(Error::Overflow)?)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@contracts/tournament-system/src/lib.rs` around lines 425 - 427, Replace the
raw arithmetic on `count` with checked operations: use
`count.checked_add(1).ok_or(Error::Overflow)?` and then
`checked_div(2).ok_or(Error::Overflow)?` (or equivalent chained checked calls)
so the ceiling-division on `participants.len()` returns an error on overflow
instead of wrapping; ensure you still return the result as `Ok(...)` and
reference `Error::Overflow`, `participants`, and `count` when making the change.

Comment on lines +456 to +470
let mut round: u32 = 1;

while round <= current_round {
let participants: soroban_sdk::Vec<Address> = env
.storage()
.persistent()
.get(&DataKey::RoundParticipants(id, round))
.unwrap_or(soroban_sdk::Vec::new(&env));

if participants.contains(&player) {
last_round_active = round;
rounds_played = rounds_played.checked_add(1).ok_or(Error::Overflow)?;
}

round += 1;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, let's check if the file exists and get its total line count
wc -l contracts/tournament-system/src/lib.rs 2>/dev/null || echo "File not found, searching for lib.rs in tournament-system"

Repository: theblockcade/stellarcade

Length of output: 110


🏁 Script executed:

# Search for the tournament-system contract
find . -path "*/tournament-system/src/lib.rs" -type f

Repository: theblockcade/stellarcade

Length of output: 108


🏁 Script executed:

# If found, read the relevant lines around 456-470
if [ -f "contracts/tournament-system/src/lib.rs" ]; then
  sed -n '450,475p' contracts/tournament-system/src/lib.rs
fi

Repository: theblockcade/stellarcade

Length of output: 919


🏁 Script executed:

# Search for Error enum definition to verify Overflow variant exists
rg "enum Error" contracts/tournament-system/src/lib.rs -A 20

Repository: theblockcade/stellarcade

Length of output: 720


Guard the manual round increment against overflow.

round += 1 can overflow at u32::MAX, which breaks the while round <= current_round loop termination logic instead of returning the contract's Overflow error. This loop already uses checked arithmetic for rounds_played; the round cursor needs the same treatment.

Suggested fix
-            round += 1;
+            round = round.checked_add(1).ok_or(Error::Overflow)?;

As per coding guidelines, "Use safe arithmetic operations (checked_add, checked_div) with explicit Overflow error variants in Soroban contracts".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@contracts/tournament-system/src/lib.rs` around lines 456 - 470, The manual
increment "round += 1" can overflow; replace it with a checked addition on the
round cursor and propagate Error::Overflow on failure (same pattern used for
rounds_played). Specifically, within the loop that iterates while round <=
current_round (using variables round and current_round and accessing
DataKey::RoundParticipants and updating rounds_played), call
round.checked_add(1).ok_or(Error::Overflow)? and assign the result back to round
so any overflow returns the contract Overflow error instead of wrapping.

Comment on lines +776 to +777
// 4 participants → 2 matches
assert_eq!(client.remaining_match_count(&id), Ok(2));
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

wc -l contracts/tournament-system/src/lib.rs

Repository: theblockcade/stellarcade

Length of output: 110


🏁 Script executed:

head -850 contracts/tournament-system/src/lib.rs | tail -100

Repository: theblockcade/stellarcade

Length of output: 3204


🏁 Script executed:

sed -n '760,880p' contracts/tournament-system/src/lib.rs

Repository: theblockcade/stellarcade

Length of output: 4115


🏁 Script executed:

find contracts/tournament-system -name "*.rs" | head -20

Repository: theblockcade/stellarcade

Length of output: 106


🏁 Script executed:

grep -r "impl Client" contracts/tournament-system/ 2>/dev/null | head -5

Repository: theblockcade/stellarcade

Length of output: 50


🏁 Script executed:

grep -r "remaining_match_count\|elimination_path" contracts/tournament-system/src/lib.rs | head -20

Repository: theblockcade/stellarcade

Length of output: 1313


🏁 Script executed:

# Look for the contract spec or interface definition
find contracts/tournament-system -type f \( -name "*.rs" -o -name "*.toml" \) -exec basename {} \; | sort | uniq

Repository: theblockcade/stellarcade

Length of output: 85


🏁 Script executed:

cat contracts/tournament-system/Cargo.toml

Repository: theblockcade/stellarcade

Length of output: 321


🏁 Script executed:

# Search for where the client is generated or used
grep -n "pub struct Client\|fn setup\|soroban-sdk" contracts/tournament-system/src/lib.rs | head -30

Repository: theblockcade/stellarcade

Length of output: 152


🏁 Script executed:

# Look at the setup function to understand how client is created
sed -n '1,100p' contracts/tournament-system/src/lib.rs

Repository: theblockcade/stellarcade

Length of output: 3119


🏁 Script executed:

sed -n '547,600p' contracts/tournament-system/src/lib.rs

Repository: theblockcade/stellarcade

Length of output: 1742


🏁 Script executed:

# Search for other usages of client methods to understand the pattern
grep -n "client\." contracts/tournament-system/src/lib.rs | grep -E "remaining_match_count|elimination_path" | head -30

Repository: theblockcade/stellarcade

Length of output: 703


🏁 Script executed:

# Check if there are any comments or imports that hint at client generation
sed -n '520,570p' contracts/tournament-system/src/lib.rs

Repository: theblockcade/stellarcade

Length of output: 1690


🏁 Script executed:

# Look for other client method calls to understand the pattern
grep -n "client\.\(get_\|is_\|create_\|join_\)" contracts/tournament-system/src/lib.rs | head -20

Repository: theblockcade/stellarcade

Length of output: 1458


🏁 Script executed:

# Check if there are unwrap or other patterns used with client calls
sed -n '560,610p' contracts/tournament-system/src/lib.rs

Repository: theblockcade/stellarcade

Length of output: 1586


🌐 Web query:

Soroban SDK 25.1.1 generated client API try_ methods pattern

💡 Result:

In Soroban SDK version 25.1.1 (confirmed released 2026-02-13), generated client APIs from #[contractimpl], #[contractclient], or #[contractimport] follow a pattern where each contract method has two variants: a panicking infallible version and a fallible "try_" prefixed version. The infallible method (e.g., allowance(&self, from: &Address, spender: &Address) -> i128) assumes success and panics on contract errors. The try_ method (e.g., try_allowance(&self, from: &Address, spender: &Address) -> Result<i128, <Self as TryFromVal<Env, InvokeError>>::Error>) returns a Result, propagating contract errors as InvokeError via TryFromVal. This pattern is demonstrated in the standard TokenClient (soroban_sdk::token::TokenClient), which is generated from TokenInterface using the same macros. Examples include try_allowance, try_balance, try_transfer, try_burn, etc. The try_ methods enable error handling without panics, suitable for tests or production where failure is possible. To use: let result = client.try_allowance(&from, &spender); match result { Ok(amount) => ..., Err(e) => ... } This applies to all generated clients in SDK 25.1.1, as the macros consistently produce both method types.

Citations:


🏁 Script executed:

# Get exact lines to verify the pattern
sed -n '776,778p' contracts/tournament-system/src/lib.rs
sed -n '796,798p' contracts/tournament-system/src/lib.rs
sed -n '812,814p' contracts/tournament-system/src/lib.rs
sed -n '841,845p' contracts/tournament-system/src/lib.rs
sed -n '867,876p' contracts/tournament-system/src/lib.rs

Repository: theblockcade/stellarcade

Length of output: 1072


🏁 Script executed:

# Check if this is a valid pattern or if there's implicit conversion happening
# Look at method return types to confirm
grep -A 5 "pub fn get_tournament\|pub fn remaining_match_count\|pub fn elimination_path" contracts/tournament-system/src/lib.rs | head -40

Repository: theblockcade/stellarcade

Length of output: 881


Remove Ok() wrapper and .expect() calls from non-try_ client method assertions.

The Soroban SDK 25.1.1 generated client provides both infallible and fallible method variants. Non-try_ methods (e.g., remaining_match_count(), elimination_path()) return success types directly; try_ methods return Result for error handling. Fix assertions at lines 776–777, 796–797, 812–813 to compare against unwrapped values, and remove .expect() calls from elimination_path() at lines 841–845 and 867–876.

Suggested fix
-        assert_eq!(client.remaining_match_count(&id), Ok(2));
+        assert_eq!(client.remaining_match_count(&id), 2);
@@
-        let path = client.elimination_path(&id, &player).expect("path present");
+        let path = client.elimination_path(&id, &player);
📝 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.

Suggested change
// 4 participants → 2 matches
assert_eq!(client.remaining_match_count(&id), Ok(2));
// 4 participants → 2 matches
assert_eq!(client.remaining_match_count(&id), 2);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@contracts/tournament-system/src/lib.rs` around lines 776 - 777, The
assertions are using Result wrappers and .expect() on non-try_ client methods:
update assertions that call remaining_match_count(...) and similar non-try_
methods to compare against the raw value (remove the Ok(...) wrapper) and remove
.expect() calls on elimination_path(...) results (use the returned value
directly), while leaving try_ methods unchanged; specifically edit usages of
remaining_match_count, elimination_path, and other non-try_ methods in the tests
to assert against their direct return types and drop any .expect() wrapping so
the comparisons use the unwrapped success values.

Comment on lines +199 to 214
const currentStepIndex =
phase === TxPhase.IDLE ? 0 :
phase === TxPhase.SUBMITTED ? 1 :
phase === TxPhase.PENDING ? 2 :
phase === TxPhase.CONFIRMED ? 3 :
isFailed ? 2 : 0;

const resolveStepStatus = (stepIndex: number): TimelineItemStatus => {
if (isFailed && stepIndex === currentStepIndex) return 'error';
if (stepIndex < currentStepIndex && !isFailed) return 'completed';
if (
(phase === TxPhase.SUBMITTED && stepIndex === 1) ||
(phase === TxPhase.PENDING && stepIndex === 2)
) return 'active';
return 'idle';
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Confirmed and failed phases are mapped to the wrong step states.

With the current resolveStepStatus() logic, a confirmed transaction leaves the "Confirmed" step idle, and a failed transaction leaves earlier successful steps idle because of the && !isFailed guard. The refactored timeline will look incomplete in both terminal states.

Suggested fix
     const resolveStepStatus = (stepIndex: number): TimelineItemStatus => {
-      if (isFailed && stepIndex === currentStepIndex) return 'error';
-      if (stepIndex < currentStepIndex && !isFailed) return 'completed';
+      if (isFailed) {
+        if (stepIndex < currentStepIndex) return 'completed';
+        if (stepIndex === currentStepIndex) return 'error';
+        return 'idle';
+      }
+      if (phase === TxPhase.CONFIRMED) {
+        return stepIndex <= currentStepIndex ? 'completed' : 'idle';
+      }
+      if (stepIndex < currentStepIndex) return 'completed';
       if (
         (phase === TxPhase.SUBMITTED && stepIndex === 1) ||
         (phase === TxPhase.PENDING && stepIndex === 2)
       ) return 'active';
       return 'idle';

Also applies to: 216-233

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/src/components/v1/TxStatusPanel.tsx` around lines 199 - 214,
currentStepIndex and resolveStepStatus are mis-mapped for terminal states;
compute currentStepIndex directly from phase (TxPhase.IDLE->0, SUBMITTED->1,
PENDING->2, CONFIRMED->3) and do not force a blanket index for failures—keep the
phase-derived index even when isFailed so failures show at the correct step.
Then simplify resolveStepStatus: if isFailed and stepIndex === currentStepIndex
return 'error'; else if stepIndex < currentStepIndex return 'completed'; else if
stepIndex === currentStepIndex return 'active'; otherwise return 'idle'. Update
the functions/variables currentStepIndex, resolveStepStatus, TxPhase and
isFailed accordingly.

Comment on lines +604 to +611
.sc-timeline--vertical .sc-timeline__item {
display: grid;
grid-template-columns: var(--sc-tl-dot-size) 1fr auto;
align-items: center;
gap: 0.5rem;
padding: 0.4rem 0;
position: relative;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add a fourth vertical grid column for metadata.

Line 606 only provisions three columns, but the shared timeline entry can render dot, label, timestamp, and metadata. When both optional slots are present, the metadata cell drops onto a second row instead of staying aligned with the entry.

Suggested fix
 .sc-timeline--vertical .sc-timeline__item {
   display: grid;
-  grid-template-columns: var(--sc-tl-dot-size) 1fr auto;
+  grid-template-columns: var(--sc-tl-dot-size) minmax(0, 1fr) auto auto;
   align-items: center;
   gap: 0.5rem;
   padding: 0.4rem 0;
   position: relative;
 }
📝 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.

Suggested change
.sc-timeline--vertical .sc-timeline__item {
display: grid;
grid-template-columns: var(--sc-tl-dot-size) 1fr auto;
align-items: center;
gap: 0.5rem;
padding: 0.4rem 0;
position: relative;
}
.sc-timeline--vertical .sc-timeline__item {
display: grid;
grid-template-columns: var(--sc-tl-dot-size) minmax(0, 1fr) auto auto;
align-items: center;
gap: 0.5rem;
padding: 0.4rem 0;
position: relative;
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/src/index.css` around lines 604 - 611, The grid for the vertical
timeline items currently defines only three columns on the
.sc-timeline--vertical .sc-timeline__item selector, causing the metadata cell to
wrap to a new row when both label and timestamp are present; update the
grid-template-columns for .sc-timeline--vertical .sc-timeline__item to add a
fourth column reserved for metadata (so dot, label, timestamp, metadata each
have their own column) and ensure the metadata cell (the element/slot that
renders metadata) is placed in that fourth column rather than allowed to span or
flow to a new row.

Comment on lines +25 to +30
const FirstTimeChecklist: React.FC<FirstTimeChecklistProps> = ({ onDismiss }) => {
const [checked, setChecked] = useState<Record<string, boolean>>({});

const toggle = useCallback((id: string) => {
setChecked((prev) => ({ ...prev, [id]: !prev[id] }));
}, []);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Checklist completion still resets on reload.

Lines 26-29 always start from an empty checked map, and Lines 106-111 only persist the explicit dismiss path. A user who completes all three items without clicking dismiss will see the card again on the next visit, which misses the persistence part of #477. Persist either the per-item state or a derived “completed” flag and include it in the initial visibility check.

Also applies to: 102-112

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/src/pages/GameLobby.tsx` around lines 25 - 30, The checklist’s
checked state is not persisted so FirstTimeChecklist (state managed by useState
and toggled by toggle) always starts empty; initialize checked from a persisted
store (e.g. localStorage) when the component mounts, update that store whenever
toggle runs, and persist a derived “completed” flag (or persist all per-item
booleans) so the initial visibility check (the same logic that uses onDismiss)
reads that persisted completed/dismissed value and hides the card on reload;
update the code paths around setChecked, toggle, and onDismiss to read/write the
same storage key and to call onDismiss (or set dismissed) when the persisted
completed flag becomes true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant