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.

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

feat: inital support for Boojum format #48

Merged
merged 5 commits into from
Jan 2, 2024
Merged

feat: inital support for Boojum format #48

merged 5 commits into from
Jan 2, 2024

Conversation

zeapoz
Copy link
Member

@zeapoz zeapoz commented Dec 20, 2023

Introduces structs and calldata-parsing for blocks formatted with the newer format.

@zeapoz zeapoz requested a review from tuommaki December 20, 2023 11:27
@zeapoz zeapoz self-assigned this Dec 20, 2023
Introduces structs and calldata parsing for blocks formatted with the newer format.
@zeapoz zeapoz force-pushed the feat/boojum-support branch from 0566611 to 18e725e Compare December 20, 2023 11:28
Comment on lines +93 to +96
struct Contracts {
v1: Contract,
v2: Contract,
}
Copy link
Member Author

@zeapoz zeapoz Dec 20, 2023

Choose a reason for hiding this comment

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

I'm not a huge fan of this sort of structure, but it was the easiest to implement without merging the two ABI-files. If you have any thoughts/ideas, do let me know

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is a good middle-ground solution. I have related notes below where I elaborate on this.

Copy link
Collaborator

@tuommaki tuommaki left a comment

Choose a reason for hiding this comment

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

Couple suggestions, but overall I think this looks good 👍

Comment on lines +93 to +96
struct Contracts {
v1: Contract,
v2: Contract,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is a good middle-ground solution. I have related notes below where I elaborate on this.

@@ -237,7 +244,7 @@ impl L1Fetcher {
disable_polling: bool,
) -> Result<tokio::task::JoinHandle<()>> {
let metrics = self.metrics.clone();
let event = self.contract.events_by_name("BlockCommit")?[0].clone();
let event = self.contracts.v1.events_by_name("BlockCommit")?[0].clone();
Copy link
Collaborator

Choose a reason for hiding this comment

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

e.g. this reads quite well for me and I like this structure.

}
let block_info = {
if use_new_format {
CommitBlockInfo::V2(v2::CommitBlockInfo::try_from(d)?)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here I was thinking that maybe there could be some kind of a generic structure where the CommitBlockInfo could be built in a way that this could be written like CommitBlockInfo::<V2>::try_from(d)? - maybe?

I cannot figure out a concrete suggestion how the type would specifically look like in this case, but this is just an intuitive gut feeling.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea! I pushed a new commit that abstracts away the whole enum. So, it's now only used internally when creating the type.

V2(v2::CommitBlockInfo),
}

// TODO: Do we need this?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Most probably not. If there is a need in the future, git would have the code so therefore I guess this can be zapped.

Comment on lines 23 to 24
/// NOTE: Does this make sense?
is_reapeated_write: bool,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say yes in this case, when it works, but there is a slight typo though 😛 is_reapeated_write -> is_repeated_write 😉

state-reconstruct-fetcher/src/types/v2.rs Outdated Show resolved Hide resolved
repeated_storage_changes: block.repeated_storage_changes,
factory_deps: block.factory_deps,
},
CommitBlockInfo::V2(_block) => todo!(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this todo!() on purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this will come later in the PR that fully implements the state reconstruction from these blocks. Figured it was easier to do split the functionality, rather than one PR with 3k LOC or so.

@zeapoz zeapoz merged commit 2e9566b into main Jan 2, 2024
4 checks passed
@zeapoz zeapoz deleted the feat/boojum-support branch January 2, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants