-
Notifications
You must be signed in to change notification settings - Fork 19
feat: Rust codec implementation #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
Comments
I thought about how we would like the codec interface to be. My main concern when designing the interface was to avoid leaking implementation details for each codec through the interface. With this in mind, I was looking for an API that looked like: let codec = Codec::from_spec(spec);
let decoded = codec.decode(input); I think I can achieve this goal with a very simple implementation, without the need for traits. The implementation also stays easy to update, as extending the codec would just result in extending the enums: pub enum Codec {
V0,
V1,
V2,
V3,
V4,
V5,
V6,
V7,
}
impl Codec {
fn from_spec(spec: SpecId) -> Self {
todo!()
}
fn decode(&self, input: CommitDataSource) -> Result<CommitData> {
match self {
Codec::V0 => Ok(CommitData::Chunks(decode_calldata_to_chunks(
input
.as_raw_chunks()
.ok_or(DecodingError::IncorrectDataSource),
)?)),
...
}
}
}
pub enum CommitDataSource {
RawChunks(Vec<Vec<u8>>),
RawChunksAndBlob((Vec<Vec<u8>>, Blob)),
Blob(Blob),
}
pub enum CommitData {
Chunks(Vec<Chunk>),
ChunksWithL1MessageInfo(ChunksWithL1MessageInfo),
}
pub struct Chunks(Vec<Chunk>);
pub struct Chunk {
transactions: Vec<Transaction>,
l2_block: Block,
}
pub struct ChunksWithL1MessageInfo {
chunk: Chunks,
prev_l1_message_hash: B256,
post_l1_message_hash: B256,
} Let me know what you think @jonastheis |
This looks reasonable to me. If you want to modularise a little more, you could have pub enum Codec {
V0(CodecV0),
V1(CodecV1),
V2(CodecV2),
..
} and then implement |
You also may want to consider deriving |
My understanding of Rust is still bare minimum so I'm not sure I understand the full implications of the proposed interface.
Agreed. I think this is what we should aim for as much as possible. Some questions:
|
|
|
I made some changes to the proposed layout, trying to shadow the implementation details as much as possible. Let me know what you think @jonastheis @frisitano. // Note: we don't need this structure to be an enum for decoding, but let's keep it this way
// in case we ever implement encoding as well.
pub enum Codec {
V0,
V1,
V2,
V3,
V4,
V5,
V6,
V7,
}
impl Codec {
fn decode<T: CommitDataSource>(input: T) -> Result<Box<dyn DaCommitPayload>, ()> {
todo!()
}
}
/// Values that implement the trait can provide data from a transaction's calldata or blob.
pub trait CommitDataSource {
fn calldata(&self) -> &[u8];
fn blob(&self, index: u8) -> Option<&[u8]>;
}
pub struct L2Block {
transactions: Vec<Transaction>,
context: BlockContext,
}
pub struct BlockContext {
number: u64,
timestamp: u64,
base_fee: U256,
gas_limit: u64,
num_transactions: u16,
num_l1_messages: u16,
}
pub trait CommitPayload {
fn l2_blocks(&self) -> Vec<L2Block>;
}
pub trait MaybeCommitPayload {
fn prev_l1_message_queue_hash(&self) -> Option<B256> {
None
}
fn post_l1_message_queue_hash(&self) -> Option<B256> {
None
}
}
pub trait DaCommitPayload: CommitPayload + MaybeCommitPayload {}
impl<T> DaCommitPayload for T where T: MaybeCommitPayload + CommitPayload {} Here was my process:
|
I think this is a good solution. What's the benefit of using the trait object instead of having a concrete struct type or an enum with different variants? Doesn't the trait object limit the usage of the type downstream to the trait methods? Do we want this? Would a concrete type with the |
An enum with different variants which exposes the same methods as the trait would be equivalent. That would be easier indeed and clearer as well. |
I somehow find it a bit contradictory that |
We have already some codec implementations in https://github.com/scroll-tech/zkvm-prover/tree/dc20b0536805159c3557eceeca0f5782159839be/crates/circuits/types/src/batch/payload (could be useful as reference to better design da-codec's structure) |
Yes agree, I'll follow the comment from @frisitano, I think it makes more sense to just use an enum for this instead of the trait object. I'll expose prev/post message queue hashes as methods on the enum. |
I think there will be future codecs where there are fields also in addition to Either an impl Codec {
fn decode<T: CommitDataSource>(input: T) -> Result<T::CommitPayload, ()> {
todo!()
}
}
pub trait CommitDataSource {
type CommitPayload: CommitPayload;
fn calldata(&self) -> &[u8];
fn blob(&self, index: u8) -> Option<&[u8]>;
} EDIT: pub trait CommitPayload {
fn base_payload(&self) -> BaseCommitPayload;
/* ... */
} Does this sound interesting to you? |
We would need to extend the enum and add the corresponding method on the enum.
I'm just wondering if this helps when extending the codec. Let's say we have codec v8, which now also returns a
I feel like both are quite similar on the amount of code required, but I might be missing something? |
Both are indeed quite similar. I personally prefer |
Oh sorry, I don't mean using pub enum CommitPayload {
Base(Vec<L2Block>)
...
} which was proposed by @frisitano |
This issue tracks the design and discussion for the Rust implementation of the da-codec (currently focusing on the decoding).
The text was updated successfully, but these errors were encountered: