Feat/transfer stream endpoint - Add transfer_stream endpoint to change recipient address - #669
Open
abbys-code-hub wants to merge 6 commits into
Open
Conversation
- Add transferStream() to streamStore: validates stream state, submits Soroban transfer_stream tx, updates recipient in SQLite, records 'transferred' event, triggers webhook - Add transferStreamSchema: validates sender and newRecipient as Stellar account IDs - Add 'transferred' to VALID_EVENT_TYPES in schemas - Fix duplicate elapsed/ratio declarations in calculateProgress - Fix indentation in cancelStream triggerWebhook call
…on tests - Add transfer route: validates stream ID, sender auth, body schema, calls transferStream, returns updated stream - Add 8 integration tests covering: success, wrong sender, sender mismatch, same recipient, canceled stream, not found, invalid ID, no auth - Fix duplicate query/data declarations in recipient streams route handler
- Add transferStream() API function with auth token support - Add 'transferred' to StreamEvent eventType union - Add Transfer Stream button in StreamDetailDrawer for sender with inline Stellar address input and validation - Add handleTransfer callback in DashboardPage - Add CSS styles for transfer input group with dark mode
- Add migration 005 to create cliff_seconds column on streams and stream_archive tables - Restore runMigrations() call in initDb() (lost in Postgres PR) - Add syncFtsIndex and searchStreamsFts stub functions that were removed during Postgres backend refactor - Fix duplicate validateEnv() function and duplicate isProduction declaration in validateEnv.ts
|
@abbys-code-hub is attempting to deploy a commit to the ritik4ever's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds end-to-end stream recipient transfers with Soroban execution, authenticated API handling, SQLite event updates, frontend controls, and integration tests. It also updates migrations, FTS helpers, progress calculations, recipient sorting, persistence, and environment validation. ChangesStream transfer workflow
Backend consistency updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant StreamDetailDrawer
participant DashboardPage
participant API
participant TransferRoute
participant transferStream
participant Soroban
participant SQLite
User->>StreamDetailDrawer: Enter new recipient
StreamDetailDrawer->>DashboardPage: Invoke onTransfer
DashboardPage->>API: POST transfer request
API->>TransferRoute: Send authenticated request
TransferRoute->>transferStream: Validate and transfer stream
transferStream->>Soroban: Submit transfer_stream
Soroban-->>transferStream: Return transaction result
transferStream->>SQLite: Update recipient and record event
SQLite-->>TransferRoute: Return updated stream
TransferRoute-->>API: Return transfer response
API-->>DashboardPage: Return updated stream
DashboardPage-->>StreamDetailDrawer: Refresh and show status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
🔧 Fix failing CI
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: 12
🧹 Nitpick comments (6)
backend/src/integration.test.ts (2)
1436-1462: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSeeded rows are never cleaned up.
Each
beforeEachinserts a stream that persists for the rest of the file's run; suites asserting on list totals or stats could become order-dependent. Add anafterEachdeleting the seeded stream and its events.🤖 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/integration.test.ts` around lines 1436 - 1462, In the test setup surrounding beforeEach, add an afterEach cleanup that deletes the stream identified by transferStreamId and removes any associated events from the database. Ensure cleanup runs after every test so list totals and stats remain isolated and order-independent.
1544-1558: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering the paused and scheduled cases.
The suite covers canceled (400) but not the "paused/scheduled transfers are allowed" behavior that
transferStreamexplicitly permits.🤖 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/integration.test.ts` around lines 1544 - 1558, Add integration coverage alongside the canceled-stream test for paused and scheduled streams, verifying that transferStream permits both cases and returns the expected successful response. Reuse the existing stream setup, authentication, keypairs, and transfer request pattern, while updating each stream’s state to represent paused or scheduled behavior.backend/src/services/streamStore.ts (2)
1308-1308: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueFloating promise on
triggerWebhook.
triggerWebhookisasync; leaving it unawaited without a.catchrisks an unhandled rejection. Prefervoid triggerWebhook(...).catch(...)or await it.🤖 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` at line 1308, Update the triggerWebhook call in the surrounding stream transfer flow to handle its returned promise explicitly: await it when the caller supports asynchronous control flow, or use void with a catch handler to report failures. Preserve the existing "transferred" event and stream arguments.
1210-1213: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winService accepts no
sender, so authorization lives only in the route.The doc comment states "Only the sender may transfer", but the function cannot enforce it. Any future caller (worker, script, another route) bypasses the check. Consider taking
senderand assertingstream.sender === senderhere as defense in depth.🤖 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 1210 - 1213, Update transferStream to accept a sender identity and enforce that the stream’s sender matches it before transferring. Add the authorization check inside transferStream so all callers are protected, and update its callers to pass the authenticated sender while preserving the existing transfer behavior for authorized requests.frontend/src/components/StreamDetailDrawer.tsx (1)
74-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
cliff_reachedhas no icon/label mapping.
StreamEvent["eventType"]includescliff_reached, which falls through to the raw snake_case string in the history list. Worth adding alongsidetransferred.Also applies to: 87-87
🤖 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 `@frontend/src/components/StreamDetailDrawer.tsx` at line 74, Add an icon/label mapping for the cliff_reached event type alongside the transferred mapping in the event display configuration, so StreamEvent history renders a friendly label instead of the raw snake_case value.frontend/src/index.css (1)
553-560: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
outline: nonedrops the focus indicator in forced-colors mode.The
box-shadowreplacement is stripped in Windows High Contrast Mode, leaving no visible focus. Keep a transparent outline on focus so the UA can render one.♻️ Proposed tweak
.transfer-recipient-input:focus { border-color: `#0ea5a5`; box-shadow: 0 0 0 3px rgba(14, 165, 165, 0.15); + outline: 2px solid transparent; + outline-offset: 2px; }🤖 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 `@frontend/src/index.css` around lines 553 - 560, Update the .transfer-recipient-input:focus styles to avoid removing the browser focus indicator in forced-colors mode: replace the outline:none behavior with a transparent outline that allows the user agent to render its forced-colors focus indicator, while preserving the existing border-color and box-shadow styles.
🤖 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/config/validateEnv.ts`:
- Around line 125-126: Use one canonical Stellar environment contract throughout
validateEnv: update validation and returned configuration to consistently read
STELLAR_CONTRACT_ID, SOROBAN_RPC_URL, and STELLAR_NETWORK instead of legacy
keys, or restore the removed mapping. Ensure deployments supplying the canonical
variables validate successfully and legacy-only values do not bypass validation,
then update adjacent tests to cover the canonical names.
In `@backend/src/integration.test.ts`:
- Around line 1484-1489: Update the transfer-event assertions near the
`stream_events` query to parse `events[0].metadata` and verify it contains both
the previous recipient and `newRecipientKeypair.publicKey()`. Preserve the
existing single-event assertion and use the test’s existing symbol for the old
recipient.
In `@backend/src/services/db.ts`:
- Around line 342-344: In the FTS handling around the shown catch blocks in the
database service, only suppress the expected missing-table condition; log and
rethrow all other SQL, schema, malformed-MATCH, and I/O errors so search
failures propagate to the API instead of returning empty success results. Apply
this to both the catch near lines 342 and 357, preserving normal handling when
the FTS table is genuinely absent.
- Around line 347-348: Update searchStreamsFts so PostgreSQL does not silently
return an empty successful result: implement equivalent PostgreSQL full-text
search, or explicitly reject/disable the search request with the endpoint’s
established error behavior. Preserve the existing non-PostgreSQL search path and
ensure /api/streams/search communicates the unsupported or implemented
PostgreSQL outcome.
- Around line 338-355: Update the SQLite statements in the streams FTS write
path and searchStreamsFts to replace positional ? placeholders with named `@name`
bindings, and pass matching named-property objects to run() and all(). Preserve
the existing values, query behavior, and result handling.
- Line 328: Update the migration flow invoked by runMigrations so an existing
database with streams but an empty schema_migrations table does not return after
baseline seeding; execute the pending migration up scripts, including additions
such as cliff_seconds, while preserving baseline behavior for already-current
schemas.
In `@backend/src/services/streamStore.ts`:
- Around line 1299-1305: The transfer transaction around upsertStream must not
clear an archived stream’s archived_at value. Update upsertStream or the
transfer flow to preserve the existing archivedAt when transferring, while
retaining null for genuinely unarchived streams; alternatively reject transfers
of archived streams before the transaction.
- Around line 1287-1305: Move the cache invalidation calls and stats/metrics
cache resets in the stream transfer flow to after the `db.transaction` commit,
so readers cannot repopulate stale values before persistence completes. Also
update the `stream.recipient` mutation in this flow to occur only after a
successful transaction, or restore the previous recipient when the transaction
fails, ensuring in-memory state remains consistent with SQLite.
- Line 1245: Validate the stream ID before the nativeToScVal call in the
surrounding stream operation, requiring the entire id to be a valid numeric
value rather than allowing parseInt to partially parse it. Reject invalid IDs,
including values like “12abc” or “abc”, before conversion, and use an explicit
radix when converting the validated ID to the u64 argument.
In `@frontend/src/components/StreamDetailDrawer.tsx`:
- Around line 550-561: Update the transfer UI in StreamDetailDrawer to retain a
ref to the “⇄ Transfer Stream” trigger and return focus to it whenever the
transfer input closes, including the Cancel handler and successful submission
path. Focus the trigger after the input unmounts so keyboard navigation remains
anchored within the drawer.
- Around line 122-125: Reset showTransferInput, newRecipient, and transferError
when streamId changes, alongside the existing fetchData reset logic in
StreamDetailDrawer. Ensure the drawer starts each stream with the transfer input
hidden, an empty recipient, and no transfer error.
In `@frontend/src/pages/DashboardPage.tsx`:
- Around line 272-289: Update the page-level handleTransfer function to rethrow
transfer failures after displaying the toast, including the disconnected-wallet
early-return path, so StreamDetailDrawer.handleTransfer can enter its failure
branch and keep the form open with transferError. Preserve the existing success
flow only for completed transfers.
---
Nitpick comments:
In `@backend/src/integration.test.ts`:
- Around line 1436-1462: In the test setup surrounding beforeEach, add an
afterEach cleanup that deletes the stream identified by transferStreamId and
removes any associated events from the database. Ensure cleanup runs after every
test so list totals and stats remain isolated and order-independent.
- Around line 1544-1558: Add integration coverage alongside the canceled-stream
test for paused and scheduled streams, verifying that transferStream permits
both cases and returns the expected successful response. Reuse the existing
stream setup, authentication, keypairs, and transfer request pattern, while
updating each stream’s state to represent paused or scheduled behavior.
In `@backend/src/services/streamStore.ts`:
- Line 1308: Update the triggerWebhook call in the surrounding stream transfer
flow to handle its returned promise explicitly: await it when the caller
supports asynchronous control flow, or use void with a catch handler to report
failures. Preserve the existing "transferred" event and stream arguments.
- Around line 1210-1213: Update transferStream to accept a sender identity and
enforce that the stream’s sender matches it before transferring. Add the
authorization check inside transferStream so all callers are protected, and
update its callers to pass the authenticated sender while preserving the
existing transfer behavior for authorized requests.
In `@frontend/src/components/StreamDetailDrawer.tsx`:
- Line 74: Add an icon/label mapping for the cliff_reached event type alongside
the transferred mapping in the event display configuration, so StreamEvent
history renders a friendly label instead of the raw snake_case value.
In `@frontend/src/index.css`:
- Around line 553-560: Update the .transfer-recipient-input:focus styles to
avoid removing the browser focus indicator in forced-colors mode: replace the
outline:none behavior with a transparent outline that allows the user agent to
render its forced-colors focus indicator, while preserving the existing
border-color and box-shadow styles.
🪄 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: de38cce2-bcc2-4955-875e-e2a047f7ca3c
⛔ Files ignored due to path filters (1)
backend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
backend/migrations/005_add_cliff_seconds.down.sqlbackend/migrations/005_add_cliff_seconds.sqlbackend/src/config/validateEnv.tsbackend/src/index.tsbackend/src/integration.test.tsbackend/src/services/db.tsbackend/src/services/streamStore.tsbackend/src/validation/schemas.tsfrontend/src/components/StreamDetailDrawer.tsxfrontend/src/index.cssfrontend/src/pages/DashboardPage.tsxfrontend/src/services/api.ts
Comment on lines
+1287
to
+1305
| const now = nowInSeconds(); | ||
| stream.recipient = newRecipient; | ||
|
|
||
| // Invalidate cache | ||
| await invalidateCache(`stream:${id}`); | ||
| await invalidateCache("streams:list:"); | ||
| await invalidateCache("streams:export:"); | ||
| resetStatsCache(); | ||
| resetStreamMetricsCache(); | ||
|
|
||
| // Atomically write the updated stream row and the transfer event. | ||
| const db = getDb(); | ||
| db.transaction(() => { | ||
| upsertStream(stream); | ||
| recordEventWithDb(db, stream.id, "transferred", now, stream.sender, undefined, { | ||
| oldRecipient, | ||
| newRecipient, | ||
| }); | ||
| })(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Cache is invalidated before the DB write commits.
Between the invalidateCache calls and the db.transaction(...) execution there is an await-free but still interleavable window (the invalidations are awaited, so other requests run in between) during which a concurrent read repopulates stream:<id> / list caches with the old recipient. That stale entry then survives the commit. Move the invalidation after the transaction. Also note stream.recipient is mutated before persistence, so a failed transaction leaves an in-memory record that disagrees with SQLite.
♻️ Proposed reordering
const now = nowInSeconds();
stream.recipient = newRecipient;
- // Invalidate cache
- await invalidateCache(`stream:${id}`);
- await invalidateCache("streams:list:");
- await invalidateCache("streams:export:");
- resetStatsCache();
- resetStreamMetricsCache();
-
// Atomically write the updated stream row and the transfer event.
const db = getDb();
db.transaction(() => {
upsertStream(stream);
recordEventWithDb(db, stream.id, "transferred", now, stream.sender, undefined, {
oldRecipient,
newRecipient,
});
})();
+
+ // Invalidate cache only after the write is durable.
+ await invalidateCache(`stream:${id}`);
+ await invalidateCache("streams:list:");
+ await invalidateCache("streams:export:");
+ resetStatsCache();
+ resetStreamMetricsCache();📝 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
| const now = nowInSeconds(); | |
| stream.recipient = newRecipient; | |
| // Invalidate cache | |
| await invalidateCache(`stream:${id}`); | |
| await invalidateCache("streams:list:"); | |
| await invalidateCache("streams:export:"); | |
| resetStatsCache(); | |
| resetStreamMetricsCache(); | |
| // Atomically write the updated stream row and the transfer event. | |
| const db = getDb(); | |
| db.transaction(() => { | |
| upsertStream(stream); | |
| recordEventWithDb(db, stream.id, "transferred", now, stream.sender, undefined, { | |
| oldRecipient, | |
| newRecipient, | |
| }); | |
| })(); | |
| const now = nowInSeconds(); | |
| stream.recipient = newRecipient; | |
| // Atomically write the updated stream row and the transfer event. | |
| const db = getDb(); | |
| db.transaction(() => { | |
| upsertStream(stream); | |
| recordEventWithDb(db, stream.id, "transferred", now, stream.sender, undefined, { | |
| oldRecipient, | |
| newRecipient, | |
| }); | |
| })(); | |
| // Invalidate cache only after the write is durable. | |
| await invalidateCache(`stream:${id}`); | |
| await invalidateCache("streams:list:"); | |
| await invalidateCache("streams:export:"); | |
| resetStatsCache(); | |
| resetStreamMetricsCache(); |
🤖 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 1287 - 1305, Move the cache
invalidation calls and stats/metrics cache resets in the stream transfer flow to
after the `db.transaction` commit, so readers cannot repopulate stale values
before persistence completes. Also update the `stream.recipient` mutation in
this flow to occur only after a successful transaction, or restore the previous
recipient when the transaction fails, ensuring in-memory state remains
consistent with SQLite.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/services/api.ts (1)
257-268: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInclude the required
senderfield in the transfer request.The transfer endpoint contract requires
{ sender, newRecipient }, but this wrapper serializes onlynewRecipient. Pass the authenticated wallet/sender address from the caller and update the related call sites and tests; otherwise the request can fail validation or lack the sender identity required for authorization.🤖 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 `@frontend/src/services/api.ts` around lines 257 - 268, Update transferStream to accept the authenticated sender address from its caller and serialize both sender and newRecipient in the transfer request body. Update all transferStream call sites and related tests to provide and verify the sender field while preserving the existing authorization-header behavior.
🤖 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/config/validateEnv.ts`:
- Around line 126-130: Update the environment normalization logic in validateEnv
so STELLAR_NETWORK values are converted to their corresponding Stellar network
passphrases before populating NETWORK_PASSPHRASE. Preserve the existing
passphrase input behavior, and ensure values such as “public” resolve to the
established actual passphrase expected by the returned configuration.
- Around line 114-124: Update the environment normalization logic in validateEnv
to detect when both legacy and canonical variables are set with different values
for CONTRACT_ID/STELLAR_CONTRACT_ID or RPC_URL/SOROBAN_RPC_URL. Fail fast on
each conflict, or consistently normalize both names to one canonical value
before validation and config construction so validation and runtime use
identical settings.
In `@backend/src/services/migrations.ts`:
- Around line 81-95: Update the migration execution around loadMigrationSql and
db.exec so a whole-script “already exists” or “duplicate column” error is not
treated as successful. Split and execute statements with per-statement
idempotency handling, or validate all required legacy-schema postconditions
before recording the migration; only insert the migration record after every
statement is applied or confirmed present, while rethrowing unrelated errors.
---
Outside diff comments:
In `@frontend/src/services/api.ts`:
- Around line 257-268: Update transferStream to accept the authenticated sender
address from its caller and serialize both sender and newRecipient in the
transfer request body. Update all transferStream call sites and related tests to
provide and verify the sender field while preserving the existing
authorization-header behavior.
🪄 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: fa5e78e6-0e37-45ad-976f-9745a0bedb70
📒 Files selected for processing (10)
backend/src/config/validateEnv.tsbackend/src/index.tsbackend/src/integration.test.tsbackend/src/services/db.tsbackend/src/services/migrations.tsbackend/src/services/streamStore.tsbackend/src/validation/schemas.tsfrontend/src/components/StreamDetailDrawer.tsxfrontend/src/pages/DashboardPage.tsxfrontend/src/services/api.ts
💤 Files with no reviewable changes (2)
- backend/src/validation/schemas.ts
- backend/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/src/pages/DashboardPage.tsx
- backend/src/integration.test.ts
- backend/src/services/db.ts
- frontend/src/components/StreamDetailDrawer.tsx
- backend/src/services/streamStore.ts
Comment on lines
+114
to
+124
| if (!process.env.STELLAR_CONTRACT_ID && process.env.CONTRACT_ID) { | ||
| process.env.STELLAR_CONTRACT_ID = process.env.CONTRACT_ID; | ||
| } | ||
| if (!process.env.CONTRACT_ID && process.env.STELLAR_CONTRACT_ID) { | ||
| process.env.CONTRACT_ID = process.env.STELLAR_CONTRACT_ID; | ||
| } | ||
| if (!process.env.SOROBAN_RPC_URL && process.env.RPC_URL) { | ||
| process.env.SOROBAN_RPC_URL = process.env.RPC_URL; | ||
| } | ||
| if (!process.env.RPC_URL && process.env.SOROBAN_RPC_URL) { | ||
| process.env.RPC_URL = process.env.SOROBAN_RPC_URL; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject conflicting legacy and canonical values.
When both names are set with different values, this block leaves both unchanged. Validation then uses env.CONTRACT_ID/env.RPC_URL, while the returned config uses STELLAR_CONTRACT_ID/SOROBAN_RPC_URL, allowing startup to validate one contract or RPC endpoint and operate against another. Normalize to one canonical value before validation, or fail fast on conflicts.
🤖 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/config/validateEnv.ts` around lines 114 - 124, Update the
environment normalization logic in validateEnv to detect when both legacy and
canonical variables are set with different values for
CONTRACT_ID/STELLAR_CONTRACT_ID or RPC_URL/SOROBAN_RPC_URL. Fail fast on each
conflict, or consistently normalize both names to one canonical value before
validation and config construction so validation and runtime use identical
settings.
Comment on lines
+126
to
+130
| if (!process.env.STELLAR_NETWORK && process.env.NETWORK_PASSPHRASE) { | ||
| process.env.STELLAR_NETWORK = process.env.NETWORK_PASSPHRASE; | ||
| } | ||
| if (!process.env.NETWORK_PASSPHRASE && process.env.STELLAR_NETWORK) { | ||
| process.env.NETWORK_PASSPHRASE = process.env.STELLAR_NETWORK; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not copy STELLAR_NETWORK directly into NETWORK_PASSPHRASE.
For STELLAR_NETWORK=public, this assigns "public" as the passphrase; the returned config consequently exposes "public" instead of "Public Global Stellar Network ; October 2015", contrary to backend/src/config/validateEnv.test.ts:706-715. Map network names to their actual passphrases before returning the config.
🤖 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/config/validateEnv.ts` around lines 126 - 130, Update the
environment normalization logic in validateEnv so STELLAR_NETWORK values are
converted to their corresponding Stellar network passphrases before populating
NETWORK_PASSPHRASE. Preserve the existing passphrase input behavior, and ensure
values such as “public” resolve to the established actual passphrase expected by
the returned configuration.
Comment on lines
+81
to
+95
| const upSql = loadMigrationSql(migration.upPath); | ||
| try { | ||
| db.exec(upSql); | ||
| } catch (err: any) { | ||
| // Only suppress "already exists" errors from legacy schemas | ||
| // (e.g., CREATE TABLE, ALTER TABLE ADD COLUMN). Rethrow others. | ||
| if (err?.message && ( | ||
| err.message.includes("already exists") || | ||
| err.message.includes("duplicate column") | ||
| )) { | ||
| // Expected — migration already reflected in legacy schema. | ||
| } else { | ||
| throw err; | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
while IFS= read -r file; do
count=$(( (rg -o ';' "$file" || true) | wc -l ))
if [ "$count" -gt 1 ]; then
printf '%s contains multiple SQL statements\n' "$file"
fi
done < <(fd --type f --extension sql . backend/migrations | sort)Repository: ritik4ever/stellar-stream
Length of output: 215
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'Repository files matching migrations.sql/migrations.ts:\n'
git ls-files | rg '(^backend/src/services/migrations\.ts$|^backend/migrations/.+\.sql$|migrations)' | sed -n '1,200p'
printf '\nbackend/src/services/migrations.ts relevant section:\n'
if [ -f backend/src/services/migrations.ts ]; then
nl -ba backend/src/services/migrations.ts | sed -n '1,150p'
fi
printf '\nMigration SQL files with statement counts:\n'
python3 - <<'PY'
import subprocess, pathlib
files = subprocess.check_output(['git','ls-files'], text=True).splitlines()
sql = [f for f in files if f.startswith('backend/migrations/') and f.endswith('.sql')]
for f in sql:
txt = pathlib.Path(f).read_text(errors='replace')
print(f"{f}: {sum(1 for i,c in enumerate(txt) if c==';' and not txt[i-7:i+3].lower().endswith('begin;'))}")
PYRepository: ritik4ever/stellar-stream
Length of output: 974
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'Relevant migration service implementation:\n'
awk '{printf "%6d\t%s\n", NR, $0}' backend/src/services/migrations.ts | sed -n '1,160p'
printf '\nMigration SQL statement counts:\n'
python3 - <<'PY'
import subprocess, pathlib
files = subprocess.check_output(['git','ls-files'], text=True).splitlines()
for f in files:
if f.startswith('backend/migrations/') and f.endswith('.sql'):
txt = pathlib.Path(f).read_text(errors='replace')
stmts = txt.count(';')
print(f"{f}: {stmts}")
PYRepository: ritik4ever/stellar-stream
Length of output: 6391
Do not record a migration applied after suppressing a whole-script failure.
backend/migrations/001_initial_schema.sql contains multiple SQL statements, and db.exec(upSql) will execute them all. Suppressing “already exists” / “duplicate column” errors after a failed statement can record a partially applied schema as complete. Apply idempotency per statement or check legacy schema postconditions before inserting the migration record.
🧰 Tools
🪛 OpenGrep (1.25.0)
[ERROR] 83-83: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 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/migrations.ts` around lines 81 - 95, Update the
migration execution around loadMigrationSql and db.exec so a whole-script
“already exists” or “duplicate column” error is not treated as successful. Split
and execute statements with per-statement idempotency handling, or validate all
required legacy-schema postconditions before recording the migration; only
insert the migration record after every statement is applied or confirmed
present, while rethrowing unrelated errors.
Source: MCP tools
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/src/services/streamStore.ts (2)
1235-1277: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not persist a transfer when Soroban is unavailable.
All Soroban dependencies and
CONTRACT_IDare optional here; if any are missing, execution falls through to the SQLite update as though the on-chain transfer succeeded. Fail with a configuration/service error instead, preserving the required on-chain-before-local-update invariant.🤖 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 1235 - 1277, Update the transfer_stream transaction flow around getSorobanContext, rpcServer, serverKeypair, and CONTRACT_ID so any missing Soroban dependency or contract configuration throws a configuration/service error instead of continuing to the SQLite update. Preserve the existing on-chain submission and confirmation behavior, and only allow the local transfer persistence after the on-chain transaction succeeds.
468-479: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake zero-duration progress start-aware and finite.
The
durationSeconds <= 0branch reports 100% vested even whenat < startAt, producing ascheduledstream with its full amount vested. It also returnsInfinity, which serializes tonullin JSON. Use a ratio of0beforestartAt,1afterward, and a finite/API-supported rate value.🤖 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 468 - 479, Update the progress calculations in the stream status return block to make zero-duration streams start-aware: use a ratio of 0 when at is before stream.startAt and 1 at or after it, so elapsed and vested amounts remain unstarted before the start time. Replace the zero-duration ratePerSecond Infinity result with a finite API-supported value while preserving the existing behavior for positive durations.
🤖 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 175-181: Update parseStreamIdAsNumber to preserve valid unsigned
64-bit stream IDs without Number conversion: return a typed bigint and pass that
value directly to Soroban u64 arguments. Apply the same change at the call sites
around lines 499, 540, 1153, and 1245, while retaining numeric-format validation
and the existing invalid-ID error behavior.
---
Outside diff comments:
In `@backend/src/services/streamStore.ts`:
- Around line 1235-1277: Update the transfer_stream transaction flow around
getSorobanContext, rpcServer, serverKeypair, and CONTRACT_ID so any missing
Soroban dependency or contract configuration throws a configuration/service
error instead of continuing to the SQLite update. Preserve the existing on-chain
submission and confirmation behavior, and only allow the local transfer
persistence after the on-chain transaction succeeds.
- Around line 468-479: Update the progress calculations in the stream status
return block to make zero-duration streams start-aware: use a ratio of 0 when at
is before stream.startAt and 1 at or after it, so elapsed and vested amounts
remain unstarted before the start time. Replace the zero-duration ratePerSecond
Infinity result with a finite API-supported value while preserving the existing
behavior for positive durations.
🪄 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: f2265c1e-7de0-4175-a8b3-0fbea79eed48
📒 Files selected for processing (3)
backend/src/index.tsbackend/src/services/db.tsbackend/src/services/streamStore.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/index.ts
Comment on lines
+175
to
+181
| function parseStreamIdAsNumber(id: string): number { | ||
| if (!/^\d+$/.test(id)) { | ||
| const err: any = new Error("Invalid stream ID: must be a numeric value."); | ||
| err.statusCode = 400; | ||
| throw err; | ||
| } | ||
| return parseInt(id, 10); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n 'parseStreamIdAsNumber|nativeToScVal' backend/src/services/streamStore.ts
npm view `@stellar/stellar-sdk`@14.5.0 versionRepository: ritik4ever/stellar-stream
Length of output: 1136
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== streamStore relevant sections =="
sed -n '160,190p;520,555p;1135,1165p;1230,1260p' backend/src/services/streamStore.ts
echo
echo "== static bigint/scVal evidence from npm package =="
tmp="$(mktemp -d)"
(
cd "$tmp"
npm pack `@stellar/stellar-sdk`@14.5.0 >/dev/null
tar -xzf stellar-stellar-sdk-14.5.0.tgz
rg -n "nativeToScVal|function nativeToScVal|nativeToScVal:" package -C 4
node - <<'JS'
const StellarSdk = require('./package');
for (const id of ["9007199254740992", "9007199254740993", "9223372036854775807", "9223372036854775808"]) {
for (const arg of [id, BigInt(id), parseInt(id, 10)]) {
try {
const scVal = StellarSdk.xdr.ScVal.nativeToScVal(arg, { type: "u64" });
console.log(`id=${id} nativeToScVal(${arg === BigInt(id) ? `BigInt(${id})` : arg}) type=${scVal()?.type?.value?.toString() || scVal?.value?.toString()}`);
} catch (e) {
console.log(`id=${id} nativeToScVal(${arg === BigInt(id) ? `BigInt(${id})` : arg}) ERROR ${String(e.message ?? e)}`);
}
}
}
JS
)
rm -rf "$tmp"Repository: ritik4ever/stellar-stream
Length of output: 50382
Keep Soroban ID arguments as strings/bigints.
parseStreamIdAsNumber() and parseInt(id, 10) can round valid unsigned 64-bit stream IDs above Number.MAX_SAFE_INTEGER, such as "9007199254740993", causing Soroban calls to target the wrong stream. Return a typed bigint and pass it directly as the u64 argument, or reject IDs above the safe-integer limit before converting.
Also applies to lines 499, 540, 1153, and 1245.
🤖 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 175 - 181, Update
parseStreamIdAsNumber to preserve valid unsigned 64-bit stream IDs without
Number conversion: return a typed bigint and pass that value directly to Soroban
u64 arguments. Apply the same change at the call sites around lines 499, 540,
1153, and 1245, while retaining numeric-format validation and the existing
invalid-ID error behavior.
4 tasks
Owner
|
Hi @abbys-code-hub, 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! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add transfer_stream endpoint to change recipient address
Closes #325
Summary
Adds
POST /api/streams/:id/transferendpoint and a "Transfer Stream" button inStreamDetailDrawer, allowing the sender to transfer a stream to a new recipient on-chain.Changes
Backend
POST /api/streams/:id/transfer— new route accepting{ sender, newRecipient }body with Stellar JWT auth. Validates stream state, verifies sender identity, callstransferStream(), returns updated stream with progress.transferStream()instreamStore.ts— validates the stream is not finalized and new recipient differs. Submits a Sorobantransfer_streamtransaction (skipped if no contract configured), updates therecipientfield in SQLite, records astream_transferredevent with old/new recipient in metadata, triggers a webhook.transferStreamSchemainschemas.ts— Zod schema validatingsenderandnewRecipientas valid Stellar account IDs.8 integration tests covering: success, wrong sender (403), sender mismatch (403), same recipient (400), canceled stream (400), not found (404), invalid ID (400), no auth (401).
"transferred"added toVALID_EVENT_TYPESfor event filtering and webhook registration.Frontend
Transfer Stream button in
StreamDetailDrawer— visible to the sender for non-finalized streams (active/scheduled/paused). Clicking reveals an inline input for the new Stellar recipient address with client-side format validation.transferStream()API function with auth token support and JSON body.handleTransfer()callback inDashboardPage— calls the API with the connected wallet address as sender.CSS styles for the transfer input group with dark mode support.
Infrastructure fixes (pre-existing issues)
cliff_secondscolumn tostreamsandstream_archivetables.initDb()— restoredrunMigrations()call lost during the PostgreSQL backend refactor (feat(database): migrate SQLite schema to support optional PostgreSQL … #640).syncFtsIndex/searchStreamsFts— restored stub functions removed during the PostgreSQL refactor.calculateProgress()— fixed duplicateelapsed/ratiodeclarations.validateEnv.ts— fixed duplicatevalidateEnv()function and duplicateisProductiondeclaration.parsedQuery/query/datadeclarations in the route handler.Testing
All 8 integration tests pass ✅
Commits
ab84d755b4f30b365422c9abdfd9Summary by CodeRabbit
New Features
POST /api/streams/:id/transferendpoint and matching client helper.transferredactivity events with webhook support.cliff_secondsto stream records (defaulting to0).Bug Fixes
Tests