refactor: replace with_indexer with photon_url in LightClientConfig#1814
refactor: replace with_indexer with photon_url in LightClientConfig#1814ananas-block merged 3 commits intomainfrom
with_indexer with photon_url in LightClientConfig#1814Conversation
…fig` and adjust related tests and RPC calls
WalkthroughThis change replaces the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PoolBuilder
participant ConnectionManager
participant LightClient
User->>PoolBuilder: set photon_url (optional)
PoolBuilder->>ConnectionManager: build with photon_url
ConnectionManager->>LightClient: create with LightClientConfig(photon_url)
LightClient->>LightClient: Optionally create PhotonIndexer if photon_url is Some
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
♻️ Duplicate comments (1)
xtask/src/create_state_tree.rs (1)
47-52: Same concern as increate_batch_state_tree.rsRepeating the suggestion: allow callers to supply a
photon_urlinstead of hard-codingNone.
🧹 Nitpick comments (8)
xtask/src/create_batch_state_tree.rs (1)
47-52: Exposephoton_urlinstead of hard-codingNoneHard-coding
photon_url: Noneforces downstream users to recompile if they want to hit a remote Photon service. Consider wiring it through theOptionsCLI (e.g.--photon-url) or allowing an env var fallback so scripts stay flexible.+ #[clap(long)] + photon_url: Option<String>, ... - photon_url: None, + photon_url: options.photon_url,program-tests/utils/src/setup_accounts.rs (1)
13-18: DeriveDefaultforLightClientConfigto cut boilerplateEvery call site now has to be touched whenever a new field is added. Implementing
Default(if not already present) lets you build configs concisely:- let mut rpc = light_client::rpc::LightClient::new(LightClientConfig { - commitment_config: Some(CommitmentConfig::confirmed()), - url: url.to_string(), - photon_url: None, - fetch_active_tree: false, - }) + let mut rpc = light_client::rpc::LightClient::new(LightClientConfig { + url: url.to_string(), + ..Default::default() + })This reduces churn the next time a field is added.
xtask/src/create_batch_address_tree.rs (1)
42-47: Provide a way to setphoton_urlFor parity with other tooling, expose
--photon-url(or env var) and pass it through instead of always usingNone.forester/tests/batched_state_test.rs (1)
70-76: Consider theLightClientConfig::local()helperThe manual construction duplicates the built-in helper intended for local validators and keeps the tests slightly harder to tweak.
-let mut rpc = LightClient::new(LightClientConfig { - url: RpcUrl::Localnet.to_string(), - photon_url: None, - commitment_config: Some(commitment_config), - fetch_active_tree: false, -}) +let mut rpc = LightClient::new( + LightClientConfig::local() + .with_commitment(commitment_config) // assuming the builder exposes this +)Up to you, but adopting the helper keeps test setup consistent across files.
forester/src/epoch_manager.rs (1)
476-481: Avoid copy-pastingLightClientConfigliteralsThe three call-sites build nearly identical configs by hand. Extracting a helper (e.g.
fn forester_rpc_config(indexer: Option<String>) -> LightClientConfig) will:
- guarantee all options stay in sync with future field additions
- remove boilerplate
- simplify unit-testing of configuration
Also applies to: 545-551, 1196-1202
forester/tests/e2e_test.rs (1)
406-411: Avoid duplicating manual literals
Now thatlocal_no_indexer()exists, consider using it here as well (or a helper likedevnet(None)) instead of an in-place struct literal – this keeps the configuration source-of-truth in one place:- let mut rpc = LightClient::new(LightClientConfig { - url: RpcUrl::Localnet.to_string(), - photon_url: None, - commitment_config: Some(CommitmentConfig::confirmed()), - fetch_active_tree: false, - }) + let mut rpc = LightClient::new(LightClientConfig::local_no_indexer())sdk-libs/client/src/rpc/rpc_trait.rs (1)
61-68: Minor API ergonomics
devnet(photon_url: Option<String>)is flexible, but most callers will pass a concrete URL. Acceptingimpl Into<String>and wrapping it inSome()internally would remove boilerplate:pub fn devnet<P: Into<String>>(photon_url: Option<P>) -> Self { … }Low priority, but worth considering.
forester-utils/src/rpc_pool.rs (1)
137-140: Builder takesOption<String>directly
Passing anOptioninto a builder feels a bit clunky (.photon_url(Some(url))). Allowing a plainString(and handlingNoneby omission) would make the API smoother, but this is purely cosmetic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
forester-utils/src/rpc_pool.rs(7 hunks)forester/src/epoch_manager.rs(3 hunks)forester/src/forester_status.rs(1 hunks)forester/src/lib.rs(2 hunks)forester/tests/address_v2_test.rs(3 hunks)forester/tests/batched_address_test.rs(1 hunks)forester/tests/batched_state_async_indexer_test.rs(2 hunks)forester/tests/batched_state_indexer_test.rs(2 hunks)forester/tests/batched_state_test.rs(1 hunks)forester/tests/e2e_test.rs(2 hunks)forester/tests/priority_fee_test.rs(2 hunks)program-tests/utils/src/setup_accounts.rs(1 hunks)sdk-libs/client/src/rpc/client.rs(1 hunks)sdk-libs/client/src/rpc/rpc_trait.rs(2 hunks)xtask/src/create_batch_address_tree.rs(1 hunks)xtask/src/create_batch_state_tree.rs(1 hunks)xtask/src/create_state_tree.rs(1 hunks)xtask/src/create_update_protocol_config_ix.rs(1 hunks)xtask/src/new_deployment.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
forester/tests/priority_fee_test.rs (2)
sdk-libs/client/src/rpc/rpc_trait.rs (3)
new(35-42)new(73-75)local(52-59)sdk-libs/client/src/rpc/client.rs (1)
new(419-424)
forester/tests/batched_state_async_indexer_test.rs (2)
sdk-libs/client/src/rpc/rpc_trait.rs (3)
new(35-42)new(73-75)local(52-59)sdk-libs/client/src/rpc/client.rs (1)
new(419-424)
⏰ Context from checks skipped due to timeout of 90000ms (22)
- GitHub Check: system-programs (token-escrow-test, [ "cargo test-sbf -p token-escrow" ])
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-test"])
- GitHub Check: system-programs (counter-test, ["cargo test-sbf -p counter"])
- GitHub Check: test-2-foresters
- GitHub Check: test-address-batched
- GitHub Check: test-double-registration
- GitHub Check: test-state-photon-batched
- GitHub Check: test-state-batched
- GitHub Check: test-e2e
- GitHub Check: stateless-js
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: system-programs (system-cpi-test-v2, ["cargo-test-sbf -p system-cpi-v2-test"])
- GitHub Check: system-programs (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test"])
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-test"])
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: Test sdk-libs
- GitHub Check: Test program-libs
- GitHub Check: lint
🔇 Additional comments (14)
forester/tests/batched_address_test.rs (1)
66-70: Config update looks correctThe test now matches the new
LightClientConfigsignature and keeps behaviour unchanged.forester/tests/batched_state_indexer_test.rs (1)
64-64: Switch toLightClientConfig::local()– good callThe helper cleans up boilerplate and keeps the test focused on business logic. Nice tidy-up.
forester/tests/priority_fee_test.rs (2)
9-12: Import appears correct – no issues spotted
Signeris needed forpubkey()so the new import is appropriate.
80-83: Confirm the target RPC endpoint before hard-codingLightClientConfig::local()
LightClientConfig::local()forces the client tohttp://localhost:8899, while the rest of the test suite pulls RPC / WS URLs from the environment.
If those env vars ever point to a non-local validator the test will silently talk to the wrong cluster.Consider wiring the env-provided URL into
LightClientConfig::new(...)instead, or assert thatFORESTER_RPC_URL=="http://localhost:8899".forester/src/lib.rs (2)
54-59: Disabling the photon indexer – is this intentional?Setting
photon_url: Nonemeans the light client instantiated inrun_queue_infocannot use the photon indexer even when one is configured elsewhere.
If that is intentional (e.g. to minimise network I/O) 👍. Otherwise, consider threading the value fromconfig.external_services.indexer_url.
101-105:photon_url()is fed withindexer_url– naming mismatch could confuseThe builder now binds
.photon_url(config.external_services.indexer_url.clone())Verify that
indexer_urlindeed holds the photon service endpoint, not some other GRPC/REST indexer. A quick rename in the config struct might avoid future confusion.forester/tests/address_v2_test.rs (2)
12-15: Updated imports align with new APIThe switch to the re-exported traits (
MerkleTreeExt,Rpc) is consistent with the refactor.
72-76: Ensure the local-validator is actually onlocalhost:8899
LightClientConfig::local()hard-codes the RPC URL; theinit()helper spins up a validator, but if it binds to a different port the test will fail.forester/tests/batched_state_async_indexer_test.rs (2)
15-16: Import cleanup looks good
RpcUrlis no longer referenced in this file after switching to the convenience constructor, so removing the sub-module import is the right call.
224-226: Confirm commitment level change
LightClientConfig::local()defaults toCommitmentConfig::confirmed()whereas the previous hand-rolled config usedprocessed().
For heavily time-sensitive tests this tighter commitment can introduce extra slots of delay and may hide race-conditions that only occur atprocessed. Make sure all call-sites are happy with the stronger commitment or revert tolocal_no_indexer()and override the commitment explicitly.forester/tests/e2e_test.rs (1)
63-66: Nice simplification
Replacing the verbose struct literal withLightClientConfig::local_no_indexer()improves readability and avoids repetition.sdk-libs/client/src/rpc/rpc_trait.rs (2)
30-32: Field introduced cleanly
photon_url: Option<String>integrates without breaking existing semantics – good job.
35-42:new()now forcesfetch_active_tree = true– double-check callers
Previously some call-sites relied onfetch_active_treebeingfalseunless explicitly enabled. The new helper toggles it totrue, which causes an extra RPC round-trip on every light-client instantiation. If that’s unintended, exposefetch_active_treeas a parameter or keep the default consistent with the other constructors (local*/devnet).forester-utils/src/rpc_pool.rs (1)
28-35: Good propagation ofphoton_url
Adding the field to the connection manager keeps configuration plumbing straightforward.
| let rpc = LightClient::new(LightClientConfig { | ||
| url: rpc_url, | ||
| photon_url: None, | ||
| commitment_config: None, | ||
| fetch_active_tree: false, | ||
| with_indexer: false, | ||
| }) | ||
| .await | ||
| .unwrap(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle LightClient initialization errors instead of panicking
unwrap() will crash the CLI on transient network issues or mis-configuration. Because the surrounding function already returns anyhow::Result<()>, you can propagate the error cleanly and optionally surface more context.
- let rpc = LightClient::new(LightClientConfig {
- url: rpc_url,
- photon_url: None,
- commitment_config: None,
- fetch_active_tree: false,
- })
- .await
- .unwrap();
+ let rpc = LightClient::new(LightClientConfig {
+ url: rpc_url,
+ photon_url: None, // consider wiring this from an env/CLI flag
+ commitment_config: None,
+ fetch_active_tree: false,
+ })
+ .await
+ .context("failed to initialise LightClient")?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let rpc = LightClient::new(LightClientConfig { | |
| url: rpc_url, | |
| photon_url: None, | |
| commitment_config: None, | |
| fetch_active_tree: false, | |
| with_indexer: false, | |
| }) | |
| .await | |
| .unwrap(); | |
| let rpc = LightClient::new(LightClientConfig { | |
| url: rpc_url, | |
| photon_url: None, // consider wiring this from an env/CLI flag | |
| commitment_config: None, | |
| fetch_active_tree: false, | |
| }) | |
| .await | |
| .context("failed to initialise LightClient")?; |
🤖 Prompt for AI Agents
In xtask/src/create_update_protocol_config_ix.rs around lines 35 to 42, the
LightClient initialization uses unwrap(), which causes a panic on errors.
Replace unwrap() with the ? operator to propagate the error properly, and
optionally add context using .context() from anyhow to provide clearer error
messages while maintaining the function's anyhow::Result return type.
| let mut rpc = LightClient::new(LightClientConfig { | ||
| url: rpc_url, | ||
| photon_url: None, | ||
| commitment_config: None, | ||
| fetch_active_tree: false, | ||
| with_indexer: false, | ||
| }) | ||
| .await | ||
| .unwrap(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace unwrap() with ? and provide error context
For long-running provisioning scripts, a panic is painful to diagnose. Propagate the error with context instead.
- let mut rpc = LightClient::new(LightClientConfig {
+ let mut rpc = LightClient::new(LightClientConfig {
url: rpc_url,
photon_url: None,
commitment_config: None,
fetch_active_tree: false,
- })
- .await
- .unwrap();
+ })
+ .await
+ .context("failed to create LightClient")?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let mut rpc = LightClient::new(LightClientConfig { | |
| url: rpc_url, | |
| photon_url: None, | |
| commitment_config: None, | |
| fetch_active_tree: false, | |
| with_indexer: false, | |
| }) | |
| .await | |
| .unwrap(); | |
| let mut rpc = LightClient::new(LightClientConfig { | |
| url: rpc_url, | |
| photon_url: None, | |
| commitment_config: None, | |
| fetch_active_tree: false, | |
| }) | |
| .await | |
| .context("failed to create LightClient")?; |
🤖 Prompt for AI Agents
In xtask/src/new_deployment.rs around lines 56 to 63, replace the use of
unwrap() on the LightClient::new call with the ? operator to propagate errors
instead of panicking. Additionally, wrap the error with context using a method
like .context() or map_err() to provide a clear error message that aids in
diagnosing failures during provisioning.
| let mut rpc = LightClient::new(LightClientConfig { | ||
| url: config.external_services.rpc_url.to_string(), | ||
| photon_url: None, | ||
| commitment_config: None, | ||
| fetch_active_tree: false, | ||
| with_indexer: false, | ||
| }) | ||
| .await | ||
| .unwrap(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use configured photon_url and avoid panic on LightClient build
The ForesterConfig already carries external_services.photon_url; passing None disables the indexer even when the user enabled it in the config. At the same time, unwrap() will abort the status command on any RPC hiccup.
- let mut rpc = LightClient::new(LightClientConfig {
- url: config.external_services.rpc_url.to_string(),
- photon_url: None,
+ let mut rpc = LightClient::new(LightClientConfig {
+ url: config.external_services.rpc_url.to_string(),
+ photon_url: config.external_services.photon_url.clone(),
commitment_config: None,
fetch_active_tree: false,
- })
- .await
- .unwrap();
+ })
+ .await
+ .context("failed to create LightClient")?;This preserves the new configuration surface and improves robustness.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let mut rpc = LightClient::new(LightClientConfig { | |
| url: config.external_services.rpc_url.to_string(), | |
| photon_url: None, | |
| commitment_config: None, | |
| fetch_active_tree: false, | |
| with_indexer: false, | |
| }) | |
| .await | |
| .unwrap(); | |
| let mut rpc = LightClient::new(LightClientConfig { | |
| url: config.external_services.rpc_url.to_string(), | |
| photon_url: config.external_services.photon_url.clone(), | |
| commitment_config: None, | |
| fetch_active_tree: false, | |
| }) | |
| .await | |
| .context("failed to create LightClient")?; |
🤖 Prompt for AI Agents
In forester/src/forester_status.rs around lines 176 to 183, replace the
hardcoded None for photon_url with the configured photon_url from
external_services to respect user settings. Also, instead of using unwrap() on
the LightClient::new call, handle the Result properly to avoid panics on RPC
failures, for example by using match or if let to gracefully handle errors and
improve robustness.
| let retry_config = retry_config.unwrap_or_default(); | ||
|
|
||
| let indexer = if config.with_indexer { | ||
| if config.url == RpcUrl::Localnet.to_string() { | ||
| Some(PhotonIndexer::new( | ||
| "http://127.0.0.1:8784".to_string(), | ||
| None, | ||
| )) | ||
| } else { | ||
| Some(PhotonIndexer::new( | ||
| config.url.to_string(), | ||
| None, // TODO: test that this is not required | ||
| )) | ||
| } | ||
| } else { | ||
| None | ||
| }; | ||
| let indexer = config.photon_url.map(|path| PhotonIndexer::new(path, None)); | ||
|
|
||
| let mut new = Self { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Photon API key silently dropped
PhotonIndexer::new(path, None) ignores any API key that may be required for production use. If the key is available but not forwarded, authenticated requests will fail.
- let indexer = config.photon_url.map(|path| PhotonIndexer::new(path, None));
+ // Propagate the optional API key once LightClientConfig is extended
+ let indexer = config
+ .photon_url
+ .as_ref()
+ .map(|path| PhotonIndexer::new(path.clone(), config.photon_api_key.clone()));This will need:
- Adding
photon_api_key: Option<String>toLightClientConfig. - Populating it in the builders/tests that already carry the key.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In sdk-libs/client/src/rpc/client.rs around lines 111 to 115, the PhotonIndexer
is created without passing the API key, causing it to be silently dropped and
leading to failed authenticated requests. To fix this, add a photon_api_key
field of type Option<String> to the LightClientConfig struct, update all
builders and tests that instantiate LightClientConfig to populate this new field
with the API key if available, and modify the PhotonIndexer::new call to pass
the API key instead of None so that authenticated requests can succeed.
2 participants
Summary by CodeRabbit
New Features
Refactor
Tests