feat: Read only logic for transactions#66
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:
WalkthroughAdded read-only transaction detection to prevent unnecessary channel acquisition. New Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant Simulation
participant Relayer
participant Channel
Client->>Handler: submitSorobanTransaction
Handler->>Simulation: simulateReadOnlyCheck
Simulation->>Relayer: simulateTransaction (read-only check)
Relayer-->>Simulation: simulation result
alt isReadOnly detected
Simulation-->>Handler: {isReadOnly: true, returnValue, latestLedger}
Handler-->>Client: {status: 'readonly', returnValue, latestLedger, transactionId: null, hash: null}
else requires submission
Simulation-->>Handler: {isReadOnly: false}
Handler->>Channel: acquireChannel
Handler->>Relayer: submitTransaction
Relayer-->>Handler: submission result
Handler-->>Client: result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/plugin/simulation.ts`:
- Around line 109-115: ReadOnlyCheckResult declares latestLedger: number but
simResult.latestLedger (from RawSimulateTransactionResponse) can be undefined;
either make ReadOnlyCheckResult.latestLedger optional (latestLedger?: number) to
match the source type, or when building/returning the ReadOnlyCheckResult set
latestLedger using a safe fallback (e.g., latestLedger: simResult.latestLedger
?? 0) so you never assign undefined; locate the assignment where the
ReadOnlyCheckResult is constructed/returned (the code that reads
simResult.latestLedger) and apply one of these two fixes.
src/plugin/handler.ts
Outdated
| tracker?: FeeTracker | ||
| ): Promise<ChannelAccountsResponse> { | ||
| // Read-only check: simulate before acquiring a channel to detect read-only calls | ||
| const readOnlyCheck = await simulateReadOnlyCheck(func, auth, fundAddress, fundRelayer, networkPassphrase); |
There was a problem hiding this comment.
We are simulating below this also, lets check if we can extract smulation result somehow to reuse for both so we only have to simulate once. Its a network call so we should try and only do it once
Summary:
Summary by CodeRabbit
New Features
Tests