feat: Enforce soroban auth simulation & Improve inner failure details#78
feat: Enforce soroban auth simulation & Improve inner failure details#78
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR enhances the plugin's simulation and transaction submission flows by introducing enforced authentication mode validation, classifying auth-related simulation failures into dedicated error codes, and enriching transaction error diagnostics with Stellar Lab URLs for easier debugging. Changes
Sequence DiagramsequenceDiagram
actor Plugin
participant Simulation as Simulation Service
participant Classifier as Error Classifier
participant Plugin2 as Submit Handler
participant StellarLab as Stellar Lab URL
Plugin->>Simulation: simulateTransaction(tx, authMode: 'enforce')
alt Simulation Success
Simulation-->>Plugin: Success result
else Simulation Error
Simulation-->>Classifier: Return error response
Classifier->>Classifier: Classify error pattern
alt Auth Validation Failed
Classifier-->>Plugin: SIGNED_AUTH_VALIDATION_FAILED
else Other Failure
Classifier-->>Plugin: SIMULATION_FAILED
end
Plugin->>Plugin2: Submit with classified error
Plugin2->>StellarLab: Generate diagnostic URL
Plugin2-->>Plugin: Enhanced error with labUrl
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/plugin/submit.ts (1)
207-222: URL construction uses Stellar Lab-specific encoding.The double-slash encoding (
https:////) and manual parameter encoding appear to be specific to Stellar Lab's expected URL format. ThetxHashis passed without encoding, which is safe since transaction hashes are always hex strings.Consider adding a brief comment explaining why the unusual
https:////encoding is used, for future maintainability.📝 Suggested comment for clarity
+// Stellar Lab expects double-slashed protocol in URL params (https:////) const horizonParam = horizonUrl.replace('https://', 'https:////'); const rpcParam = rpcUrl.replace('https://', 'https:////');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugin/submit.ts` around lines 207 - 222, Add a short clarifying comment inside buildStellarLabTransactionUrl explaining that the horizonUrl.replace('https://', 'https:////') and rpcUrl.replace('https://', 'https:////') transformations are intentional to match Stellar Lab's URL encoding scheme (it expects four slashes for its network params), and note that txHash is left unencoded because transaction hashes are hex strings; place the comment next to the horizonParam, rpcParam, and passphraseParam lines so future readers understand the nonstandard encoding and manual replacements.src/plugin/simulation.ts (1)
203-223: Classification patterns are reasonable but/\bsignature\b/iis broad.The classification logic correctly gates on
AUTH_MODE === 'enforce'before categorizing asSIGNED_AUTH_VALIDATION_FAILED. The regex patterns cover common auth-related error patterns from Stellar RPC.The
/\bsignature\b/ipattern is the broadest and could theoretically match non-auth errors containing "signature", though in practice Stellar simulation errors follow predictable formats. Consider if this is intentional or if it should be more specific (e.g.,/signature.*expired|invalid.*signature/i).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugin/simulation.ts` around lines 203 - 223, Narrow the overly broad /\bsignature\b/i check inside classifySimulationFailure by replacing it with more specific auth-related signature patterns (e.g., match phrases like "invalid signature", "signature expired", "bad_signature" or similar Stellar RPC token formats) so the isEnforcedAuthValidation branch only triggers for true auth signature errors; update the regex list used in the isEnforcedAuthValidation boolean in classifySimulationFailure (which already checks SIMULATION.AUTH_MODE) to include these targeted patterns instead of the single broad word match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/plugin/simulation.ts`:
- Around line 203-223: Narrow the overly broad /\bsignature\b/i check inside
classifySimulationFailure by replacing it with more specific auth-related
signature patterns (e.g., match phrases like "invalid signature", "signature
expired", "bad_signature" or similar Stellar RPC token formats) so the
isEnforcedAuthValidation branch only triggers for true auth signature errors;
update the regex list used in the isEnforcedAuthValidation boolean in
classifySimulationFailure (which already checks SIMULATION.AUTH_MODE) to include
these targeted patterns instead of the single broad word match.
In `@src/plugin/submit.ts`:
- Around line 207-222: Add a short clarifying comment inside
buildStellarLabTransactionUrl explaining that the horizonUrl.replace('https://',
'https:////') and rpcUrl.replace('https://', 'https:////') transformations are
intentional to match Stellar Lab's URL encoding scheme (it expects four slashes
for its network params), and note that txHash is left unencoded because
transaction hashes are hex strings; place the comment next to the horizonParam,
rpcParam, and passphraseParam lines so future readers understand the nonstandard
encoding and manual replacements.
|
|
||
| return { | ||
| feeCharged: Number(result.feeCharged().toBigInt()), | ||
| resultCode, |
There was a problem hiding this comment.
I think we should simplify response. We can remove new fields and jut use resultCode. If innerResultCode exists we use that for resultCode, if not we just use outerResultCode.
There was a problem hiding this comment.
Or we can mix it resultCode = {outerResultCode}:innerResultCode only when innerResultCode exists
src/plugin/simulation.ts
Outdated
|
|
||
| if (isEnforcedAuthValidation) { | ||
| return { | ||
| code: 'SIGNED_AUTH_VALIDATION_FAILED', |
There was a problem hiding this comment.
Maybe to prefix with SIMULATION_
src/plugin/constants.ts
Outdated
| MIN_TIME_BOUND: 0, | ||
| MAX_TIME_BOUND_OFFSET_SECONDS: 120, | ||
| MAX_FUTURE_TIME_BOUND_SECONDS: 120, | ||
| AUTH_MODE: 'enforce', |
src/plugin/submit.ts
Outdated
| hash: final.hash ?? null, | ||
| outerResultCode: decoded?.outerResultCode ?? null, | ||
| innerResultCode: decoded?.innerResultCode ?? null, | ||
| labUrl, |
There was a problem hiding this comment.
Does it make sense to add bit description to labUrl field like:
labUrl: Debug this failure in Stellar Lab: ${labUrl}
There was a problem hiding this comment.
Pull request overview
This PR enhances Soroban transaction simulation and error reporting by enforcing authentication validation during simulation and providing better debugging tools for transaction failures.
Changes:
- Added enforce mode for auth validation during transaction simulation to catch signature-related issues earlier
- Enhanced error diagnostics for fee bump inner failures by unwrapping to show actual failure codes
- Integrated Stellar Lab URLs in error messages for easier transaction debugging
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/plugin/constants.ts | Added SIMULATION_AUTH_MODE constant set to 'enforce' |
| src/plugin/simulation.ts | Added authMode parameter to simulation calls and new error classification for auth validation failures |
| src/plugin/submit.ts | Added buildStellarLabTransactionUrl function, enhanced error messages with Lab URLs, improved fee bump inner failure result code formatting |
| test/read-only.test.ts | Added test for auth validation error mapping and verification of authMode parameter passing |
| test/error-sanitization.test.ts | Updated assertions and added tests for Stellar Lab URL generation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary by CodeRabbit
New Features
Bug Fixes