-
Notifications
You must be signed in to change notification settings - Fork 3
WEB3-560: feat: Helpers for queries over multiple blocks of the same chain #46
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
base: main
Are you sure you want to change the base?
Conversation
nategraf
left a comment
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 think this looks pretty good, and a definite improvement over the version I put together. I think this will be quite nice for consumers, and should avoid them needing to use SteelVerifier directly. The token-stats example looks quite nice with this.
| .call(); | ||
| let envs = input.into_env(Ð_MAINNET_CHAIN_SPEC); | ||
|
|
||
| // Execute the view calls on each EVM state. |
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.
As written, the host can provide any number of rates, with any interval, and the guest will report the average. I'm not exactly sure what the right way to handle this is, but I do think we should show some constraint or reporting of the time frame here, as this may be a common pitfall for people using the multiblock features.
One simple and accurate idea would be to report all block numbers that were considered as a list in the journal. Its a bit inelegant though.
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.
Fair enough. I agree, but that's exactly how the example was before... I'm not sure how to improve this example though without distracting from its core.
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 have changed the example so that the environments must be exactly 7,200 blocks (24 hours) apart, and it commits to the number of such intervals.
crates/steel/src/multiblock.rs
Outdated
| /// While this pattern requires `#[allow(private_bounds)]` on a public method that use it, | ||
| /// the benefit of improved code structure and maintainability is a worthwhile trade-off | ||
| /// for this internal abstraction. | ||
| pub(super) trait InputBuilder<D, F: EvmFactory> { |
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.
Its worth considering making this public. Without it being public, a user may have significant difficulty in implementing some kinds of generic functionality in their code. E.g. it would be difficult to have a single function that works with two kinds of commitments, to switch between using Anvil and live chains like Eth Mainnet and OP.
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 have made the InputBuilder trait public and moved it to host/builder.rs
crates/steel/src/multiblock.rs
Outdated
| // ignore the builder and use the available env with block commitment directly | ||
| env.into_input().await |
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.
This does seem odd. It seems your intention is essentially to take the EVM data (e.g. storage and event inclusion proofs) and combine it with the commitment provided by self. However, both in this case and the other implementations, self and env both contain the EVM data and the commitment. This on in particular is a bit awkward in that any EVM data on self will be dropped, but that feels more like a symptom than the problem itself.
Would it be reasonable to drop the env arg here? It seems like it would require an alterred usage pattern and I can imagine how the type propagation for the commit might be a bit painful.
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.
It seems your intention is essentially to take the EVM data (e.g. storage and event inclusion proofs) and combine it with the commitment provided by self.
Exactly. Use the builder self to attach the correct commitment to the evm data, and also convert it into a type EvmImput, that does not depend on an generics.
However, both in this case and the other implementations, self and env both contain the EVM data and the commitment.
No, self is a builder, so by definition it does not contain any EVM data but just config and env is a HostEvmEnv<D, F, ()>, while the final () actually assures that it does not contain any specific commitment.
I agree that the entire InputBuilder design is not ideal. However, it attaches different commitments to EVM data and tries to minimize code duplication, which is exactly what it is supposed to do.
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 thought about this design quite a bit and I still believe it is the cleanest way to create a generic commitment for an HostEvmEnv<D, F, ()> which is exactly what the Multiblock stuff needs.
To make it more clear, I have added corresponding documentation to the InputBuilder trait.
crates/steel/src/multiblock.rs
Outdated
| /// Creates a new [HostMultiblockEvmEnv] from the given [EvmEnvBuilder]. | ||
| /// | ||
| /// This ignores any potential EVM execution block set in the builder, but all other options | ||
| /// specified with this builder are incorporated when creating the individual [EvmEnv]. | ||
| pub fn from_builder(builder: EvmEnvBuilder<P, F, &'a ChainSpec<F::SpecId>, B>) -> Self { | ||
| Self { | ||
| builder, | ||
| env: MultiblockEvmEnv(BTreeMap::new()), | ||
| } | ||
| } |
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.
When I implemented it this way, it felt like a bit of a hack. My main motivation for doing so was to avoid duplicating all the methods on the EvmEnvBuilder struct here. It may be better to do that though now that we are looking to make this a stable API. I think the required tweaks to the API could result in a cleaner factoring, as the builder cloning is kind of a the root of some of awkwardness I believe.
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.
Honestly, I like this design because we get the staged builder pattern from the builder for free. Of course, we can make HostMultiblockEvmEnv the builder, but then we'd have to still double all these different config steps as well as the different commitment types. Ii would probably be worth it if this would allows us to get rid of the InputBuilder trait but I dont see that.
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.
In order to make the HostMultiblockEvmEnv a sufficient builder, we would need to add all the following
provider: P,
provider_config: ProviderConfig,
block: BlockId,
chain_spec: S,
commitment_config: B,
phantom: PhantomData<F>,and also implement staged builder pattern for this. This would lead to big code duplication.
I also think that the pattern
let builder = EthEvmEnv::builder().provider(p).chain_spec(Ð_MAINNET_CHAIN_SPEC);
let mut envs = HostMultiblockEvmEnv::from_builder(builder);is closer to our existing pattern of how to build a HostEvmEnv. Thus, I have kept this design.
crates/steel/src/host/builder.rs
Outdated
| /// Returns a copy of the builder with elided commitment config and set EVM execution block. | ||
| pub(crate) fn to_block(&self, block: impl Into<BlockId>) -> EvmEnvBuilder<P, F, S, ()> | ||
| where | ||
| P: Clone, | ||
| S: Clone, | ||
| { | ||
| EvmEnvBuilder { | ||
| provider: self.provider.clone(), | ||
| provider_config: self.provider_config.clone(), | ||
| block: block.into(), | ||
| chain_spec: self.chain_spec.clone(), | ||
| commitment_config: (), | ||
| phantom: PhantomData, | ||
| } | ||
| } |
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.
Looking at the callsite for this function in multiblock, it was not obvious to me that this clones rather than consuming self like many of the other methods do. I might suggest making this consume self, and adding an explicit clone in front of the calls.
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.
According to the Rust naming conventions to_ is borrowed -> owned for non-Copy types, like Path::to_str or str::to_lowercase().
So the alternative would be to rename this to an into_ and combine it will an explicit clone, I'll check whether that might help readability, even though it will always be combined with a clone.
Co-authored-by: Victor Snyder-Graf <victor@risczero.com>
No description provided.