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

WIP: convert protos methods to methods in firehose-protos #26

Closed
wants to merge 1 commit into from
Closed

WIP: convert protos methods to methods in firehose-protos #26

wants to merge 1 commit into from

Conversation

suchapalaver
Copy link
Contributor

No description provided.

@linear Linear
Copy link

linear bot commented Oct 24, 2024

BACK-113 Convert methods converting `firehose-protos` types to methods on those types

This moves towards ensuring testing of public methods by reducing unnecessary complexity, removing the need to implement conversions as part of tested code in flat-files-decoder.
We have lots of previous work working around the orphan rule and implementing difficult to read conversion functions outside of the crate where the types were compiled. Moving these conversions to firehose-protos allows for easy encapsulation and greater clarity about testing requirements.

@suchapalaver suchapalaver self-assigned this Oct 24, 2024
@suchapalaver suchapalaver force-pushed the joseph/back-113-convert-methods-converting-firehose-protos-types-to-methods branch from 7724abb to 52e6719 Compare October 24, 2024 02:21
Base automatically changed from joseph/back-107-remove-unnecessary-public-visibility-and-unused-code to main October 24, 2024 16:00
@suchapalaver suchapalaver force-pushed the joseph/back-113-convert-methods-converting-firehose-protos-types-to-methods branch from 52e6719 to 3062bfc Compare October 24, 2024 16:04
@suchapalaver suchapalaver force-pushed the joseph/back-113-convert-methods-converting-firehose-protos-types-to-methods branch from 3062bfc to f0192b6 Compare October 25, 2024 03:10
anirudh2
anirudh2 approved these changes Oct 25, 2024
View reviewed changes
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.

I think this is a massive improvement over what we had before! Nice work! In the future, please try to make your commits atomic. You have one massive commit that does multiple things. I commented a few examples, not all.

crates/flat-files-decoder/src/transactions/error.rs
#[error("Invalid BigInt")]
InvalidBigInt(String),
#[error("EIP-4844 not supported")]
EIP4844NotSupported,
#[error("Invalid Signature: {0}")]
Copy link
Member

Choose a reason for hiding this comment

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

This commit doesn't look atomic. You removed InvalidSignature entirely. I don't think I see it having been moved.

Copy link
Member

Choose a reason for hiding this comment

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

For example

crates/firehose-protos/Cargo.toml
@@ -8,8 +8,11 @@ doctest = false
path = "src/lib.rs"

[dependencies]
alloy-consensus.workspace = true
Copy link
Member

Choose a reason for hiding this comment

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

This should be part of a build commit

crates/firehose-protos/src/ethereum_v2.rs
@@ -157,6 +162,305 @@ impl TryFrom<Response> for Block {
}
}

impl TryFrom<&Log> for alloy_primitives::Log {
Copy link
Member

Choose a reason for hiding this comment

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

This is more than a simple move. It also heavily refactors. This is probably at least two commits: refactor: move stuff and refactor: improve code

@suchapalaver suchapalaver changed the title refactor(decoder): convert protos methods to methods in firehose-protos WIP: convert protos methods to methods in firehose-protos Oct 25, 2024
@suchapalaver
Copy link
Contributor Author

Everything here has been merged in other PRs.

@suchapalaver suchapalaver deleted the joseph/back-113-convert-methods-converting-firehose-protos-types-to-methods branch November 29, 2024 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anirudh2 anirudh2 anirudh2 approved these changes

Assignees

@suchapalaver suchapalaver

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@suchapalaver @anirudh2