WIP prep for breaking change to SDK - #15
Conversation
📝 WalkthroughWalkthroughThe PR updates the Soroban starter project to migrate from Horizon-based operations to XDR-based types and patterns. It introduces a text-encoding polyfill, updates the network endpoint and configuration, adjusts dependency resolution to use local Stellar SubQL packages, and refactors mapping handlers to process operations and events through new data structures. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5150b8ae0d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| endpoint: ["https://stellar.api.onfinality.io/public"], | ||
| }, |
There was a problem hiding this comment.
The project still registers a StellarHandlerKind.Event handler, but the network config now only sets endpoint (Horizon) and omits sorobanEndpoint. Event handlers are served via Soroban RPC, and the other starter configs in this repo (e.g., soroban-testnet-starter/project.ts) keep sorobanEndpoint when events are enabled. Without it, event indexing will fail at runtime because there is no RPC endpoint to fetch Soroban events.
Useful? React with 👍 / 👎.
| const fromAccount = await checkAndGetAccount( | ||
| decodeAddress(from), | ||
| event.ledger!.sequence, | ||
| scValToNative(from), | ||
| event.event.ledger, | ||
| ); | ||
| const toAccount = await checkAndGetAccount( | ||
| decodeAddress(to), | ||
| event.ledger!.sequence, | ||
| scValToNative(to), |
There was a problem hiding this comment.
scValToNative is used directly to decode the from/to address ScVals that come from events, but this function is known to fail for addresses in the other starter templates in this repo (see the comment in soroban-testnet-starter/src/mappings/mappingHandlers.ts). Here you only log failures in a try/catch and then call scValToNative again outside the guard, which will throw and stop indexing whenever an address ScVal can’t be decoded. This will break processing for real transfer events unless a fallback decoder is restored.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@Stellar/soroban-starter/package.json`:
- Around line 35-38: The resolutions block currently pins local portal paths
which will break external clones; remove or replace the "resolutions" entries
that reference portal:../../../subql/packages (specifically
"@subql/types-stellar" and "@subql/common-stellar") and instead use real
published version specifiers (or remove the block entirely) so the package.json
no longer depends on local filesystem portal paths before merging.
In `@Stellar/soroban-starter/src/mappings/mappingHandlers.ts`:
- Line 130: The debug log contains a typo: change the logger call that currently
logs "VALUE TPE" (the statement using logger.info and referencing
event.event.value.switch().name) to "VALUE TYPE" so the message reads "VALUE
TYPE ${event.event.value.switch().name}" to correct the spelling while
preserving the existing event value output.
- Line 137: The contract field currently uses a non-null assertion after
optional chaining (event.event.contractId?.contractId().toString()!) which can
produce runtime errors; update the mapping in mappingHandlers.ts to handle a
missing contractId explicitly by using nullish coalescing or conditional logic
on event.event.contractId (e.g., supply a default string with ?? or return/throw
when contractId is absent) so that contract is never derived from undefined;
reference the event.event.contractId and its contractId() call when applying the
fix.
- Around line 105-128: The try/catch only logs decoding failures but then calls
scValToNative(from) and scValToNative(to) again, which will rethrow; update
mappingHandlers.ts to decode once and handle failures: call scValToNative(from)
and scValToNative(to) inside the try blocks, assign the results to local
variables (e.g. decodedFrom, decodedTo), and on decode failure either
return/skip processing or set a safe fallback and exit; then pass decodedFrom
and decodedTo into checkAndGetAccount(decodedFrom, event.event.ledger) and
checkAndGetAccount(decodedTo, event.event.ledger) instead of re-invoking
scValToNative. Ensure logger messages include context and keep error handling
consistent.
🧹 Nitpick comments (5)
Stellar/soroban-starter/project.ts (2)
61-74: Commented-out code should be removed or tracked.These commented-out handlers for
handleCreditandhandleDebitreferenceStellarHandlerKind.Effects. If they're being removed as part of the breaking change, consider deleting them entirely. If they're planned for future re-enablement, track this with a TODO or issue reference.
58-58: Use a string literal for the operation type filter.Since the filter's
typefield expects a string (as evidenced by other handlers using"account_credited"and"account_debited"), you can simplifyxdr.OperationType.payment().namedirectly to"payment":Suggested change
filter: { - type: xdr.OperationType.payment().name, + type: "payment", },This improves readability and eliminates an unnecessary method call and property extraction.
Stellar/soroban-starter/package.json (2)
24-26: Using"latest"for dependencies reduces reproducibility.Pinning to
"latest"means builds may behave differently over time. Consider pinning to specific versions once the breaking change SDK is released.
31-31: Unusual version specifier"^0"for types package.While valid (matches any 0.x.x version), this is unconventional. If
text-encodingtypes are stable, consider specifying a more explicit version like"^0.0.39"for clarity.Stellar/soroban-starter/src/mappings/mappingHandlers.ts (1)
2-2: Unused imports:CreditandDebit.Since
handleCreditandhandleDebitare commented out, these imports are unused. Remove them to keep the code clean, or uncomment the handlers if they're still needed.
| "resolutions": { | ||
| "@subql/types-stellar": "portal:../../../subql/packages/types", | ||
| "@subql/common-stellar": "portal:../../../subql/packages/common-stellar" | ||
| } |
There was a problem hiding this comment.
Local portal resolutions will break for external users.
The resolutions block with portal: paths references local filesystem directories (../../../subql/packages/...). This will fail for anyone cloning this repository who doesn't have the same local setup.
For a WIP branch this may be intentional for testing, but ensure these are removed or replaced with proper version specifiers before merging to main.
🤖 Prompt for AI Agents
In `@Stellar/soroban-starter/package.json` around lines 35 - 38, The resolutions
block currently pins local portal paths which will break external clones; remove
or replace the "resolutions" entries that reference
portal:../../../subql/packages (specifically "@subql/types-stellar" and
"@subql/common-stellar") and instead use real published version specifiers (or
remove the block entirely) so the package.json no longer depends on local
filesystem portal paths before merging.
| try { | ||
| scValToNative(from); | ||
| } catch (e) { | ||
| logger.info( | ||
| `decode from failed ${from.switch().name}, ${JSON.stringify(from, null, 2)}, error: ${e}`, | ||
| ); | ||
| } | ||
|
|
||
| try { | ||
| decodeAddress(from); | ||
| decodeAddress(to); | ||
| scValToNative(to); | ||
| } catch (e) { | ||
| logger.info(`decode address failed`); | ||
| logger.info( | ||
| `decode to failed ${event.event.topic.length}, ${JSON.stringify(event.event.topic, null, 2)}, error: ${e}`, | ||
| ); | ||
| } | ||
|
|
||
| const fromAccount = await checkAndGetAccount( | ||
| decodeAddress(from), | ||
| event.ledger!.sequence, | ||
| scValToNative(from), | ||
| event.event.ledger, | ||
| ); | ||
| const toAccount = await checkAndGetAccount( | ||
| decodeAddress(to), | ||
| event.ledger!.sequence, | ||
| scValToNative(to), | ||
| event.event.ledger, | ||
| ); |
There was a problem hiding this comment.
Error handling does not prevent subsequent failure.
The try-catch blocks at lines 105-111 and 113-119 catch and log decoding errors, but the code proceeds to call scValToNative(from) and scValToNative(to) again at lines 122 and 126. If decoding failed in the try block, it will throw again when creating accounts.
Either:
- Store the decoded values from the try block and reuse them, or
- Return early / skip processing if decoding fails
🐛 Proposed fix: Store decoded values and handle failures
- try {
- scValToNative(from);
- } catch (e) {
- logger.info(
- `decode from failed ${from.switch().name}, ${JSON.stringify(from, null, 2)}, error: ${e}`,
- );
- }
-
- try {
- scValToNative(to);
- } catch (e) {
- logger.info(
- `decode to failed ${event.event.topic.length}, ${JSON.stringify(event.event.topic, null, 2)}, error: ${e}`,
- );
- }
-
- const fromAccount = await checkAndGetAccount(
- scValToNative(from),
- event.event.ledger,
- );
- const toAccount = await checkAndGetAccount(
- scValToNative(to),
- event.event.ledger,
- );
+ let fromAddress: string;
+ let toAddress: string;
+ try {
+ fromAddress = scValToNative(from);
+ } catch (e) {
+ logger.warn(
+ `decode from failed ${from.switch().name}, ${JSON.stringify(from, null, 2)}, error: ${e}`,
+ );
+ return;
+ }
+
+ try {
+ toAddress = scValToNative(to);
+ } catch (e) {
+ logger.warn(
+ `decode to failed ${to.switch().name}, ${JSON.stringify(to, null, 2)}, error: ${e}`,
+ );
+ return;
+ }
+
+ const fromAccount = await checkAndGetAccount(fromAddress, event.event.ledger);
+ const toAccount = await checkAndGetAccount(toAddress, event.event.ledger);📝 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.
| try { | |
| scValToNative(from); | |
| } catch (e) { | |
| logger.info( | |
| `decode from failed ${from.switch().name}, ${JSON.stringify(from, null, 2)}, error: ${e}`, | |
| ); | |
| } | |
| try { | |
| decodeAddress(from); | |
| decodeAddress(to); | |
| scValToNative(to); | |
| } catch (e) { | |
| logger.info(`decode address failed`); | |
| logger.info( | |
| `decode to failed ${event.event.topic.length}, ${JSON.stringify(event.event.topic, null, 2)}, error: ${e}`, | |
| ); | |
| } | |
| const fromAccount = await checkAndGetAccount( | |
| decodeAddress(from), | |
| event.ledger!.sequence, | |
| scValToNative(from), | |
| event.event.ledger, | |
| ); | |
| const toAccount = await checkAndGetAccount( | |
| decodeAddress(to), | |
| event.ledger!.sequence, | |
| scValToNative(to), | |
| event.event.ledger, | |
| ); | |
| let fromAddress: string; | |
| let toAddress: string; | |
| try { | |
| fromAddress = scValToNative(from); | |
| } catch (e) { | |
| logger.warn( | |
| `decode from failed ${from.switch().name}, ${JSON.stringify(from, null, 2)}, error: ${e}`, | |
| ); | |
| return; | |
| } | |
| try { | |
| toAddress = scValToNative(to); | |
| } catch (e) { | |
| logger.warn( | |
| `decode to failed ${to.switch().name}, ${JSON.stringify(to, null, 2)}, error: ${e}`, | |
| ); | |
| return; | |
| } | |
| const fromAccount = await checkAndGetAccount(fromAddress, event.event.ledger); | |
| const toAccount = await checkAndGetAccount(toAddress, event.event.ledger); |
🤖 Prompt for AI Agents
In `@Stellar/soroban-starter/src/mappings/mappingHandlers.ts` around lines 105 -
128, The try/catch only logs decoding failures but then calls
scValToNative(from) and scValToNative(to) again, which will rethrow; update
mappingHandlers.ts to decode once and handle failures: call scValToNative(from)
and scValToNative(to) inside the try blocks, assign the results to local
variables (e.g. decodedFrom, decodedTo), and on decode failure either
return/skip processing or set a safe fallback and exit; then pass decodedFrom
and decodedTo into checkAndGetAccount(decodedFrom, event.event.ledger) and
checkAndGetAccount(decodedTo, event.event.ledger) instead of re-invoking
scValToNative. Ensure logger messages include context and keep error handling
consistent.
| event.event.ledger, | ||
| ); | ||
|
|
||
| logger.info(`VALUE TPE ${event.event.value.switch().name} `); |
There was a problem hiding this comment.
Typo in debug log: "TPE" should be "TYPE".
- logger.info(`VALUE TPE ${event.event.value.switch().name} `);
+ logger.info(`VALUE TYPE ${event.event.value.switch().name}`);📝 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.
| logger.info(`VALUE TPE ${event.event.value.switch().name} `); | |
| logger.info(`VALUE TYPE ${event.event.value.switch().name}`); |
🤖 Prompt for AI Agents
In `@Stellar/soroban-starter/src/mappings/mappingHandlers.ts` at line 130, The
debug log contains a typo: change the logger call that currently logs "VALUE
TPE" (the statement using logger.info and referencing
event.event.value.switch().name) to "VALUE TYPE" so the message reads "VALUE
TYPE ${event.event.value.switch().name}" to correct the spelling while
preserving the existing event value output.
| id: event.event.id, | ||
| ledger: event.event.ledger, | ||
| date: new Date(event.event.ledgerClosedAt), | ||
| contract: event.event.contractId?.contractId().toString()!, |
There was a problem hiding this comment.
Non-null assertion after optional chaining can cause runtime errors.
event.event.contractId?.contractId().toString()! — if contractId is nullish, the optional chain returns undefined, but the ! assertion incorrectly claims it's non-null. This creates a type-safety gap and could cause issues downstream.
🛡️ Proposed fix: Use nullish coalescing
- contract: event.event.contractId?.contractId().toString()!,
+ contract: event.event.contractId?.contractId().toString() ?? "",Or handle the missing contract case explicitly if an empty string is not appropriate.
📝 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.
| contract: event.event.contractId?.contractId().toString()!, | |
| contract: event.event.contractId?.contractId().toString() ?? "", |
🧰 Tools
🪛 Biome (2.3.13)
[error] 137-137: Forbidden non-null assertion after optional chaining.
Optional chaining already handles nullish values. Using non-null assertion defeats its purpose and may cause runtime errors.
Consider using the nullish coalescing operator ?? or optional chaining throughout the chain instead.
(lint/suspicious/noNonNullAssertedOptionalChain)
🤖 Prompt for AI Agents
In `@Stellar/soroban-starter/src/mappings/mappingHandlers.ts` at line 137, The
contract field currently uses a non-null assertion after optional chaining
(event.event.contractId?.contractId().toString()!) which can produce runtime
errors; update the mapping in mappingHandlers.ts to handle a missing contractId
explicitly by using nullish coalescing or conditional logic on
event.event.contractId (e.g., supply a default string with ?? or return/throw
when contractId is absent) so that contract is never derived from undefined;
reference the event.event.contractId and its contractId() call when applying the
fix.
2 participants
Summary by CodeRabbit
Release Notes
New Features
Improvements
Chores