Refactor: Unified Soroban Client & Generic Contract Call Tool#46
Open
wolf1276 wants to merge 3 commits intoStellar-Tools:mainfrom
Open
Refactor: Unified Soroban Client & Generic Contract Call Tool#46wolf1276 wants to merge 3 commits intoStellar-Tools:mainfrom
wolf1276 wants to merge 3 commits intoStellar-Tools:mainfrom
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
9 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/stakeF.ts">
<violation number="1" location="lib/stakeF.ts:45">
P2: Catch blocks assume thrown values always have `.message`, which can return `undefined` for non-`Error` throws and regress error reporting.</violation>
</file>
<file name="tools/soroban.ts">
<violation number="1" location="tools/soroban.ts:6">
P2: RPC URL and network passphrase are derived independently, allowing testnet RPC + mainnet passphrase mismatch when `SOROBAN_RPC_URL` is unset.</violation>
</file>
<file name="lib/contract.ts">
<violation number="1" location="lib/contract.ts:52">
P2: Catch blocks unsafely assume thrown values always have `.message`, which can throw in the error handler and hide the original exception.</violation>
</file>
<file name="tests/unit/tools/soroban.test.ts">
<violation number="1" location="tests/unit/tools/soroban.test.ts:8">
P2: Success-path test does not verify that request fields are forwarded to `SorobanClient.call`, so parameter-mapping regressions can pass unnoticed.</violation>
</file>
<file name="lib/soroban.ts">
<violation number="1" location="lib/soroban.ts:80">
P1: Transaction source is caller-dependent, but signing always uses a fixed private key, causing source/signer mismatch and likely auth failures for non-matching callers.</violation>
</file>
<file name="tools/balance.ts">
<violation number="1" location="tools/balance.ts:36">
P1: Non-native asset lookup is ambiguous because issuer is optional, so balance can be returned for the wrong issuer when asset codes collide.</violation>
<violation number="2" location="tools/balance.ts:74">
P2: Non-native balances are incorrectly forced into credit-asset format, which can misreport balances like liquidity pool shares.</violation>
</file>
<file name="agent.ts">
<violation number="1" location="agent.ts:144">
P1: New AgentClient wrappers bypass client network/rpc configuration, causing balance and Soroban calls to use env/default network instead of `this.network`/`this.rpcUrl`.</violation>
<violation number="2" location="agent.ts:169">
P2: `sorobanCall` does not validate `publicKey`/`caller`, so an empty caller can pass through and fail at runtime in Soroban account loading.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| throw new Error(`Failed to prepare transaction: ${err.message}`); | ||
| }); | ||
|
|
||
| const signedXDR = signTransaction(preparedTx.toXDR(), this.networkPassphrase); |
There was a problem hiding this comment.
P1: Transaction source is caller-dependent, but signing always uses a fixed private key, causing source/signer mismatch and likely auth failures for non-matching callers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/soroban.ts, line 80:
<comment>Transaction source is caller-dependent, but signing always uses a fixed private key, causing source/signer mismatch and likely auth failures for non-matching callers.</comment>
<file context>
@@ -0,0 +1,121 @@
+ throw new Error(`Failed to prepare transaction: ${err.message}`);
+ });
+
+ const signedXDR = signTransaction(preparedTx.toXDR(), this.networkPassphrase);
+ const tx = TransactionBuilder.fromXDR(signedXDR, this.networkPassphrase);
+
</file context>
| const errorMessage = error instanceof Error ? error.message : String(error); | ||
| return errorMessage; | ||
| } catch (error: any) { | ||
| return error.message; |
There was a problem hiding this comment.
P2: Catch blocks assume thrown values always have .message, which can return undefined for non-Error throws and regress error reporting.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/stakeF.ts, line 45:
<comment>Catch blocks assume thrown values always have `.message`, which can return `undefined` for non-`Error` throws and regress error reporting.</comment>
<file context>
@@ -27,123 +34,57 @@ import {
- const errorMessage = error instanceof Error ? error.message : String(error);
- return errorMessage;
+ } catch (error: any) {
+ return error.message;
}
}
</file context>
Suggested change
| return error.message; | |
| return error instanceof Error ? error.message : String(error); |
| import { SorobanClient } from "../lib/soroban"; | ||
| import { Networks, nativeToScVal } from "@stellar/stellar-sdk"; | ||
|
|
||
| const rpcUrl = process.env.SOROBAN_RPC_URL || "https://soroban-testnet.stellar.org"; |
There was a problem hiding this comment.
P2: RPC URL and network passphrase are derived independently, allowing testnet RPC + mainnet passphrase mismatch when SOROBAN_RPC_URL is unset.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tools/soroban.ts, line 6:
<comment>RPC URL and network passphrase are derived independently, allowing testnet RPC + mainnet passphrase mismatch when `SOROBAN_RPC_URL` is unset.</comment>
<file context>
@@ -0,0 +1,50 @@
+import { SorobanClient } from "../lib/soroban";
+import { Networks, nativeToScVal } from "@stellar/stellar-sdk";
+
+const rpcUrl = process.env.SOROBAN_RPC_URL || "https://soroban-testnet.stellar.org";
+const networkPassphrase = process.env.STELLAR_NETWORK === "mainnet" ? Networks.PUBLIC : Networks.TESTNET;
+
</file context>
| const errorMessage = error instanceof Error ? error.message : String(error); | ||
| console.error("Failed to get share ID:", errorMessage); | ||
| } catch (error: any) { | ||
| console.error("Failed to get share ID:", error.message); |
There was a problem hiding this comment.
P2: Catch blocks unsafely assume thrown values always have .message, which can throw in the error handler and hide the original exception.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/contract.ts, line 52:
<comment>Catch blocks unsafely assume thrown values always have `.message`, which can throw in the error handler and hide the original exception.</comment>
<file context>
@@ -33,127 +40,16 @@ import {
- const errorMessage = error instanceof Error ? error.message : String(error);
- console.error("Failed to get share ID:", errorMessage);
+ } catch (error: any) {
+ console.error("Failed to get share ID:", error.message);
throw error;
}
</file context>
| vi.mock("../../../lib/soroban", () => { | ||
| return { | ||
| SorobanClient: vi.fn().mockImplementation(() => ({ | ||
| call: vi.fn().mockResolvedValue("mocked_result"), |
There was a problem hiding this comment.
P2: Success-path test does not verify that request fields are forwarded to SorobanClient.call, so parameter-mapping regressions can pass unnoticed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/unit/tools/soroban.test.ts, line 8:
<comment>Success-path test does not verify that request fields are forwarded to `SorobanClient.call`, so parameter-mapping regressions can pass unnoticed.</comment>
<file context>
@@ -0,0 +1,42 @@
+vi.mock("../../../lib/soroban", () => {
+ return {
+ SorobanClient: vi.fn().mockImplementation(() => ({
+ call: vi.fn().mockResolvedValue("mocked_result"),
+ })),
+ };
</file context>
| args: any[] = [], | ||
| operationType: string = "other" | ||
| ) { | ||
| const caller = this.publicKey; |
There was a problem hiding this comment.
P2: sorobanCall does not validate publicKey/caller, so an empty caller can pass through and fail at runtime in Soroban account loading.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At agent.ts, line 169:
<comment>`sorobanCall` does not validate `publicKey`/`caller`, so an empty caller can pass through and fail at runtime in Soroban account loading.</comment>
<file context>
@@ -130,6 +132,50 @@ export class AgentClient {
+ args: any[] = [],
+ operationType: string = "other"
+ ) {
+ const caller = this.publicKey;
+ return await stellarGenericSorobanCallTool.func({
+ caller,
</file context>
Suggested change
| const caller = this.publicKey; | |
| const caller = this.publicKey; | |
| if (!caller) { | |
| throw new Error("sorobanCall requires a configured publicKey (AgentConfig.publicKey or STELLAR_PUBLIC_KEY)"); | |
| } |
…hare support - Update network parsing to be more flexible (allow 'stellar-mainnet') - Detect and prevent mismatched asset matches by requiring/suggesting assetIssuer - Add support for formatting liquidity pool share balances - Ensure AgentClient balance methods respect client network/rpc config - Update unit tests to verify account loading and new balance logic
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tools/balance.ts">
<violation number="1" location="tools/balance.ts:38">
P2: Substring matching for network selection can route to mainnet for unintended inputs; use strict matching to avoid wrong endpoint selection.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| let horizonUrl = rpcUrl; | ||
| if (!horizonUrl) { | ||
| horizonUrl = actualNetwork.includes("mainnet") |
There was a problem hiding this comment.
P2: Substring matching for network selection can route to mainnet for unintended inputs; use strict matching to avoid wrong endpoint selection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tools/balance.ts, line 38:
<comment>Substring matching for network selection can route to mainnet for unintended inputs; use strict matching to avoid wrong endpoint selection.</comment>
<file context>
@@ -8,39 +8,73 @@ export const stellarGetBalanceTool = new DynamicStructuredTool({
+
+ let horizonUrl = rpcUrl;
+ if (!horizonUrl) {
+ horizonUrl = actualNetwork.includes("mainnet")
+ ? "https://horizon.stellar.org"
+ : "https://horizon-testnet.stellar.org";
</file context>
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
1 participant
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.
This PR introduces a major refactor and enhancement to the SDK's Soroban interaction capability.
Summary of Changes:
This improvement significantly reduces technical debt and makes the SDK future-proof for any Stellar-based protocol.
Summary by cubic
Unifies Soroban contract interactions into a single
SorobanClientand adds a generic contract call tool to call any contract with less code. Adds balance tools and agent methods, and refactors LP and Staking to the new client.New Features
lib/soroban.tswithSorobanClienthandling simulate, prepare, sign, submit, and result parsing.stellar_generic_soroban_callandAgentClient.sorobanCall()for any contract/function call.stellar_get_balanceandstellar_get_all_balances, plusAgentClient.getBalance()andgetAllBalances(); exported inindex.tswith unit tests.Bug Fixes
stellar-mainnet) and LP share formatting.assetIssuer.rpcUrlandnetworkthrough to tools; tests updated to cover new logic.Written for commit aa32e14. Summary will update on new commits.