chore: Introduce client cache support and add to signers#729
Merged
Conversation
WalkthroughA new global client caching infrastructure is introduced via Changes
Sequence DiagramssequenceDiagram
participant Caller
participant AsyncClientCache
participant OnceCell
participant InitFn as Init Closure
rect rgba(100, 150, 200, 0.5)
Note over Caller,InitFn: First concurrent caller for a key
Caller->>AsyncClientCache: get_or_try_init(key, init_fn)
AsyncClientCache->>AsyncClientCache: lookup key in DashMap
AsyncClientCache->>OnceCell: get_or_try_init (empty cell)
OnceCell->>InitFn: run init closure
InitFn-->>OnceCell: Arc<V> or Error
OnceCell-->>AsyncClientCache: result
AsyncClientCache-->>Caller: Arc<V>
end
rect rgba(150, 200, 100, 0.5)
Note over Caller,InitFn: Concurrent caller for same key
Caller->>AsyncClientCache: get_or_try_init(key, init_fn)
AsyncClientCache->>AsyncClientCache: lookup key in DashMap
AsyncClientCache->>OnceCell: await existing cell
OnceCell-->>AsyncClientCache: cached Arc<V>
AsyncClientCache-->>Caller: Arc<V>
end
sequenceDiagram
participant Signer as Stellar Signer
participant OnceCell
participant KmsService as KMS Service
participant DeriveHelper as derive_signature_hint()
rect rgba(100, 150, 200, 0.5)
Note over Signer,DeriveHelper: First call to get_signature_hint
Signer->>OnceCell: get_or_try_init
OnceCell->>KmsService: get_stellar_address()
KmsService-->>OnceCell: Address
OnceCell->>DeriveHelper: derive_signature_hint(address)
DeriveHelper->>DeriveHelper: parse Stellar address to Ed25519 pk
DeriveHelper->>DeriveHelper: extract last 4 bytes
DeriveHelper-->>OnceCell: SignatureHint
OnceCell-->>Signer: SignatureHint (cached)
end
rect rgba(150, 200, 100, 0.5)
Note over Signer,DeriveHelper: Subsequent calls
Signer->>OnceCell: get_or_try_init (already initialized)
OnceCell-->>Signer: cached SignatureHint (no recomputation)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces reusable client-caching primitives and applies them across signers/providers to avoid recreating SDK/RPC/HTTP clients and to cache deterministic Stellar signature hints.
Changes:
- Added
AsyncClientCache/SyncClientCacheto guarantee exactly-once initialization per key. - Cached AWS KMS SDK clients by region and cached Stellar/Solana RPC clients by URL (and config).
- Centralized RPC
reqwest::Clientconstruction (shared base settings + shared/no-timeout client for Stellar raw HTTP) and cached Stellar signature hints viaOnceCell.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/services/signer/stellar/mod.rs | Adds shared derive_signature_hint helper for reuse by Stellar signers. |
| src/services/signer/stellar/google_cloud_kms_signer.rs | Caches Stellar signature hint via tokio::sync::OnceCell. |
| src/services/signer/stellar/aws_kms_signer.rs | Caches Stellar signature hint via tokio::sync::OnceCell. |
| src/services/provider/stellar/mod.rs | Caches Soroban RPC clients by URL and switches to shared reqwest::Client for raw HTTP calls. |
| src/services/provider/solana/mod.rs | Caches Solana RpcClient instances keyed by URL/timeout/commitment. |
| src/services/provider/mod.rs | Introduces shared/base reqwest::Client builders for RPC usage. |
| src/services/provider/evm/mod.rs | Uses centralized RPC HTTP client builder with timeout (instead of inline builder). |
| src/services/mod.rs | Exposes new client_cache module within the crate. |
| src/services/client_cache.rs | New cache primitives with unit tests. |
| src/services/aws_kms/mod.rs | Caches AWS KMS SDK client by region and wraps Client::new() in catch_unwind. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. @@ Coverage Diff @@
## main #729 +/- ##
==========================================
+ Coverage 90.20% 90.26% +0.05%
==========================================
Files 289 290 +1
Lines 120730 121553 +823
==========================================
+ Hits 108905 109719 +814
- Misses 11825 11834 +9
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/services/signer/stellar/google_cloud_kms_signer.rs (1)
215-236: Add a repeated-sign regression test forcached_hint.The cache only changes behavior on the second call. The current tests still pass if
get_signature_hint()stops reusing the firstget_stellar_address()result, so a two-call test withexpect_get_stellar_address().times(1)would lock the new behavior down.As per coding guidelines, "Keep tests minimal, deterministic, and fast".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/signer/stellar/google_cloud_kms_signer.rs` around lines 215 - 236, Add a regression test that calls get_signature_hint() twice to verify caching: mock GoogleCloudKmsStellarService::get_stellar_address (or the test mock helper using expect_get_stellar_address()) to return the address once and assert it is invoked exactly once (e.g., .times(1)), then call signer.get_signature_hint().await twice and assert both results are equal; this ensures cached_hint is reused and protects the behavior of cached_hint, get_signature_hint, and the interaction with get_stellar_address.src/services/signer/stellar/aws_kms_signer.rs (1)
209-223: Add a regression test for theOnceCellhit path.The current tests only exercise one hint lookup. A focused test in this module can call
get_signature_hint()twice and assertexpect_get_stellar_address().times(1)so future refactors do not quietly reintroduce the extra KMS round-trip.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/signer/stellar/aws_kms_signer.rs` around lines 209 - 223, Add a regression test in this module that exercises the OnceCell cached path by calling Signer::get_signature_hint() (or the relevant method that uses self.cached_hint) twice and verifying the mocked aws_kms_service.get_stellar_address() was invoked exactly once (use expect_get_stellar_address().times(1) on the mock). The test should initialize the signer with a mocked aws_kms_service that returns a known address, call get_signature_hint() twice, assert both calls return the same hint, and assert the mock's times(1) expectation to ensure no second KMS round-trip occurs.src/services/client_cache.rs (1)
31-38: Remove the unusedClonebound fromK.Neither cache clones the key, so
K: Clonejust narrows the API and excludes otherwise valid key types.♻️ Proposed API cleanup
-impl<K: Eq + Hash + Clone, V> AsyncClientCache<K, V> { +impl<K: Eq + Hash, V> AsyncClientCache<K, V> { @@ -impl<K: Eq + Hash + Clone, V> Default for AsyncClientCache<K, V> { +impl<K: Eq + Hash, V> Default for AsyncClientCache<K, V> { @@ -impl<K: Eq + Hash + Clone, V> SyncClientCache<K, V> { +impl<K: Eq + Hash, V> SyncClientCache<K, V> { @@ -impl<K: Eq + Hash + Clone, V> Default for SyncClientCache<K, V> { +impl<K: Eq + Hash, V> Default for SyncClientCache<K, V> {Also applies to: 67-71, 84-91, 122-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/client_cache.rs` around lines 31 - 38, The impl for AsyncClientCache unnecessarily requires K: Clone—remove the Clone bound from the impl generics (change impl<K: Eq + Hash + Clone, V> AsyncClientCache<K, V> to impl<K: Eq + Hash, V> AsyncClientCache<K, V>) and likewise remove Clone from any other AsyncClientCache impl blocks and method signatures (e.g., the impls that include get_or_try_init and related methods referenced in the comment) so keys are not constrained to Clone when not cloned; ensure only Eq + Hash are required in all AsyncClientCache impl blocks and method headers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/aws_kms/mod.rs`:
- Around line 931-944: The test test_kms_client_creation_returns_error_not_panic
currently sets AwsKmsSignerConfig.region to None which triggers
resolve_aws_region() and yields AwsKmsError::ConfigError before reaching
Client::new()/catch_unwind; change the test to provide a concrete region (e.g.,
Some("us-west-2".to_string())) on AwsKmsSignerConfig so resolve_aws_region is
bypassed, rename the test to reflect it verifies panic-to-error conversion, call
get_or_create_kms_client(&config).await, and tighten the assertion to
assert!(matches!(result, Err(e) if !matches!(e, AwsKmsError::ConfigError(_))))
or assert the specific error variant produced by the
catch_unwind/panic-conversion branch to ensure the new path is exercised.
In `@src/services/client_cache.rs`:
- Line 10: Update the public docs in src/services/client_cache.rs (e.g., the
module-level comment and the doc comments for the ClientCache type and its
get_or_try_init() method) to reflect the actual contract: there is at most one
in-flight initializer per key under concurrent access, but initializers that
return Err are retried on subsequent calls (so initialization is not strictly
"exactly once" on failures). Add or replace the existing non-document-comments
with /// doc comments on all public items referenced in the review (module,
ClientCache, get_or_try_init(), and any other public functions around lines
noted) to state this behavior clearly and precisely.
In `@src/services/provider/solana/mod.rs`:
- Around line 63-64: The cache used for RPC clients (SOLANA_RPC_CLIENT_CACHE /
STELLAR_RPC_CLIENT_CACHE built from SyncClientCache) blocks the Tokio executor
because initialize_provider() is called from async retry_rpc_call() via
get_provider() -> provider_initializer; replace the blocking SyncClientCache
with an async-aware solution (e.g., use a tokio::sync::RwLock-wrapped map or an
async cache crate) so get_or_try_init-style initialization does not take a
std::sync::Mutex in async contexts, or alternatively move the blocking
initialization into tokio::task::spawn_blocking() inside initialize_provider()
to ensure RPC client creation does not block the executor. Ensure references to
SyncClientCache, SOLANA_RPC_CLIENT_CACHE, STELLAR_RPC_CLIENT_CACHE,
retry_rpc_call, get_provider, provider_initializer, and initialize_provider are
updated accordingly.
In `@src/services/provider/stellar/mod.rs`:
- Around line 420-424: The error path currently includes the raw RPC URL in the
ProviderError (in the STELLAR_RPC_CLIENT_CACHE.get_or_try_init closure that
calls Client::new and maps errors to ProviderError::NetworkConfiguration);
replace the raw `url` with a redacted form by calling
`normalize_url_for_log(url)` (or a local `let redacted =
normalize_url_for_log(url)` and use `redacted` inside the map_err message) so
sensitive userinfo/query tokens are not leaked in logs/telemetry.
---
Nitpick comments:
In `@src/services/client_cache.rs`:
- Around line 31-38: The impl for AsyncClientCache unnecessarily requires K:
Clone—remove the Clone bound from the impl generics (change impl<K: Eq + Hash +
Clone, V> AsyncClientCache<K, V> to impl<K: Eq + Hash, V> AsyncClientCache<K,
V>) and likewise remove Clone from any other AsyncClientCache impl blocks and
method signatures (e.g., the impls that include get_or_try_init and related
methods referenced in the comment) so keys are not constrained to Clone when not
cloned; ensure only Eq + Hash are required in all AsyncClientCache impl blocks
and method headers.
In `@src/services/signer/stellar/aws_kms_signer.rs`:
- Around line 209-223: Add a regression test in this module that exercises the
OnceCell cached path by calling Signer::get_signature_hint() (or the relevant
method that uses self.cached_hint) twice and verifying the mocked
aws_kms_service.get_stellar_address() was invoked exactly once (use
expect_get_stellar_address().times(1) on the mock). The test should initialize
the signer with a mocked aws_kms_service that returns a known address, call
get_signature_hint() twice, assert both calls return the same hint, and assert
the mock's times(1) expectation to ensure no second KMS round-trip occurs.
In `@src/services/signer/stellar/google_cloud_kms_signer.rs`:
- Around line 215-236: Add a regression test that calls get_signature_hint()
twice to verify caching: mock GoogleCloudKmsStellarService::get_stellar_address
(or the test mock helper using expect_get_stellar_address()) to return the
address once and assert it is invoked exactly once (e.g., .times(1)), then call
signer.get_signature_hint().await twice and assert both results are equal; this
ensures cached_hint is reused and protects the behavior of cached_hint,
get_signature_hint, and the interaction with get_stellar_address.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ecabf19-54bf-416d-aa99-261bb0b7849c
📒 Files selected for processing (10)
src/services/aws_kms/mod.rssrc/services/client_cache.rssrc/services/mod.rssrc/services/provider/evm/mod.rssrc/services/provider/mod.rssrc/services/provider/solana/mod.rssrc/services/provider/stellar/mod.rssrc/services/signer/stellar/aws_kms_signer.rssrc/services/signer/stellar/google_cloud_kms_signer.rssrc/services/signer/stellar/mod.rs
| //! - [`SyncClientCache`] — for synchronous client constructors | ||
| //! (e.g., `soroban_rs::Client::new(url)`, Solana `RpcClient::new(...)`) | ||
| //! | ||
| //! Both guarantee exactly-once initialization per key under concurrent access. |
There was a problem hiding this comment.
Clarify the public contract around failed initializers.
Both caches retry after init() returns Err, so the current "exactly once" wording is stronger than the implementation. Please put the actual contract on get_or_try_init() as well: one in-flight initializer per key, with retries after failure.
As per coding guidelines, "Include relevant doc comments (///) on public functions, structs, and modules."
Also applies to: 24-25, 31-54, 77-78, 84-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/client_cache.rs` at line 10, Update the public docs in
src/services/client_cache.rs (e.g., the module-level comment and the doc
comments for the ClientCache type and its get_or_try_init() method) to reflect
the actual contract: there is at most one in-flight initializer per key under
concurrent access, but initializers that return Err are retried on subsequent
calls (so initialization is not strictly "exactly once" on failures). Add or
replace the existing non-document-comments with /// doc comments on all public
items referenced in the review (module, ClientCache, get_or_try_init(), and any
other public functions around lines noted) to state this behavior clearly and
precisely.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Summary
AsyncClientCacheandSyncClientCacheprimitives for exactly-once client initialization per keysoroban_rs::Client) and Solana (RpcClient) clients by URL to avoid recreating on every retryreqwest::Clientacross Stellar raw HTTP providers; EVM builds per-call clients with baked-in timeout viabuild_rpc_http_client_with_timeout()(alloy transport requires client-level timeout)base_rpc_client_builder()to centralize reqwest pool/TLS/keepalive settingsOnceCelland extract sharedderive_signature_hinthelperClient::new()incatch_unwindto convert TLS panics to typed errors in containerized environmentsTest plan
cargo test --lib client_cache— 11 tests covering async/sync cache semantics, concurrency, error recoverycargo test --lib aws_kms— 45 tests including KMS client cache reuse verificationcargo test --lib services::provider::stellar— 47 testscargo test --lib services::provider::solana— 34 testscargo test --lib services::provider::evm— 15 tests🤖 Generated with Claude Code