Skip to content

feat: add guarded v1 actions and contract monitoring/gas analysis tooling#171

Merged
CMI-James merged 1 commit intoTheBlockCade:mainfrom
Themancalledpg:feat/resolve-34-38-120-123
Feb 26, 2026
Merged

feat: add guarded v1 actions and contract monitoring/gas analysis tooling#171
CMI-James merged 1 commit intoTheBlockCade:mainfrom
Themancalledpg:feat/resolve-34-38-120-123

Conversation

@Themancalledpg
Copy link
Contributor

@Themancalledpg Themancalledpg commented Feb 26, 2026

PR: Guarded V1 Action Components + Contract Monitoring and Gas Analysis Tooling

PR Link

#171

Overview

This PR delivers four assigned issues in one cohesive change set:

The implementation follows existing project patterns and keeps scope practical: reusable frontend components, minimal but complete Soroban contract modules, backend integration helpers, and documentation.

What Changed

Frontend

  • Added frontend/src/components/v1/AsyncStateBoundary.tsx

    • Shared async lifecycle renderer for idle | loading | success | error.
    • Supports per-branch render props and retry behavior.
    • Handles empty-success data deterministically.
  • Added frontend/src/components/v1/ContractActionButton.tsx

    • Guarded mutation trigger for contract actions.
    • Enforces preconditions (walletConnected, networkSupported).
    • Prevents duplicate in-flight triggers.
    • Maps errors via shared error mapper and exposes callbacks:
      • onSuccess(result)
      • onError(mappedError)
  • Exported both from frontend/src/components/v1/index.ts.

  • Added docs/examples:

    • frontend/src/components/v1/AsyncStateBoundary.md
    • frontend/src/components/v1/ContractActionButton.md
  • Added tests:

    • frontend/tests/components/v1/AsyncStateBoundary.test.tsx
    • frontend/tests/components/v1/ContractActionButton.test.tsx

Contracts

  • Added new Soroban module: contracts/contract-monitoring

    • Files:
      • contracts/contract-monitoring/Cargo.toml
      • contracts/contract-monitoring/src/lib.rs
      • contracts/contract-monitoring/README.md
    • Implements:
      • event ingestion pipeline with duplicate-event guard
      • metrics aggregation
      • health snapshot flags (failed settlements, high error rate, paused)
      • alert events for anomaly conditions
      • admin authorization enforcement
  • Added new Soroban module: contracts/gas-optimization-analysis

    • Files:
      • contracts/gas-optimization-analysis/Cargo.toml
      • contracts/gas-optimization-analysis/src/lib.rs
      • contracts/gas-optimization-analysis/README.md
    • Implements:
      • per-method compute/storage sampling
      • hotspot scoring
      • deterministic optimization recommendations
      • estimated savings output suitable for CI artifacts
      • admin authorization and metric validation

Backend Integration

  • Added backend/src/services/contractMonitoring.service.js
    • In-memory monitoring service for ingesting events, tracking metrics, and producing health payloads.
  • Added backend/src/utils/contractMonitoringAlerts.js
    • Shared alert rule evaluation helpers.
  • Added test:
    • backend/tests/unit/contractMonitoringAlerts.test.js

Docs

  • Added:
    • docs/contracts/contract-monitoring.md
    • docs/contracts/gas-optimization-analysis.md

Security / Reliability Notes

  • Explicit role checks for privileged contract paths.
  • Duplicate processing guard using event id tracking.
  • Deterministic state updates and health evaluations.
  • Defensive handling of optional/invalid frontend inputs.
  • In-flight action lock to prevent duplicate user-triggered mutations.

Testing

Commands were attempted, but local environment constraints prevented full execution in this workspace:

  • Frontend Jest run failed because @testing-library/jest-dom was unavailable locally.
  • Backend Jest run failed because jest was unavailable locally.
  • Rust contract tests could not run because cargo was unavailable locally.

Issue Linking

Closes #123
Closes #120
Closes #38
Closes #34

@coderabbitai
Copy link

coderabbitai bot commented Feb 26, 2026

📝 Walkthrough

Walkthrough

Introduces two new Soroban smart contracts for contract monitoring and gas optimization analysis, adds backend services for event ingestion and alert evaluation, and implements frontend components for async state handling and guarded contract mutations.

Changes

Cohort / File(s) Summary
Contract Monitoring Backend
backend/src/services/contractMonitoring.service.js, backend/src/utils/contractMonitoringAlerts.js
New singleton service and utility for event ingestion, metrics tracking, and threshold-based alert evaluation (settlement failures, error rate).
Contract Monitoring Tests
backend/tests/unit/contractMonitoringAlerts.test.js
Unit tests validating alert thresholds and metrics computation.
Contract Monitoring Soroban Contract
contracts/contract-monitoring/src/lib.rs, contracts/contract-monitoring/Cargo.toml, contracts/contract-monitoring/README.md
On-chain contract with admin-gated event ingestion, duplicate prevention, persistent metrics, health evaluation, and alert emission.
Gas Optimization Analysis Soroban Contract
contracts/gas-optimization-analysis/src/lib.rs, contracts/gas-optimization-analysis/Cargo.toml, contracts/gas-optimization-analysis/README.md
On-chain contract for profiling method performance and generating optimization recommendations based on CPU and storage hotspots.
Contract Documentation
docs/contracts/contract-monitoring.md, docs/contracts/gas-optimization-analysis.md
Dashboard-ready documentation describing health flags, metrics, and analysis outputs.
AsyncStateBoundary Component
frontend/src/components/v1/AsyncStateBoundary.tsx, frontend/src/components/v1/AsyncStateBoundary.md
Reusable React component for rendering async lifecycle states (idle, loading, success, error) with customizable render props and default fallbacks.
AsyncStateBoundary Tests
frontend/tests/components/v1/AsyncStateBoundary.test.tsx
Tests covering all state branches, retry callback, and custom render prop handling.
ContractActionButton Component
frontend/src/components/v1/ContractActionButton.tsx, frontend/src/components/v1/ContractActionButton.md
Guarded mutation trigger component with wallet/network precondition enforcement, in-flight locking, and error mapping.
ContractActionButton Tests
frontend/tests/components/v1/ContractActionButton.test.tsx
Tests for precondition checks, debouncing, action execution, error handling, and callback invocation.
Component Exports
frontend/src/components/v1/index.ts
Updated public API to export AsyncStateBoundary and ContractActionButton with their prop types.

Sequence Diagrams

sequenceDiagram
    participant Admin
    participant Contract as Contract<br/>Monitoring
    participant Storage as Persistent<br/>Storage
    participant Metrics as Metrics<br/>Evaluation
    participant Events as Event<br/>Emission

    Admin->>Contract: ingest_event(event_id, kind)
    Contract->>Storage: check SeenEvent(event_id)
    alt Duplicate Found
        Contract-->>Admin: Error: DuplicateEvent
    else New Event
        Contract->>Metrics: apply_event(kind)
        Metrics-->>Contract: updated Metrics
        Contract->>Storage: persist SeenEvent(event_id)
        Contract->>Metrics: evaluate_health()
        Metrics-->>Contract: HealthSnapshot
        Contract->>Events: emit EventIngested
        Contract->>Events: emit AlertRaised (if alerts)
        Contract-->>Admin: return Metrics
    end
Loading
sequenceDiagram
    participant Admin
    participant Contract as Gas Optimization<br/>Contract
    participant Storage as Method<br/>Storage
    participant Analysis as Analysis<br/>Engine
    participant Results as Result<br/>Aggregation

    Admin->>Contract: record_sample(method, cpu, read, write)
    Contract->>Storage: get or create MethodProfile
    Storage-->>Contract: profile
    Contract->>Analysis: aggregate metrics
    Analysis-->>Contract: updated profile
    Contract->>Storage: persist profile
    Contract-->>Admin: return profile

    Admin->>Contract: get_hotspots(limit)
    Contract->>Storage: iterate all profiles
    Contract->>Results: rank by cpu score
    Results-->>Contract: sorted hotspots
    Contract-->>Admin: return hotspots

    Admin->>Contract: get_recommendations(limit)
    Contract->>Storage: iterate profiles
    Contract->>Analysis: evaluate each profile
    Analysis-->>Contract: recommendations
    Contract->>Results: filter and limit
    Results-->>Contract: final recommendations
    Contract-->>Admin: return recommendations
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Suggested reviewers

  • CMI-James

Poem

🐰 Hops with glee through Soroban's embrace,
Two contracts spring forth at a frantic pace—
Monitoring metrics, gas optimized tight,
React components render just right!
Events ingested, alerts now take flight!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main additions: guarded v1 action components and contract monitoring/gas analysis tooling across frontend, contracts, and backend.
Linked Issues check ✅ Passed All four linked issues (#123, #120, #38, #34) are substantially addressed with implementation, tests, and documentation across frontend components, Soroban contracts, and backend utilities.
Out of Scope Changes check ✅ Passed All changes align with the four linked issues. Frontend components, contract implementations, backend services, and documentation files are all directly scoped to the stated objectives.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


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.

@CMI-James CMI-James merged commit c4336f0 into TheBlockCade:main Feb 26, 2026
4 of 6 checks passed
Copy link

@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: 8

🧹 Nitpick comments (11)
backend/src/utils/contractMonitoringAlerts.js (1)

5-11: Redundant condition in guard clause.

The check totalEvents === 0 is redundant since 0 < HIGH_ERROR_RATE_MIN_SAMPLE (10) is always true. The first condition already covers the zero case.

🔧 Suggested simplification
 const isHighErrorRate = (errorEvents, totalEvents) => {
-  if (totalEvents < HIGH_ERROR_RATE_MIN_SAMPLE || totalEvents === 0) {
+  if (totalEvents < HIGH_ERROR_RATE_MIN_SAMPLE) {
     return false;
   }

   return ((errorEvents * 100) / totalEvents) >= HIGH_ERROR_RATE_PERCENT;
 };
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/src/utils/contractMonitoringAlerts.js` around lines 5 - 11, The guard
clause in isHighErrorRate redundantly checks totalEvents === 0 because
totalEvents < HIGH_ERROR_RATE_MIN_SAMPLE already covers zero; simplify the
condition by removing the === 0 check so the function only tests totalEvents <
HIGH_ERROR_RATE_MIN_SAMPLE (using the existing HIGH_ERROR_RATE_MIN_SAMPLE
constant) before computing the percentage against HIGH_ERROR_RATE_PERCENT.
backend/src/services/contractMonitoring.service.js (2)

78-78: Singleton export prevents testing isolation.

Exporting a singleton instance makes it difficult to test the service in isolation since state persists across tests. Consider exporting the class alongside the instance, or providing a factory/reset method.

🔧 Suggested approach for testability
-module.exports = new ContractMonitoringService();
+const instance = new ContractMonitoringService();
+
+module.exports = instance;
+module.exports.ContractMonitoringService = ContractMonitoringService;

This allows tests to instantiate fresh instances while preserving the singleton for production use.

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

In `@backend/src/services/contractMonitoring.service.js` at line 78, The current
export of a singleton instance (module.exports = new
ContractMonitoringService()) prevents test isolation; change the module to
export the ContractMonitoringService class itself and also export a
default/legacy singleton or provide a factory/reset function so tests can
instantiate fresh ContractMonitoringService instances (or call resetFactory())
while production code can continue to use the existing singleton; update
references to the exported symbol accordingly (ContractMonitoringService, the
exported instance/factory/reset).

26-41: Silent fallthrough for unknown event kinds.

Unknown event kind values are silently ignored. Consider logging unrecognized kinds for observability, or throwing an error if strict validation is desired.

🔧 Suggested improvement
       case 'paused':
         this.metrics.pausedEvents += 1;
         break;
       default:
+        // Optionally log or track unknown event kinds
+        console.warn(`Unknown event kind: ${kind}`);
         break;
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/src/services/contractMonitoring.service.js` around lines 26 - 41, The
switch on the incoming event "kind" in contractMonitoring.service (the switch
that updates
this.metrics.settlementSuccess/settlementFailed/errorEvents/pausedEvents)
silently ignores unknown kinds; update the default branch to record or surface
unexpected values by logging a warning with the unknown kind (e.g., via
this.logger.warn or console.warn) including the raw event payload and/or
timestamp for observability, and optionally throw or emit an error if you want
strict validation (make this behavior configurable). Locate the switch handling
"kind" in the method that processes contract events and modify the default case
to log the unrecognized kind and context (or raise an error) rather than doing
nothing.
contracts/gas-optimization-analysis/src/lib.rs (1)

54-62: Consider emitting an event on contract initialization.

Per coding guidelines, Soroban contracts should emit events via env.events().publish() for state changes. Adding an initialization event would improve observability and align with the contract-monitoring module's event-driven approach.

📢 Suggested event emission
         admin.require_auth();
         env.storage().instance().set(&DataKey::Admin, &admin);
         env.storage().instance().set(&DataKey::Methods, &Vec::<Symbol>::new(&env));
+        env.events().publish((Symbol::new(&env, "initialized"),), admin);
         Ok(())
     }

As per coding guidelines: "Emit contract events via env.events().publish() or #[contractevent] macro in Soroban contracts."

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

In `@contracts/gas-optimization-analysis/src/lib.rs` around lines 54 - 62, The
init function currently sets Admin and Methods but doesn't emit an event; update
init (the function named init) to call env.events().publish() after successful
initialization to emit an initialization event (include relevant data such as
DataKey::Admin/Address and any metadata), so observers can detect the contract
deployment/state change; use the contract events API (env.events().publish or
#[contractevent] if preferred) and ensure the publish call occurs after
env.storage().instance().set(...) and before returning Ok(()).
contracts/gas-optimization-analysis/README.md (1)

20-22: Update documentation if arithmetic strategy changes.

The README states "All counters use saturating arithmetic," but coding guidelines require checked_add with explicit Overflow errors. If the implementation is updated per the review comment on lib.rs, this documentation should reflect "checked arithmetic with overflow errors" instead.

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

In `@contracts/gas-optimization-analysis/README.md` around lines 20 - 22, Update
the README statement "All counters use saturating arithmetic" to reflect the new
implementation in lib.rs: mention that counters use checked arithmetic (e.g.,
checked_add) and will return explicit Overflow errors on overflow; reference the
counters/metrics behavior and the Overflow error type so readers know to expect
checked arithmetic semantics and error handling rather than saturating behavior.
contracts/contract-monitoring/src/lib.rs (3)

11-19: Add Overflow error variant for safe arithmetic handling.

The coding guidelines require using safe arithmetic operations with explicit Overflow error variants in Soroban contracts. The Error enum is missing an Overflow variant that would be needed if checked_div is used in is_high_error_rate.

Proposed addition
 pub enum Error {
     AlreadyInitialized = 1,
     NotInitialized = 2,
     NotAuthorized = 3,
     DuplicateEvent = 4,
+    Overflow = 5,
 }

Based on learnings: "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/contract-monitoring/src/lib.rs` around lines 11 - 19, The Error
enum lacks an Overflow variant required for safe arithmetic; add an Overflow
variant to the pub enum Error (which is referenced by functions like
is_high_error_rate that use checked_div/checked_add) and update any places that
map arithmetic failures to Error::Overflow so checked_* failures return this
variant instead of panicking or returning a generic error; ensure the new
variant follows the existing #[repr(u32)] ordering and derives so it integrates
with the contracterror macro and error handling paths.

172-177: Use checked_div instead of direct division for safe arithmetic.

Per coding guidelines, Soroban contracts must use safe arithmetic operations. While saturating_mul is used on line 176, the subsequent division uses / directly. If total_events is unexpectedly 0 (despite the guard), this would panic. Using checked_div with proper error propagation is safer and aligns with the project's arithmetic safety requirements.

Proposed refactor with checked_div
-fn is_high_error_rate(error_events: u64, total_events: u64) -> bool {
+fn is_high_error_rate(error_events: u64, total_events: u64) -> Result<bool, Error> {
     if total_events < ERROR_RATE_MIN_SAMPLE || total_events == 0 {
-        return false;
+        return Ok(false);
     }
-    (error_events.saturating_mul(100) / total_events) >= ERROR_RATE_ALERT_PERCENT
+    let rate = error_events
+        .saturating_mul(100)
+        .checked_div(total_events)
+        .ok_or(Error::Overflow)?;
+    Ok(rate >= ERROR_RATE_ALERT_PERCENT)
 }

This would require updating callers (evaluate_health) to handle the Result.

Based on learnings: "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/contract-monitoring/src/lib.rs` around lines 172 - 177, Change
is_high_error_rate to use checked arithmetic and return a Result instead of
doing unchecked division: replace the saturating_mul + `/` with checked_mul and
checked_div on error_events and total_events, return an
Err(ContractError::Overflow) (or the contract's equivalent overflow variant) if
any checked operation returns None, otherwise Ok(bool) with the comparison to
ERROR_RATE_ALERT_PERCENT; update any callers such as evaluate_health to handle
the Result (propagate the error or map it to the contract error path) so
unchecked division is removed and arithmetic safety is enforced.

173-174: Minor: Redundant zero check.

The condition total_events == 0 is redundant since total_events < ERROR_RATE_MIN_SAMPLE (where ERROR_RATE_MIN_SAMPLE = 10) already covers the zero case.

Simplify condition
-    if total_events < ERROR_RATE_MIN_SAMPLE || total_events == 0 {
+    if total_events < ERROR_RATE_MIN_SAMPLE {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@contracts/contract-monitoring/src/lib.rs` around lines 173 - 174, The if
condition redundantly checks total_events == 0; update the check that guards
early return to just use total_events < ERROR_RATE_MIN_SAMPLE (where
ERROR_RATE_MIN_SAMPLE = 10) so the zero case is covered implicitly; locate the
early-return branch that references total_events and ERROR_RATE_MIN_SAMPLE and
remove the redundant equality check to simplify the condition.
docs/contracts/contract-monitoring.md (1)

5-9: Consider adding threshold values for consistency with README.

The README specifies the exact thresholds (>= 3 for failed settlements, >= 20% for error rate with minimum 10 events), but this documentation omits them. Adding the thresholds would make this file more self-contained and useful for dashboard implementers.

Proposed enhancement
 ## Health Flags

-- `failed_settlement_alert` when failed settlements reach threshold.
-- `high_error_rate` when error ratio crosses threshold.
+- `failed_settlement_alert` when failed settlements >= 3.
+- `high_error_rate` when error ratio >= 20% (requires at least 10 events).
 - `paused` when the system is paused.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/contracts/contract-monitoring.md` around lines 5 - 9, The Health Flags
list (failed_settlement_alert, high_error_rate, paused) is missing numeric
thresholds referenced in the README; update the contract-monitoring.md to
include the exact threshold rules: state that failed_settlement_alert triggers
when failed settlements >= 3, high_error_rate triggers when error ratio >= 20%
with a minimum of 10 events, and keep paused as is, so implementers and
dashboards can apply the same thresholds used in the README.
frontend/tests/components/v1/ContractActionButton.test.tsx (1)

26-40: Add explicit coverage for unsupported-network precondition.

You already test wallet gating; add a companion test for networkSupported={false} to cover the second guard branch and message.

🧪 Suggested test addition
+  it('blocks when network is not supported', () => {
+    const action = jest.fn().mockResolvedValue({});
+
+    render(
+      <ContractActionButton
+        label="Execute"
+        action={action}
+        walletConnected={true}
+        networkSupported={false}
+      />,
+    );
+
+    expect(screen.getByTestId('contract-action-button')).toBeDisabled();
+    expect(screen.getByTestId('contract-action-button-precondition')).toHaveTextContent('supported network');
+  });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/tests/components/v1/ContractActionButton.test.tsx` around lines 26 -
40, Add a new test alongside the existing wallet-gating test that renders
ContractActionButton with networkSupported={false} (and walletConnected={true})
to exercise the unsupported-network guard; verify the button
(getByTestId('contract-action-button')) is disabled and that
getByTestId('contract-action-button-precondition') contains the component's
unsupported-network message string (match the exact text the
ContractActionButton uses for the unsupported-network branch).
frontend/tests/components/v1/AsyncStateBoundary.test.tsx (1)

5-69: Add branch tests for idle and isEmpty callback paths.

Current suite is solid, but these two branches are part of the public behavior and should be locked down as edge-case coverage.

🧪 Suggested tests
+  it('renders idle branch (default null)', () => {
+    render(
+      <AsyncStateBoundary
+        status="idle"
+        renderSuccess={() => <div>ok</div>}
+      />,
+    );
+
+    expect(screen.queryByTestId('async-state-boundary-loading')).not.toBeInTheDocument();
+    expect(screen.queryByTestId('async-state-boundary-error')).not.toBeInTheDocument();
+  });
+
+  it('renders empty when isEmpty returns true', () => {
+    render(
+      <AsyncStateBoundary
+        status="success"
+        data={[] as string[]}
+        isEmpty={(items) => items.length === 0}
+        renderSuccess={() => <div>ok</div>}
+      />,
+    );
+
+    expect(screen.getByTestId('async-state-boundary-empty')).toBeInTheDocument();
+  });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/tests/components/v1/AsyncStateBoundary.test.tsx` around lines 5 -
69, Add two tests to AsyncStateBoundary.test.tsx: one that renders
<AsyncStateBoundary status="idle" renderSuccess={() => <div>ok</div>} /> and
asserts the loading UI is shown (expect
screen.getByTestId('async-state-boundary-loading')...). The second should
exercise the isEmpty callback by rendering <AsyncStateBoundary status="success"
data={{...}} isEmpty={() => true} renderSuccess={(d) => <div>{d.id}</div>} />
and asserting the empty UI is shown (expect
screen.getByTestId('async-state-boundary-empty')...); use jest.fn where needed
and mirror the style of the existing tests for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@backend/src/services/contractMonitoring.service.js`:
- Around line 11-16: The seenEventIds Set in the ContractMonitoringService
constructor grows unbounded; add a bounded cache or pruning strategy (e.g.,
introduce this.maxSeenEventIds in the constructor and replace seenEventIds with
an eviction policy) and update the ingestEvent method to evict oldest entries
when at capacity or to store timestamps and periodically prune entries older
than a sliding window; reference the ContractMonitoringService constructor,
seenEventIds, ingestEvent and add a configurable maxSeenEventIds and
eviction/prune logic to prevent unbounded memory growth.

In `@contracts/contract-monitoring/src/lib.rs`:
- Around line 179-203: Add integration tests that use soroban-sdk::testutils to
instantiate and exercise the contract's public entry points (init, ingest_event,
set_paused, get_metrics, get_health) instead of only unit tests for internal
helpers like Metrics, apply_event, and EventKind; create an Env via
Env::default(), register the contract with env.register(), use the generated
client to call each entry point with proper auth mocking, assert emitted events
and returned state (metrics totals, paused flag, health status), and include
tests that exercise authorization checks for set_paused and ingest_event to
mirror patterns used in streak-bonus/src/test.rs.

In `@contracts/gas-optimization-analysis/src/lib.rs`:
- Around line 97-115: get_hotspots currently iterates methods in insertion order
and pushes the first hotspots found into out until reaching max, so it doesn't
return the top-scoring MethodHotspot items; change get_hotspots to collect all
candidate hotspots by calling Self::get_method_profile(env.clone(),
method.clone()) for each method into a temporary Vec<MethodHotspot>, then sort
that Vec by score descending (or implement a small insertion-sort if std
alloc/sort isn't available) and finally truncate/keep only the top max entries
before returning; reference get_hotspots, MethodHotspot, get_method_profile,
DataKey::Methods, limit and out when locating and replacing the current
loop-based selection logic.
- Around line 6-14: The Error enum is missing an Overflow variant required for
safe arithmetic handling; add a new variant (e.g., Overflow = 5) to the pub enum
Error (keeping #[repr(u32)] and derive attributes) and update any places that
perform arithmetic (functions using checked_add/checked_div) to map None results
to Error::Overflow (e.g., return Err(Error::Overflow.into()) or propagate via ?
after converting) so all checked arithmetic paths return this explicit error
variant.
- Around line 80-83: Replace the silent saturating additions with checked
arithmetic and return an explicit Overflow error on failure: where
profile.calls, profile.total_cpu, profile.total_read_bytes, and
profile.total_write_bytes are updated, use checked_add(...) for each
accumulation and propagate or return the contract's Overflow error variant if
any checked_add returns None; ensure you update the surrounding function (the
caller that updates profile) to convert the None into the Overflow error so
accounting invariants are preserved.

In `@frontend/src/components/v1/AsyncStateBoundary.md`:
- Around line 3-15: The docs for AsyncStateBoundary are missing the explicit
empty-state lifecycle branch; update the lifecycle description to list `idle`,
`loading`, `success`, `empty`, and `error`, and add an example snippet showing
usage of `renderEmpty` and `isEmpty` (e.g. include `renderEmpty={() => <p>No
items</p>}` and `isEmpty={(d) => !d || d.length === 0}` alongside the existing
props like `status`, `data`, `error`, `onRetry`, `renderLoading`, and
`renderSuccess`) so the README reflects the full public API of
AsyncStateBoundary.

In `@frontend/src/components/v1/AsyncStateBoundary.tsx`:
- Around line 43-55: The retry callback is wired directly to UI handlers
(onRetry) which can return a rejected Promise and cause unhandled rejections;
create a local wrapper (e.g., handleRetry) inside AsyncStateBoundary that calls
onRetry() via await or Promise.resolve(), catches any thrown/rejected value, and
routes it into the component's error path (for example by calling the
component's existing error setter/state update or invoking the same error
handler used when safeStatus === 'error'), then use handleRetry in the JSX
(replace direct references to onRetry at the Retry button and in renderError
invocation) so all rejections are caught and handled.

In `@frontend/src/components/v1/ContractActionButton.tsx`:
- Around line 59-66: The current try/catch wraps both the core action and the
onSuccess/onError callbacks so a rejection from onSuccess gets misclassified as
an action failure and a rejection from onError can escape; change the flow in
the function using action, onSuccess, onError, toAppError, and setError so that
you only try/catch the action itself (await action()), map any action error with
toAppError and call setError, then call onError(mapped) inside its own try/catch
to prevent unhandled rejections; likewise call onSuccess(result) in a separate
try/catch so its rejection is not remapped into an action error. Ensure awaits
remain and that callback errors are handled/logged locally without altering
action error semantics.

---

Nitpick comments:
In `@backend/src/services/contractMonitoring.service.js`:
- Line 78: The current export of a singleton instance (module.exports = new
ContractMonitoringService()) prevents test isolation; change the module to
export the ContractMonitoringService class itself and also export a
default/legacy singleton or provide a factory/reset function so tests can
instantiate fresh ContractMonitoringService instances (or call resetFactory())
while production code can continue to use the existing singleton; update
references to the exported symbol accordingly (ContractMonitoringService, the
exported instance/factory/reset).
- Around line 26-41: The switch on the incoming event "kind" in
contractMonitoring.service (the switch that updates
this.metrics.settlementSuccess/settlementFailed/errorEvents/pausedEvents)
silently ignores unknown kinds; update the default branch to record or surface
unexpected values by logging a warning with the unknown kind (e.g., via
this.logger.warn or console.warn) including the raw event payload and/or
timestamp for observability, and optionally throw or emit an error if you want
strict validation (make this behavior configurable). Locate the switch handling
"kind" in the method that processes contract events and modify the default case
to log the unrecognized kind and context (or raise an error) rather than doing
nothing.

In `@backend/src/utils/contractMonitoringAlerts.js`:
- Around line 5-11: The guard clause in isHighErrorRate redundantly checks
totalEvents === 0 because totalEvents < HIGH_ERROR_RATE_MIN_SAMPLE already
covers zero; simplify the condition by removing the === 0 check so the function
only tests totalEvents < HIGH_ERROR_RATE_MIN_SAMPLE (using the existing
HIGH_ERROR_RATE_MIN_SAMPLE constant) before computing the percentage against
HIGH_ERROR_RATE_PERCENT.

In `@contracts/contract-monitoring/src/lib.rs`:
- Around line 11-19: The Error enum lacks an Overflow variant required for safe
arithmetic; add an Overflow variant to the pub enum Error (which is referenced
by functions like is_high_error_rate that use checked_div/checked_add) and
update any places that map arithmetic failures to Error::Overflow so checked_*
failures return this variant instead of panicking or returning a generic error;
ensure the new variant follows the existing #[repr(u32)] ordering and derives so
it integrates with the contracterror macro and error handling paths.
- Around line 172-177: Change is_high_error_rate to use checked arithmetic and
return a Result instead of doing unchecked division: replace the saturating_mul
+ `/` with checked_mul and checked_div on error_events and total_events, return
an Err(ContractError::Overflow) (or the contract's equivalent overflow variant)
if any checked operation returns None, otherwise Ok(bool) with the comparison to
ERROR_RATE_ALERT_PERCENT; update any callers such as evaluate_health to handle
the Result (propagate the error or map it to the contract error path) so
unchecked division is removed and arithmetic safety is enforced.
- Around line 173-174: The if condition redundantly checks total_events == 0;
update the check that guards early return to just use total_events <
ERROR_RATE_MIN_SAMPLE (where ERROR_RATE_MIN_SAMPLE = 10) so the zero case is
covered implicitly; locate the early-return branch that references total_events
and ERROR_RATE_MIN_SAMPLE and remove the redundant equality check to simplify
the condition.

In `@contracts/gas-optimization-analysis/README.md`:
- Around line 20-22: Update the README statement "All counters use saturating
arithmetic" to reflect the new implementation in lib.rs: mention that counters
use checked arithmetic (e.g., checked_add) and will return explicit Overflow
errors on overflow; reference the counters/metrics behavior and the Overflow
error type so readers know to expect checked arithmetic semantics and error
handling rather than saturating behavior.

In `@contracts/gas-optimization-analysis/src/lib.rs`:
- Around line 54-62: The init function currently sets Admin and Methods but
doesn't emit an event; update init (the function named init) to call
env.events().publish() after successful initialization to emit an initialization
event (include relevant data such as DataKey::Admin/Address and any metadata),
so observers can detect the contract deployment/state change; use the contract
events API (env.events().publish or #[contractevent] if preferred) and ensure
the publish call occurs after env.storage().instance().set(...) and before
returning Ok(()).

In `@docs/contracts/contract-monitoring.md`:
- Around line 5-9: The Health Flags list (failed_settlement_alert,
high_error_rate, paused) is missing numeric thresholds referenced in the README;
update the contract-monitoring.md to include the exact threshold rules: state
that failed_settlement_alert triggers when failed settlements >= 3,
high_error_rate triggers when error ratio >= 20% with a minimum of 10 events,
and keep paused as is, so implementers and dashboards can apply the same
thresholds used in the README.

In `@frontend/tests/components/v1/AsyncStateBoundary.test.tsx`:
- Around line 5-69: Add two tests to AsyncStateBoundary.test.tsx: one that
renders <AsyncStateBoundary status="idle" renderSuccess={() => <div>ok</div>} />
and asserts the loading UI is shown (expect
screen.getByTestId('async-state-boundary-loading')...). The second should
exercise the isEmpty callback by rendering <AsyncStateBoundary status="success"
data={{...}} isEmpty={() => true} renderSuccess={(d) => <div>{d.id}</div>} />
and asserting the empty UI is shown (expect
screen.getByTestId('async-state-boundary-empty')...); use jest.fn where needed
and mirror the style of the existing tests for consistency.

In `@frontend/tests/components/v1/ContractActionButton.test.tsx`:
- Around line 26-40: Add a new test alongside the existing wallet-gating test
that renders ContractActionButton with networkSupported={false} (and
walletConnected={true}) to exercise the unsupported-network guard; verify the
button (getByTestId('contract-action-button')) is disabled and that
getByTestId('contract-action-button-precondition') contains the component's
unsupported-network message string (match the exact text the
ContractActionButton uses for the unsupported-network branch).

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 869183a and c59d5e0.

📒 Files selected for processing (18)
  • backend/src/services/contractMonitoring.service.js
  • backend/src/utils/contractMonitoringAlerts.js
  • backend/tests/unit/contractMonitoringAlerts.test.js
  • contracts/contract-monitoring/Cargo.toml
  • contracts/contract-monitoring/README.md
  • contracts/contract-monitoring/src/lib.rs
  • contracts/gas-optimization-analysis/Cargo.toml
  • contracts/gas-optimization-analysis/README.md
  • contracts/gas-optimization-analysis/src/lib.rs
  • docs/contracts/contract-monitoring.md
  • docs/contracts/gas-optimization-analysis.md
  • frontend/src/components/v1/AsyncStateBoundary.md
  • frontend/src/components/v1/AsyncStateBoundary.tsx
  • frontend/src/components/v1/ContractActionButton.md
  • frontend/src/components/v1/ContractActionButton.tsx
  • frontend/src/components/v1/index.ts
  • frontend/tests/components/v1/AsyncStateBoundary.test.tsx
  • frontend/tests/components/v1/ContractActionButton.test.tsx

Comment on lines +11 to +16
class ContractMonitoringService {
constructor() {
this.metrics = { ...INITIAL_METRICS };
this.paused = false;
this.seenEventIds = new Set();
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Unbounded memory growth in seenEventIds Set.

The seenEventIds Set grows indefinitely as events are ingested. For a long-running service, this will eventually exhaust memory. Consider implementing a bounded cache (e.g., LRU with a max size) or periodic pruning strategy.

🛡️ Example approach using a sliding window
// Option 1: Use a Map with timestamps and periodically prune old entries
// Option 2: Use a bounded Set that evicts oldest entries when full
// Option 3: If event IDs are time-ordered, keep only IDs from last N minutes

constructor() {
  this.metrics = { ...INITIAL_METRICS };
  this.paused = false;
  this.seenEventIds = new Set();
  this.maxSeenEventIds = 100000; // Configure based on expected volume
}

ingestEvent({ eventId, kind }) {
  if (this.seenEventIds.has(eventId)) {
    throw new Error(`Duplicate event id: ${eventId}`);
  }

  // Prune if at capacity (simple FIFO approximation)
  if (this.seenEventIds.size >= this.maxSeenEventIds) {
    const oldest = this.seenEventIds.values().next().value;
    this.seenEventIds.delete(oldest);
  }

  this.seenEventIds.add(eventId);
  // ...
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/src/services/contractMonitoring.service.js` around lines 11 - 16, The
seenEventIds Set in the ContractMonitoringService constructor grows unbounded;
add a bounded cache or pruning strategy (e.g., introduce this.maxSeenEventIds in
the constructor and replace seenEventIds with an eviction policy) and update the
ingestEvent method to evict oldest entries when at capacity or to store
timestamps and periodically prune entries older than a sliding window; reference
the ContractMonitoringService constructor, seenEventIds, ingestEvent and add a
configurable maxSeenEventIds and eviction/prune logic to prevent unbounded
memory growth.

Comment on lines +179 to +203
#[cfg(test)]
mod tests {
use super::*;

#[test]
fn marks_error_rate_when_threshold_crossed() {
assert!(!is_high_error_rate(1, 5));
assert!(!is_high_error_rate(1, 10));
assert!(is_high_error_rate(2, 10));
assert!(is_high_error_rate(3, 10));
}

#[test]
fn applies_event_counts_deterministically() {
let mut metrics = Metrics::default();
apply_event(&mut metrics, &EventKind::SettlementSuccess);
apply_event(&mut metrics, &EventKind::SettlementFailed);
apply_event(&mut metrics, &EventKind::Error);

assert_eq!(metrics.total_events, 3);
assert_eq!(metrics.settlement_success, 1);
assert_eq!(metrics.settlement_failed, 1);
assert_eq!(metrics.error_events, 1);
}
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if there are integration tests elsewhere or examples to reference
fd -e rs . contracts/ --exec grep -l "soroban_sdk::testutils\|register_contract" {} \;

Repository: TheBlockCade/StellarCade

Length of output: 409


🏁 Script executed:

# First, let's check the structure of the contract-monitoring file
wc -l contracts/contract-monitoring/src/lib.rs

Repository: TheBlockCade/StellarCade

Length of output: 112


🏁 Script executed:

# Get the full content of the contract file to understand entry points
cat -n contracts/contract-monitoring/src/lib.rs

Repository: TheBlockCade/StellarCade

Length of output: 7535


🏁 Script executed:

# Verify the pattern - check one of the contracts that does use soroban-sdk testutils
head -50 contracts/streak-bonus/src/test.rs

Repository: TheBlockCade/StellarCade

Length of output: 1685


Add integration tests using soroban-sdk testutils for public entry points.

The contract lacks integration tests that instantiate the contract and test the public entry points (init, ingest_event, set_paused, get_metrics, get_health) using Soroban's test environment. Per coding guidelines, smart contract tests must use soroban-sdk::testutils to properly verify authorization checks, event emissions, and contract state interactions.

Reference the pattern in other contracts (e.g., streak-bonus/src/test.rs) which uses Env::default(), env.register() to instantiate the contract, and a generated client to call entry points with proper auth mocking.

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

In `@contracts/contract-monitoring/src/lib.rs` around lines 179 - 203, Add
integration tests that use soroban-sdk::testutils to instantiate and exercise
the contract's public entry points (init, ingest_event, set_paused, get_metrics,
get_health) instead of only unit tests for internal helpers like Metrics,
apply_event, and EventKind; create an Env via Env::default(), register the
contract with env.register(), use the generated client to call each entry point
with proper auth mocking, assert emitted events and returned state (metrics
totals, paused flag, health status), and include tests that exercise
authorization checks for set_paused and ingest_event to mirror patterns used in
streak-bonus/src/test.rs.

Comment on lines +6 to +14
#[contracterror]
#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)]
#[repr(u32)]
pub enum Error {
AlreadyInitialized = 1,
NotInitialized = 2,
NotAuthorized = 3,
InvalidMetric = 4,
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Missing Overflow error variant for safe arithmetic.

Per coding guidelines and retrieved learnings, Soroban contracts should use checked_add/checked_div with an explicit Overflow error variant. The current Error enum lacks this variant.

🛠️ Suggested addition
 #[contracterror]
 #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)]
 #[repr(u32)]
 pub enum Error {
     AlreadyInitialized = 1,
     NotInitialized = 2,
     NotAuthorized = 3,
     InvalidMetric = 4,
+    Overflow = 5,
 }

Based on learnings: "Use safe arithmetic operations (checked_add, checked_div) with explicit Overflow error variants in Soroban contracts."

📝 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
#[contracterror]
#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)]
#[repr(u32)]
pub enum Error {
AlreadyInitialized = 1,
NotInitialized = 2,
NotAuthorized = 3,
InvalidMetric = 4,
}
#[contracterror]
#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)]
#[repr(u32)]
pub enum Error {
AlreadyInitialized = 1,
NotInitialized = 2,
NotAuthorized = 3,
InvalidMetric = 4,
Overflow = 5,
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@contracts/gas-optimization-analysis/src/lib.rs` around lines 6 - 14, The
Error enum is missing an Overflow variant required for safe arithmetic handling;
add a new variant (e.g., Overflow = 5) to the pub enum Error (keeping
#[repr(u32)] and derive attributes) and update any places that perform
arithmetic (functions using checked_add/checked_div) to map None results to
Error::Overflow (e.g., return Err(Error::Overflow.into()) or propagate via ?
after converting) so all checked arithmetic paths return this explicit error
variant.

Comment on lines +80 to +83
profile.calls = profile.calls.saturating_add(1);
profile.total_cpu = profile.total_cpu.saturating_add(cpu);
profile.total_read_bytes = profile.total_read_bytes.saturating_add(read_bytes);
profile.total_write_bytes = profile.total_write_bytes.saturating_add(write_bytes);
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Use checked_add instead of saturating_add for explicit overflow handling.

saturating_add silently caps at u64::MAX, hiding potential overflow conditions. Per coding guidelines, use checked_add and return an explicit Overflow error to maintain accounting invariants.

🛠️ Suggested fix
-        profile.calls = profile.calls.saturating_add(1);
-        profile.total_cpu = profile.total_cpu.saturating_add(cpu);
-        profile.total_read_bytes = profile.total_read_bytes.saturating_add(read_bytes);
-        profile.total_write_bytes = profile.total_write_bytes.saturating_add(write_bytes);
+        profile.calls = profile.calls.checked_add(1).ok_or(Error::Overflow)?;
+        profile.total_cpu = profile.total_cpu.checked_add(cpu).ok_or(Error::Overflow)?;
+        profile.total_read_bytes = profile.total_read_bytes.checked_add(read_bytes).ok_or(Error::Overflow)?;
+        profile.total_write_bytes = profile.total_write_bytes.checked_add(write_bytes).ok_or(Error::Overflow)?;

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

📝 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
profile.calls = profile.calls.saturating_add(1);
profile.total_cpu = profile.total_cpu.saturating_add(cpu);
profile.total_read_bytes = profile.total_read_bytes.saturating_add(read_bytes);
profile.total_write_bytes = profile.total_write_bytes.saturating_add(write_bytes);
profile.calls = profile.calls.checked_add(1).ok_or(Error::Overflow)?;
profile.total_cpu = profile.total_cpu.checked_add(cpu).ok_or(Error::Overflow)?;
profile.total_read_bytes = profile.total_read_bytes.checked_add(read_bytes).ok_or(Error::Overflow)?;
profile.total_write_bytes = profile.total_write_bytes.checked_add(write_bytes).ok_or(Error::Overflow)?;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@contracts/gas-optimization-analysis/src/lib.rs` around lines 80 - 83, Replace
the silent saturating additions with checked arithmetic and return an explicit
Overflow error on failure: where profile.calls, profile.total_cpu,
profile.total_read_bytes, and profile.total_write_bytes are updated, use
checked_add(...) for each accumulation and propagate or return the contract's
Overflow error variant if any checked_add returns None; ensure you update the
surrounding function (the caller that updates profile) to convert the None into
the Overflow error so accounting invariants are preserved.

Comment on lines +97 to +115
pub fn get_hotspots(env: Env, limit: u32) -> Vec<MethodHotspot> {
let methods: Vec<Symbol> = env.storage().instance().get(&DataKey::Methods).unwrap_or(vec![&env]);
let mut out = vec![&env];
let max = if limit == 0 { methods.len() } else { core::cmp::min(limit, methods.len()) };

let mut i = 0;
while i < methods.len() && out.len() < max {
let method = methods.get(i).unwrap();
let profile = Self::get_method_profile(env.clone(), method.clone());
if profile.calls > 0 {
let avg_cpu = profile.total_cpu / profile.calls;
let score = avg_cpu.saturating_add(profile.total_write_bytes / profile.calls);
out.push_back(MethodHotspot { method, score, avg_cpu });
}
i += 1;
}

out
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Hotspot retrieval may not return the highest-scoring methods.

The loop iterates through methods in insertion order and stops when out.len() >= max, but doesn't sort by score. This means the returned hotspots may not be the actual top limit hotspots by score.

🔧 Suggested approach

Either:

  1. Document that results are not sorted by score (if intentional)
  2. Collect all hotspots first, then sort by score descending before truncating to limit

Since sorting in no_std without alloc is limited in Soroban, if sorting is desired, consider using a simple insertion-sort approach or maintaining a sorted structure during collection.

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

In `@contracts/gas-optimization-analysis/src/lib.rs` around lines 97 - 115,
get_hotspots currently iterates methods in insertion order and pushes the first
hotspots found into out until reaching max, so it doesn't return the top-scoring
MethodHotspot items; change get_hotspots to collect all candidate hotspots by
calling Self::get_method_profile(env.clone(), method.clone()) for each method
into a temporary Vec<MethodHotspot>, then sort that Vec by score descending (or
implement a small insertion-sort if std alloc/sort isn't available) and finally
truncate/keep only the top max entries before returning; reference get_hotspots,
MethodHotspot, get_method_profile, DataKey::Methods, limit and out when locating
and replacing the current loop-based selection logic.

Comment on lines +3 to +15
Shared renderer for async lifecycle branches (`idle`, `loading`, `success`, `error`).

## Example

```tsx
<AsyncStateBoundary
status={status}
data={data}
error={error}
onRetry={reload}
renderLoading={() => <p>Loading...</p>}
renderSuccess={(items) => <ItemList items={items} />}
/>
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Document the empty-state branch explicitly.

The component supports renderEmpty/isEmpty, but the lifecycle description and example omit that branch. Add one empty-state usage snippet so the docs reflect the full public API.

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

In `@frontend/src/components/v1/AsyncStateBoundary.md` around lines 3 - 15, The
docs for AsyncStateBoundary are missing the explicit empty-state lifecycle
branch; update the lifecycle description to list `idle`, `loading`, `success`,
`empty`, and `error`, and add an example snippet showing usage of `renderEmpty`
and `isEmpty` (e.g. include `renderEmpty={() => <p>No items</p>}` and
`isEmpty={(d) => !d || d.length === 0}` alongside the existing props like
`status`, `data`, `error`, `onRetry`, `renderLoading`, and `renderSuccess`) so
the README reflects the full public API of AsyncStateBoundary.

Comment on lines +43 to +55
if (safeStatus === 'error') {
if (renderError) {
return <>{renderError({ error, retry: onRetry })}</>;
}

return (
<div data-testid={`${testId}-error`}>
<p>Something went wrong.</p>
{onRetry && (
<button type="button" onClick={onRetry} data-testid={`${testId}-retry`}>
Retry
</button>
)}
Copy link

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:

find frontend -name "AsyncStateBoundary.tsx" -type f

Repository: TheBlockCade/StellarCade

Length of output: 117


🏁 Script executed:

cat -n frontend/src/components/v1/AsyncStateBoundary.tsx

Repository: TheBlockCade/StellarCade

Length of output: 2539


Guard retry callback execution before wiring it to UI handlers.

The onRetry callback signature (line 8) allows () => void | Promise<void>, meaning it can return a rejected Promise. At line 45 and 52, onRetry is passed directly without wrapping, causing any promise rejections to surface as unhandled rejections instead of controlled error paths.

🔧 Proposed fix
 export function AsyncStateBoundary<T, E = unknown>({
@@
 }: AsyncStateBoundaryProps<T, E>) {
   const safeStatus: AsyncStatus = VALID_STATUS.includes(status) ? status : 'idle';
+  const handleRetry = () => {
+    if (!onRetry) return;
+    void Promise.resolve(onRetry()).catch(() => {
+      // Keep boundary stable if consumer retry handler fails.
+    });
+  };
@@
   if (safeStatus === 'error') {
     if (renderError) {
-      return <>{renderError({ error, retry: onRetry })}</>;
+      return <>{renderError({ error, retry: handleRetry })}</>;
     }
@@
         <p>Something went wrong.</p>
         {onRetry && (
-          <button type="button" onClick={onRetry} data-testid={`${testId}-retry`}>
+          <button type="button" onClick={handleRetry} data-testid={`${testId}-retry`}>
             Retry
           </button>
         )}
       </div>
     );
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/src/components/v1/AsyncStateBoundary.tsx` around lines 43 - 55, The
retry callback is wired directly to UI handlers (onRetry) which can return a
rejected Promise and cause unhandled rejections; create a local wrapper (e.g.,
handleRetry) inside AsyncStateBoundary that calls onRetry() via await or
Promise.resolve(), catches any thrown/rejected value, and routes it into the
component's error path (for example by calling the component's existing error
setter/state update or invoking the same error handler used when safeStatus ===
'error'), then use handleRetry in the JSX (replace direct references to onRetry
at the Retry button and in renderError invocation) so all rejections are caught
and handled.

Comment on lines +59 to +66
try {
const result = await action();
await onSuccess?.(result);
} catch (err) {
const mapped = toAppError(err);
setError(mapped);
await onError?.(mapped);
} finally {
Copy link

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:

# First, locate and read the file
fd -t f "ContractActionButton.tsx" && wc -l frontend/src/components/v1/ContractActionButton.tsx

Repository: TheBlockCade/StellarCade

Length of output: 174


🏁 Script executed:

# Read the file around lines 59-66 with broader context (40-80)
head -80 frontend/src/components/v1/ContractActionButton.tsx | tail -40

Repository: TheBlockCade/StellarCade

Length of output: 980


🏁 Script executed:

# Check the full handleClick function
sed -n '50,100p' frontend/src/components/v1/ContractActionButton.tsx

Repository: TheBlockCade/StellarCade

Length of output: 1004


🏁 Script executed:

# Read the full file to see prop types
cat -n frontend/src/components/v1/ContractActionButton.tsx

Repository: TheBlockCade/StellarCade

Length of output: 3074


🏁 Script executed:

# Search for type definitions related to callbacks
rg -B5 -A5 "onSuccess|onError" frontend/src/components/v1/ContractActionButton.tsx

Repository: TheBlockCade/StellarCade

Length of output: 1031


Separate action failure handling from callback failure handling.

At line 61, a thrown/rejected onSuccess callback is currently caught and remapped as an action failure, misclassifying callback errors. At line 65, a thrown/rejected onError callback can escape as an unhandled promise rejection since it lies outside the try/catch block. Both callbacks explicitly support Promise<void> return types (lines 13-14), making rejection possible. Guard callbacks independently so action error semantics remain deterministic and callback failures do not cause unhandled rejections.

🔧 Proposed fix
 export function ContractActionButton<T = unknown>({
@@
 }: ContractActionButtonProps<T>) {
@@
+  const invokeSafely = async <A extends unknown[]>(
+    callback: ((...args: A) => void | Promise<void>) | undefined,
+    ...args: A
+  ) => {
+    if (!callback) return;
+    try {
+      await callback(...args);
+    } catch {
+      // Keep UI flow stable if consumer callback fails.
+    }
+  };
+
   const handleClick = async () => {
@@
     try {
       const result = await action();
-      await onSuccess?.(result);
+      await invokeSafely(onSuccess, result);
     } catch (err) {
       const mapped = toAppError(err);
       setError(mapped);
-      await onError?.(mapped);
+      await invokeSafely(onError, mapped);
     } finally {
       setIsLoading(false);
     }
   };
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/src/components/v1/ContractActionButton.tsx` around lines 59 - 66,
The current try/catch wraps both the core action and the onSuccess/onError
callbacks so a rejection from onSuccess gets misclassified as an action failure
and a rejection from onError can escape; change the flow in the function using
action, onSuccess, onError, toAppError, and setError so that you only try/catch
the action itself (await action()), map any action error with toAppError and
call setError, then call onError(mapped) inside its own try/catch to prevent
unhandled rejections; likewise call onSuccess(result) in a separate try/catch so
its rejection is not remapped into an action error. Ensure awaits remain and
that callback errors are handled/logged locally without altering action error
semantics.

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

Labels

None yet

Projects

None yet

2 participants