Add pause_stream / resume_stream backend API endpoints wired to Soroban (#324) - #662
Add pause_stream / resume_stream backend API endpoints wired to Soroban (#324)#662samjay8 wants to merge 2 commits into
Conversation
… API (ritik4ever#324) - Add on-chain pause_stream transaction in pauseStream() before updating SQLite - Add on-chain resume_stream transaction in resumeStream() before updating SQLite - Both functions retry with exponential backoff via retryWithBackoff() - pausedAt and pausedDuration updated atomically in a DB transaction - Pause/resume events recorded in stream_events via recordEventWithDb() - Cache invalidated and webhooks triggered after state changes - Add integration tests for pause active stream, resume paused stream, wrong-sender rejection - Fix calculateProgress vesting computation to use cleaner ratio/elapsed logic
|
@samjay8 is attempting to deploy a commit to the ritik4ever's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@samjay8 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughPause and resume now submit matching Soroban transactions before local updates. Integration tests cover successful transitions, validation errors, missing streams, authorization, event metadata, event ordering, and test database cleanup. ChangesPause/resume stream flow
Estimated code review effort: 4 (Complex) | ~40 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant StreamStore
participant Soroban
participant SQLite
participant Webhooks
Client->>StreamStore: Request pause or resume
StreamStore->>Soroban: Submit pause_stream or resume_stream
Soroban-->>StreamStore: Return confirmation or failure
StreamStore->>SQLite: Save stream state and event history
StreamStore->>Webhooks: Emit stream update
StreamStore-->>Client: Return stream response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/src/services/streamStore.pauseResume.integration.test.ts (1)
69-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffNo coverage for the new Soroban submission path.
These tests exercise pause/resume success purely through the local DB/API layer; nothing here mocks
getSorobanContext/rpcServer/serverKeypair, so the on-chain block inpauseStream/resumeStreamis presumably skipped entirely via its truthy guard. The PR's core new behavior — submitting and confirming the Soroban transaction before the local update — is therefore never actually exercised, and a regression like the swallowed-failure issue flagged instreamStore.tswould go undetected here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/services/streamStore.pauseResume.integration.test.ts` around lines 69 - 123, Extend the pause/resume integration tests to mock and exercise the Soroban submission dependencies used by pauseStream and resumeStream, including getSorobanContext, rpcServer, and serverKeypair. Assert that each operation submits and confirms its transaction before validating the local response and history, and add failure coverage verifying a Soroban submission error prevents or reports the local update rather than being swallowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/src/services/streamStore.pauseResume.integration.test.ts`:
- Around line 69-123: Extend the pause/resume integration tests to mock and
exercise the Soroban submission dependencies used by pauseStream and
resumeStream, including getSorobanContext, rpcServer, and serverKeypair. Assert
that each operation submits and confirms its transaction before validating the
local response and history, and add failure coverage verifying a Soroban
submission error prevents or reports the local update rather than being
swallowed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fe5fe993-c6c9-402f-8535-4b93c1c299de
📒 Files selected for processing (2)
backend/src/services/streamStore.pauseResume.integration.test.tsbackend/src/services/streamStore.ts
|
Hi @samjay8, This PR could not be merged because it has merge conflicts with the target branch. Please resolve the merge conflicts, push the updated changes, and the PR can be reviewed and merged. Thank you! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
backend/src/services/streamStore.ts (5)
1217-1219: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winA Soroban failure is downgraded to a warning, so SQLite and the chain diverge. Both handlers catch every submission error, log at
warn, and let execution continue to thedb.transactionblock. The stream is then paused or resumed locally while the contract state is unchanged.retryWithBackoffinbackend/src/utils/sorobanRetry.tsalready throws a typedSorobanSubmitErrorwith a status code after retries are exhausted, so the caller can map the failure to an HTTP response. Issue#324requires the Soroban transaction to be submitted before the SQLite update; swallowing the error defeats that ordering.
backend/src/services/streamStore.ts#L1217-L1219: rethrow the error instead of logging a warning, sopauseStreamdoes not persistpausedAtafter a failedpause_streamtransaction.backend/src/services/streamStore.ts#L1297-L1299: rethrow the error instead of logging a warning, soresumeStreamdoes not persist the updatedpausedDurationanddurationSecondsafter a failedresume_streamtransaction.If a partial failure must stay recoverable, record the stream in a pending-reconciliation state and let
reconcileStreamsettle it, rather than committing an unverified state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/services/streamStore.ts` around lines 1217 - 1219, In backend/src/services/streamStore.ts at lines 1217-1219 and 1297-1299, update the error handlers in pauseStream and resumeStream to rethrow Soroban submission errors instead of logging warnings and continuing to db.transaction. Preserve the existing transaction ordering so SQLite is updated only after the corresponding chain transaction succeeds; use pending reconciliation only if the implementation already supports that path.
1187-1188: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a radix to
parseIntand validate the parsed ID.
parseInt(id)omits the radix.idarrives as a string from the route, so a value such as"0x10"parses as 16, and"12abc"parses as 12.nativeToScValthen encodes au64that does not match the requested stream.NaNis also possible. The same call exists at line 1267 inresumeStream.Validate the ID once with a Zod schema and pass a number to the service.
As per coding guidelines: "apply validation by parsing, transforming, and refining Zod schemas in
backend/src/validation/schemas.tsbefore passing data to services".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/services/streamStore.ts` around lines 1187 - 1188, Update the relevant route validation in backend/src/validation/schemas.ts to parse the stream ID with an explicit decimal radix, transform it to a number, and refine it to reject invalid or out-of-range u64 values before invoking the service. Pass the validated numeric ID through the stream service paths, including the call sites near the existing nativeToScVal usage and resumeStream, eliminating direct parseInt(id) calls and ensuring malformed strings such as hexadecimal, trailing characters, and NaN are rejected.Source: Coding guidelines
1251-1255: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDo not add
durationSecondsduring resume.
resumeStreamadds the paused time to bothpausedDurationanddurationSeconds, whileresume_streamonly shiftsstart_timeandend_timeand does not change the stream duration. KeepingdurationSecondsunchanged makes the local progress calculation keep the original stream rate after resume.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/services/streamStore.ts` around lines 1251 - 1255, Update resumeStream around the paused-duration calculation to stop adding elapsed paused time to stream.durationSeconds. Continue accumulating elapsed in pausedDuration, clear pausedAt, and preserve the existing duration value so progress calculations retain the original stream rate after resume.
800-805: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
MAX(id)+1can produce duplicate stream IDs.Two concurrent
createStreamcalls read the samemaxIdbefore either inserts, so both compute the samenextNumericId. The read and the later insert are not in one transaction. The query also ignoresstream_archive, so an ID can be reused after a row is archived and removed fromstreams.Allocate the ID inside the same transaction as the insert, or use a dedicated sequence table.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/services/streamStore.ts` around lines 800 - 805, Update createStream so numeric stream ID allocation and the subsequent insert occur within one database transaction, preventing concurrent calls from reusing the same MAX(id)+1 value. Ensure allocation considers IDs present in both streams and stream_archive, or use a dedicated sequence table, while preserving the existing stream creation behavior.
811-826: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winResolve the merge-conflict remnant. The file does not parse.
Lines 811-819 open two statements that overlap:
const txToSimulate = new TransactionBuilder(...)andconst built = await rpcServer.prepareTransaction(new TransactionBuilder(...)). Biome reports parse errors at lines 812 and 819. The whole module therefore fails to compile, and every export instreamStore.tsis unusable, including the newpauseStreamandresumeStreamflows.Keep one statement.
txToSimulateis the variable used at lines 821 and 826, andbuiltis never read.🐛 Proposed fix to restore a single build statement
const txToSimulate = new TransactionBuilder(sourceAccount, { - const built = await rpcServer.prepareTransaction( - new TransactionBuilder(sourceAccount, { fee: "1000", networkPassphrase: netPass, }) .addOperation(op) .setTimeout(30) .build();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/services/streamStore.ts` around lines 811 - 826, Remove the merge-conflict remnant in the transaction simulation setup by keeping a single valid TransactionBuilder statement assigned to txToSimulate. Preserve its existing fee, networkPassphrase, operation, timeout, and build configuration, then continue using txToSimulate for simulation and preparation; remove the unused built declaration.Source: Linters/SAST tools
🧹 Nitpick comments (1)
backend/src/services/streamStore.ts (1)
1070-1086: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
getStreamand align the absent-value convention.
getStreamByIdrepeats the query and row mapping already ingetStreamat lines 1056-1062. It also returnsnullwhilegetStreamreturnsundefinedfor the same condition, and it exposesarchived_atin snake_case inside an otherwise camelCase record. Callers must then handle two conventions.Keep the duplicate query only if you need
archived_at, whichrowToRecorddrops.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/services/streamStore.ts` around lines 1070 - 1086, Update getStreamById to reuse getStream for lookup and mapping instead of repeating the database query, return undefined when the stream is absent to match getStream, and preserve archived_at only if it is required by this function, using the established camelCase property convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@backend/src/services/streamStore.ts`:
- Around line 1217-1219: In backend/src/services/streamStore.ts at lines
1217-1219 and 1297-1299, update the error handlers in pauseStream and
resumeStream to rethrow Soroban submission errors instead of logging warnings
and continuing to db.transaction. Preserve the existing transaction ordering so
SQLite is updated only after the corresponding chain transaction succeeds; use
pending reconciliation only if the implementation already supports that path.
- Around line 1187-1188: Update the relevant route validation in
backend/src/validation/schemas.ts to parse the stream ID with an explicit
decimal radix, transform it to a number, and refine it to reject invalid or
out-of-range u64 values before invoking the service. Pass the validated numeric
ID through the stream service paths, including the call sites near the existing
nativeToScVal usage and resumeStream, eliminating direct parseInt(id) calls and
ensuring malformed strings such as hexadecimal, trailing characters, and NaN are
rejected.
- Around line 1251-1255: Update resumeStream around the paused-duration
calculation to stop adding elapsed paused time to stream.durationSeconds.
Continue accumulating elapsed in pausedDuration, clear pausedAt, and preserve
the existing duration value so progress calculations retain the original stream
rate after resume.
- Around line 800-805: Update createStream so numeric stream ID allocation and
the subsequent insert occur within one database transaction, preventing
concurrent calls from reusing the same MAX(id)+1 value. Ensure allocation
considers IDs present in both streams and stream_archive, or use a dedicated
sequence table, while preserving the existing stream creation behavior.
- Around line 811-826: Remove the merge-conflict remnant in the transaction
simulation setup by keeping a single valid TransactionBuilder statement assigned
to txToSimulate. Preserve its existing fee, networkPassphrase, operation,
timeout, and build configuration, then continue using txToSimulate for
simulation and preparation; remove the unused built declaration.
---
Nitpick comments:
In `@backend/src/services/streamStore.ts`:
- Around line 1070-1086: Update getStreamById to reuse getStream for lookup and
mapping instead of repeating the database query, return undefined when the
stream is absent to match getStream, and preserve archived_at only if it is
required by this function, using the established camelCase property convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d23954d6-59fc-4656-b236-9d2f2805978e
📒 Files selected for processing (1)
backend/src/services/streamStore.ts
Hey Chief, @ritik4ever , it has been resolved, kindly review. |
2 participants
Closes #324
Summary
This PR wires the pause_stream and resume_stream Soroban contract functions to the backend API, enabling full lifecycle management of paused streams through HTTP endpoints.
Changes
Backend API Endpoints
POST /api/streams/:id/pause — Sender pausing an active stream
POST /api/streams/:id/resume — Sender resuming a paused stream
streamStore.ts Improvements
Integration Tests
Files Changed
Verification
All acceptance criteria met:
Summary by CodeRabbit
New Features
Bug Fixes
Tests