Skip to content
This repository was archived by the owner on Jul 4, 2026. It is now read-only.

on_providers and refactor#54

Closed
0xgleb wants to merge 5 commits into
mainfrom
fix/refactor-and-multicall
Closed

on_providers and refactor#54
0xgleb wants to merge 5 commits into
mainfrom
fix/refactor-and-multicall

Conversation

@0xgleb

@0xgleb 0xgleb commented Jun 18, 2025

Copy link
Copy Markdown
Contributor

Motivation

Any time additional functionality on providers is needed from ReadableClient, e.g. new native Alloy multicall support, ReadableClient needs to be updated to utilize all available providers.

Solution

Introduce .on_providers that takes an arbitrary operation on a provider and tries to do it on all available ones until one succeeds.

Checks

By submitting this for review, I'm confirming I've done the following:

  • made this PR as small as possible
  • unit-tested any new functionality
  • linked any relevant issues or PRs
  • included screenshots (if this involves a front-end change)

Summary by CodeRabbit

  • Refactor
    • Reorganized modules by removing the transaction module and introducing read, write, and write_transaction modules, with all public items now re-exported at the crate root for easier access.
    • Updated the ReadableClient to use a concrete provider type for improved clarity and performance.
    • Introduced a streamlined method for handling multiple providers, improving error aggregation and operation flow.
    • Adjusted import paths to reflect the new module structure.

@coderabbitai

coderabbitai Bot commented Jun 18, 2025

Copy link
Copy Markdown
## Walkthrough

The codebase refactors its module structure by removing the `transaction` module and introducing three new modules: `read`, `write`, and `write_transaction`. Public items from these new modules are now re-exported at the crate root. Additionally, the `ReadableClient` implementation is updated to use a concrete provider type alias instead of a boxed trait object.

## Changes

| File(s)                                  | Change Summary                                                                                     |
|-----------------------------------------|--------------------------------------------------------------------------------------------------|
| src/lib.rs                              | Removed `transaction` module; added `read`, `write`, and `write_transaction` modules; re-exported all their public items at the crate root. |
| src/read.rs                             | Changed `ReadableClient` to use a concrete `ReadProvider` type alias instead of boxed trait objects; updated related methods and tests.       |
| src/transaction/mod.rs                  | Deleted; previously re-exported submodules for the `transaction` module.                         |
| src/write_transaction.rs                | Updated imports to reference items directly from the crate root instead of the `transaction` submodule. |

## Sequence Diagram(s)

```mermaid
sequenceDiagram
    participant User
    participant CrateRoot
    participant ReadModule
    participant WriteModule
    participant WriteTransactionModule

    User->>CrateRoot: use crate::{ReadableClient, WritableClient, ...}
    CrateRoot->>ReadModule: Re-export ReadableClient, etc.
    CrateRoot->>WriteModule: Re-export WritableClient, etc.
    CrateRoot->>WriteTransactionModule: Re-export WriteTransaction, etc.
sequenceDiagram
    participant User
    participant ReadableClient
    participant ReadProvider

    User->>ReadableClient: new(HashMap<String, ReadProvider>)
    ReadableClient->>ReadProvider: Use directly (no boxing)
Loading

Possibly related PRs

Suggested reviewers

  • hardyjosh

<!-- walkthrough_end -->


---

<details>
<summary>📜 Recent review details</summary>

**Configuration used: CodeRabbit UI**
**Review profile: ASSERTIVE**
**Plan: Pro**


<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between 566a52ec5843e9839633d3b184a9bfde9b44d8c9 and 6476f8fee685c1ab3c39dd3ded1b33b769223e6a.

</details>

<details>
<summary>📒 Files selected for processing (1)</summary>

* `src/read.rs` (14 hunks)

</details>

<details>
<summary>🧰 Additional context used</summary>

<details>
<summary>🧠 Learnings (1)</summary>

<details>
<summary>src/read.rs (4)</summary>

Learnt from: 0xgleb
PR: #54
File: src/read.rs:50-56
Timestamp: 2025-06-18T16:36:12.115Z
Learning: The user prefers keeping type aliases like ReadProvider in their respective modules (e.g., read module) rather than moving them to dedicated modules like providers.rs.


Learnt from: 0xgleb
PR: #54
File: src/read.rs:143-163
Timestamp: 2025-06-18T16:31:42.674Z
Learning: In the ReadableClient's on_providers method, providers are called sequentially (not concurrently) by design to avoid rate limits. The purpose of having multiple providers is rate limit avoidance - calling them concurrently would drain all providers at the same time, defeating this purpose.


Learnt from: 0xgleb
PR: #54
File: src/read.rs:143-163
Timestamp: 2025-06-18T16:38:53.646Z
Learning: In ReadableClient's on_providers method (src/read.rs), the HashMap iteration order is intentionally non-deterministic to provide natural load balancing across multiple providers. This prevents rate-limited providers from being hit repeatedly first, which helps avoid cascading failures. The random order is a feature for resilience, not a bug.


Learnt from: 0xgleb
PR: #54
File: src/read.rs:182-209
Timestamp: 2025-06-18T16:39:41.964Z
Learning: 0xgleb prefers to avoid micro-optimizations like avoiding clones unless there's evidence they're causing performance bottlenecks, considering them premature optimization.


</details>

</details>

</details>

<details>
<summary>⏰ Context from checks skipped due to timeout of 90000ms (5)</summary>

* GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts)
* GitHub Check: standard-tests (macos-latest, rainix-rs-test)
* GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
* GitHub Check: standard-tests (ubuntu-latest, rainix-rs-test)
* GitHub Check: standard-tests (macos-latest, rainix-rs-artifacts)

</details>

<details>
<summary>🔇 Additional comments (9)</summary><blockquote>

<details>
<summary>src/read.rs (9)</summary>

`3-6`: **LGTM! Import additions support the provider refactor.**

The new imports for `FillProvider`, `JoinedRecommendedFillers`, and related provider types are necessary for the transition from boxed trait objects to concrete types.

---

`50-51`: **Excellent refactor from boxed trait objects to concrete types.**

The `ReadProvider` type alias eliminates heap allocation and dynamic dispatch overhead while maintaining a clean abstraction. This improves both performance and type safety.

---

`53-55`: **Struct refactor correctly implements concrete type usage.**

Adding `Debug` and `Clone` derives is appropriate, and changing the `providers` field to use `ReadProvider` directly eliminates the boxing overhead while maintaining type safety.

---

`68-68`: **Constructor updates correctly implement concrete type usage.**

The changes to use `ReadProvider` directly in the constructors are consistent with the refactor and maintain the same functionality while improving performance.



Also applies to: 95-95, 104-104, 108-108

---

`81-86`: **Method correctly updated for concrete type approach.**

The change to return tuples of `(url, ProviderBuilder)` is consistent with the refactor and allows the concrete `ReadProvider` type to be used without boxing.

---

`118-163`: **Excellent abstraction for provider fallback logic.**

The `on_providers` method provides a clean, reusable pattern for sequential provider fallback. The comprehensive documentation, proper error aggregation, and sequential approach (for rate limiting) make this a well-designed addition.

---

`182-210`: **Clean refactor using the new provider abstraction.**

The `read` method effectively uses the new `on_providers` helper, reducing code duplication while maintaining comprehensive error handling for both provider errors and ABI decoding failures.

---

`214-221`: **Simplified method implementations using provider abstraction.**

Both `get_chainid` and `get_block_number` are much cleaner using the `on_providers` helper, demonstrating the value of the abstraction in reducing code duplication.



Also applies to: 224-231

---

`426-429`: **Comprehensive test coverage for the refactored functionality.**

The test updates correctly use the concrete `ReadProvider` type, and the new tests for `on_providers` provide excellent coverage of both success and failure scenarios for the sequential provider fallback logic.



Also applies to: 493-497, 594-597, 626-628, 681-684, 713-715, 746-834

</details>

</blockquote></details>

</details>
<!-- internal state start -->


<!-- DwQgtGAEAqAWCWBnSTIEMB26CuAXA9mAOYCmGJATmriQCaQDG+Ats2bgFyQAOFk+AIwBWJBrngA3EsgEBPRvlqU0AgfFwA6NPEgQAfACgjoCEYDEZyAAUASpETZWaCrKNwSPbABsvkCiQBHbGlcSHFcLzpIACJ8DAB9XnwJeCUKZEx6fwAzNDF8CmjIOUhsREpIAAYADyJIgXRaWn9EcuRcWA9yKOyCyDJYTAYe7AwxeDi0L3V5OJ4KZNTKZAB3dQQsDo8AAxsSNFoVSIBhafZtjRhOyGZ8cQlqCaxEGmZkbIXme2GMZwnVzr+Rq0dRPKbzRZpRhobgqeDTcTSAA09mwDFg6GQv3uHgAgj58PJtsxvOIGFMvNtUdxuAVcCj/EF4EDdvtDgITmcMLgqQRIJEpFRSOgfOgHvCjh4kik0sgqFs+B1MOh7PAMHUPHESBo3NdEPgvHgnihuQtaGjpCryCsbiQOopoT4ohc4okFjLlhcrqg2Pb6HlhtxcBksM41LgqC5+NxlOI5nyYxRehQvnM0BCPXxMuhcK8g+18P1qqI8B51OgGAtWiLfGhxV5JRmlul7IFgtz4BT5KNxL4tdHY8aHAxhnREJc4KgYUk8hiyA4WmFBqFWQdJad4OdoVgjdMAF4eLaQbLeXxUdWa7JNqEnsZx34Izerdb4PB+NvMtVEUrcQ40d59Ps6KQNax77Lg2D+DqBjuJ4oqMsELwmgwhpKMgozlv+wbHn0R6gbe4yTAisgoqgADWJBBjcarwMw4Jqt8+Axii2bTBgZFyiQAqYKESAOJafS2OOkAAJKhFM+oKBg2TMm8jSAmQwxhIWTCmvAAhGuqkBENgSxsZa2Zqih2BoegGDyOQwytM48iIJWJDzrAdzIPAV7TtM5IctqVweLYKAZPWjbJu+KQkDa1CQLAubcIgHAAPRxUQ6zYAIGhMMwcVUGqDbqtgaCkHFFKEmAdqAogYC4LIMbki8cXcKecUAKwACzQfoxjgFAZD0Pgbl4IQpDkPKUTpWw3JcLw/DCKIOIyPITBpCo4ZaDo7UmFAk4uSGOAEMQZCxiNLBjZwfhoDaDhOFGJQLcoqjqCtuhgIYHWmAYiAUAwcXTKl6QcAY0QAwYFiQLiIl7UN1BRBddFRr1jCDBeiC6h4tzmpEkDbBGmCIHk95UisaCcbcUj+hgWSUQ2wz0CUHT+F0YU3Io3jSFw2z+Ac2wotsKwUOoJCc2Z9Dc7zNDxFjGA44RGBeriTSgkRXgkTWngcvADAoK87yfEu0iHrAdMgQzqPMxkQL+CV1S0hQND+qER6VpDfj4HcKJ0WRX4618IL+GIisVlZiDqejfJlMs/BXvbw2QGs9pvt0IJaXyiDVa58hHsb6Owh00HA/iNDyk8BY65ASgoc4jxxMgcMkJbdJRH09Wq+r7CgtIRhQHsxNRI3HmM2jJCs+Lkt4wYUCy0o9A92rfcm6z7O0ALwt84vPN82L57D082yjyDTTdylvfmzXVvBlwFIayQckfCwGPzyvIv8yxZMY6votD7jW8z5EyApOm2yN6UcoVIXiQ2OkjAG0R26vXep9eeGhfr/UBrnMGg0Dr0GhjZcO8NMCkCRjBa4q52Sck3NyYBEY0S8U4rkfI/h6B8n8NwSmesPChywQIfAxY6FZVCIIEQYhkAAAptgACEOHAFoLILAVh3TNmALicyAA5O0KwChkT0HobYABKaO6wVSqXsjQa8FRKoxhFJ2ZAhDpGQkoALJQMlyD+gsQAMXhF4KxmZgAACl8BqjoHsUa3U6AuKdOkFENhna4HcbI+RsglG4BURQNRKIYlxISWor0sE1T5wfBjaUzZEBUkGIgWAdFuAgXwOdAgi5LEyLSFSNUICxiWm9jNRWE5riqReBQChfRszUKqfIX0TlaCmxYb+SG/oRwFATnUNOhYAyUXEs/fwEEKBYBqdYig9SJa4CGJaGOTk3zsOqF+dpUpakVHshXLADEjyYxcPEa+zB6nMEYZfdg1z/J91cpuOhhYVmQU2NgN5VcrzbAEZBLwKIolpGEbpLwaRNH4xfEcjhpyjC4kNjaXgkhHaE0keiBYGBXzIE6F4RMto/QY1dHk2U9SXKmiZlTE0GM9hri8huc47SfSlQdAs/MeivD4AXB4BhLQW5aWVPisY+s4gkoHAXeMhYBAeBriWG2/AsBAQxLSioDFyhBEUiwjsfZyCohHA5EZlwxIKCdPw/oFAFgtiecrXI8IojUDzNhbMAK1kZCkvqaYf5nRsqISQTl3IACijqCgcA4PiNxFz0hOO0JEBeDqnUoDcqKXVzrU1nKLEgcQWkhmKAsXfLmpBcDxHRNoDAqQBbZm2FW+IHJ8AMDIvEDAjgVVbPQGbEg/SChRBDuUJcqBQJksTCiQOrzpjZFkB7LYzJ+T4CSurEo/hiYe2Ph5csQqmIhnoJQTN+UiD+CINc6CsET19ARrQNi34hXrpZWzNk9L7ARn2MwfS1NBmLETtcUuigPbZlvXwUp3APYNKWMXFCwrIJSkJuUP51KEi5oKQW0t9BiU2l9RLYuMl0ihGHAHE8vhgMT3fA4LwPCsyhiIOekgl6NXgazcrDDx581uBCNCNo/axnBtQw03ZHZHaEPXFyHkOiOisrZDCmxJpGlWRLsyVp8gRNsjYRwkd3Cpp8ODJcce8sclYVGcCEdhZBSp2LiqwYKQ+hwzwgzbYNKk0FMilxadyFUIezsmQP4wro4KUI8yJCuauPwiPSqBwAgDXth4Wa0jlrEBP3oCsEL59ONuqhf0CWkEPZJApWxs9F6r1GBEq8uk5mDhCDKBqvkRlUI7GCYmzZi9vG+NoP4o6gTaCtc9Gl1lESFN9qTsCk+xdQIRZMR4KY5jLgKJUgjXByli412yeCXEVgRKqiINiRD5mslmgtNTEgsg4h0OuLN7BiN3wNga4WI8EWXgFHytqcwlg86Dkrmto8pcGyKoI9XWu1t658Cns3DsiI8EbSqnN6YhMLO0FZgAm7obRuQAALyQFa6NrxPiHE9dYH1gboTIDhLuPjlJyjVF6GSYo2niS9AAG5tkYxgZlNk8CCk7wAMrkLEHJ9lxCtwyS4kLDDvJ4e3dIPQF12wAAShNYAAFkYTAAF7zdUKJRHVHEZI6wSa5GM/iXT9RvJCxK5V+r7gmuIxfjCfJpNGjX2c7gekbeUBVe8vQfAfb1BEOy7oKjlKx4sDWgERhrgyvim2/t9roguuxESKkcbmnZvmfqO0U9Cn0hSSa64tkJ3Ivw1SejU613gAkwlyWH7IEewpR/czHm3GuteO7zwcUbegc96DzzR3AhevDF872GiNuAK8FFdwxCTHKpPs+2O77nnud7SNxYY6V6tsPI9Zpv8Pmq3SbMQMAAA5GgFE0AUROLwFfvQAiABkZ/WzD5RExH7GAuBON7/3gvl/R+SZIQnxjQoFdwy0oBa0/wwFVzwAfyfwxyTW/2v3pFxzwE/zwEQ2AAAHk8B6pQgcc9gB9gA/9Q0AD2BJ8QC+8ABqSAM/BfEgufQAhfJfA4HnbeCBKBMAIwTnd+KWOKVGHnP6dg5BcGNBVES6WYK8WtRGZGY8d1DnD6OKHg+8PgxQVg6OJHJQSIG2bld4OQ3gEgBzMoP2coCgEmTEFUDOObRjMrKpHCRUAhJQz+Sw4oeQSHP2I+UHYtb8WmByVEAQSw2KW+d9LmV+R+QWF+B+debGD+OIL0G1AHZwEdToMdHwjwWLAI8IrYHcFDWvBoUOMhUBdgIuDwybc+ZdCHA+aePmN4V2NAd2QDS+f2aQQOLyVTH2CINOWVbAIgDEO5Rw2Ir+LoNANgZOPIbyWCGSdGVSXZLrcpYEEzcEZ9NWFENje9R9V/PgaYhYXwbIIVG0FVC7Z+LYMdZwhIoHaLEo6raCT7EGWjd/Iuf7UQQHa5UFIsE+cHFWXuFuGHduHyQYjGfo6WOYoVC8CopuP2Y+arYudIpmb+IIjmEIh+RtZ+JeN+DeGIoE3+DGfgz3HUdg0eaBBQ0IqIiWDEgQxBSBYQ1BYadBRwGGCQ4PPBTJKra2T9Qo7kOwjGAAdRFlINIS5h5PUD5KAKdWRKFkFJoGODiCxjECsHLl9E9HUOQCkLl2PG1lohPiXWSLLE1nsBThknJB8HkBdT6PRKlmARSmcKcw6SjkaxZK8M9jaPUzVJvkjkdgWDuB0O+XF3aGuA1LpB4GoAxA0lCFoHwEtGJXEmyGyBmkI1GClnm0qn4D4CWPVmtI8BumuKBi+zuPOL+yAyePLnvFeMhLBx6jBK+OhyfCMCW3IApKgWenWjyx6j6l2mpImQUBJ3GlOnOjpMwWukUFumWm0EekMCbNGnUHiFSEQHiH8FChWDoHiBAVZPagMCbLoAADZmoABmWgAAdkqAAEYABOSoRqXc2gbIAQS8485qbIE8hgQ87IbcgADmag3OPOPIYAEDQG3IEEakalHLXM6kgGaj3IEHfIACY9zDyRxKg9zmoXyPyNzDy3zty9zGotz0KGBoLoyNzty0BILILmoiKgKmzMKNy0BGpILRBGo0KSBjyXztzjz8Ldy/zULmo0BjyBBsglBuLmpmpaAXyGBjyyKQKty9yNzsgXyYySANyXzGpHyVBtyGBmKmhdy6BDyBBty/zJLjyiLty5L0xVzxyjpJzpzZyDDNwFzaB4hupRygA=== -->

<!-- internal state end -->
<!-- finishing_touch_checkbox_start -->

<details open="true">
<summary>✨ Finishing Touches</summary>

- [ ] <!-- {"checkboxId": "7962f53c-55bc-4827-bfbf-6a18da830691"} --> 📝 Generate Docstrings

</details>

<!-- finishing_touch_checkbox_end -->
<!-- tips_start -->

---

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.

<details>
<summary>❤️ Share</summary>

- [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai)
- [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai)
- [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai)
- [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)

</details>

<details>
<summary>🪧 Tips</summary>

### Chat

There are 3 ways to chat with [CodeRabbit](https://coderabbit.ai?utm_source=oss&utm_medium=github&utm_campaign=rainlanguage/alloy-ethers-typecast&utm_content=54):

- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  - `I pushed a fix in commit <commit_id>, please review it.`
  - `Explain this complex logic.`
  - `Open a follow-up GitHub issue for this discussion.`
- Files and specific lines of code (under the "Files changed" tab): Tag `@coderabbitai` in a new review comment at the desired location with your query. Examples:
  - `@coderabbitai explain this code block.`
  -	`@coderabbitai modularize this function.`
- PR comments: Tag `@coderabbitai` in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  - `@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.`
  - `@coderabbitai read src/utils.ts and explain its main purpose.`
  - `@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.`
  - `@coderabbitai help me debug CodeRabbit configuration file.`

### Support

Need help? Create a ticket on our [support page](https://www.coderabbit.ai/contact-us/support) 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)

- `@coderabbitai pause` to pause the reviews on a PR.
- `@coderabbitai resume` to resume the paused reviews.
- `@coderabbitai review` to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
- `@coderabbitai full review` to do a full review from scratch and review all the files again.
- `@coderabbitai summary` to regenerate the summary of the PR.
- `@coderabbitai generate docstrings` to [generate docstrings](https://docs.coderabbit.ai/finishing-touches/docstrings) for this PR.
- `@coderabbitai generate sequence diagram` to generate a sequence diagram of the changes in this PR.
- `@coderabbitai resolve` resolve all the CodeRabbit review comments.
- `@coderabbitai configuration` to show the current CodeRabbit configuration for the repository.
- `@coderabbitai help` to get help.

### Other keywords and placeholders

- Add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed.
- Add `@coderabbitai summary` to generate the high-level summary at a specific location in the PR description.
- Add `@coderabbitai` anywhere in the PR title to generate the title automatically.

### CodeRabbit Configuration File (`.coderabbit.yaml`)

- You can programmatically configure CodeRabbit by adding a `.coderabbit.yaml` file to the root of your repository.
- Please see the [configuration documentation](https://docs.coderabbit.ai/guides/configure-coderabbit) for more information.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: `# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json`

### Documentation and Community

- Visit our [Documentation](https://docs.coderabbit.ai) for detailed information on how to use CodeRabbit.
- Join our [Discord Community](http://discord.gg/coderabbit) to get help, request features, and share feedback.
- Follow us on [X/Twitter](https://twitter.com/coderabbitai) for updates and announcements.

</details>

<!-- tips_end -->

@0xgleb 0xgleb changed the title Refactor and multicall Refactor and on_provider Jun 18, 2025
@0xgleb 0xgleb changed the title Refactor and on_provider on_provider and refactor Jun 18, 2025
@0xgleb
0xgleb marked this pull request as ready for review June 18, 2025 16:21
@0xgleb
0xgleb requested review from findolor and hardyjosh June 18, 2025 16:22

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🔭 Outside diff range comments (4)
src/read.rs (4)

72-97: new_from_http_urls silently drops non-HTTP(S) URLs

url.parse() failure or a non-http scheme skips the entry without recording why. In a multi-URL setup this can mask a typo.

Consider surfacing the rejected URLs so callers know something was ignored:

-let providers: HashMap<String, _> = urls
-    .iter()
-    .filter_map(|url| { … })
-    .collect();
+let mut providers = HashMap::new();
+let mut skipped = Vec::new();
+
+for url in urls {
+    match Url::parse(&url) {
+        Ok(uri) if uri.scheme().starts_with("http") => {
+            providers.insert(
+                url.clone(),
+                ProviderBuilder::new().network::<AnyNetwork>().connect_http(uri),
+            );
+        }
+        _ => skipped.push(url),
+    }
+}
+
+if !skipped.is_empty() {
+    tracing::warn!("Ignored non-HTTP urls: {:?}", skipped);
+}

135-166: The three provider loops duplicate error-aggregation logic

read, get_chainid, and get_block_number all implement the same “try providers sequentially, collect errors” pattern. Factor this out into a single helper to simplify maintenance and guarantee identical behaviour.

@@
-        let mut errors: HashMap<String, ReadableClientError> = HashMap::new();
-
-        for (url, provider) in &self.providers {
-            match provider
-                .call(WithOtherFields::new(transaction_request.clone()))
-                .await
-            {
-                Ok(res) => {
-                    return C::abi_decode_returns(res.to_vec().as_slice()).map_err(|err| {
-                        ReadableClientError::ReadDecodeReturnError(err.to_string())
-                    });
-                }
-                Err(provider_err) => {
-
-                    errors.insert(url.clone(), error_to_insert);
-                }
-            }
-        }
-
-        Err(ReadableClientError::AllProvidersFailed(errors))
+        self.on_providers(|url, provider| async move {
+            let res = provider
+                .call(WithOtherFields::new(transaction_request.clone()))
+                .await
+                .map_err(|e| map_call_error(url, e).await)?;
+
+            C::abi_decode_returns(res.to_vec().as_slice())
+                .map_err(|e| ReadableClientError::ReadDecodeReturnError(e.to_string()))
+        })
+        .await

And add once:

async fn on_providers<R, Fut, F>(&self, mut op: F) -> Result<R, ReadableClientError>
where
    F: FnMut(&str, &ReadProvider) -> Fut,
    Fut: std::future::Future<Output = Result<R, ReadableClientError>>,
{
    let mut errors = HashMap::new();
    for (url, provider) in &self.providers {
        match op(url, provider).await {
            Ok(v) => return Ok(v),
            Err(e) => {
                errors.insert(url.clone(), e);
            }
        }
    }
    Err(ReadableClientError::AllProvidersFailed(errors))
}

This eliminates 100+ lines of nearly identical code.


171-204: Sequential provider attempts increase latency

The current for loop waits on one provider before trying the next. Using futures::future::select_ok (or the above on_providers helper with futures::future::join_all) could issue the requests concurrently and return the first success, dramatically reducing tail latency.

Not blocking, but worth considering if read performance is critical.


108-116: new(providers) permits empty map through Ok(Self { .. }) path

The guard earlier prevents empty maps, but providers is taken by value; callers can move an empty HashMap in after a successful check with ReadableClient::new(HashMap::new()).
Return the error immediately if providers.is_empty() before moving the map:

pub fn new(providers: HashMap<String, ReadProvider>) -> Result<Self, ReadableClientError> {
    if providers.is_empty() {
        return Err(ReadableClientError::CreateReadableClientHttpError(
            "cannot initiate a read client with no providers given".to_string(),
        ));
    }
    Ok(Self { providers })
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f7b5bfd and ed643d7.

📒 Files selected for processing (4)
  • src/lib.rs (1 hunks)
  • src/read.rs (11 hunks)
  • src/transaction/mod.rs (0 hunks)
  • src/write_transaction.rs (1 hunks)
💤 Files with no reviewable changes (1)
  • src/transaction/mod.rs
🔇 Additional comments (1)
src/write_transaction.rs (1)

1-1: Import-path update looks correct

The crate-root now re-exports WritableClient, WritableClientError, and WriteContractParameters, so the shortened path is the right call.
No further action needed.

Comment thread src/lib.rs
Comment on lines +1 to +7
pub mod read;
pub mod write;
pub mod write_transaction;

pub use read::*;
pub use write::*;
pub use write_transaction::*;

@coderabbitai coderabbitai Bot Jun 18, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Avoid wildcard re-exports at the crate root

Re-exporting every public item from the three modules (*) pollutes the top-level namespace and makes it harder to track what is part of the public API. Prefer explicit re-exports or keep the items grouped under their respective modules.

-pub use read::*;
-pub use write::*;
-pub use write_transaction::*;
+pub use read::{ReadableClient, ReadableClientError, ReadContractParameters};
+pub use write::{WritableClient, WritableClientError, WriteContractParameters};
+pub use write_transaction::{WriteTransaction, WriteTransactionStatus};

This keeps the surface area obvious and improves docs.

🤖 Prompt for AI Agents
In src/lib.rs lines 1 to 7, avoid using wildcard re-exports (pub use read::*;
etc.) at the crate root as it pollutes the top-level namespace. Instead,
explicitly re-export only the necessary public items from each module or keep
the modules grouped without re-exporting everything. This will make the public
API clearer and improve documentation clarity.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This keeps existing pattern and is fine for the scope of these modules

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0xgleb, understood! I'll note your preference for maintaining the existing pattern with wildcard re-exports in this context.

    (\   /)
   ( ._.)
  o_(")(")  *nods thoughtfully*

✏️ Learnings added
Learnt from: 0xgleb
PR: rainlanguage/alloy-ethers-typecast#54
File: src/lib.rs:1-7
Timestamp: 2025-06-18T16:26:46.561Z
Learning: In the rainlanguage/alloy-ethers-typecast codebase, wildcard re-exports at the crate root (pub use module::*;) are preferred to maintain existing patterns and are considered appropriate for the scope of the modules.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Comment thread src/read.rs
Comment on lines +50 to 56
pub type ReadProvider =
FillProvider<JoinedRecommendedFillers, RootProvider<AnyNetwork>, AnyNetwork>;

#[derive(Debug, Clone)]
pub struct ReadableClient {
providers: HashMap<String, Box<dyn Provider<AnyNetwork>>>,
providers: HashMap<String, ReadProvider>,
}

@coderabbitai coderabbitai Bot Jun 18, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Type alias is 👍 – consider moving to a dedicated providers module

ReadProvider is fairly verbose but very helpful. Moving it (and maybe WriteProvider later) into a small providers.rs keeps read.rs focused on client logic and makes the alias reusable elsewhere.
No code change strictly required.

🤖 Prompt for AI Agents
In src/read.rs around lines 50 to 56, the type alias ReadProvider should be
moved to a new dedicated module named providers.rs to keep read.rs focused on
client logic and improve reusability. Create providers.rs, move the ReadProvider
alias there, and update imports in read.rs accordingly. This refactor does not
require code changes beyond moving the alias and adjusting module references.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

read module is a reasonable place for ReadProvider

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0xgleb Understood! You're right that keeping ReadProvider in the read module maintains good cohesion and locality. I'll note your preference for future reviews.


✏️ Learnings added
Learnt from: 0xgleb
PR: rainlanguage/alloy-ethers-typecast#54
File: src/read.rs:50-56
Timestamp: 2025-06-18T16:36:12.115Z
Learning: The user prefers keeping type aliases like ReadProvider in their respective modules (e.g., read module) rather than moving them to dedicated modules like providers.rs.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
src/read.rs (1)

50-52: Alias-location refactor remains unresolved

Moving ReadProvider to a small providers.rs (as suggested earlier) would keep read.rs focused on client logic and make the alias reusable by other modules.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ed643d7 and 47b4627.

📒 Files selected for processing (1)
  • src/read.rs (14 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
  • GitHub Check: standard-tests (ubuntu-latest, rainix-rs-test)
  • GitHub Check: standard-tests (macos-latest, rainix-rs-artifacts)
  • GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts)
  • GitHub Check: standard-tests (macos-latest, rainix-rs-test)
🔇 Additional comments (3)
src/read.rs (3)

214-220: Chain-ID helper looks good
The refactor cleanly delegates to on_providers, preserving previous semantics while reducing duplication.


224-230: Block-number helper looks good
Same observation as above—concise and readable.


745-834: Comprehensive tests for on_providers 👍

The new test cases cover both success-fallback and all-fail scenarios, ensuring the helper behaves as intended.

Comment thread src/read.rs
Comment thread src/read.rs Outdated
Comment on lines +143 to +163
async fn on_providers<'a, T, Fut, F>(
&'a self,
mut operation: F,
) -> Result<T, ReadableClientError>
where
F: FnMut(&'a ReadProvider) -> Fut,
Fut: std::future::Future<Output = Result<T, ReadableClientError>> + 'a,
{
let mut errors: HashMap<String, ReadableClientError> = HashMap::new();

for (url, provider) in &self.providers {
match operation(provider).await {
Ok(value) => return Ok(value),
Err(err) => {
errors.insert(url.clone(), err);
}
}
}

Err(ReadableClientError::AllProvidersFailed(errors))
}

@coderabbitai coderabbitai Bot Jun 18, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider running provider calls concurrently

on_providers iterates sequentially. With N remote RPC endpoints, the worst-case latency is cumulative—one long timeout per provider. Using FuturesUnordered (or tokio::select!) would let you fire all requests immediately and return on the first success while still collecting the individual errors:

-        for (url, provider) in &self.providers {
-            match operation(provider).await {
-                Ok(value) => return Ok(value),
-                Err(err) => {
-                    errors.insert(url.clone(), err);
-                }
-            }
-        }
+        use futures::{stream::FuturesUnordered, StreamExt};
+        let mut futs: FuturesUnordered<_> = self
+            .providers
+            .iter()
+            .map(|(url, provider)| {
+                let url = url.clone();
+                async move { (url, operation(provider).await) }
+            })
+            .collect();
+
+        while let Some((url, res)) = futs.next().await {
+            match res {
+                Ok(v) => return Ok(v),
+                Err(e) => {
+                    errors.insert(url, e);
+                }
+            }
+        }

This keeps external behaviour identical but avoids unnecessary serial waits.

📝 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.

Suggested change
async fn on_providers<'a, T, Fut, F>(
&'a self,
mut operation: F,
) -> Result<T, ReadableClientError>
where
F: FnMut(&'a ReadProvider) -> Fut,
Fut: std::future::Future<Output = Result<T, ReadableClientError>> + 'a,
{
let mut errors: HashMap<String, ReadableClientError> = HashMap::new();
for (url, provider) in &self.providers {
match operation(provider).await {
Ok(value) => return Ok(value),
Err(err) => {
errors.insert(url.clone(), err);
}
}
}
Err(ReadableClientError::AllProvidersFailed(errors))
}
async fn on_providers<'a, T, Fut, F>(
&'a self,
mut operation: F,
) -> Result<T, ReadableClientError>
where
F: FnMut(&'a ReadProvider) -> Fut,
Fut: std::future::Future<Output = Result<T, ReadableClientError>> + 'a,
{
let mut errors: HashMap<String, ReadableClientError> = HashMap::new();
// Fire off all provider calls concurrently and collect their futures
use futures::{stream::FuturesUnordered, StreamExt};
let mut futs: FuturesUnordered<_> = self
.providers
.iter()
.map(|(url, provider)| {
let url = url.clone();
async move { (url, operation(provider).await) }
})
.collect();
// Await them as they complete, returning on first success
while let Some((url, res)) = futs.next().await {
match res {
Ok(v) => return Ok(v),
Err(e) => {
errors.insert(url, e);
}
}
}
Err(ReadableClientError::AllProvidersFailed(errors))
}
🤖 Prompt for AI Agents
In src/read.rs around lines 143 to 163, the on_providers function currently
calls each provider sequentially, causing cumulative latency. Refactor it to run
all provider operations concurrently using FuturesUnordered or tokio::select!,
so all requests start immediately and the function returns on the first
successful response while still collecting errors from all providers. This will
maintain the same external behavior but improve performance by avoiding serial
waits.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the purpose of using multiple providers is to avoid rate limits and this would drain all providers at the same time

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0xgleb You're absolutely right! I apologize for missing that key design consideration. The sequential approach is intentional and smart - by trying providers one at a time, you preserve the rate limits on the unused providers for future requests. Concurrent calls would indeed defeat the whole purpose by hitting all providers simultaneously.

Thank you for the clarification - that's exactly the kind of context that makes the design decision clear.

🐰💡
thoughtfully adjusts whiskers while taking notes


✏️ Learnings added
Learnt from: 0xgleb
PR: rainlanguage/alloy-ethers-typecast#54
File: src/read.rs:143-163
Timestamp: 2025-06-18T16:31:42.674Z
Learning: In the ReadableClient's `on_providers` method, providers are called sequentially (not concurrently) by design to avoid rate limits. The purpose of having multiple providers is rate limit avoidance - calling them concurrently would drain all providers at the same time, defeating this purpose.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🔭 Outside diff range comments (2)
src/read.rs (2)

72-97: Minor API ergonomics – accept any IntoIterator<Item = impl AsRef<str>>

new_from_http_urls presently requires Vec<String>. Accepting a generic iterator (or impl IntoIterator<Item = impl AsRef<str>>) removes the caller’s need to materialise a Vec and keeps the signature consistent with typical builder patterns.
Nit only; no functional impact.


36-37: Unused error variant

ReadableClientError::RpcProviderError isn’t constructed anywhere in this file.
If obsolete, remove to keep the enum lean; if future-facing, add a todo!() usage note or implement its emission.

♻️ Duplicate comments (1)
src/read.rs (1)

50-52: Type-alias still sits in read.rs; prior feedback remains applicable

Earlier review suggested moving ReadProvider (and any future aliases) into a dedicated providers.rs to keep this module focused on client logic and to improve reuse.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 47b4627 and 566a52e.

📒 Files selected for processing (1)
  • src/read.rs (14 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/read.rs (1)
Learnt from: 0xgleb
PR: rainlanguage/alloy-ethers-typecast#54
File: src/read.rs:143-163
Timestamp: 2025-06-18T16:31:42.674Z
Learning: In the ReadableClient's `on_providers` method, providers are called sequentially (not concurrently) by design to avoid rate limits. The purpose of having multiple providers is rate limit avoidance - calling them concurrently would drain all providers at the same time, defeating this purpose.
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: standard-tests (macos-latest, rainix-rs-test)
  • GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
  • GitHub Check: standard-tests (macos-latest, rainix-rs-artifacts)
  • GitHub Check: standard-tests (ubuntu-latest, rainix-rs-test)
  • GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts)

Comment thread src/read.rs
Comment on lines +182 to +209
self.on_providers(|provider| {
let transaction_request = transaction_request.clone();
async move {
match provider
.call(WithOtherFields::new(transaction_request))
.await
{
Ok(res) => C::abi_decode_returns(res.as_ref())
.map_err(|err| ReadableClientError::ReadDecodeReturnError(err.to_string())),
Err(provider_err) => {
if let Some(rpc_err) = provider_err.as_error_resp() {
match AbiDecodedErrorType::try_from_json_rpc_error(rpc_err.clone())
.await
{
Ok(decoded_err) => {
Err(ReadableClientError::AbiDecodedErrorType(decoded_err))
}
Err(decode_failed_err) => Err(
ReadableClientError::AbiDecodeFailedErrors(decode_failed_err),
),
}
} else {
Err(ReadableClientError::RpcTransportKindError(provider_err))
}
} else {
ReadableClientError::RpcTransportKindError(provider_err)
};
errors.insert(url.clone(), error_to_insert);
}
}
}
}

Err(ReadableClientError::AllProvidersFailed(errors))
})

@coderabbitai coderabbitai Bot Jun 18, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Tiny optimisation – avoid cloning the transaction request per iteration

transaction_request is cloned for every provider, yet only one provider call will succeed.
Since the value isn’t mutated, a reference suffices:

-self.on_providers(|provider| {
-    let transaction_request = transaction_request.clone();
+let tx = &transaction_request;
+self.on_providers(|provider| {

A micro-optimisation, but it removes one allocation per failed provider.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In src/read.rs around lines 182 to 209, the transaction_request is cloned inside
the closure for each provider, causing unnecessary allocations. To fix this,
change the closure to capture a reference to transaction_request instead of
cloning it. Pass a reference to transaction_request into WithOtherFields::new so
that cloning is avoided and only one allocation occurs for the successful
provider call.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

premature optimization imho

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0xgleb Fair point! You're right that this level of micro-optimization probably isn't worth the cognitive overhead at this stage. Better to focus on more impactful improvements first.

🐰💭


✏️ Learnings added
Learnt from: 0xgleb
PR: rainlanguage/alloy-ethers-typecast#54
File: src/read.rs:182-209
Timestamp: 2025-06-18T16:39:41.964Z
Learning: 0xgleb prefers to avoid micro-optimizations like avoiding clones unless there's evidence they're causing performance bottlenecks, considering them premature optimization.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Comment thread src/read.rs Outdated
Comment on lines +143 to +163
async fn on_providers<'a, T, Fut, F>(
&'a self,
mut operation: F,
) -> Result<T, ReadableClientError>
where
F: FnMut(&'a ReadProvider) -> Fut,
Fut: std::future::Future<Output = Result<T, ReadableClientError>> + 'a,
{
let mut errors: HashMap<String, ReadableClientError> = HashMap::new();

for (url, provider) in &self.providers {
match operation(provider).await {
Ok(value) => return Ok(value),
Err(err) => {
errors.insert(url.clone(), err);
}
}
}

Err(ReadableClientError::AllProvidersFailed(errors))
}

@coderabbitai coderabbitai Bot Jun 18, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Fail-over order is nondeterministic – consider an ordered container

on_providers iterates over &self.providers, but HashMap iteration is hash-order dependent and thus unpredictable across runs & platforms.
If deterministic provider priority (e.g., most-reliable first) is desirable, switch to Vec<(String, ReadProvider)> or indexmap::IndexMap, or sort the keys before iteration:

-for (url, provider) in &self.providers {
+for (url, provider) in self
+    .providers
+    .iter()
+    .sorted_by_key(|(url, _)| url)   // requires itertools/IndexMap, or keep a Vec
+{

Exact ordering isn’t mandatory for correctness, but stable behaviour eases debugging and makes tests less brittle.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In src/read.rs around lines 143 to 163, the iteration over self.providers uses a
HashMap, which results in nondeterministic order. To ensure deterministic
fail-over order, replace the HashMap with an ordered container such as
Vec<(String, ReadProvider)> or indexmap::IndexMap, or alternatively sort the
keys before iterating. This change will make the provider iteration order stable
and predictable, improving debugging and test reliability.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good if anything. We use multiple providers for the sake of avoiding rate limits, so hitting them in random order prevents a rate limited one from failing over and over again

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0xgleb That's an excellent point! The random iteration order actually serves as a natural load balancing mechanism across providers, preventing any single rate-limited provider from being consistently hit first and creating cascading failures. This is a smart architectural decision for resilience.

Thank you for the clarification - I'll note this design rationale for future reviews.

🐰💡

   /|   /|  
  ( :v:  )
   |(_)|

✏️ Learnings added
Learnt from: 0xgleb
PR: rainlanguage/alloy-ethers-typecast#54
File: src/read.rs:143-163
Timestamp: 2025-06-18T16:38:53.646Z
Learning: In ReadableClient's on_providers method (src/read.rs), the HashMap iteration order is intentionally non-deterministic to provide natural load balancing across multiple providers. This prevents rate-limited providers from being hit repeatedly first, which helps avoid cascading failures. The random order is a feature for resilience, not a bug.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

@0xgleb 0xgleb changed the title on_provider and refactor on_providers and refactor Jun 18, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant