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

Initial attempt to make Source generic over JsonRpcClient #68

Closed
wants to merge 3 commits into from

Conversation

0xprames
Copy link

Draft PR for #65 attempting to make Source generic over impl ethers::providers::JsonRpcClient to allow for both a Ws and HttpProvider

This currently doesn't compile because the current implementation route makes the Dataset trait non-object safe!

error[E0038]: the trait `types::datatypes::scalar::Dataset` cannot be made into an object
   --> crates/freeze/src/types/datatypes/multi.rs:54:49
    |
54  |     fn datasets(&self) -> HashMap<Datatype, Box<dyn Dataset>> {
    |                                                 ^^^^^^^^^^^ `types::datatypes::scalar::Dataset` cannot be made into an object
    |
note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
   --> crates/freeze/src/types/datatypes/scalar.rs:177:14
    |
148 | pub trait Dataset: Sync + Send {
    |           ------- this trait cannot be made into an object...
...
177 |     async fn collect_chunk(
    |              ^^^^^^^^^^^^^ ...because method `collect_chunk` has generic type parameters
...
196 |     async fn collect_block_chunk(
    |              ^^^^^^^^^^^^^^^^^^^ ...because method `collect_block_chunk` has generic type parameters
...
207 |     async fn collect_transaction_chunk(
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^ ...because method `collect_transaction_chunk` has generic type parameters
...
218 |     async fn collect_address_chunk(
    |              ^^^^^^^^^^^^^^^^^^^^^ ...because method `collect_address_chunk` has generic type parameters
    = help: consider moving `collect_chunk` to another trait
    = help: consider moving `collect_block_chunk` to another trait
    = help: consider moving `collect_transaction_chunk` to another trait
    = help: consider moving `collect_address_chunk` to another trait

any pointers/corrections or simplifications here or via another PR welcome! I'm happy to close this out, wanted to take a stab at seeing how this would look

@0xprames 0xprames changed the title Initial attempt to make Source generic over Provider<impl JsonRpcClient> Initial attempt to make Source generic Sep 24, 2023
@0xprames 0xprames changed the title Initial attempt to make Source generic Initial attempt to make Source generic over JsonRpcClient Sep 24, 2023
Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

This should work by refactoring struct Fetcher to be generic over M: Middleware, and have that propagate to Source, instead of generalizing over the transport

@0xprames
Copy link
Author

0xprames commented Sep 25, 2023

I see yeah - that makes sense, just went through the ethers-middleware crate.
Where im still a bit confused is this:

and have that propagate to Source

Trying to understand how to get it such that we don't have to generalize Source over anything since that seems to give us the above-mentioned compiler errors. Basically how to avoid this:

pub struct Source <M> {
fetcher: Arc<Fetcher<M>>, // how to get away from specifying the generic in the attribute here, such that Source can be declared without a generic?
// other attributes

}

pub struct Fetcher<M> where M: Middleware {
// other attributes
provider: M
}

@gakonst
Copy link
Member

gakonst commented Sep 25, 2023

Define Fetcher<M> { provider: M } and then only in impl<M: Middleware> Fetcher { ...} do you restrict the trait. This ensures that you can still use struct Source<M> { fetcher: Fetcher<M> } without having to do Source<M: Middleware> { fetcher: Fetcher<M> }

@0xprames
Copy link
Author

0xprames commented Sep 26, 2023

yeah you're right that doing what you mentioned would make Source<M>generic without needing Source<M:Middelware> but what I see from this current iteration of the PR is

Source<M> as a generic seems to breaks the "object-safe" trait contract set by Dataset, specifically in how its returned here

quoted from the below link on workarounds:

So, what is object safety, and when is a trait object safe?

“Object safety” is a property of a trait that says “this trait can be turned into a trait object.” It’s accompanied by a set of rules (this is a simplification of the rules, and omits a number of complexities and corner cases):

A trait can’t have methods that do any of the following:
Take self by value as the receiver without a Sized.
Include any associated functions (which don’t take self at all).
Reference the Self type outside of the receiver position, except to access associated types for Self’s impl of the current trait or any supertraits.
Include any generic parameters.

which aligns with the error I pasted above:

 fn datasets(&self) -> HashMap<Datatype, Box<dyn Dataset>> {
    |                                                 ^^^^^^^^^^^ `types::datatypes::scalar::Dataset` cannot be made into an object
    |
note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
   --> crates/freeze/src/types/datatypes/scalar.rs:177:14
   
    async fn collect_chunk(
    |              ^^^^^^^^^^^^^ ...because method `collect_chunk` has generic type parameters

because the corresponding trait functions would need to be defined as such (IIUC):

async fn collect_chunk<M>(
        &self,
        chunk: &Chunk,
        source: &Source<M>,

there seem to be some workarounds, including using an Enum type instead of a generic, but that's a bit more of a structural change - so i wanted to kickstart a discussion.

Also - if im misunderstanding the pasted compiler error/the link above, let me know. I could be wrong in how I understand the error.

@0xprames
Copy link
Author

going to close this out for now

dont want to leave a stale PR draft hanging, not sure how to work around what I mentioned above without reworking either the Dataset trait or making Source accept an enum Fetcher/Provider, which would significantly change the code (matches everywhere).

Im also not sure adding WS is a super critical feature, since cryo functions fine as is, so I dont think restructuring the code is worth it here.

@0xprames 0xprames closed this Sep 27, 2023
@0xprames 0xprames deleted the add-ws-client branch September 27, 2023 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants