Conversation
stjepangolemac
left a comment
stjepangolemac
left a comment
There was a problem hiding this comment.
A few suggestions but I'd like to reiterate my reasoning here.
If we want to make the proving logic as portable as possible we want to expose it in such a way that the caller controls everything except the proving.
In pseudocode it might look something like this:
prove(block) -> proof
Everything else should be left for the caller to do. At the same time this is still a draft so feel free to largely ignore what I'm saying if you're still in the middle of development.
| anyhow = "1.0.75" | ||
| itertools = "0.12.0" | ||
| plonky2 = "0.1.4" | ||
| ethers = { version ="2.0.11", default-features = false, features = ["rustls"]} |
There was a problem hiding this comment.
Seems like ethers is being deprecated gakonst/ethers-rs#2667
There was a problem hiding this comment.
Yeah but alloy doesn't contain everything I need at the moment.
And it's not deprecated really, they just don't ship new features. (see tweet)
| } | ||
|
|
||
| impl BlockData { | ||
| pub async fn fetch<T: Into<BlockId> + Send + Sync>(blockid: T) -> Result<Self> { |
There was a problem hiding this comment.
I might be wrong but to me it feels a bit off to have fetching logic here. I still don't have a clear description of this repo in my head so that might be the problem.
The reason behind this is that I believe we will have a suite of fetching logic for all kinds of data centralized somewhere.
There was a problem hiding this comment.
Yeah I feel you. To me too.
Reason why I put it here is I needed to get some real data, full blocks. I like when things are all integrated so I didn't want to write a separate script for fetching them etc.
The best thing would be that it's given as input to the proof library, as it is not but I want to be able to run independent tests so I still need to have that logic inside tests at the very least. I could put the fetching in a test ?
Or the best thing: probably should live in the distributed query repo actually, as a separate crate so I can depend on it without having to depend on the whole distributed query repo. Or maybe just a single crate we control.
I'm happy in any of these solutions, but probably as a future fix if you don't mind. Happy to hear you thoughts.
There was a problem hiding this comment.
I think having this logic in a separate function in tests would make sure the public functions don't do it internally and still allow you to call it very easily and continue getting the data.
There was a problem hiding this comment.
As a stop gap solution, I just included the whole module in a #[cfg(test)] attribute. Wdyt ?
lopeetall
left a comment
lopeetall
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks for the informative comments and well-chosen names!
3 participants
No description provided.