fix: return clean error messages instead of raw SDK objects by dylankilkenny · Pull Request #51 · OpenZeppelin/relayer-plugin-channels

dylankilkenny · 2026-01-05T12:01:15Z

Summary

Check simResult.error before calling assembleTransaction() to prevent massive JSON dumps with _attributes/_switch/_value being exposed to clients
Add parseSimulationError() to extract human-readable messages from Soroban diagnostic events (e.g., "signature has expired (Auth, InvalidInput)")
Add sanitizeReason() to strip provider wrapper text from transaction errors (e.g., "TxInsufficientBalance" instead of nested error chain)
Add comprehensive tests for both error sanitization functions

Test plan

Unit tests added for parseSimulationError() and sanitizeReason()
Manual testing against local relayer with expired signatures
Verify error responses are clean JSON without internal SDK objects

Summary by CodeRabbit

Bug Fixes
- Improved error messaging for simulation failures with more human-readable and diagnostic information
- Enhanced error messages for on-chain transaction failures with clearer, sanitized error details
- Added better logging for RPC errors to support debugging and troubleshooting
Tests
- Added comprehensive unit tests covering error handling scenarios, edge cases, and message sanitization

_{✏️ Tip: You can customize this high-level summary in your review settings.}

- Check simResult.error before calling assembleTransaction to prevent massive JSON dumps with _attributes/_switch/_value being exposed - Add parseSimulationError() to extract human-readable messages from Soroban diagnostic events (e.g., "signature has expired (Auth, InvalidInput)") - Add sanitizeReason() to strip provider wrapper text from transaction errors (e.g., "TxInsufficientBalance" instead of nested error chain) - Add comprehensive tests for both error sanitization functions

coderabbitai · 2026-01-05T12:01:28Z

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This PR improves error handling across the plugin by introducing two new helper functions to extract human-readable messages from RPC and transaction failures. The functions parse diagnostic data and sanitize error strings, with comprehensive unit tests validating edge cases and specific error code extraction.

Changes

Cohort / File(s)	Summary
Error handling helpers `src/plugin/simulation.ts`, `src/plugin/submit.ts`	Added `parseSimulationError()` to extract readable messages from RPC diagnostic data and `sanitizeReason()` to sanitize transaction error strings. Enhanced error logging for RPC failures and on-chain transaction failures with human-readable details.
Error sanitization tests `test/error-sanitization.test.ts`	Comprehensive unit tests covering both helper functions with multiple input formats, whitespace handling, empty inputs, specific error code extraction (e.g., TxInsufficientBalance, TxBadSeq), array vs. string formats, and fallback behaviors.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐰 When errors came forth, cryptic and wild,
Our helpers now parse them, gentle and mild,
With simulations clearer and reasons refined,
The debugging path is much easier to find! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: improving error handling to return clean, human-readable error messages instead of exposing raw SDK objects.
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch fix/clean-error-messages

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Copilot

Pull request overview

This PR improves error handling by preventing raw SDK objects from being exposed to API clients. It introduces two new error sanitization functions to extract human-readable error messages from both Soroban simulation errors and transaction submission failures.

Adds early simResult.error checking before calling assembleTransaction() to prevent exposing internal SDK objects
Implements parseSimulationError() to extract meaningful messages from Soroban diagnostic event logs
Implements sanitizeReason() to strip provider wrapper text from transaction errors

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.

File	Description
test/error-sanitization.test.ts	Comprehensive test suite covering both error sanitization functions with various edge cases
src/plugin/simulation.ts	Adds `parseSimulationError()` function and updates error handling to check simulation errors before assembly
src/plugin/submit.ts	Adds `sanitizeReason()` function and applies it to transaction failure messages before throwing errors

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-05T12:04:50Z

src/plugin/simulation.ts

+  const arrayMatch = error.match(/data:\s*\["([^"]+)"/);
+  if (arrayMatch?.[1] && arrayMatch[1].length > 3) {
+    return errorType ? `${arrayMatch[1]} (${errorType})` : arrayMatch[1];
+  }
+
+  const stringMatch = error.match(/data:\s*"([^"]+)"/);


The regex pattern data:\s*\["([^"]+)" will fail to match data arrays that contain escaped quotes within the message string. For example, if the diagnostic event contains a message like ["error with \"quotes\"", more data], the pattern will stop at the first escaped quote instead of capturing the full message. Consider using a more robust parsing approach or updating the regex to handle escaped characters.

Suggested change

const arrayMatch = error.match(/data:\s*\["([^"]+)"/);

if (arrayMatch?.[1] && arrayMatch[1].length > 3) {

return errorType ? `${arrayMatch[1]} (${errorType})` : arrayMatch[1];

}

const stringMatch = error.match(/data:\s*"([^"]+)"/);

const arrayMatch = error.match(/data:\s*\["((?:[^"\\]|\\.)*)"/);

if (arrayMatch?.[1] && arrayMatch[1].length > 3) {

return errorType ? `${arrayMatch[1]} (${errorType})` : arrayMatch[1];

}

const stringMatch = error.match(/data:\s*"((?:[^"\\]|\\.)*)"/);

Copilot · 2026-01-05T12:04:50Z

src/plugin/simulation.ts

+  const arrayMatch = error.match(/data:\s*\["([^"]+)"/);
+  if (arrayMatch?.[1] && arrayMatch[1].length > 3) {
+    return errorType ? `${arrayMatch[1]} (${errorType})` : arrayMatch[1];
+  }
+
+  const stringMatch = error.match(/data:\s*"([^"]+)"/);


The regex pattern data:\s*"([^"]+)" will fail to match data strings that contain escaped quotes. For example, if the diagnostic event contains a message like "error with \"quotes\"", the pattern will stop at the first escaped quote instead of capturing the full message. Consider using a more robust parsing approach or updating the regex to handle escaped characters.

Suggested change

const arrayMatch = error.match(/data:\s*\["([^"]+)"/);

if (arrayMatch?.[1] && arrayMatch[1].length > 3) {

return errorType ? `${arrayMatch[1]} (${errorType})` : arrayMatch[1];

}

const stringMatch = error.match(/data:\s*"([^"]+)"/);

const arrayMatch = error.match(/data:\s*\["((?:[^"\\]|\\.)*)"/);

if (arrayMatch?.[1] && arrayMatch[1].length > 3) {

return errorType ? `${arrayMatch[1]} (${errorType})` : arrayMatch[1];

}

const stringMatch = error.match(/data:\s*"((?:[^"\\]|\\.)*)"/);

Copilot · 2026-01-05T12:04:50Z

src/plugin/submit.ts

  return data !== null && typeof data === 'object' && 'signature' in data && 'signedXdr' in data;
 }
+
+/** Strip provider wrapper text, extract last segment (e.g., "TxInsufficientBalance") */


The comment says "Strip provider wrapper text, extract last segment" but the function also performs length validation (must be > 2 characters) and checks that the segment doesn't contain "provider". The comment should be updated to reflect all the conditions that determine whether the last segment is returned or the fallback is used.

Suggested change

/** Strip provider wrapper text, extract last segment (e.g., "TxInsufficientBalance") */

/**

* Normalize an error reason by stripping provider wrapper text.

* - Split on colon and trim whitespace, then consider the last segment (e.g., "TxInsufficientBalance").

* - Return the last segment only if it is longer than 2 characters and does not contain "provider" (case-insensitive).

* - Otherwise, return the original reason, truncated to 100 characters with "..." if it is longer than 100 characters.

*/

Copilot · 2026-01-05T12:04:51Z

src/plugin/simulation.ts

+  const errorType = firstLine.match(/Error\(([^)]+)\)/)?.[1];
+
+  const arrayMatch = error.match(/data:\s*\["([^"]+)"/);
+  if (arrayMatch?.[1] && arrayMatch[1].length > 3) {


The test comment claims "Messages <= 3 chars are ignored" but the implementation checks length > 3 which means messages with exactly 3 characters will also be ignored. If the intent is to ignore messages with 3 or fewer characters, the comment is correct but misleading. If the intent is to accept 3-character messages, the implementation should use length >= 3 instead.

Copilot · 2026-01-05T12:04:51Z

src/plugin/submit.ts

+export function sanitizeReason(reason: string): string {
+  const segments = reason.split(/:\s*/);
+  const last = segments[segments.length - 1]?.trim();
+  if (last && last.length > 2 && !last.toLowerCase().includes('provider')) {


The condition checks last.length > 2 which means messages with exactly 2 characters are filtered out, while the test at line 124 expects empty strings to pass through. The logic is inconsistent - a 1 or 2 character error code like "OK" or "NO" would trigger the fallback path, but an empty string (length 0) would also satisfy !last and trigger the fallback. Consider if the length check threshold of 2 is appropriate for all use cases.

Suggested change

if (last && last.length > 2 && !last.toLowerCase().includes('provider')) {

if (last && !last.toLowerCase().includes('provider')) {

Copilot · 2026-01-05T12:04:51Z

src/plugin/simulation.ts

+    throw pluginError('Simulation RPC failed', {
      code: 'SIMULATION_RPC_FAILURE',
+      status: HTTP_STATUS.BAD_GATEWAY,
+      details: { message: 'RPC provider error' },


The error message was changed from "Simulation RPC execution failed" to "Simulation RPC failed", and the details object now only contains a generic message "RPC provider error" instead of the specific RPC error details (rpcCode, message, description/data). This removes valuable debugging information that could help diagnose RPC-level failures. Consider preserving at least the message field from the RPC error response while still sanitizing the output appropriately.

Suggested change

details: { message: 'RPC provider error' },

details: {

// Preserve the RPC error message for debugging while keeping the top-level error generic.

message: typeof message === 'string' && message.length > 0 ? message : 'RPC provider error',

},

@copilot details for the rpc error is added to console.error above and want to keep this generic as to not expose details to the client

Copilot · 2026-01-05T12:04:52Z

src/plugin/simulation.ts

+  const arrayMatch = error.match(/data:\s*\["([^"]+)"/);
+  if (arrayMatch?.[1] && arrayMatch[1].length > 3) {
+    return errorType ? `${arrayMatch[1]} (${errorType})` : arrayMatch[1];
+  }
+
+  const stringMatch = error.match(/data:\s*"([^"]+)"/);
+  if (stringMatch?.[1] && stringMatch[1].length > 3) {
+    return errorType ? `${stringMatch[1]} (${errorType})` : stringMatch[1];


The function performs three regex matches sequentially on potentially large error strings. For better performance, consider using a single regex with capture groups or using String.prototype.indexOf() to locate the relevant sections before applying regex matches. This would be more efficient especially for long error messages with extensive event logs.

Suggested change

const arrayMatch = error.match(/data:\s*\["([^"]+)"/);

if (arrayMatch?.[1] && arrayMatch[1].length > 3) {

return errorType ? `${arrayMatch[1]} (${errorType})` : arrayMatch[1];

}

const stringMatch = error.match(/data:\s*"([^"]+)"/);

if (stringMatch?.[1] && stringMatch[1].length > 3) {

return errorType ? `${stringMatch[1]} (${errorType})` : stringMatch[1];

// Use a single regex with capture groups to extract either array-style or string-style data

const dataMatch = error.match(/data:\s*(?:\["([^"]+)"|"([^"]+)")/);

const extracted = dataMatch?.[1] ?? dataMatch?.[2];

if (extracted && extracted.length > 3) {

return errorType ? `${extracted} (${errorType})` : extracted;

Copilot · 2026-01-05T12:04:52Z

test/error-sanitization.test.ts

+  test('filters out segments containing provider', () => {
+    const reason = 'Error: provider error';
+    // Last segment contains "provider", should fallback
+    expect(sanitizeReason(reason)).toBe('Error: provider error');
+  });


The test expects that when the last segment contains "provider", the entire original reason should be returned (line 107). However, looking at the implementation in submit.ts (lines 143-150), when the last segment contains "provider" OR when the segment is too short, the fallback returns either the truncated reason (if > 100 chars) or the full reason. This test passes only because "Error: provider error" is less than 100 characters. The test should be clearer about what behavior it's actually testing - it seems to be testing the fallback path but doesn't verify truncation isn't applied in this case.

Copilot · 2026-01-05T12:04:52Z

src/plugin/submit.ts

+      const rawReason = final.status_reason || 'Transaction failed';
+      console.error(`[channels] Transaction failed: ${rawReason}`);
+      const reason = sanitizeReason(rawReason);


The variable name rawReason might be misleading since it contains final.status_reason || 'Transaction failed' which already includes a fallback. Consider naming it something like statusReason or originalReason to better reflect that it's the unsanitized reason from the status response.

Suggested change

const rawReason = final.status_reason || 'Transaction failed';

console.error(`[channels] Transaction failed: ${rawReason}`);

const reason = sanitizeReason(rawReason);

const statusReason = final.status_reason || 'Transaction failed';

console.error(`[channels] Transaction failed: ${statusReason}`);

const reason = sanitizeReason(statusReason);

Copilot · 2026-01-05T12:04:52Z

test/error-sanitization.test.ts

+  test('ignores short data messages', () => {
+    // Messages <= 3 chars are ignored (likely not human-readable)
+    const error = `HostError: Error(Test, Code)
+
+Event log:
+   0: [Diagnostic Event] data:"ab"`;
+
+    expect(parseSimulationError(error)).toBe('HostError: Error(Test, Code)');
+  });


The test description says "ignores short data messages" and the comment claims "Messages <= 3 chars are ignored", but the test only verifies that a 2-character message ("ab") is ignored. To properly test the boundary condition, add a test case with a 3-character message to verify it's also ignored, and a 4-character message to verify it's accepted. This would confirm the implementation's length > 3 check is correct.

Copilot · 2026-01-07T05:27:46Z

@tirumerla I've opened a new pull request, #52, to work on those changes. Once the pull request is ready, I'll request review from you.

tirumerla

thanks, lgtm. Please take a look at regex comments from copilot.

tirumerla · 2026-01-07T05:29:40Z

src/plugin/simulation.ts

+    throw pluginError('Simulation failed', {
+      code: 'SIMULATION_FAILED',
      status: HTTP_STATUS.BAD_REQUEST,
-      details: {
-        rpcCode: code,
-        message,
-        description: description || data,
-      },
+      details: { error: parseSimulationError(simResult.error) },


Should we add console.error log here

tirumerla · 2026-01-07T05:29:52Z

src/plugin/simulation.ts

+    throw pluginError('Simulation failed', {
      code: 'SIMULATION_FAILED',


Signed-off-by: Dylan Kilkenny <dylankilkenny95@gmail.com>

tirumerla

lgtm

Copilot AI review requested due to automatic review settings January 5, 2026 12:01

Copilot started reviewing on behalf of dylankilkenny January 5, 2026 12:01 View session

Copilot AI reviewed Jan 5, 2026

View reviewed changes

Copilot AI mentioned this pull request Jan 7, 2026

Clarify RPC error handling design in simulation.ts #52

Draft

tirumerla reviewed Jan 7, 2026

View reviewed changes

fix: add error logging and handle escaped quotes in regex

a6214b7

Signed-off-by: Dylan Kilkenny <dylankilkenny95@gmail.com>

tirumerla approved these changes Jan 8, 2026

View reviewed changes

dylankilkenny merged commit 0799579 into main Jan 8, 2026
11 checks passed

This was referenced Feb 11, 2026

feat: Read only logic for transactions #66

Merged

chore: Add more context to submission failure log #74

Closed

feat: Enforce soroban auth simulation & Improve inner failure details #78

Merged

-/** Strip provider wrapper text, extract last segment (e.g., "TxInsufficientBalance") */
+/**
+ * Normalize an error reason by stripping provider wrapper text.
+ * - Split on colon and trim whitespace, then consider the last segment (e.g., "TxInsufficientBalance").
+ * - Return the last segment only if it is longer than 2 characters and does not contain "provider" (case-insensitive).
+ * - Otherwise, return the original reason, truncated to 100 characters with "..." if it is longer than 100 characters.
+ */

	if (last && last.length > 2 && !last.toLowerCase().includes('provider')) {
	if (last && !last.toLowerCase().includes('provider')) {

		throw pluginError('Simulation failed', {
		code: 'SIMULATION_FAILED',

Conversation

dylankilkenny commented Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Walkthrough

Changes

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

tirumerla Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Jan 7, 2026

Uh oh!

tirumerla left a comment

Choose a reason for hiding this comment

Uh oh!

tirumerla Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

tirumerla Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

tirumerla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dylankilkenny commented Jan 5, 2026 •

edited

Loading

coderabbitai bot commented Jan 5, 2026 •

edited

Loading