Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: ProviderCall #788

Merged
merged 46 commits into from
Sep 18, 2024
Merged

feature: ProviderCall #788

merged 46 commits into from
Sep 18, 2024

Conversation

prestwich
Copy link
Member

@prestwich prestwich commented May 27, 2024

Closes #736

Motivation

Allow providers to get outputs from arbitrary sources, including synchronous

Solution

  • Add a ProviderCall future that wraps 4 data sources (rpc, batch rpc, other future, ready)
  • change provider return types
  • change inner type of eth call and with_block

drive-by:

  • generalize into_owned_params
  • alphabetize some mod defs
  • improve mapping internal to rpccall
  • add map to waiter

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@prestwich prestwich added enhancement New feature or request debt Tech debt which needs to be addressed labels May 27, 2024
@prestwich prestwich self-assigned this May 27, 2024
@prestwich prestwich force-pushed the prestwich/provider-call-fut branch from b465c20 to b1496cb Compare May 28, 2024 09:02
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

makes sense,

this is still easy to follow imo

@prestwich prestwich force-pushed the prestwich/provider-call-fut branch from b1496cb to 21b11f1 Compare May 31, 2024 10:23
fix: rename to prov_call for disambiguation
@prestwich prestwich force-pushed the prestwich/provider-call-fut branch from 068c2e4 to f14aa77 Compare May 31, 2024 11:48
@prestwich
Copy link
Member Author

the main problem right now is figuring out how to integrate it into WithBlock and EthCall 🤔

@yash-atreya
Copy link
Member

hey @prestwich

Taking this over, need it for #954

@yash-atreya
Copy link
Member

yash-atreya commented Jun 27, 2024

hey @prestwich

Taking this over, need it for #954

Would appreciate it if you have any pointers on how to move forward from here.

I think only integrating ProviderCall with RpcWithBlock and EthCall remains?

@yash-atreya
Copy link
Member

I think we can now unify Caller impls for WithBlock and EthCall? or maybe just do impl Caller for WeakClient<T>?

impl Caller for WeakClient<T> makes sense

@klkvr
Copy link
Member

klkvr commented Aug 12, 2024

@yash-atreya one thing I find a bit confusing here is that by design of Caller it does not receive more context than a RequestPacket we are working with when overriding transport layers.

I think one valid usecase for this PR is for example changing Provider implementation to reading storage slots from some other source for get_storage_at requests. Currently this would require implementing a custom Caller which would manually deserialize eth_getStorageAt params to determine address, slot and block which adds an additional fallible step which wasn't required earlier when those methods returned just TransportResult

I think what we want for WithBlock is something that can produce a ProviderCall given a block_id: BlockId, and also is able to capture context such as Params. This is essentially fn(BlockId) -> ProviderCall, though we'd likely need more concrete type here to handle lifetimes in cases of borrowed rpc call params

klkvr and others added 2 commits August 21, 2024 07:31
…1159)

* feat: RpcCall:map_params

* WithBlockInner

* Ready variant

* fmt

* use ProviderCall

* clippy + fmt

* add From<F>

* make ProviderCall::Ready return a result

* add docs

* Update request.rs

Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>

* fmt

---------

Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>
@yash-atreya
Copy link
Member

@mattsse I believe this can be merged now

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

I think this should work now,

mind giving this another close look @klkvr ?

@klkvr
Copy link
Member

klkvr commented Aug 22, 2024

@yash-atreya wondering how do we decide which Provider methods return ProviderCall vs do async fn -> TransportResult<...>?

@yash-atreya
Copy link
Member

yash-atreya commented Aug 26, 2024

@yash-atreya wondering how do we decide which Provider methods return ProviderCall vs do async fn -> TransportResult<...>?

Sorry for the delayed response.

TransportResult is also overridable like ProviderCall.

I believe subscribe/watch methods should be TransportResult since they return PollerBuilder, similar logic applies to send_tx_* methods.

Also, any RPC method where we accept a parameter by reference, should be TransportResult since replacing it with ProviderCall will introduce lifetimes. eg. get_fee_history

Rest can be made to return ProviderCall.

Should extension traits such as AdminApi, TraceApi etc. be moved to ProviderCall as well?

@mattsse wdyt?

@klkvr
Copy link
Member

klkvr commented Aug 26, 2024

Also, any RPC method where we accept a parameter by reference, should be TransportResult since replacing it with ProviderCall will introduce lifetimes. eg. get_fee_history

Lifetimes should be fine, we already have them in e.g. estimate_gas

Should extension traits such as AdminApi, TraceApi etc. be moved to ProviderCall as well?

We are already using RpcWithBlock in TraceApi, so I wouldn't say that those aren't tied to provider already. Though I can see how changing their returns types to ProviderCall might break usecases unrelated to provider

@yash-atreya
Copy link
Member

Lifetimes should be fine, we already have them in e.g. estimate_gas

We want to avoid introducing lifetimes in the trait as much as possible, eth_call and estimate_gas are exceptions since the addition of EthCall.

how changing their returns types to ProviderCall might break usecases unrelated to provider

+1

use std::borrow::Cow;

/// A caller that helper convert `RpcWithBlock` and `EthCall` into a `ProviderCall`.
pub trait Caller<T, Params, Resp>: Send + Sync
Copy link
Member

Choose a reason for hiding this comment

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

given we're now wonly using this for EthCall, I think we should make it more call-specific, eg make it accept TransactionRequest, block and overrides directly to allow implementations to directly call underlying evm with them.

Copy link
Member

@klkvr klkvr left a comment

Choose a reason for hiding this comment

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

lgtm overall, just a note on Caller trait, we can address this in a follow-up

@yash-atreya yash-atreya mentioned this pull request Sep 9, 2024
14 tasks
@mattsse mattsse merged commit 6369bcc into main Sep 18, 2024
26 checks passed
@mattsse mattsse deleted the prestwich/provider-call-fut branch September 18, 2024 10:06
lwedge99 pushed a commit to sentioxyz/alloy that referenced this pull request Oct 8, 2024
* feature: ProviderCall

fix: rename to prov_call for disambiguation

* refactor: replace output types

* doc: add note about fn pointers

* feat: convert `EthCall` to `ProviderCall`

* use `ClientRef` in EthCall instead of `WeakClient`

* change lifetimes

* add *_internal methods that return `EthCall`

* doc nits

* use `ClientRef` instead of `WeakClient` in `RpcWithBlock`

* Revert "use `ClientRef` instead of `WeakClient` in `RpcWithBlock`"

This reverts commit 569d168.

* feat: RpcCallWithBlock enum val in provider call

* nits

* feat: integrate `RpcWithBlock` into `ProviderCall`

* rm `RpcWithBlock` val from ProviderCall

* feat(provider): caller trait for with_block calls

* rm `WeakClient` from `RpcWithBlock`

* introduce `Caller` into `RpcWithBlock`

* use `WithBlockCall` in `RpcWithBlock`. polling doesn't work.

* nit

* feat(provider): add `WithBlockFut` - polls `ProviderCall` and maps response.

* nit

* revert 2ff402

* Revert "feat: convert `EthCall` to `ProviderCall`"

This reverts commit 1cb1579.

* doc nits

* feat(provider): introduce `Caller` to `EthCall`. Arc it so that its cloneable

* feat(provider): add `EthCaller` - converts EthCall into ProviderCall and strengthen test

* update `EthCallFut`
* holds `ProviderCall`
* removes Clone

* impl poll_preparing and poll_running on `EthCallFut` instead of `EthCallFutInner`

* poll `ProviderCall` in `EthCall`using `EthCaller`
* removes WeakClient from `EthCall`
* updates `EthCallFutInner::Running` to store map

* nit

* wrap Caller in Arc to make `RpcWithBlock` cloneable

* store in `ProvideCall` fut instance in `States`

* store `ProviderCall` fut in `EthCallFutInner`

* feat(provider): add `ParamsWithBlock` for `RpcWithBlock`

* rm `block_id` from `Caller` trait

* impl `Caller` for `WeakClient`

* refactor(`ProviderCall`): don't rely on `Caller` in `RpcWithBlock` (alloy-rs#1159)

* feat: RpcCall:map_params

* WithBlockInner

* Ready variant

* fmt

* use ProviderCall

* clippy + fmt

* add From<F>

* make ProviderCall::Ready return a result

* add docs

* Update request.rs

Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>

* fmt

---------

Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>

* refac(provider): use `ProviderCall` as return in trait

* nit

* doc nits

* fix

---------

Co-authored-by: Yash Atreya <44857776+yash-atreya@users.noreply.github.com>
Co-authored-by: Arsenii Kulikov <klkvrr@gmail.com>
Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debt Tech debt which needs to be addressed enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Allow custom implementation for Provider methods
6 participants