Bump alloy-ethers-typecast and refactor ReadableClient constructors#1902
Bump alloy-ethers-typecast and refactor ReadableClient constructors#1902
Conversation
|
Warning Rate limit exceeded@findolor has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 51 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
""" WalkthroughThis change updates all instantiations of Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ReadableClientHttp
Caller->>ReadableClientHttp: new_from_urls([rpc_url])
ReadableClientHttp-->>Caller: Client instance (supports multiple URLs)
Caller->>ReadableClientHttp: Perform RPC call
alt All providers fail
ReadableClientHttp-->>Caller: Error: AllProvidersFailed { url -> error }
else Success
ReadableClientHttp-->>Caller: Result
end
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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. 🪧 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: 9
🔭 Outside diff range comments (2)
crates/common/src/add_order.rs (2)
101-110: 🛠️ Refactor suggestionAvoid per-token HTTP client re-construction – cache by RPC URL.
Inside the input loop a brand-new
ReadableClientHttpinstance is built for every token missingdecimals.
On a deployment containing n tokens sharing the same network this spawns n identical hyper clients, TLS hand-shakes, etc. – unnecessary CPU and connection overhead.Consider re-using one client per RPC URL:
- let client = - ReadableClientHttp::new_from_urls(vec![input_token.network.rpc.to_string()])?; + let rpc = input_token.network.rpc.to_string(); + // Lazily cache clients per RPC in a local HashMap. + let client = rpc_clients + .entry(rpc.clone()) + .or_insert_with(|| { + // safe: unwrap bubbled up later through `?` + ReadableClientHttp::new_from_urls(vec![rpc]).expect("valid rpc url") + }) + .clone();(A similar optimisation applies to the outputs loop below.)
This keeps the change local and preserves the public API.
132-140: 🧹 Nitpick (assertive)Duplicate pattern from input loop – centralise logic
The output loop repeats the same fetch-decimals pattern. After introducing the cache above, extract the body into a small helper (
async fn fetch_decimals(token_cfg, rpc_clients) -> Result<u8, …>).
Reduces ~20 lines of duplication, makes the intent explicit, and eases future maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktauri-app/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.toml(2 hunks)crates/cli/src/commands/order/calldata.rs(1 hunks)crates/common/src/add_order.rs(7 hunks)crates/common/src/dotrain_order/mod.rs(1 hunks)crates/common/src/fuzz/impls.rs(2 hunks)crates/common/src/unit_tests/mod.rs(1 hunks)crates/quote/src/order_quotes.rs(1 hunks)tauri-app/src-tauri/Cargo.toml(2 hunks)tauri-app/src-tauri/src/commands/charts.rs(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tauri-app/src-tauri/src/commands/charts.rs (1)
crates/cli/src/commands/chart/mod.rs (1)
get_settings(73-98)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: git-clean
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: Deploy-Preview
- GitHub Check: test
🔇 Additional comments (5)
tauri-app/src-tauri/Cargo.toml (1)
41-41: Confirm transitive compatibility after git-rev bumpsBoth
alloy-ethers-typecastandrain-error-decodingnow point to newer commits.
Please runcargo update -p alloy-ethers-typecast --precise adda117b1f43e1a4dfa012299e71f2b0e73f46f4(and the same forrain-error-decoding) and execute the full test suite to ensure there are no semver-breaking changes or feature-flag mismatches.Also applies to: 64-64
Cargo.toml (1)
15-15: Workspace lockfile needs regenerationUpdating the git revisions at workspace level requires committing the refreshed
Cargo.lockto keep downstream CI reproducible.
Runcargo generate-lockfilein the workspace root and include the diff.Also applies to: 54-54
tauri-app/src-tauri/src/commands/charts.rs (1)
31-32: Import correctly scoped & necessaryThe added
ReadableClientErrorimport is scoped inside the test module and is required for pattern-matching the new aggregated error variant. Looks good.crates/common/src/add_order.rs (2)
1166-1202: 👍 Updated test correctly targets aggregated provider errorThe new assertion accurately reflects the
AllProvidersFailedvariant introduced byalloy-ethers-typecast.
1541-1562: 👍 Consistent error‐variant expectation in calldata testMirrors the previous adjustment; looks good.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
crates/common/src/add_order.rs (2)
132-134: Same optimisation opportunity as for inputsThe output-token branch duplicates the per-iteration client creation noted above. Re-use the cached client here as well.
226-230: Redundantclone()persistsEarlier feedback pointed out that
rpc_url.clone()allocates unnecessarily when the string is already owned. The same line still exists:ReadableClientHttp::new_from_urls(vec![rpc_url.clone()])?Drop the
clone()call:-ReadableClientHttp::new_from_urls(vec![rpc_url.clone()])? +ReadableClientHttp::new_from_urls(vec![rpc_url.to_string()])?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
crates/common/src/add_order.rs(7 hunks)crates/common/src/dotrain_order/mod.rs(1 hunks)crates/quote/src/order_quotes.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: git-clean
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
🔇 Additional comments (5)
crates/common/src/dotrain_order/mod.rs (1)
353-355: Instantiation looks goodSwitching to
new_from_urlsis consistent with the refactor and avoids the redundantclone()allocation highlighted earlier.crates/common/src/add_order.rs (4)
164-165: LGTM – clone removedConstructing the client with the owned
rpc_urldirectly is cleaner and avoids the extra allocation.
235-236: LGTM – unnecessary clone removedPassing the owned
rpc_urldirectly is cleaner.
1165-1201: (No actionable issues – test updates correctly reflect the new aggregated-error variant.)
1541-1560: (No actionable issues – test updates correctly reflect the new aggregated-error variant.)
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
crates/common/src/add_order.rs(8 hunks)crates/quote/src/order_quotes.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (2)
crates/common/src/add_order.rs (2)
87-88: 👍 Cached RPC clients eliminate N× overheadCreating
rpc_clients: HashMap<String, ReadableClientHttp>up-front is a solid optimisation and directly addresses the earlier review about redundant client instantiation.
1178-1214: Tests accurately validate aggregated-error variantThe updated assertions correctly introspect
ReadableClientError::AllProvidersFailedand ensure the nested per-URL error is of the expected kind. Looks great.Also applies to: 1553-1574
0xgleb
left a comment
0xgleb
left a comment
There was a problem hiding this comment.
Probably would have been easier to keep new_from_url alongside new_from_urls and only use new_from_urls when there is actually more than one URL available. And new_from_urls could also have taken something generic that can be converted to String. But this is all for alloy-ethers-typecast, this PR works
| .entry(input_token.network.rpc.to_string()) | ||
| .or_insert_with(|| { | ||
| ReadableClientHttp::new_from_urls(vec![input_token.network.rpc.to_string()]) | ||
| .expect("Failed to create RPC client") |
There was a problem hiding this comment.
We shouldn't be using .expect in non-test code
There was a problem hiding this comment.
Yeah already updated that one no problem
3 participants
Caution
Do not merge before this PR merges rainlanguage/rainlang#330
Motivation
See issue: #1674
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
fix #1674
Summary by CodeRabbit
Bug Fixes
Chores