Conversation
|
📝 WalkthroughWalkthroughAdds JWT-based auth and token caching to Aleo HTTP clients, generics the HTTP client stack around a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant JWTClient as JWTBaseHttpClient
participant AuthCache as AuthCache
participant AuthServer
participant Remote as RemoteService
Client->>JWTClient: request(path)
JWTClient->>AuthCache: check_token()
alt cached & valid
AuthCache-->>JWTClient: token
else missing/expired
JWTClient->>Remote: initial request (reads x-auth-url)
Remote-->>JWTClient: response with x-auth-url
JWTClient->>AuthServer: POST auth_url (credentials)
AuthServer-->>JWTClient: Authorization token
JWTClient->>AuthCache: store(token, expiry)
AuthCache-->>JWTClient: token
end
JWTClient->>Remote: request with Authorization header
Remote-->>JWTClient: service response
JWTClient-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
rust/main/chains/hyperlane-aleo/src/provider/base.rs (1)
145-170: Token refresh be like layers, gotta handle 'em right.The token caching logic is solid overall. A few observations:
The 15-minute expiry is hardcoded (line 166). If the actual token lifetime differs, you might want to parse the expiry from the JWT or a response header. For now it's acceptable if the proving service guarantees 15+ minute tokens.
There's a potential race condition: multiple concurrent requests hitting an expired token will all try to refresh simultaneously. The second reader to acquire the write lock will overwrite the first's token (harmless but wasteful). Consider using a "refresh in progress" flag or accept this minor inefficiency.
Lines 169: The
result.clone()is redundant since you're returning after setting.🔎 Minor cleanup
let expires = Instant::now() + Duration::from_secs(60 * 15); let mut auth_token = self.auth_token.write().await; *auth_token = Some((result.clone(), expires)); - Ok(result.clone()) + Ok(result)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
rust/main/chains/hyperlane-aleo/src/error.rs(1 hunks)rust/main/chains/hyperlane-aleo/src/provider/aleo.rs(4 hunks)rust/main/chains/hyperlane-aleo/src/provider/base.rs(4 hunks)rust/main/chains/hyperlane-aleo/src/provider/fallback.rs(3 hunks)rust/main/chains/hyperlane-aleo/src/provider/metric.rs(5 hunks)rust/main/chains/hyperlane-aleo/src/provider/traits.rs(1 hunks)rust/main/helm/hyperlane-agent/templates/external-secret.yaml(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-12T15:37:21.497Z
Learnt from: yjamin
Repo: hyperlane-xyz/hyperlane-monorepo PR: 7606
File: rust/main/chains/hyperlane-aleo/src/provider/aleo.rs:43-63
Timestamp: 2025-12-12T15:37:21.497Z
Learning: In Aleo (rust/main/chains/hyperlane-aleo/src/provider/aleo.rs), fee estimates for a program function are input-independent. The FeeEstimateCacheKey can safely cache based on program_id, function_name, and network_id alone because execution costs depend on the function's circuit structure and execution trace, not the specific input values passed to the function.
Applied to files:
rust/main/chains/hyperlane-aleo/src/provider/aleo.rs
📚 Learning: 2025-11-20T12:20:42.691Z
Learnt from: yjamin
Repo: hyperlane-xyz/hyperlane-monorepo PR: 7414
File: rust/main/chains/hyperlane-aleo/src/provider/traits.rs:252-0
Timestamp: 2025-11-20T12:20:42.691Z
Learning: In rust/main/chains/hyperlane-aleo/src/provider/traits.rs, the `get_state_paths_for_commitments` and `get_state_paths_for_commitments_async` methods intentionally use `.unwrap_or_default()` to return an empty Vec on any error. This behavior matches SnarkVM's internal QueryTrait implementation and should not be changed to propagate errors.
Applied to files:
rust/main/chains/hyperlane-aleo/src/provider/aleo.rs
🧬 Code graph analysis (5)
rust/main/chains/hyperlane-aleo/src/provider/traits.rs (2)
rust/main/chains/hyperlane-aleo/src/utils.rs (1)
get_tx_id(36-41)rust/main/chains/hyperlane-aleo/src/provider/base.rs (2)
build(99-101)build(225-227)
rust/main/chains/hyperlane-aleo/src/provider/fallback.rs (1)
rust/main/chains/hyperlane-aleo/src/provider/metric.rs (1)
new(52-68)
rust/main/chains/hyperlane-aleo/src/provider/metric.rs (4)
rust/main/chains/hyperlane-aleo/src/provider/aleo.rs (3)
provider(599-601)deref(589-591)new(92-128)rust/main/chains/hyperlane-aleo/src/provider/fallback.rs (2)
MetricHttpClient(36-36)new(25-44)rust/main/chains/hyperlane-aleo/src/provider/traits.rs (2)
deref(73-75)build(30-30)rust/main/chains/hyperlane-aleo/src/provider/base.rs (4)
new(30-48)new(116-142)build(99-101)build(225-227)
rust/main/chains/hyperlane-aleo/src/provider/aleo.rs (3)
rust/main/chains/hyperlane-aleo/src/provider/base.rs (2)
new(30-48)new(116-142)rust/main/chains/hyperlane-aleo/src/provider/fallback.rs (1)
new(25-44)rust/main/chains/hyperlane-aleo/src/provider/metric.rs (1)
new(52-68)
rust/main/chains/hyperlane-aleo/src/provider/base.rs (2)
rust/main/utils/reqwest-utils/src/lib.rs (1)
parse_custom_rpc_headers(18-52)rust/main/chains/hyperlane-aleo/src/provider/traits.rs (4)
request(37-41)serde_json(284-284)request_post(53-57)build(30-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build-and-push-to-gcr
- GitHub Check: infra-test
- GitHub Check: e2e-matrix (cosmosnative)
- GitHub Check: e2e-matrix (evm)
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: e2e-matrix (radix)
- GitHub Check: e2e-matrix (cosmwasm)
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: lander-coverage
- GitHub Check: test-rs
- GitHub Check: lint-rs
🔇 Additional comments (15)
rust/main/chains/hyperlane-aleo/src/error.rs (1)
52-54: Looks good to me, donkey!This new error variant fits right in with the rest of the swamp—er, error handling. It's used appropriately in
JWTBaseHttpClient::get_auth_tokenwhen the auth endpoint doesn't return an Authorization header. Clean and simple, just how I like it.rust/main/helm/hyperlane-agent/templates/external-secret.yaml (1)
33-35: This'll do nicely for the swamp.The Aleo proving service URL handling follows the same pattern as the cosmos and sealevel protocols. The secret key naming and remote ref conventions are consistent. Far, far away from any issues here.
Also applies to: 59-63
rust/main/chains/hyperlane-aleo/src/provider/traits.rs (1)
23-31: Like layers on an onion, these traits have depth.The
AleoClienttrait alias bundles the necessary bounds nicely, and the blanket impl means any type meeting those requirements automatically qualifies. TheHttpClientBuildertrait provides a clean factory pattern that's used consistently inFallbackHttpClient::newandMetricHttpClient::new. Solid foundation work.rust/main/chains/hyperlane-aleo/src/provider/aleo.rs (1)
55-55: Get out of my swamp... unless you've got proper authentication!The separation makes sense here: proving services need JWT authentication (the fancy stuff), while regular RPC calls use the simpler
BaseHttpClient. The concrete type forproving_serviceis appropriate since it's always constructed the same way in production. The genericC: AleoClientremains for testing flexibility on the main client.Also applies to: 99-117
rust/main/chains/hyperlane-aleo/src/provider/base.rs (2)
51-94: The HTTP client implementations look proper and tidy.Both
BaseHttpClientandJWTBaseHttpClientimplementHttpClientconsistently. The JWT variant correctly fetches and attaches the auth token before each request. Error handling follows the established patterns with proper error type conversions.Also applies to: 174-220
96-102: Builder implementations are straightforward—nothing to fuss about here.Both
HttpClientBuilderimpls delegate to their respective constructors cleanly. This enables the generic factory pattern used inFallbackHttpClientandMetricHttpClient.Also applies to: 222-228
rust/main/chains/hyperlane-aleo/src/provider/metric.rs (5)
1-18: Well-layered generics here, looks solid.The struct signature with
C: AleoClient = BaseHttpClientmaintains backward compatibility while opening the door for JWT-based clients. Theinner: RpcClient<C>composition is clean - like layers of an onion, each one wrapping the next.
20-26: Deref impl looks proper.This follows the same pattern seen in
RpcClient's Deref impl from traits.rs. Gives transparent access to the underlyingRpcClient<C>without extra ceremony.
28-48: Clone and Drop symmetry for metrics is well handled.The increment on clone and decrement on drop keeps the provider instance count accurate. This is exactly what ye want for observability - no surprises when things get cleaned up.
50-68: Builder pattern via turbofish - clean approach.The
new::<Builder>pattern lets callers specify construction strategy without polluting the struct with builder state. TheBuilder::build(url, network)?delegation properly propagates errors from eitherBaseHttpClient::neworJWTBaseHttpClient::newdepending on the builder type.
71-97: HttpClient impl properly delegates with metrics wrapping.The timing and success/failure tracking around
inner.requestandinner.request_postis preserved through the generification. No behavioral changes, just type flexibility - which is the way to do it.rust/main/chains/hyperlane-aleo/src/provider/fallback.rs (4)
13-21: Struct generification mirrors the metric.rs pattern nicely.The
FallbackProvider<RpcClient<MetricHttpClient<C>>, RpcClient<MetricHttpClient<C>>>type is a bit of a mouthful, but that's the nature of composing these layers. Each layer adds its own behavior - metrics wrapping, then RPC semantics, then fallback retry logic.
23-44: Builder pattern propagation through to MetricHttpClient is consistent.The turbofish
MetricHttpClient::new::<Builder>on line 36 threads the builder type down properly. The collect pattern with early error propagation viacollect::<ChainResult<Vec<_>>>()?is a solid way to fail fast if any client construction fails.
47-53: BlockNumberGetter bounds differ from AleoClient - and that's fine.This impl is for
RpcClient<C>whereC: HttpClient + Debug + Send + Sync, not specifically forFallbackHttpClient. These are the minimal bounds needed for the block number getter logic. No change needed here, just noting the trait bound difference is intentional.
55-88: HttpClient impl with FallbackProvider.call() closure pattern is preserved.The async closure wrapping for
pathandquery/bodycloning remains unchanged - just the impl header now carriesC: AleoClient. The fallback semantics are unaffected by the generification.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
rust/main/chains/hyperlane-aleo/src/provider/base.rs (2)
168-170: The overflow fallback could be clearer.On the extraordinarily unlikely overflow case, setting
expirestoInstant::now()makes the token immediately stale, which is safe but might cause repeated auth calls until the system restarts. Consider logging or panicking instead, since overflow would only occur after ~584 years of uptime.🔎 Clearer handling
- let expires = Instant::now() - .checked_add(Duration::from_secs(60 * 15)) - .unwrap_or(Instant::now()); // Tokens last 15 minutes + let expires = Instant::now() + .checked_add(Duration::from_secs(60 * 15)) + .expect("Token expiry calculation overflowed - system uptime exceeded limits");
172-173: Minor efficiency: redundant clone.The token is cloned when storing and again when returning. You could store without cloning the second time since
resultisn't used after line 173.🔎 Remove redundant clone
let mut auth_token = self.auth_token.write().await; - *auth_token = Some((result.clone(), expires)); - Ok(result.clone()) + *auth_token = Some((result.clone(), expires)); + Ok(result)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rust/main/chains/hyperlane-aleo/src/provider/base.rs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: infra-test
- GitHub Check: build-and-push-to-gcr
- GitHub Check: e2e-matrix (radix)
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: e2e-matrix (evm)
- GitHub Check: e2e-matrix (cosmosnative)
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: e2e-matrix (cosmwasm)
- GitHub Check: test-rs
- GitHub Check: lander-coverage
- GitHub Check: lint-rs
🔇 Additional comments (8)
rust/main/chains/hyperlane-aleo/src/provider/base.rs (8)
1-20: LGTM on imports and constants.The additions support JWT auth with token caching properly. The 60-second request timeout is reasonable for proving service calls.
39-44: Network 2 support looks good.The canary network is properly handled alongside mainnet and testnet, with appropriate error fallback for unknown network IDs.
52-95: HttpClient implementation is solid.The GET and POST methods handle errors consistently and construct URLs properly with the pre-suffixed base_url.
97-103: Builder implementation looks fine.Simple delegation to the constructor keeps things clean.
105-144: JWT client structure is well-designed.Separating the suffix from base_url and storing the auth token with expiry tracking sets up clean URL construction and token management.
177-224: Authenticated requests handled properly.Token fetching on each request is fine since
get_auth_tokenmanages the cache, and the Authorization header is correctly applied to both GET and POST with the network-suffixed URLs.
226-232: Builder pattern implemented consistently.Mirrors the BaseHttpClient builder, keeping the API uniform across both client types.
169-169: Check if the 15-minute token lifetime matches Aleo's proving service requirements.The hardcoded 15-minute expiry for JWT tokens is reasonable for short-lived security credentials, but verify it matches Aleo's actual token lifetime expectation to avoid premature re-auth or token expiration issues during long-running proof generation tasks.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rust/main/chains/hyperlane-aleo/src/provider/base.rs (1)
174-174: Redundant clone on the return.
resultis already a clone from line 168, so the.clone()on line 174 is unnecessary.🔎 Suggested fix
- Ok(result.clone()) + Ok(result)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rust/main/chains/hyperlane-aleo/src/provider/base.rs(4 hunks)rust/main/chains/hyperlane-aleo/src/provider/traits.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-20T12:20:42.691Z
Learnt from: yjamin
Repo: hyperlane-xyz/hyperlane-monorepo PR: 7414
File: rust/main/chains/hyperlane-aleo/src/provider/traits.rs:252-0
Timestamp: 2025-11-20T12:20:42.691Z
Learning: In rust/main/chains/hyperlane-aleo/src/provider/traits.rs, the `get_state_paths_for_commitments` and `get_state_paths_for_commitments_async` methods intentionally use `.unwrap_or_default()` to return an empty Vec on any error. This behavior matches SnarkVM's internal QueryTrait implementation and should not be changed to propagate errors.
Applied to files:
rust/main/chains/hyperlane-aleo/src/provider/traits.rs
📚 Learning: 2025-12-12T15:37:21.497Z
Learnt from: yjamin
Repo: hyperlane-xyz/hyperlane-monorepo PR: 7606
File: rust/main/chains/hyperlane-aleo/src/provider/aleo.rs:43-63
Timestamp: 2025-12-12T15:37:21.497Z
Learning: In Aleo (rust/main/chains/hyperlane-aleo/src/provider/aleo.rs), fee estimates for a program function are input-independent. The FeeEstimateCacheKey can safely cache based on program_id, function_name, and network_id alone because execution costs depend on the function's circuit structure and execution trace, not the specific input values passed to the function.
Applied to files:
rust/main/chains/hyperlane-aleo/src/provider/traits.rs
🧬 Code graph analysis (2)
rust/main/chains/hyperlane-aleo/src/provider/traits.rs (3)
rust/main/chains/hyperlane-aleo/src/provider/aleo.rs (1)
get_tx_id(633-633)rust/main/chains/hyperlane-aleo/src/utils.rs (1)
get_tx_id(36-41)rust/main/chains/hyperlane-aleo/src/provider/base.rs (2)
build(101-103)build(230-232)
rust/main/chains/hyperlane-aleo/src/provider/base.rs (3)
rust/main/utils/reqwest-utils/src/lib.rs (1)
parse_custom_rpc_headers(18-52)rust/main/chains/hyperlane-aleo/src/provider/traits.rs (3)
request(37-41)serde_json(284-284)build(30-30)rust/main/chains/hyperlane-aleo/src/config.rs (1)
new(29-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: infra-test
- GitHub Check: build-and-push-to-gcr
- GitHub Check: lint-rs
- GitHub Check: test-rs
- GitHub Check: lander-coverage
- GitHub Check: e2e-matrix (cosmwasm)
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: e2e-matrix (evm)
- GitHub Check: e2e-matrix (radix)
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: e2e-matrix (cosmosnative)
🔇 Additional comments (3)
rust/main/chains/hyperlane-aleo/src/provider/traits.rs (1)
23-31: Nice abstraction layer.The
AleoClienttrait alias andHttpClientBuildercleanly separate construction from usage. The blanket impl keeps things flexible while the builder pattern standardizes how clients get wired up across the provider stack.rust/main/chains/hyperlane-aleo/src/provider/base.rs (2)
53-104: LGTM on the BaseHttpClient implementation.The HttpClient and HttpClientBuilder implementations are straightforward. Error handling is consistent, and the builder delegation keeps construction logic centralized.
178-225: JWT injection looks solid.The auth token gets fetched fresh (or from cache) before each request, and the header injection is clean. The async lock handling won't block request threads unnecessarily thanks to the read-lock fast path.
🦀 Rust Agent Docker Image Built SuccessfullyImage Tags: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7656 +/- ##
============================
============================
🚀 New features to boost your workflow:
|
2 participants
Description
Aleo changed how the proving service is authenticated. It now uses a dynamic JWT instead of a static API key, this PR allows us to request JWT tokens from the proving service.
Drive-by changes
Adds custom proving service urls to the secret pipeline.
Related issues
Backward compatibility
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.