Add external read-only contract storage access#1691
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR adds new host APIs (and corresponding test Wasm) to support “try-get” style reads that preserve stored Void values, and introduces protocol-28-gated external contract storage reads that record read-only footprint entries.
Changes:
- Added
try_get_contract_dataand external contract data read host functions (has/get/try_get) gated at protocol 28. - Introduced a new test contract Wasm (
test_external_storage) and wired it into the test Wasm workspace and exported byte-include list. - Expanded host tests and added an ignored benchmark to validate footprint behavior and compare performance.
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| soroban-test-wasms/wasm-workspace/external_storage/src/lib.rs | New test contract that reads another contract’s storage via *_external SDK APIs. |
| soroban-test-wasms/wasm-workspace/external_storage/Cargo.toml | Defines new test Wasm crate and enables soroban-sdk next feature. |
| soroban-test-wasms/wasm-workspace/Cargo.toml | Adds external_storage workspace member and bumps env-common dep for building protocol-28 Wasms. |
| soroban-test-wasms/src/lib.rs | Exposes protocol-28 optimized Wasm bytes for host tests. |
| soroban-env-host/src/test/storage.rs | Adds tests for stored-Void semantics, external reads, and footprint recording. |
| soroban-env-host/src/test/protocol_gate.rs | Adds protocol gating test for external contract data reads in native mode. |
| soroban-env-host/src/test/metering_benchmark.rs | Adds an ignored benchmark comparing external read path vs contract view call. |
| soroban-env-host/src/host/data_helper.rs | Refactors put_contract_data to update existing ledger entries via a single full-entry read. |
| soroban-env-host/src/host/conversion.rs | Adds helper to build ledger keys from (contract address, key) pairs for external reads. |
| soroban-env-host/src/host.rs | Implements new VM host functions: try_get_contract_data and external read variants. |
| soroban-env-host/src/builtin_contracts/storage_utils.rs | Adds internal helpers for try-get value retrieval and external contract data reads. |
| soroban-env-host/src/builtin_contracts/stellar_asset_contract/balance.rs | Switches internal reads to the new try_get_contract_data_value helper. |
| soroban-env-host/src/builtin_contracts/stellar_asset_contract/allowance.rs | Switches internal reads to the new try_get_contract_data_value helper. |
| soroban-env-common/env.json | Declares new host functions and their protocol gating / docs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn try_get_external_instance_data_value( | ||
| &self, | ||
| contract: AddressObject, | ||
| k: Val, | ||
| ) -> Result<Option<Val>, HostError> { | ||
| let contract_id = self.contract_id_from_address(contract)?; | ||
| let instance_key = self.contract_instance_ledger_key(&contract_id)?; | ||
| let entry = self | ||
| .try_borrow_storage_mut()? | ||
| .try_get(&instance_key, self, None)?; | ||
| let Some(entry) = entry else { | ||
| return Ok(None); | ||
| }; | ||
| let instance = self.extract_contract_instance_from_ledger_entry(&entry)?; | ||
| let instance_storage = InstanceStorageMap::from_instance_xdr(&instance, self)?; | ||
| instance_storage | ||
| .map | ||
| .get(&k, self) | ||
| .map(|value| value.copied()) | ||
| } |
| self.try_get_external_contract_data_value(contract, k, t)? | ||
| .ok_or_else(|| { | ||
| self.err( | ||
| ScErrorType::Storage, | ||
| ScErrorCode::MissingValue, | ||
| "key is missing from external contract storage", | ||
| &[contract.to_val(), k], | ||
| ) | ||
| }) |
| pub(crate) fn try_get_contract_data_value( | ||
| &self, | ||
| k: Val, | ||
| t: StorageType, | ||
| ) -> Result<Option<Val>, HostError> { | ||
| if self.has_contract_data(k, t)?.into() { | ||
| Ok(Some(self.get_contract_data(k, t)?)) | ||
| } else { | ||
| Ok(None) | ||
| match t { | ||
| StorageType::Temporary | StorageType::Persistent => { | ||
| let key = self.storage_key_from_val(k, t.try_into()?)?; | ||
| let entry = self | ||
| .try_borrow_storage_mut()? | ||
| .try_get(&key, self, Some(k))?; | ||
| entry | ||
| .map(|entry| self.contract_data_entry_value(&entry.data)) | ||
| .transpose() | ||
| } | ||
| StorageType::Instance => { | ||
| self.with_instance_storage(|s| s.map.get(&k, self).map(|v| v.copied())) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn try_get_external_contract_data_value( | ||
| &self, | ||
| contract: AddressObject, | ||
| k: Val, | ||
| t: StorageType, | ||
| ) -> Result<Option<Val>, HostError> { | ||
| match t { | ||
| StorageType::Temporary | StorageType::Persistent => { | ||
| let key = self.storage_key_from_address_val(contract, k, t.try_into()?)?; | ||
| let entry = self | ||
| .try_borrow_storage_mut()? | ||
| .try_get(&key, self, Some(k))?; | ||
| entry | ||
| .map(|entry| self.contract_data_entry_value(&entry.data)) | ||
| .transpose() | ||
| } | ||
| StorageType::Instance => self.try_get_external_instance_data_value(contract, k), | ||
| } | ||
| } |
| let missing_key = Symbol::try_from_small_str("missing").unwrap().to_val(); | ||
| assert_eq!( | ||
| bool::from(host.has_external_contract_data(target_addr, missing_key, storage_type)?), | ||
| false | ||
| ); | ||
| assert!(HostError::result_matches_err( | ||
| host.get_external_contract_data(target_addr, missing_key, storage_type), | ||
| (ScErrorType::Storage, ScErrorCode::MissingValue) | ||
| )); |
| pub fn get_persistent(env: Env, contract: Address, key: Symbol) -> u64 { | ||
| env.storage() | ||
| .persistent() | ||
| .get_external(&contract, &key) | ||
| .unwrap() | ||
| } |
![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: 01341fe32b
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let entry = self | ||
| .try_borrow_storage_mut()? | ||
| .try_get(&instance_key, self, None)?; |
There was a problem hiding this comment.
When has/get_external_contract_data is used with StorageType::Instance for a contract that is currently on the call stack (including the current contract's own address), this reads the instance ledger entry instead of the frame's in-memory Context.storage. Instance storage writes are only persisted on frame exit, so a contract that does put_instance(k, v) and then reads get_external(self, k) in the same invocation will see the old ledger value or None rather than the just-written value. The external instance path should consult the active frame's instance storage when the target contract is active.
Useful? React with 👍 / 👎.
|
Thank you for opening this PR! However, as this is a protocol change, we need a CAP and a protocol discussion first. Would you mind starting a discussion at https://github.com/orgs/stellar/discussions? You're also welcome to make the respective CAP contribution at https://github.com/stellar/stellar-protocol/tree/master/core, but it's also fine to just start with the discussion and we can work on the CAP on our side (either way, it should be rather straightforward). Also, I must say that while the change is straightforward implementation-wise, there might be some security implications for the existing contracts, which is why the discussion is necessary, and it's also not clear if we can proceed with this approach in the first place. |
dmkozh
left a comment
dmkozh
left a comment
There was a problem hiding this comment.
Given my response on the PR re CAP, I haven't looked into this beyond the function interface (which is relevant for the discussion and spec).
| }, | ||
| { | ||
| "export": "k", | ||
| "name": "try_get_external_contract_data", |
There was a problem hiding this comment.
This interface is rather awkward and inconsistent with the existing storage functions. I think we should either:
- get rid of this function - has and get have us covered on the SDK-side for the
try_function implementation - if there is a good evidence that dedicated
try_function saves us a lot of instructions vs has + get, then we should return an 'option' value from the function instead of writing the output to linear memory. But I really think just dropping this is easier and not significantly more expensive.
Hey, I will start a discussion right away 👍 I drafted this PR quickly to get some feedback since I hit different issues building our lending protocol which went a bit over the limits and made my read more of how it actually works behind 🙏 |
BTW on this note, I'm not sure I trust the numbers you've shared in the PR - what was the methodology for collecting these? I would recommend testing your contract setup on the testnet or a local network and lookup the view call numbers that are coming from the simulation (e.g. compare a no-op function call with a function call that does a cross-contract call to a different function). The reason for why I'm skeptical about this is that 80M instructions seems like too much for a VM instantiation, even if your contract is large. I could be wrong, but it wouldn't hurt to double-check - the issue might not be as bad as tests present. |
You were right to be skeptical of the original numbers. The 80-85M figure was not a single simulation result; it was an aggregate across 100 iterations, and it measured the old cross-contract getter path rather than the new external read path. I reran the comparison using local P28 simulation/preflight with the local env + SDK + RPC changes. Default Rust/Wasm stack:
So external reads are roughly 2x cheaper than cross-contract getters for persistent/temporary storage, and still cheaper for instance storage. While looking into the memory numbers, I found that recent contracts built with the default Rust/Wasm settings seem to reserve about 1MiB of Wasm linear memory per contract invocation. That explains why cross-contract calls can become expensive in memory: each invoked contract brings its own baseline reservation. After digging into the compile flags, I rebuilt my own contracts with:
That reduced the baseline memory a lot. Cross-contract calls between contracts deployed with this smaller stack are much cheaper in metered memory. But if my contract calls an already deployed oracle contract that was built with the default stack, the call still pays that contract’s larger memory reservation. With
This matters for my lending deployment because instructions usually fit under the 400M limit, but many oracle price reads pushed memory toward the limit. So I have two questions:
|
I think 2x is not exactly the right number to look at here; in the end we're talking about a few hundred thousand instructions per call. I realize it's not nothing, but it's also not that high either compared to the limit.
Well, I haven't seen your exact setup/code, so I can't tell if it's fully accurate, but the numbers seem to be in the right ballpark, so this is probably good enough.
I don't think it's expected, but I need to double-check with the team. 1 MB seems excessive to me. 16K may or may not be sufficient. Default probably should be somewhere in-between, but closer to 16K - I wouldn't expect contracts to do many nested function calls and use many locals. Basically this requires some additional research on our side. But as long your contract works fine it definitely makes sense to reduce the stack size (if you can still upgrade it, that is). |
We’ll most likely deploy with the 16 KB Wasm page-size flag if final tests keep confirming it’s safe. This should help our lending protocol, but also anything built on top of it, like vaults or more complex position managers. What I’d like to check is whether teams like Reflector or RedStone could also safely build with this flag. If yes, it could help us, Blend, and other DeFi protocols support more positions per user and more complex logic. For reference, Rust’s Wasm target already exposes these kinds of low-level linker settings here: Not saying everyone should blindly use it, but it’s probably worth testing/reviewing at ecosystem level. |
Discussion: stellar/stellar-protocol#1958
What
Adds protocol-28 Soroban host functions for read-only external contract storage access:
has_external_contract_data(contract, key, storage_type)get_external_contract_data(contract, key, storage_type)Persistent and temporary reads resolve to the target contract-data key. Instance reads resolve to the target contract instance entry and look up the embedded instance-storage map.
This also adds host tests, protocol-gate coverage, dispatch coverage, and a Wasm fixture importing the new host functions.
Why
Some contracts need read-only state owned by another contract, but do not need to execute that contract's Wasm. A lending controller is one example: it may need market config, reserve parameters, oracle references, collateral factors, or pause flags owned by market/governance/oracle contracts.
Today that read requires a cross-contract invoke to a getter. This PR adds a direct read-only path for intentionally public storage while preserving contract ownership boundaries.
Local P28 preflight measurement
Measured with local protocol-28 RPC preflight using two minimal contracts:
Store:noop, persistent getter, temporary getter, instance getterCaller:noop, cross-contract invoke-to-getter methods, direct external-storage read methodsThe instance case uses a 32-entry target instance map.
Result:
483,703instructions vs950,847for cross-contract getter.483,703instructions vs950,843for cross-contract getter.586,622instructions vs929,322for cross-contract getter.Persistent and temporary external reads are close to a direct getter baseline and avoid the nested VM dispatch plus extra footprint entries from invoking the target contract. Instance reads still save work compared to a getter call, but are less close to native reads because the target instance entry and storage map must be loaded and searched.
Open protocol questions
Limitations
Validation
cargo test -p soroban-env-host external_contract_data --features nextcargo check -p soroban-sdk --features nextcargo test -p soroban-sdk --test external_storage_next --features "next testutils"git diff --check