refactor: Simplify batch metadata and related types#1170
refactor: Simplify batch metadata and related types#1170
Conversation
kostia244
changed the title
| #[serde(rename = "commit_batch_info")] | ||
| pub batch_info: BatchInfo, | ||
| pub batch_info: CommitBatchInfoExt, | ||
| pub chain_address: Address, |
There was a problem hiding this comment.
Keeping it here only because of senders -- I could pass it to sol_cal but then the argument list gets bloated, it's a separate issue IMO.
| // backwards-compatibility. | ||
| #[serde(rename = "commit_batch_info")] | ||
| pub batch_info: BatchInfo, | ||
| pub batch_info: CommitBatchInfoExt, |
There was a problem hiding this comment.
Should I implement deref to make access easier?
There was a problem hiding this comment.
I'd implement Deref yeah
There was a problem hiding this comment.
Bikeshedding: Personally, I agree with Rust API Guidelines that Deref / DerefMut should be implemented for smart pointers only, since otherwise, reasoning about field accesses / method calls quickly becomes a mess. But this rule is already violated in the codebase, so 🤷
There was a problem hiding this comment.
Oh, I did not realize guidelines have such a strong wording on this. I have this pattern be used in a few codebases, interesting. I would not do that then, yeah
Test results291 tests 291 ✅ 22m 24s ⏱️ Results for commit 9bdea28. ♻️ This comment has been updated with latest results. |
| // todo: these models are used throughout the batcher subsystem - not only l1 sender | ||
| // we will move them to `types` or `batcher_types` when an analogous crate is created in `zksync-os` |
There was a problem hiding this comment.
I don't understand this fully, is it already time to move it?
There was a problem hiding this comment.
Yes, the comment was written loooong before batch_types crate was even a thing. Let's move it there if possible.
| // backwards-compatibility. | ||
| #[serde(rename = "commit_batch_info")] | ||
| pub batch_info: BatchInfo, | ||
| pub batch_info: CommitBatchInfoExt, |
There was a problem hiding this comment.
I'd implement Deref yeah
| // todo: these models are used throughout the batcher subsystem - not only l1 sender | ||
| // we will move them to `types` or `batcher_types` when an analogous crate is created in `zksync-os` |
There was a problem hiding this comment.
Yes, the comment was written loooong before batch_types crate was even a thing. Let's move it there if possible.
| /// todo: these fields should be a part of `CommitBatchInfo` but needs to be changed on L1 contracts' side first | ||
| #[derive(Clone, Serialize, Deserialize, Debug)] | ||
| pub struct BatchInfo { | ||
| pub struct CommitBatchInfoExt { |
There was a problem hiding this comment.
Bikeshedding: _Ext suffix makes it look this is an extension trait. Maybe, use ExtendedCommitBatchInfo?
| // backwards-compatibility. | ||
| #[serde(rename = "commit_batch_info")] | ||
| pub batch_info: BatchInfo, | ||
| pub batch_info: CommitBatchInfoExt, |
There was a problem hiding this comment.
Bikeshedding: Personally, I agree with Rust API Guidelines that Deref / DerefMut should be implemented for smart pointers only, since otherwise, reasoning about field accesses / method calls quickly becomes a mess. But this rule is already violated in the codebase, so 🤷
| blocks: Vec<( | ||
| &BlockOutput, | ||
| &BlockContext, | ||
| &[ZkTransaction], | ||
| &zksync_os_merkle_tree::TreeBatchOutput, | ||
| )>, |
There was a problem hiding this comment.
Not sure whether this is in the scope of the PR, but this looks like too much data supplied, since BlockContext and TreeBatchOutput are only read for the last block.
| self.batches | ||
| .first() | ||
| .unwrap() | ||
| .batch | ||
| .batch_info | ||
| .protocol_version |
There was a problem hiding this comment.
Micro-nit: You can extract this to a local var to use in the matched expression so that it doesn't need to be duplicated here.
3 participants
Summary
Main points:
Justification
BatchInfo makes little sense to me as a data structure. It has a couple useful functions but the data flow is not right: most of the fields act like BatchMetadata members instead.
As for the functions, into_stored() was really made for CommittedBatch so it makes sense to move it there.