Simulate tx auth v2 flag#783
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Plumbs an AuthV2 flag through the RPC preflight/simulation stack to allow recording Soroban auth entries using AddressV2 credentials on current protocol versions, while keeping previous protocols on v1 credentials.
Changes:
- Add
auth_v2/AuthV2flag through C FFI, Rust preflight, and Go preflight/simulateTransaction wiring. - Introduce protocol-version-specific helpers to construct recording auth mode (v2-aware in curr, ignored in prev).
- Add unit + integration tests covering flag forwarding and behavior across protocol versions; bump Soroban host/simulation deps to
27.0.0.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/stellar-rpc/lib/preflight/src/shared.rs | Pass use_address_v2 into recording auth mode construction. |
| cmd/stellar-rpc/lib/preflight/src/lib.rs | Add curr/prev make_recording_auth_mode helpers; thread auth_v2 through exported FFI entrypoint. |
| cmd/stellar-rpc/lib/preflight.h | Extend C API for preflight_invoke_hf_op with auth_v2 flag. |
| cmd/stellar-rpc/internal/preflight/preflight.go | Add AuthV2 parameter and pass it into the C call. |
| cmd/stellar-rpc/internal/preflight/pool.go | Thread AuthV2 through worker pool getter parameters. |
| cmd/stellar-rpc/internal/preflight/preflight_test.go | Add regression test asserting AuthV2 is ignored on prev protocol without error. |
| cmd/stellar-rpc/internal/methods/simulate_transaction.go | Forward request.AuthV2 into preflight.GetterParameters. |
| cmd/stellar-rpc/internal/methods/simulate_transaction_test.go | Unit test that handler forwards authV2 (and defaults to false when omitted). |
| cmd/stellar-rpc/internal/integrationtest/simulate_transaction_test.go | Integration test asserting recorded auth credentials are v2 when requested on curr protocol. |
| Cargo.toml | Bump soroban-env-host-curr and soroban-simulation-curr to =27.0.0. |
Comments suppressed due to low confidence (1)
cmd/stellar-rpc/lib/preflight.h:1
- Adding
auth_v2to the exportedpreflight_invoke_hf_opC function is an ABI-breaking change for any existing consumers compiled against the old header (older callers will pass fewer args, causing undefined behavior at runtime). To avoid breaking downstream integrations, keep the old symbol/signature and add a new entrypoint (e.g.,preflight_invoke_hf_op_v2), or provide a second exported wrapper that defaultsauth_v2to false; alternatively, ensure the shared library’s ABI/SONAME is versioned such that dependents must relink/recompile when upgrading.
// NOTE: You could use https://michael-f-bryan.github.io/rust-ffi-guide/cbindgen.html to generate
💡 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: 7e9a5963ed
ℹ️ 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".
| Footprint: footprint, | ||
| ResourceConfig: resourceConfig, | ||
| AuthMode: request.AuthMode, | ||
| AuthV2: request.AuthV2, |
There was a problem hiding this comment.
With the committed go.mod still on github.com/stellar/go-stellar-sdk v0.4.0, this new request.AuthV2 reference only compiles if the uncommitted local replace mentioned in the commit message is present. In a clean checkout or CI build, SimulateTransactionRequest from the committed dependency will not have this field, so the RPC package fails to build before any tests can run; please bump/commit the SDK dependency that contains AuthV2 together with this change.
Useful? React with 👍 / 👎.
| [workspace.dependencies.soroban-env-host-curr] | ||
| package = "soroban-env-host" | ||
| version = "=26.1.2" | ||
| version = "=27.0.0" |
There was a problem hiding this comment.
Bumping the current host directly from 26.1.2 to 27.0.0 while leaving the previous host at 25.2.1 creates a dispatch gap: preflight_invoke_hf_op and preflight_footprint_ttl_op only route proto <= prev::PROTOCOL or proto == curr::PROTOCOL, so a node whose latest ledger is protocol 26 now falls through to unsupported protocol version even though the parent commit served it with the 26.x current host. Please retain a protocol-26 host or advance prev to 26 before making 27 the current host.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae6a9c1963
ℹ️ 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".
| [[package]] | ||
| name = "stellar-xdr" | ||
| version = "27.0.0" | ||
| source = "registry+https://github.com/rust-lang/crates.io-index" | ||
| checksum = "05ff843326969bdf1ef673dcdba94c08f4a3c8f1e58d6e6ef39b1bd4f749179a" |
There was a problem hiding this comment.
With this added stellar-xdr 27.0.0 package stanza, the lockfile now contains two entries with the same name/version/source/checksum (the other starts at line 1442), so Cargo cannot parse the lockfile at all. I verified RUSTUP_TOOLCHAIN=1.95.0-x86_64-unknown-linux-gnu cargo check -p preflight --locked, which fails before compilation with package 'stellar-xdr' is specified twice in the lockfile; please regenerate or merge the dependency list into a single stanza so Rust builds can run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d65c314a6b
ℹ️ 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".
|
|
||
| // The previous protocol's soroban-env predates v2 Address credentials, so | ||
| // `use_address_v2` is ignored and v1 `Address` credentials are always used. | ||
| pub(crate) fn make_recording_auth_mode( |
There was a problem hiding this comment.
Adding make_recording_auth_mode here while switching shared.rs to call that new name leaves the existing prev::recording_auth_mode above with no callers. This repo’s Makefile sets RUSTFLAGS=-Dwarnings -Dclippy::all -Dclippy::pedantic, so the resulting dead_code warning is promoted to a Rust build failure once the lockfile parses; remove or rename the old helper instead of keeping both.
Useful? React with 👍 / 👎.
Ryang-21
force-pushed
the
simulate-tx-auth-v2
branch
from
d65c314 to
a3c75e9
Compare
|
This is gonna fail CI until you actually reference the correct version of the SDK - |
What
Add an optional
authV2boolean to thesimulateTransactionrequest. When set, authorization entries recorded during simulation use SorobanAddressV2("v2") credentials instead ofAddress("v1"). The flag is threaded from the request through the preflight worker pool and across the CGO bridge into the Rust simulation path. Also bumps the current-protocolsoroban-env-host/soroban-simulationdependencies to27.0.0.Why
rs-soroban-env#1689 added a recording-auth parameter (
use_address_v2) that selects v1/v2Addresscredential recording independently of the ledger protocol version. As stated in that PR, the intent is to "allow RPC to select the preferred mode when calling simulation … without a hard protocol-based switch" — so v2 credentials can be exercised through RPC for testing before the protocol that requires them activates. This change surfaces that capability as a request-level opt-in.The flag only has an effect in the recording auth modes (
record/record_allow_nonroot); inenforcemode the caller supplies their own auth entries, so the credential format is already fixed.Known limitations
SimulateTransactionRequest.AuthV2field ingo-stellar-sdk(this pr). Until that lands in a release andgo.modis bumped, the field is supplied via a localreplacedirective that is intentionally not committed — so this branch does not build in CI on its own yet.authV2is silently ignored and v1 credentials are produced. This is by design, not an error.TestSimulateInvokeContractTransactionAuthV2) requires a captive-core running protocol ≥ 27 and is skipped otherwise; the unit/handler tests run without it.