feat(backend): wire Soroban create_stream contract call with simulati… - #650
Conversation
…on, signing, and SQLite fallback
|
Someone is attempting to deploy a commit to the ritik4ever's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@fredericklamar342-prog 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! 🚀 |
📝 WalkthroughWalkthrough
ChangescreateStream execution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant createStream
participant rpcServer
participant serverKeypair
participant SQLite
createStream->>rpcServer: Simulate and prepare create_stream
createStream->>serverKeypair: Sign transaction
createStream->>rpcServer: Submit and poll transaction
rpcServer-->>createStream: Return stream ID
createStream->>SQLite: Persist stream
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.5)backend/src/services/streamStore.tsFile contains syntax errors that prevent linting: Line 812: expected 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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/src/services/streamStore.ts (1)
866-872: 🚀 Performance & Scalability | 🔵 TrivialBlocking poll loop can hold the request open for ~10s.
The
getTransactionpolling loop (10 attempts × 1s, each itself wrapped inretryWithBackoff) can keep the HTTP request open well beyond typical gateway/proxy timeouts under network hiccups. Consider capping the total wall-clock budget explicitly, or moving confirmation polling out of the synchronous request path (e.g., return "pending" and confirm asynchronously/via webhook).🤖 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 866 - 872, The getTransaction polling in the transaction submission flow can block the HTTP request for too long. Update the loop around retryWithBackoff and rpcServer!.getTransaction to enforce an explicit short total wall-clock deadline or move confirmation to asynchronous processing, returning a pending result when confirmation exceeds that budget.backend/src/services/streamStore.test.ts (1)
859-894: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest title claims both
SOROBAN_ENABLED=falseandSOROBAN_DISABLED=truebut only exercises the former.Only
SOROBAN_ENABLED = "false"is set; theSOROBAN_DISABLED=truebranch ofsorobanDisabledincreateStreamis never asserted. Consider splitting into two cases (or parametrizing) so both conditions are actually covered.🤖 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.test.ts` around lines 859 - 894, Update the test covering SQLite fallback around initSoroban and createStream so it independently exercises both sorobanDisabled conditions: SOROBAN_ENABLED="false" and SOROBAN_DISABLED="true". Split or parameterize the test cases, resetting the relevant environment variables and mocks between cases, and retain the assertion that each path creates stream ID "42".
🤖 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.
Inline comments:
In `@backend/src/services/streamStore.ts`:
- Around line 826-838: Replace the numeric streamIdStr allocation in the
fallback branch and the Soroban-derived ID path with a single authoritative
local identifier strategy used by both paths, such as an atomically incremented
counter or UUID. Ensure allocation and persistence are transaction-safe across
processes, and update upsertStream and related lookups so on-chain IDs are
stored separately from the local primary key rather than being conflated.
- Around line 874-876: Update the transaction validation condition around
txResult in the stream-store flow to reject only nullish return values, not
valid falsy values such as 0. Preserve the SUCCESS status check and existing
error message for failed transactions.
---
Nitpick comments:
In `@backend/src/services/streamStore.test.ts`:
- Around line 859-894: Update the test covering SQLite fallback around
initSoroban and createStream so it independently exercises both sorobanDisabled
conditions: SOROBAN_ENABLED="false" and SOROBAN_DISABLED="true". Split or
parameterize the test cases, resetting the relevant environment variables and
mocks between cases, and retain the assertion that each path creates stream ID
"42".
In `@backend/src/services/streamStore.ts`:
- Around line 866-872: The getTransaction polling in the transaction submission
flow can block the HTTP request for too long. Update the loop around
retryWithBackoff and rpcServer!.getTransaction to enforce an explicit short
total wall-clock deadline or move confirmation to asynchronous processing,
returning a pending result when confirmation exceeds that budget.
🪄 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 Plus
Run ID: 5d5d792e-e57e-4be4-9be4-4c65a1c5258c
📒 Files selected for processing (2)
backend/src/services/streamStore.test.tsbackend/src/services/streamStore.ts
| if (sorobanDisabled || !contractId || !rpcServer || !serverKeypair) { | ||
| if (!sorobanDisabled && (!contractId || !rpcServer || !serverKeypair)) { | ||
| logger.warn( | ||
| "Soroban configuration incomplete or serverKeypair missing, falling back to local SQLite creation.", | ||
| ); | ||
| } | ||
| // Fallback SQLite-only path (e.g., local dev or SOROBAN_ENABLED=false) | ||
| const db = getDb(); | ||
| const row = db | ||
| .prepare("SELECT MAX(CAST(id AS INTEGER)) as maxId FROM streams") | ||
| .get() as { maxId: number | null } | undefined; | ||
| const nextNumericId = (row?.maxId ?? 0) + 1; | ||
| streamIdStr = nextNumericId.toString(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
SQLite fallback IDs and Soroban on-chain IDs are independent namespaces — collisions will silently overwrite unrelated streams.
streamIdStr in the fallback branch is MAX(CAST(id AS INTEGER)) + 1 computed purely from the local streams table, while the Soroban branch derives streamIdStr from the contract's own on-chain counter (scValToNative(txResult.returnValue), seen as a sequential integer in the mock server's get_next_stream_id). These two counters are not coordinated. Any time the fallback path is used (RPC outage, SOROBAN_ENABLED=false toggled temporarily, etc.) and later the Soroban path resumes, the on-chain contract can hand back an id that already exists locally (or vice versa). Since upsertStream does INSERT ... ON CONFLICT(id) DO UPDATE, a colliding id doesn't error — it silently overwrites a different, unrelated stream's row with the new stream's data.
Additionally, even within the fallback path alone, SELECT MAX(...) and the later INSERT are two separate statements not wrapped in one transaction; across multiple app instances/processes sharing the same SQLite file this is a TOCTOU race that can also produce duplicate ids.
Consider allocating stream ids from a single authoritative source regardless of path (e.g., a dedicated counter table incremented atomically, or a UUID), and/or storing the on-chain id in a separate column from the local primary key so the two are never conflated.
Also applies to: 839-880
🤖 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 826 - 838, Replace the
numeric streamIdStr allocation in the fallback branch and the Soroban-derived ID
path with a single authoritative local identifier strategy used by both paths,
such as an atomically incremented counter or UUID. Ensure allocation and
persistence are transaction-safe across processes, and update upsertStream and
related lookups so on-chain IDs are stored separately from the local primary key
rather than being conflated.
| if (txResult?.status !== "SUCCESS" || !txResult.returnValue) { | ||
| throw new Error("Tx failed on chain: " + JSON.stringify(txResult)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Falsy check rejects a valid returnValue of 0.
!txResult.returnValue treats a legitimate stream id of 0 as a failure, throwing "Tx failed on chain" even though the transaction succeeded. Use an explicit nullish check instead.
🐛 Proposed fix
- if (txResult?.status !== "SUCCESS" || !txResult.returnValue) {
+ if (txResult?.status !== "SUCCESS" || txResult.returnValue === undefined || txResult.returnValue === null) {
throw new Error("Tx failed on chain: " + JSON.stringify(txResult));
}📝 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.
| if (txResult?.status !== "SUCCESS" || !txResult.returnValue) { | |
| throw new Error("Tx failed on chain: " + JSON.stringify(txResult)); | |
| } | |
| if (txResult?.status !== "SUCCESS" || txResult.returnValue === undefined || txResult.returnValue === null) { | |
| throw new Error("Tx failed on chain: " + JSON.stringify(txResult)); | |
| } |
🤖 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 874 - 876, Update the
transaction validation condition around txResult in the stream-store flow to
reject only nullish return values, not valid falsy values such as 0. Preserve
the SUCCESS status check and existing error message for failed transactions.
|
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! |
on it @ritik4ever |
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 (4)
backend/src/services/streamStore.ts (4)
834-841: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not treat the ten-second polling window as transaction failure.
The transaction is submitted successfully with
PENDINGstatus, butcreateStreamthrows "Tx failed on chain" after tenNOT_FOUNDresponses. A retried API request can then submit another transaction and create a duplicate stream.Persist the submitted transaction hash and reconcile asynchronously, or return a pending state instead of reporting permanent failure. Add a test that exercises more than ten
NOT_FOUNDresponses.🤖 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 834 - 841, The createStream transaction flow around txResult polling must not throw a permanent “Tx failed on chain” error after ten NOT_FOUND responses. Persist the submitted transaction hash and reconcile it asynchronously, or return an explicit pending state while preserving the existing hash for retries; update the failure handling near retryWithBackoff and add coverage for more than ten NOT_FOUND responses to ensure no duplicate submission occurs.
811-819: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winFix the malformed transaction builder.
The
const built = await rpcServer.prepareTransaction(...)declaration is placed inside theTransactionBuilderobject literal opened at line 811. TypeScript object literals requirekey: valuesyntax, so this is invalid code and prevents the backend from compiling. Remove the nested declaration from the transaction builder and build the transaction once for simulation before callingprepareTransaction.🤖 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 - 819, Fix the transaction construction around TransactionBuilder and the built declaration by closing the builder expression before declaring built; construct the transaction once, then pass that built transaction to rpcServer.prepareTransaction for simulation. Ensure no const declaration remains nested inside the TransactionBuilder object literal.Source: Linters/SAST tools
864-880: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftProtect
createStreamfrom SQLite write failures after chain submission.
createStreamsubmits and waits for Soroban succeeds beforeupsertStream(stream)andrecordEventWithDb()run. If those writes fail, the on-chain stream exists while the localstreamsrow and create event are missing. A retry can create a new on-chain stream because no durable idempotency record or transaction-hash safeguard exists. Store an idempotency/chain-result record before submission, or make chain->SQLite failure recoverable and replay only for already-known stream IDs.🤖 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 864 - 880, Update createStream so chain submission is protected from subsequent SQLite write failures. Persist an idempotency/chain-result record before submitting to Soroban, or make the post-submission persistence path recoverable and replayable for an already-known stream ID, including the transaction hash. Ensure retries reuse the existing on-chain result and cannot create a second stream when upsertStream or recordEventWithDb fails.
1070-1072: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse named binding for the new SQLite lookup.
Change the
getStreamByIdquery toWHERE id =@id`` and call.get({ id }).Proposed fix
- const row = db.prepare("SELECT * FROM streams WHERE id = ?").get(id) as StreamRow | undefined; + const row = db + .prepare("SELECT * FROM streams WHERE id = `@id`") + .get({ id }) as StreamRow | undefined;🤖 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 - 1072, Update getStreamById to use a named SQLite parameter: change the query predicate to WHERE id = `@id` and pass the identifier through .get({ id }) instead of positional binding.Source: Coding guidelines
🤖 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 834-841: The createStream transaction flow around txResult polling
must not throw a permanent “Tx failed on chain” error after ten NOT_FOUND
responses. Persist the submitted transaction hash and reconcile it
asynchronously, or return an explicit pending state while preserving the
existing hash for retries; update the failure handling near retryWithBackoff and
add coverage for more than ten NOT_FOUND responses to ensure no duplicate
submission occurs.
- Around line 811-819: Fix the transaction construction around
TransactionBuilder and the built declaration by closing the builder expression
before declaring built; construct the transaction once, then pass that built
transaction to rpcServer.prepareTransaction for simulation. Ensure no const
declaration remains nested inside the TransactionBuilder object literal.
- Around line 864-880: Update createStream so chain submission is protected from
subsequent SQLite write failures. Persist an idempotency/chain-result record
before submitting to Soroban, or make the post-submission persistence path
recoverable and replayable for an already-known stream ID, including the
transaction hash. Ensure retries reuse the existing on-chain result and cannot
create a second stream when upsertStream or recordEventWithDb fails.
- Around line 1070-1072: Update getStreamById to use a named SQLite parameter:
change the query predicate to WHERE id = `@id` and pass the identifier through
.get({ id }) instead of positional binding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: efd7efed-063a-4bde-9968-6e7896a726f6
📒 Files selected for processing (2)
backend/src/services/streamStore.test.tsbackend/src/services/streamStore.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/services/streamStore.test.ts
2 participants
…on, signing, and SQLite fallback
What changed
Testing done
Related issues
Closes #321
Checklist
Summary by CodeRabbit
New Features
Bug Fixes