Add trait for error registry#20
Conversation
WalkthroughAdds a pluggable async error registry trait and a default HTTP-backed OpenChainRegistry (using url crate), refactors ABI/JSON‑RPC error decoding to accept an injected registry with selector caching and a DEFAULT_REGISTRY, updates tests to use injected registries, and adds Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant AbiDecodedErrorType
participant Registry
participant RegistryAPI
Caller->>AbiDecodedErrorType: decode_with_registry(error_data, registry)
AbiDecodedErrorType->>AbiDecodedErrorType: extract selector (first 4 bytes)
AbiDecodedErrorType->>Registry: lookup(selector)
alt Registry is OpenChainRegistry
Registry->>RegistryAPI: HTTP GET /errors?function=...&filter=...
RegistryAPI-->>Registry: JSON candidates
end
Registry-->>AbiDecodedErrorType: Vec<AlloyError>
AbiDecodedErrorType->>AbiDecodedErrorType: attempt decode with candidates (cache selector on success)
AbiDecodedErrorType-->>Caller: Result<AbiDecodedErrorType, AbiDecodeFailedErrors>
note right of AbiDecodedErrorType: selector_registry_abi_decode now delegates to<br/>decode_with_registry(..., &DEFAULT_REGISTRY) when registry not provided
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/error.rs (2)
146-158: Don’t return Unknown on cache mismatch; evict and re‑lookup.If the cached candidate fails to decode, we should evict it and fall back to a fresh registry lookup. Returning Unknown here permanently masks valid alternatives for the same selector.
Apply this diff:
- let cached_selector = Self::retrieve_from_cache(selector_hash_bytes).await?; - if let Some(error) = cached_selector { - if let Ok(result) = error.abi_decode_input(args_data) { - return Ok(AbiDecodedErrorType::Known { - name: error.name.to_string(), - args: result.iter().map(|v| format!("{:?}", v)).collect(), - sig: error.signature(), - data: error_data.to_vec(), - }); - } - return Ok(Self::Unknown(error_data.to_vec())); - } + let cached_selector = Self::retrieve_from_cache(selector_hash_bytes).await?; + if let Some(error) = cached_selector { + if let Ok(result) = error.abi_decode_input(args_data) { + return Ok(AbiDecodedErrorType::Known { + name: error.name.to_string(), + args: result.iter().map(|v| format!("{:?}", v)).collect(), + sig: error.signature(), + data: error_data.to_vec(), + }); + } else { + // Evict stale/mismatched cache entry and fall through to a fresh registry lookup. + let mut selectors = SELECTORS.lock()?; + selectors.remove(&selector_hash_bytes); + } + }
116-121: retrieve_from_cache doesn’t need to be async.The function performs only synchronous work. Consider making it a plain fn to simplify call sites and avoid needless .awaits.
Proposed signature:
pub fn retrieve_from_cache(selector_hash: [u8; 4]) -> Result<Option<AlloyError>, AbiDecodeFailedErrors>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.toml(1 hunks)src/error.rs(11 hunks)
🧰 Additional context used
🪛 GitHub Actions: Rainix CI
src/error.rs
[error] 45-71: Command failed: nix develop -c cargo build --target wasm32-unknown-unknown. Rust async function in error.rs cannot be Send across threads: future cannot be sent between threads safely and Rc<RefCell<wasm_bindgen_futures::Inner>> cannot be sent between threads safely (JsFuture) during wasm32-unknown-unknown build.
🔇 Additional comments (1)
Cargo.toml (1)
18-18: Add is fine; ensure wasm compatibility in code usage.The dependency is correct. The wasm32 build failure comes from how #[async_trait] is used in src/error.rs. Apply the suggested cfg_attr(?Send) changes there to resolve CI.
| async fn lookup(&self, selector: [u8; 4]) -> Result<Vec<AlloyError>, AbiDecodeFailedErrors> { | ||
| let selector_hash = alloy::primitives::hex::encode_prefixed(selector); | ||
| let response = self | ||
| .client | ||
| .get(&self.url) | ||
| .query(&vec![ | ||
| ("function", selector_hash.as_str()), | ||
| ("filter", "true"), | ||
| ]) | ||
| .header("accept", "application/json") | ||
| .send() | ||
| .await? | ||
| .json::<Value>() | ||
| .await?; | ||
|
|
There was a problem hiding this comment.
Harden HTTP call: check status; optionally timeout off-wasm.
Currently non-2xx responses are parsed as JSON. Add error_for_status(). Optionally add a per-request timeout on non-wasm targets.
Apply this diff:
- let response = self
- .client
- .get(&self.url)
- .query(&vec![
- ("function", selector_hash.as_str()),
- ("filter", "true"),
- ])
- .header("accept", "application/json")
- .send()
- .await?
- .json::<Value>()
- .await?;
+ let req = self
+ .client
+ .get(&self.url)
+ .query(&[
+ ("function", selector_hash.as_str()),
+ ("filter", "true"),
+ ])
+ .header("accept", "application/json");
+ #[cfg(not(target_family = "wasm"))]
+ let req = req.timeout(std::time::Duration::from_secs(5));
+ let response = req
+ .send()
+ .await?
+ .error_for_status()?
+ .json::<Value>()
+ .await?;Note: timeout is gated to avoid wasm limitations. [Based on pipeline failures]
🧰 Tools
🪛 GitHub Actions: Rainix CI
[error] 45-71: Command failed: nix develop -c cargo build --target wasm32-unknown-unknown. Rust async function in error.rs cannot be Send across threads: future cannot be sent between threads safely and Rc<RefCell<wasm_bindgen_futures::Inner>> cannot be sent between threads safely (JsFuture) during wasm32-unknown-unknown build.
There was a problem hiding this comment.
not part of this PR
There was a problem hiding this comment.
@findolor, understood! I respect your decision to keep this PR focused on its current scope.
Would you like me to create a follow-up issue to track the HTTP hardening improvements (adding error_for_status() and timeout configuration) for future consideration?
| let mut out: Vec<AlloyError> = Vec::new(); | ||
| if let Some(selectors) = response["result"]["function"][selector_hash].as_array() { | ||
| for opt_selector in selectors { | ||
| if let Some(name) = opt_selector["name"].as_str() { | ||
| if let Ok(err) = name.parse::<AlloyError>() { | ||
| out.push(err); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Ok(out) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Parsing robustness (optional): tolerate missing/alt fields.
The response shape is assumed. If OpenChain changes keys or returns non-array, decoding silently yields empty results. Consider guarding with error_for_status (already suggested) and logging/metrics or typed structs. Optional.
🧰 Tools
🪛 GitHub Actions: Rainix CI
[error] 45-71: Command failed: nix develop -c cargo build --target wasm32-unknown-unknown. Rust async function in error.rs cannot be Send across threads: future cannot be sent between threads safely and Rc<RefCell<wasm_bindgen_futures::Inner>> cannot be sent between threads safely (JsFuture) during wasm32-unknown-unknown build.
🤖 Prompt for AI Agents
In src/error.rs around lines 60 to 71, the code assumes
response["result"]["function"][selector_hash] is present and an array and
silently skips on mismatch; update the routine to explicitly check for presence
and type, return or log a descriptive error when keys are missing or the value
is not an array, and avoid silent drops. Replace the ad-hoc JSON-path access
with a safe path check (e.g. ensure response.get("result").and_then(|r|
r.get("function")).and_then(|f| f.get(selector_hash)) exists), handle non-array
cases by logging/instrumenting a metric or returning a Result::Err with context,
and prefer deserializing the expected structure into typed serde structs (with
Option fields for alternate keys) to make parsing robust against shape changes.
There was a problem hiding this comment.
not part of this PR
There was a problem hiding this comment.
@findolor Understood! I've noted that this improvement is outside the scope of this PR. Feel free to tackle it in a future change if needed.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/error.rs (1)
123-179: Make cache read sync; drop unnecessary async/await.
retrieve_from_cachedoes no async work. Make it sync and remove the.awaitat call sites to reduce overhead.- pub async fn retrieve_from_cache( + pub fn retrieve_from_cache( selector_hash: [u8; 4], ) -> Result<Option<AlloyError>, AbiDecodeFailedErrors> { - let selectors = SELECTORS.lock()?; - Ok(selectors.get(&selector_hash).cloned()) + let selectors = SELECTORS.lock()?; + Ok(selectors.get(&selector_hash).cloned()) } @@ - let cached_selector = Self::retrieve_from_cache(selector_hash_bytes).await?; + let cached_selector = Self::retrieve_from_cache(selector_hash_bytes)?;Tests referencing
.awaiton this method will need the same removal. I can supply a follow‑up patch if you want.Also applies to: 146-148
♻️ Duplicate comments (4)
src/error.rs (4)
22-26: Gate async-trait Send behavior: use ?Send only on wasm.Unconditional
#[async_trait(?Send)]makes futures non-Send on native. Keep Send on non‑wasm, relax only on wasm to avoid wasm32 build failures.-#[async_trait(?Send)] +#[cfg_attr(target_family = "wasm", async_trait(?Send))] +#[cfg_attr(not(target_family = "wasm"), async_trait)] pub trait ErrorRegistry: Send + Sync { @@ -#[async_trait(?Send)] +#[cfg_attr(target_family = "wasm", async_trait(?Send))] +#[cfg_attr(not(target_family = "wasm"), async_trait)] impl ErrorRegistry for OpenChainRegistry { @@ - #[async_trait(?Send)] + #[cfg_attr(target_family = "wasm", async_trait(?Send))] + #[cfg_attr(not(target_family = "wasm"), async_trait)] impl ErrorRegistry for FakeRegistry {Also applies to: 43-46, 291-294
47-59: Harden the HTTP call: status check, timeout (off‑wasm), avoid Vec alloc.Add
error_for_status(), gate a short timeout off‑wasm, and pass a slice to.query().- let response = self - .client - .get(&self.url) - .query(&vec![ - ("function", selector_hash.as_str()), - ("filter", "true"), - ]) - .header("accept", "application/json") - .send() - .await? - .json::<Value>() - .await?; + let req = self + .client + .get(&self.url) + .query(&[ + ("function", selector_hash.as_str()), + ("filter", "true"), + ]) + .header("accept", "application/json"); + #[cfg(not(target_family = "wasm"))] + let req = req.timeout(std::time::Duration::from_secs(5)); + let response = req + .send() + .await? + .error_for_status()? + .json::<Value>() + .await?;Confirm OpenChain query key for error selectors is correct (is
"function"accepted for errors, or should it be"error"?). If needed, I can adjust.OpenChain signature database lookup API: which query parameter is used for error selector lookups?
60-71: Parsing robustness: handle schema drift explicitly (optional).Current code silently returns empty on shape mismatches. Consider validating keys/types (or deserialize to typed structs) and returning a descriptive error or emitting a metric.
180-186: Avoid per‑call Client construction: reuse a static default registry.Constructing
OpenChainRegistryeach call is wasteful. Use aLazystatic.Add near
SELECTORS:// Reuse a single client-backed registry. pub static DEFAULT_REGISTRY: Lazy<OpenChainRegistry> = Lazy::new(OpenChainRegistry::default);Change the wrapper:
- let registry = OpenChainRegistry::default(); - Self::decode_with_registry(error_data, ®istry).await + Self::decode_with_registry(error_data, &*DEFAULT_REGISTRY).await
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/error.rs(11 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). (2)
- GitHub Check: standard-tests (macos-latest, rainix-rs-artifacts)
- GitHub Check: standard-tests (macos-latest, rainix-rs-test)
🔇 Additional comments (1)
src/error.rs (1)
34-41: Good: Default no longer panics.Using
Client::new()avoids panics inDefault. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/error.rs (1)
51-54: Prefer array slice over Vec allocation.The
vec![]macro allocates unnecessarily. Use an array slice directly.Apply this diff:
- .query(&vec![ + .query(&[ ("function", selector_hash.as_str()), ("filter", "true"), ])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/error.rs(12 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). (3)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-test)
🔇 Additional comments (7)
src/error.rs (7)
17-26: LGTM! Well-designed pluggable trait.The trait design is clean and enables dependency injection for testing and alternative implementations. The cfg_attr usage correctly handles wasm's !Send futures while keeping the Send bound on other platforms.
83-86: LGTM! Efficient shared registry.Using a static
Lazyto reuse the registry instance avoids repeated client construction and improves performance.
128-183: LGTM! Well-structured registry-based decoding.The method properly integrates the injected registry with the existing cache, handles edge cases (empty data, invalid selectors, panics), and tries multiple candidates when selector collisions occur.
185-190: LGTM! Backwards compatibility preserved.The existing public API delegates to the new registry-based flow using the default registry, maintaining compatibility while enabling injection for tests and alternative implementations.
245-262: LGTM! Consistent registry injection pattern.The registry-injected variant mirrors the existing method structure, providing consistency across the API surface.
292-309: LGTM! Clean test double.The
FakeRegistryprovides deterministic behavior for unit tests, avoiding external dependencies and enabling fast, reliable test execution.
311-427: LGTM! Comprehensive test coverage with proper isolation.The tests exercise all code paths using the injected
FakeRegistry, and theclear_cache()helper ensures test isolation by preventing cross-test cache pollution.
| #[tokio::test] | ||
| async fn test_openchain_registry_live_lookup_known_selector() { | ||
| clear_cache(); | ||
|
|
||
| let data = vec![0x1a, 0xc6, 0x69, 0x08]; | ||
|
|
||
| let registry = OpenChainRegistry::default(); | ||
| let res = AbiDecodedErrorType::decode_with_registry(&data, ®istry) | ||
| .await | ||
| .expect("OpenChain lookup failed"); | ||
|
|
||
| match res { | ||
| AbiDecodedErrorType::Known { | ||
| name, | ||
| args, | ||
| sig, | ||
| data: decoded, | ||
| } => { | ||
| assert_eq!(decoded, data); | ||
| assert!(args.is_empty(), "expected zero-arg error match"); | ||
| assert!(!name.is_empty(), "expected non-empty error name"); | ||
| assert!(sig.ends_with(')'), "expected error-like signature"); | ||
| } | ||
| AbiDecodedErrorType::Unknown(_) => panic!("expected a known error from OpenChain"), | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider marking as #[ignore] to avoid CI flakiness.
This test makes a live HTTP call to the OpenChain API, which introduces several risks:
- Network latency can slow down test suite
- API downtime or rate limiting causes intermittent failures
- Restricted CI environments may block external calls
Consider adding #[ignore] so it runs only when explicitly requested:
+ #[ignore]
#[tokio::test]
async fn test_openchain_registry_live_lookup_known_selector() {Alternatively, move this to a separate integration test suite that runs less frequently.
📝 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.
| #[tokio::test] | |
| async fn test_openchain_registry_live_lookup_known_selector() { | |
| clear_cache(); | |
| let data = vec![0x1a, 0xc6, 0x69, 0x08]; | |
| let registry = OpenChainRegistry::default(); | |
| let res = AbiDecodedErrorType::decode_with_registry(&data, ®istry) | |
| .await | |
| .expect("OpenChain lookup failed"); | |
| match res { | |
| AbiDecodedErrorType::Known { | |
| name, | |
| args, | |
| sig, | |
| data: decoded, | |
| } => { | |
| assert_eq!(decoded, data); | |
| assert!(args.is_empty(), "expected zero-arg error match"); | |
| assert!(!name.is_empty(), "expected non-empty error name"); | |
| assert!(sig.ends_with(')'), "expected error-like signature"); | |
| } | |
| AbiDecodedErrorType::Unknown(_) => panic!("expected a known error from OpenChain"), | |
| } | |
| } | |
| #[ignore] | |
| #[tokio::test] | |
| async fn test_openchain_registry_live_lookup_known_selector() { | |
| clear_cache(); | |
| let data = vec![0x1a, 0xc6, 0x69, 0x08]; | |
| let registry = OpenChainRegistry::default(); | |
| let res = AbiDecodedErrorType::decode_with_registry(&data, ®istry) | |
| .await | |
| .expect("OpenChain lookup failed"); | |
| match res { | |
| AbiDecodedErrorType::Known { | |
| name, | |
| args, | |
| sig, | |
| data: decoded, | |
| } => { | |
| assert_eq!(decoded, data); | |
| assert!(args.is_empty(), "expected zero-arg error match"); | |
| assert!(!name.is_empty(), "expected non-empty error name"); | |
| assert!(sig.ends_with(')'), "expected error-like signature"); | |
| } | |
| AbiDecodedErrorType::Unknown(_) => panic!("expected a known error from OpenChain"), | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/error.rs around lines 556 to 581, the test
test_openchain_registry_live_lookup_known_selector performs a live HTTP call to
the OpenChain API and should be made non-disruptive for CI; mark the test with
#[ignore] (place the attribute immediately above the #[tokio::test] line) so it
only runs when explicitly requested, or alternatively move the test into an
integration test file under tests/ (or a dedicated slower-tests directory) and
ensure CI does not run that suite by default.
There was a problem hiding this comment.
it's fine we don't need ignore
There was a problem hiding this comment.
@findolor Understood! I'll leave the test as-is without the #[ignore] attribute.
| /// Default OpenChain-backed registry implementation. | ||
| pub struct OpenChainRegistry { | ||
| client: Client, | ||
| url: String, |
There was a problem hiding this comment.
consider using Url instead of String
| let mut out: Vec<AlloyError> = Vec::new(); | ||
| if let Some(selectors) = response["result"]["function"][selector_hash].as_array() { | ||
| for opt_selector in selectors { | ||
| if let Some(name) = opt_selector["name"].as_str() { | ||
| if let Ok(err) = name.parse::<AlloyError>() { | ||
| out.push(err); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Ok(out) |
There was a problem hiding this comment.
| let mut out: Vec<AlloyError> = Vec::new(); | |
| if let Some(selectors) = response["result"]["function"][selector_hash].as_array() { | |
| for opt_selector in selectors { | |
| if let Some(name) = opt_selector["name"].as_str() { | |
| if let Ok(err) = name.parse::<AlloyError>() { | |
| out.push(err); | |
| } | |
| } | |
| } | |
| } | |
| Ok(out) | |
| Ok(response["result"]["function"][selector_hash] | |
| .as_array() | |
| .into_iter() | |
| .flat_map(|selectors| selectors.iter()) | |
| .filter_map(|opt_selector| opt_selector["name"].as_str()) | |
| .filter_map(|name| name.parse::<AlloyError>().ok()) | |
| .collect()) |
| let candidates = registry.lookup(selector_hash_bytes).await?; | ||
| for error in candidates { | ||
| if let Ok(result) = error.abi_decode_input(args_data) { | ||
| // cache the fetched selector | ||
| { | ||
| let mut cached_selectors = SELECTORS.lock()?; | ||
| cached_selectors.insert(selector_hash_bytes, error.clone()); | ||
| } | ||
| return Ok(Self::Known { | ||
| sig: error.signature(), | ||
| name: error.name, | ||
| args: result.iter().map(|v| format!("{:?}", v)).collect(), | ||
| data: error_data.to_vec(), | ||
| }); | ||
| } | ||
| Ok(Self::Unknown(error_data.to_vec())) | ||
| } else { | ||
| Ok(Self::Unknown(error_data.to_vec())) | ||
| } | ||
|
|
||
| Ok(Self::Unknown(error_data.to_vec())) |
There was a problem hiding this comment.
| let candidates = registry.lookup(selector_hash_bytes).await?; | |
| for error in candidates { | |
| if let Ok(result) = error.abi_decode_input(args_data) { | |
| // cache the fetched selector | |
| { | |
| let mut cached_selectors = SELECTORS.lock()?; | |
| cached_selectors.insert(selector_hash_bytes, error.clone()); | |
| } | |
| return Ok(Self::Known { | |
| sig: error.signature(), | |
| name: error.name, | |
| args: result.iter().map(|v| format!("{:?}", v)).collect(), | |
| data: error_data.to_vec(), | |
| }); | |
| } | |
| Ok(Self::Unknown(error_data.to_vec())) | |
| } else { | |
| Ok(Self::Unknown(error_data.to_vec())) | |
| } | |
| Ok(Self::Unknown(error_data.to_vec())) | |
| Ok(candidates | |
| .into_iter() | |
| .find_map(|error| -> Option<Result<Self>> { | |
| let result = error.abi_decode_input(args_data).ok()?; | |
| // cache the fetched selector | |
| let mut cached_selectors = match SELECTORS.lock() { | |
| Ok(lock) => lock, | |
| Err(e) => return Some(Err(e)), | |
| }; | |
| cached_selectors.insert(selector_hash_bytes, error.clone()); | |
| Some(Ok(Self::Known { | |
| sig: error.signature(), | |
| name: error.name, | |
| args: result.iter().map(|v| format!("{:?}", v)).collect(), | |
| data: error_data.to_vec(), | |
| })) | |
| }) | |
| .transpose()? | |
| .unwrap_or_else(|| Self::Unknown(error_data.to_vec()))) |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.toml(1 hunks)src/error.rs(12 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). (5)
- GitHub Check: standard-tests (macos-latest, rainix-rs-test)
- GitHub Check: standard-tests (macos-latest, rainix-rs-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts)
🔇 Additional comments (9)
src/error.rs (8)
18-27: LGTM!The trait definition is clean, and the
cfg_attrhandling for wasm vs non-wasm Send bounds is correct.
35-42: LGTM!The
Defaultimplementation correctly usesClient::new()(which cannot fail) and parses a hardcoded constant URL. The.unwrap()on line 39 is acceptable sinceSELECTOR_REGISTRY_URLis a compile-time constant.
80-82: LGTM!The
DEFAULT_REGISTRYstatic correctly usesLazyto avoid repeatedreqwest::Clientconstruction, improving performance.
125-191: LGTM!The registry injection pattern is well-designed: accepts
Option<&dyn ErrorRegistry>for flexibility, defaults toDEFAULT_REGISTRYwhenNone, and correctly caches successful lookups.
231-263: LGTM!The two JSON-RPC error decoding methods provide both backward-compatible (line 238 with
None) and registry-injectable (lines 246-263) variants. The minor duplication is acceptable for API compatibility.
293-310: LGTM!The
FakeRegistrytest implementation is clean and properly uses the samecfg_attrpattern as the trait definition.
312-403: LGTM!The tests are correctly updated to inject
FakeRegistry, and theclear_cache()helper (lines 398-403) properly isolates test state. Good test hygiene.
557-582: LGTM!The live integration test correctly exercises
OpenChainRegistrywith a known selector and validates the response structure. The decision to run this in CI without#[ignore]was explicitly made by the maintainer.Cargo.toml (1)
18-19: No action required—dependencies are current and secure.Verification confirms
async-trait 0.1resolves to the latest patch (0.1.89),url 2.5.7is already at the latest version, and no security advisories exist for either crate. The dependency specifications are appropriate.
| let response = self | ||
| .client | ||
| .get(self.url.as_ref()) | ||
| .query(&vec![ | ||
| ("function", selector_hash.as_str()), | ||
| ("filter", "true"), | ||
| ]) | ||
| .header("accept", "application/json") | ||
| .send() | ||
| .await? | ||
| .json::<Value>() | ||
| .await?; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid unnecessary allocation: use array slice instead of vec!.
Line 52-54 creates a Vec allocation for query parameters that could be a simple array slice.
Apply this diff:
let response = self
.client
.get(self.url.as_ref())
- .query(&vec![
+ .query(&[
("function", selector_hash.as_str()),
("filter", "true"),
])🤖 Prompt for AI Agents
In src/error.rs around lines 49 to 60, the query parameters are created with
vec! which causes an unnecessary heap allocation; replace the Vec with a fixed
array slice by passing a reference to a tuple array (e.g. &[( "function",
selector_hash.as_str() ), ( "filter", "true" )]) into .query(...) so the
parameters are borrowed from the stack and no heap allocation occurs.
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
|
Tip For best results, initiate chat on the files or code changes. SIZE=M |
3 participants
Motivation
See issues:
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Refactor
Tests