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.

Sign up for GitHub

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

Jump to bottom

Sergerad/batch provider impl #49

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Sergerad/batch provider impl #49

wants to merge 1 commit into from
+99 −1

Conversation

sergerad
Copy link
Contributor

Motivation

Closes #18

Solution

WIP

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

refcell
refcell reviewed Jan 13, 2025
View reviewed changes
crates/protocol/src/batch/traits.rs Outdated
Comment on lines 29 to 25

#[cfg_attr(target_arch = "wasm32", async_trait::async_trait(?Send))]
#[cfg_attr(not(target_arch = "wasm32"), async_trait::async_trait)]
impl<N, T, P> BatchValidationProvider<N, T> for P
where
N: Network,
T: Transport + Clone,
P: Provider<T, N>,
{
type Error = TransportError;

async fn l2_block_info_by_number(&mut self, number: u64) -> Result<L2BlockInfo, Self::Error> {
// TODO: Actual request + logic
self.client().request("eth_getBlockByNumber", (number,)).await
}

async fn block_by_number(&mut self, number: u64) -> Result<OpBlock, Self::Error> {
// TODO: Actual request + logic
self.client().request("eth_getBlockByNumber", (number, true)).await
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The reason why the BatchValidationProvider trait is defined here is to abstract away the provider logic from the protocol crate. That allows for any downstream consumers of the maili-protocol crate to define their own way of providing this data fetching. For example, BatchValidationProvider is implemented for the provider that is used when validating batches. Let me know if this makes sense!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @refcell that makes a lot of sense. I see now the issue description was clear about implementing the trait for ReqwestProvider. Have pushed a small change to that effect impl BatchValidationProvider for alloy_provider::ReqwestProvider {.

Will proceed with the rest of the implementation later today.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it thanks for clarifying. I'm supportive of these changes, but can we move this implementation into a reqwest.rs file, and hide the module behind a reqwest feature flag?

e.g. in lib.rs we could do the following.

...

#[cfg(feature = "reqwest")]
pub mod reqwest;

...

sergerad
sergerad commented Jan 14, 2025
View reviewed changes
crates/protocol/src/batch/traits.rs Outdated
let block = block.unwrap(); // TODO: handle error

// Convert the transactions.
// TODO: how/what do we actually need to do here?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@refcell Could you please advise the canonical approach to getting the OpBlock type from the results returned from alloy::Provider fns like get_block_by_number() (used above). Main issue here is converting from the vec of transactions returned by the provider into OpTxEnvelope enum. Couldn't find any relevant from impls.

Or if there is an alternative means to performing the requests here. I looked at how Kona uses ReqwestProvider and found that it did provider.client().request("debug_* ... style requests for headers etc. Unsure if that is a relevant approach here.

TY!

Copy link
Member

Choose a reason for hiding this comment

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

the crate isn't longer depending on op-allloy. merged main into your branch to reflect these changes. either it's enough to use the local maili_consensus::DepsoitTxEnvelope trait. otherwise if you have to deal with other transaction types specifically, this pr is blocked by alloy-rs/alloy#1910.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @emhane for that. I have pushed up a Reqwest impl for the BatchValidationProvider trait. I am guessing that this impl should be able to return more than just deposit txns which would mean it depends on your PR you linked there as you say.

If the intention is to only support deposit txns, then I still don't know how to convert between the eth rpc tx types and a tx type that the user of the Reqwest impl would expect.

I have left TODOs on the relevant lines. There are also TODOs for 2 fields I lack in order to construct the L2BlockInfo, LMK if you have any insight on that too.

TY!

emhane
emhane requested changes Jan 16, 2025
View reviewed changes
Copy link
Member

@emhane emhane left a comment

Choose a reason for hiding this comment

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

getting there!

crates/protocol/src/reqwest.rs Outdated
let l1_origin = (block.header.number, block.header.hash); // TODO: We don't have valid values
// for this or access to genesis for
// L2BlockInfo::from_block_and_genesis().
let seq_num = 0u64; // TODO: We don't have this value either.
Copy link
Member

Choose a reason for hiding this comment

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

@refcell appreciate your input here

crates/protocol/src/reqwest.rs Outdated
errors::ReqwestBatchValidationProviderError, BatchValidationProvider, BlockInfo, L2BlockInfo,
};

struct ReqwestBatchValidationProvider<T, P = alloy_provider::ReqwestProvider> {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
struct ReqwestBatchValidationProvider<T, P = alloy_provider::ReqwestProvider> {
struct ReqwestBatchValidationProvider<T, N: Network<TxEnvelope = T>, P = alloy_provider::ReqwestProvider<N>> {

alloy_network::Network

crates/protocol/src/errors.rs Outdated Show resolved Hide resolved
crates/protocol/src/reqwest.rs Outdated Show resolved Hide resolved
crates/protocol/src/reqwest.rs Outdated Show resolved Hide resolved
crates/protocol/src/reqwest.rs Outdated
errors::ReqwestBatchValidationProviderError, BatchValidationProvider, BlockInfo, L2BlockInfo,
};

struct ReqwestBatchValidationProvider<T, P = alloy_provider::ReqwestProvider> {
Copy link
Member

Choose a reason for hiding this comment

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

make this type public please also. you never known completely how other devs want to use your library and shouldn't limit them to explore new things by keeping types private. open-source philosophy :)

crates/protocol/src/reqwest.rs Outdated Show resolved Hide resolved
@sergerad sergerad force-pushed the sergerad/batch-provider-impl branch from e00e1a5 to 4b0dab0 Compare January 16, 2025 17:53
@sergerad
Copy link
Contributor Author

@emhane @refcell just summarizing latest state of the PR

  1. The BatchValidationProvider trait needs to be generic on all possible transaction types returned by eth rpc (blocked on alloy PR Define trait TransactionEnvelope alloy-rs/alloy#1910)
  2. The l2_block_info_by_number() fn should work fine if we can settle on what to do about the fact that we don't have access to all necessary arguments to its constructors
  3. The block_by_number() fn probably requires the Header to be generic in the BatchValidationProvider trait. This is straight forward but will require updating the mock tests too.
  4. If we don't go generic on Block type returned by block_by_number(), just need to make sure we can easily convert between rpc transaction types and those required by the current Block type.

Thanks!

@emhane emhane added A-protocol Area: protocol crate C-blocked Category: cannot move forward until something else changes labels Jan 17, 2025
@emhane
Copy link
Member

emhane commented Jan 21, 2025

@emhane @refcell just summarizing latest state of the PR

  1. The BatchValidationProvider trait needs to be generic on all possible transaction types returned by eth rpc (blocked on alloy PR Define trait TransactionEnvelope alloy-rs/alloy#1910)

would it be enough with adding the trait bound alloy_consensus::Transaction. That would allow accessing the fields that all transactions share, e.g. input, tx type, etc.

@sergerad sergerad force-pushed the sergerad/batch-provider-impl branch 2 times, most recently from 581c4eb to 8bc5bb7 Compare January 23, 2025 01:16
@sergerad sergerad force-pushed the sergerad/batch-provider-impl branch from 8bc5bb7 to e7d11ac Compare January 23, 2025 01:54
@sergerad
Copy link
Contributor Author

sergerad commented Jan 23, 2025 •
edited
Loading

@emhane @refcell just summarizing latest state of the PR

  1. The BatchValidationProvider trait needs to be generic on all possible transaction types returned by eth rpc (blocked on alloy PR Define trait TransactionEnvelope alloy-rs/alloy#1910)

would it be enough with adding the trait bound alloy_consensus::Transaction. That would allow accessing the fields that all transactions share, e.g. input, tx type, etc.

@emhane @refcell I have rebased from the latest changes (this commit having changed the trait).

Which has simplified the impl I think. But I'm wondering if there is a canonical conversion between these two types that you could point me to (I need to return OpTxEnvelope):

1. mismatched types
   expected struct `std::vec::Vec<op_alloy_consensus::OpTxEnvelope>`
      found struct `std::vec::Vec<alloy_consensus::TxEnvelope>` [E0308]

Ignore the vec in that message - in an ideal world would do .map(Into::into).collect().

I have checked the From impls for OpTxEnvelope and none seem relevant. If there isn't an existing impl somewhere - I can go ahead and try add one in this PR?

Thanks!

@emhane
Copy link
Member

emhane commented Jan 23, 2025

sorry there has been a lot of rebuilding here lately, perhaps makes sense to let this pr sleep for a week or so then check in again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refcell refcell refcell left review comments

@emhane emhane emhane requested changes

@mattsse mattsse Awaiting requested review from mattsse mattsse will be requested when the pull request is marked ready for review mattsse is a code owner

@clabby clabby Awaiting requested review from clabby clabby will be requested when the pull request is marked ready for review clabby is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
A-protocol Area: protocol crate C-blocked Category: cannot move forward until something else changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature] Extend Provider Implementations
3 participants
@sergerad @emhane @refcell