Handle built-in Stellar Asset Contracts (SACs) in contract.Client.from#1501
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves Soroban contract client creation and RPC Wasm fetching behavior by correctly handling built-in Stellar Asset Contracts (SACs), which do not have on-chain Wasm bytecode.
Changes:
- Updated
contract.Client.fromto detect SAC instances and build clients from the embeddedSAC_SPECinstead of attempting to fetch Wasm. - Updated
Server#getContractWasmByContractIdto fail fast for SACs with a clear, actionable error message. - Added/updated unit tests to cover SAC handling for both
Client.fromandgetContractWasmByContractId, plus updated generated reference docs accordingly.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/contract/client.ts |
Routes SAC contracts through embedded SAC_SPEC instead of Wasm download logic. |
src/rpc/server.ts |
Adds SAC guard + clearer error for getContractWasmByContractId. |
test/unit/contract/client_from.test.ts |
New unit coverage for Client.from (Wasm baseline + SAC case). |
test/unit/server/soroban/get_contract_wasm.test.ts |
Adds SAC-specific failure-mode test for Wasm fetching by contract ID. |
docs/reference/contracts-client.md |
Updates generated reference to document SAC behavior in Client.from. |
docs/reference/network-rpc.md |
Updates generated reference to document SAC limitation in getContractWasmByContractId. |
.gitignore |
Ignores scratch/ directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 332c637ed5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!response.entries.length || !response.entries[0]?.val) { | ||
| return Promise.reject({ | ||
| code: 404, | ||
| message: "Could not obtain contract instance from server", | ||
| }); | ||
| } |
There was a problem hiding this comment.
I think it's more accurate to say that the contract instance wasn't found, but the previous message used hash. Not sure if it's important to keep the same message. What do you think, @Ryang-21 ? 🙏
There was a problem hiding this comment.
I think this is OK. Something we could consider for the next release is to move away from error messages all together and create custom error objects that extend from the generic.
Ryang-21
left a comment
Ryang-21
left a comment
There was a problem hiding this comment.
I left a comment for the dynamic import but everything else looks good
…1502) * Add rpc.Server.queryContract for one-line read-only contract calls * Sanitize method name * Check only contract methods before invoking * e2e test happy path * Make contract.Client factories generic for typed runtime clients (#1504) * Make contract.Client factories generic for typed runtime clients * Add `rpc.Server.getContractMethods` for contract method discovery (#1505) * Add rpc.Server.getContractMethods for contract method discovery * Document queryContract and getContractMethods in the invoke-a-contract guide * Type the contract.Client examples in the invoke and auth guides
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Updates
Client.fromnow supports SACs. It fetches the contract instance, and when the executable isStellarAssetit builds the client from the embedded SAC spec; otherwise it loads the Wasm and parses the spec as before. This mirrors whatBindingGenerator.fromContractIdand stellar-cli already do.Server.getContractInstance(contractId). A new public method that fetches the contract instance ledger entry and returns itsxdr.ScContractInstance. BothClient.fromandgetContractWasmByContractIdbranch off it, so the historical{ code: 404, message }rejection for a missing instance lives in exactly one place. This preserves the pre-existing error shape for consumers that inspecterr.code(no behavioral change on failure paths).Server.getContractWasmByContractIdnow throws a clear error for a SAC (which has no Wasm bytecode), pointing callers toClient.from, instead of the cryptic XDR crash. Otherwise unchanged and non-breaking.Client.fromloads the ~9.5 KBSAC_SPECvia dynamicimport()inside the SAC branch, so bundlers can code-split it out of the commoncontract/clientpath; it's only fetched when a SAC is actually built.inlineDynamicImports: trueon the UMD dist outputs inrollup.config.mjsso the lazily-imported SAC spec stays in a single bundle (explicit guarantee against Rollup's default behavior; bundles are byte-identical).Backward compatibility
{ code: 404, message }rejection shape (missing instance / missing Wasm) and the same invalid-contract-id behavior.getContractWasmByContractIdon a SAC now throwing a descriptive error.Testing
test/unit/contract/client_from.test.ts: a Wasm baseline plus a SAC case. The SAC case reproduced the exact issue error before the fix and is green after. Fixtures are built in-memory (pureBufferops) so the file runs under both the node and browser unit environments.test/unit/server/soroban/get_contract_wasm.test.ts.