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

chore: move post-merge code from protos to forrestrie #21

Conversation

suchapalaver
Copy link
Contributor

@suchapalaver suchapalaver commented Oct 22, 2024

BACK-99

Since our first aim is a library for verifying pre-merge Ethereum data we want to separate out post-merge code for now. The main overlap is in firehose-protos where we compile the protobuffer for Beacon blocks and implement conversion methods on it. This PR moves that code to forrestrie - our library for post-merge PoC work.

Now, forrestrie and firehose-client are the only crates with post-merge code. Further work should remove them from publishing processes.

PS: when using cargo-udeps to detect unused dependencies I found a few unrelated to this work as well, hence the random dependency deletions.

@suchapalaver suchapalaver self-assigned this Oct 22, 2024
Copy link

linear bot commented Oct 22, 2024

BACK-99 Move post-merge code and protobuffer out of `firehose-protos` to `forrestrie`

joseph said in BACK-98:

I propose dealing with the coupling issues we have. Since we're working on pre-merge first I think the easiest way to tackle this is to remove the post-merge code from firehose-protos. Put it in forrestrie for now, and then when we publish on crates.io we just publish the pre-merge crates, no forrestrie, no firehose-client.

@suchapalaver suchapalaver marked this pull request as draft October 22, 2024 20:56
@suchapalaver suchapalaver marked this pull request as ready for review October 22, 2024 20:56
@suchapalaver suchapalaver force-pushed the joseph/back-99-move-post-merge-code-and-protobuffer-out-of-firehose-protos branch 2 times, most recently from 71ecdd2 to 8432fb1 Compare October 23, 2024 13:08
@suchapalaver suchapalaver force-pushed the joseph/back-99-move-post-merge-code-and-protobuffer-out-of-firehose-protos branch 3 times, most recently from 4c8b374 to ca6bee2 Compare October 23, 2024 13:19
Copy link
Member

@anirudh2 anirudh2 left a comment

Choose a reason for hiding this comment

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

Question. I don't mind this, but if we're okay with this, why wouldn't we just refactor firehose-protos out of existence too? I know you'd mentioned this possibility before @suchapalaver.

@suchapalaver
Copy link
Contributor Author

suchapalaver commented Oct 23, 2024

Question. I don't mind this, but if we're okay with this, why wouldn't we just refactor firehose-protos out of existence too? I know you'd mentioned this possibility before @suchapalaver.

Now, as opposed to when I said that, we have a monorepo rather than separate crates all with their own sets of dependencies. We have more than one crate pulling in the conversion methods for Ethereum blocks, so I say we keep firehose-protos. Otherwise, it would mean duplicating the conversion methods in each respective crate. Rust's orphan rule means we can't implement those methods outside of the crate where the protobuffers are compiled.

Copy link
Member

@anirudh2 anirudh2 left a comment

Choose a reason for hiding this comment

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

Please change your chore commit to refactor

@anirudh2
Copy link
Member

Question. I don't mind this, but if we're okay with this, why wouldn't we just refactor firehose-protos out of existence too? I know you'd mentioned this possibility before @suchapalaver.

Now, as opposed to when I said that, we have a monorepo rather than separate crates all with their own sets of dependencies. We have more than one crate pulling in the conversion methods for Ethereum blocks, so I say we keep firehose-protos. Otherwise, it would mean duplicating the conversion methods in each respective crate. Rust's orphan rule means we can't implement those methods outside of the crate where the protobuffers are compiled.

When we bring post merge into prod, would we undo this refactor?

@suchapalaver suchapalaver force-pushed the joseph/back-99-move-post-merge-code-and-protobuffer-out-of-firehose-protos branch from ca6bee2 to 7de071c Compare October 23, 2024 14:37
@suchapalaver
Copy link
Contributor Author

Question. I don't mind this, but if we're okay with this, why wouldn't we just refactor firehose-protos out of existence too? I know you'd mentioned this possibility before @suchapalaver.

Now, as opposed to when I said that, we have a monorepo rather than separate crates all with their own sets of dependencies. We have more than one crate pulling in the conversion methods for Ethereum blocks, so I say we keep firehose-protos. Otherwise, it would mean duplicating the conversion methods in each respective crate. Rust's orphan rule means we can't implement those methods outside of the crate where the protobuffers are compiled.

When we bring post merge into prod, would we undo this refactor?

Possibly! I guess we see where we are at that point? I expect with the various refactorings and cleaning up that we anticipate in header-accumulator et al it would be a pretty gnarly reversion!

@suchapalaver
Copy link
Contributor Author

Question. I don't mind this, but if we're okay with this, why wouldn't we just refactor firehose-protos out of existence too? I know you'd mentioned this possibility before @suchapalaver.

Now, as opposed to when I said that, we have a monorepo rather than separate crates all with their own sets of dependencies. We have more than one crate pulling in the conversion methods for Ethereum blocks, so I say we keep firehose-protos. Otherwise, it would mean duplicating the conversion methods in each respective crate. Rust's orphan rule means we can't implement those methods outside of the crate where the protobuffers are compiled.

When we bring post merge into prod, would we undo this refactor?

Possibly! I guess we see where we are at that point? I expect with the various refactorings and cleaning up that we anticipate in header-accumulator et al it would be a pretty gnarly reversion!

@anirudh2, what are your thoughts?

Copy link
Member

@anirudh2 anirudh2 left a comment

Choose a reason for hiding this comment

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

Alright. I guess we can leave it as a future us problem given that's what we're doing with the rest of post-merge.

@suchapalaver suchapalaver merged commit f5c1a0d into main Oct 23, 2024
8 checks passed
@suchapalaver suchapalaver deleted the joseph/back-99-move-post-merge-code-and-protobuffer-out-of-firehose-protos branch October 23, 2024 14:42
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.

2 participants