Skip to content

Conversation

@icecrasher321
Copy link
Collaborator

Summary

Dollars based outputs are synthetically constructed. We shouldn't be doing this and don't do it for any other integration.

portfolioBalanceDollars didn't resolve because of separate issue too.

Type of Change

  • Bug fix

Testing

Tested manually

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link

vercel bot commented Jan 4, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Review Updated (UTC)
docs Skipped Skipped Jan 4, 2026 1:40am

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 4, 2026

Greptile Summary

Removed synthetically constructed dollar-based outputs (balanceDollars, portfolioValueDollars) from the Kalshi balance tool to align with other integrations that return raw API values. Changed portfolioValue from optional to required with a ?? 0 fallback.

Key changes:

  • Removed balanceDollars and portfolioValueDollars outputs from response interface and outputs definition
  • Changed nullish coalescing for both balance and portfolioValue from || to ?? (more precise handling)
  • Made portfolioValue required in response type (was optional before)

Issues found:

  • The API still returns portfolio_value as optional (per KalshiBalance interface), but the response type now requires it. Consumers expecting undefined will now get 0 instead when the API doesn't return this field.
  • Documentation files in apps/docs/content/docs/*/tools/kalshi.mdx still reference the removed balanceDollars and portfolioValueDollars outputs (lines 129-130 in EN docs) and need to be updated.

Confidence Score: 3/5

  • PR is mostly safe but has a semantic change and outdated documentation
  • The removal of synthetic outputs is architecturally sound and aligns with other integrations, but changing portfolioValue from optional to required (with 0 fallback) alters behavior when the API doesn't return this field. Documentation also needs updating.
  • Pay attention to apps/sim/tools/kalshi/get_balance.ts for the semantic change in portfolioValue handling, and update documentation files that still reference removed outputs

Important Files Changed

Filename Overview
apps/sim/tools/kalshi/get_balance.ts Removed synthetic dollar-based outputs, standardized to return only cents values from API; changed optional portfolioValue to required with ?? 0 fallback

Sequence Diagram

sequenceDiagram
    participant User
    participant Tool as kalshi_get_balance
    participant API as Kalshi API
    participant Transform as transformResponse
    
    User->>Tool: Request balance (keyId, privateKey)
    Tool->>API: GET /trade-api/v2/portfolio/balance
    Note over Tool,API: Auth headers with RSA signature
    API-->>Transform: Response { balance, portfolio_value? }
    Note over Transform: balance ?? 0
    Note over Transform: portfolio_value ?? 0
    Transform-->>User: { balance: number, portfolioValue: number }
    Note over User: All values in cents (no dollar conversion)
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@icecrasher321 icecrasher321 merged commit ac94241 into staging Jan 4, 2026
6 checks passed
@waleedlatif1 waleedlatif1 deleted the fix/kalshi-outputs branch January 4, 2026 20:50
waleedlatif1 pushed a commit that referenced this pull request Jan 4, 2026
* fix(kalshi): remove synthetically constructed outputs

* fix api interface
waleedlatif1 pushed a commit that referenced this pull request Jan 8, 2026
* fix(kalshi): remove synthetically constructed outputs

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants