-
Notifications
You must be signed in to change notification settings - Fork 38
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
Hypersync source support #103
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Hi @joshstevens19 |
Thanks for this i think how you done it is pretty good i was hoping to also extend this to support ExEx on reth later on (but that's for a later thought) but as dyn trait add runtime costs due to dynamic dispatcher vs enum which uses static dispatcher (which is faster) lets pick performance here so what you done IMHO is good - thoughts? if you are in agreement il do a PR review once its out of draft and rebased. If you can also add to the docs for this once done as well to say we support hypersync endpoints |
9c04744
to
02c9d7f
Compare
02c9d7f
to
a360138
Compare
Hi @joshstevens19 |
@@ -26,11 +26,14 @@ it then ask you 2 questions: | |||
networks: | |||
- name: ethereum // [!code focus] | |||
chain_id: 1 | |||
kind: Rpc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wouldnt change the rindexer yaml i would work this out under the hood basically if rpc .includes hypersync then you know its turned on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i could make default kind = Rpc, so we won't change default rindexer yaml. But considering other impls in the future (exex, sqd) it better to specify it trough param
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah ok we can make it default RPC and then not add it to all examples but just have 1 example of how to enable it for hypersync?
core/src/provider.rs
Outdated
.map_err(|err| ProviderError::CustomError(err.to_string())) | ||
} | ||
|
||
async fn get_logs(&self, filter: &RindexerEventFilter) -> Result<Vec<Log>, ProviderError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i did comment this but worth looking at this d1683fb as we exposed a new log type to expose the block timestamp
core/src/provider.rs
Outdated
|
||
async fn get_block_number(&self) -> Result<U64, ProviderError>; | ||
|
||
async fn get_logs(&self, filter: &RindexerEventFilter) -> Result<Vec<Log>, ProviderError>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -15,8 +19,8 @@ contracts: | |||
details: | |||
- network: ethereum | |||
address: 0xae78736cd615f374d3085123a210448e74fc6393 | |||
start_block: '18600000' | |||
end_block: '18718056' | |||
start_block: '11600000' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert?
"/Users/joshstevens/code/rindexer/rindexer_rust_playground/rindexer.yaml", | ||
) | ||
.expect("Invalid path"); | ||
let path = PathBuf::from_str("./rindexer.yaml").expect("Invalid path"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx lol full path 😛
@@ -11,6 +17,8 @@ pub struct Network { | |||
|
|||
pub rpc: String, | |||
|
|||
pub kind: ProviderType, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah defo not keen on this approach would rather auto work it out by rpc URL IMHO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i could think only about 2 options:
- Deduce type based on url
- Implicitly specify it
First have the problem that it could fail for no apparent reason (change from using https://1.hypersync.xyz to localproxy/kubernetes service name will break it silently with no way to fix it unless code is changed)
Second is too verbose
Wdyt about combining approaches -> We deduce from url if no kind provided?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah ok we can make it default RPC and then not add it to all examples but just have 1 example of how to enable it for hypersync?
fb388f9
to
5677c19
Compare
Thanks a lot I’m currently on annual leave till 28th but once back we can get this in 💪💪 |
@joshstevens19 |
hey is this complete @SozinM ? |
Hi @joshstevens19 yes, should be good now |
@@ -43,6 +43,7 @@ project_type: no-code | |||
networks: | |||
- name: ethereum | |||
chain_id: 1 | |||
kind: Rpc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for this if fallbacks to RPC (same for all below here)
@@ -31,6 +31,16 @@ networks: | |||
|
|||
This will skip for you if you only have one network setup. | |||
|
|||
By setting kind to "Hypersync" you could use [hypersync](https://docs.envio.dev/docs/HyperSync/overview) as a data source. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm need to think about the best place for this callout
ok going to need to do some manual tests on this over weekend and make sure all good if so can deploy do want to fix some of the documentation call outs etc - nice job @SozinM |
So sorry guys will review this tomorrow/Monday and test it out then merge it good fully forgot 🙏 |
hey when i build this locally i get
its trying to run a build.rs of:
we shouldnt have to install capnp locally or on machine to make work imho - how could we fix this? |
a45442d
to
9c5d47a
Compare
This Pr adds hypersync integration to the rindexer.
Initially i made it with enum, but i think that dynamic dispatch fits better here. We need to choose instance during runtime via config.
This is p1 of the changes, would be nice to move streaming logic to another trait/extend current one, to be able to specialize streaming based on the upstream.