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

refactor(types): Add ToProto method for RowProof struct #1600

Open
wants to merge 8 commits into
base: v0.34.x-celestia
Choose a base branch
from

Conversation

VolodymyrBg
Copy link

Description

This PR introduces a dedicated ToProto() method for the RowProof struct, addressing a TODO comment in the codebase. The change improves code modularity and maintainability by extracting the protobuf conversion logic from ShareProof.ToProto() into a separate method.

Key Changes:

  • Added new ToProto() method to RowProof struct
  • Updated ShareProof.ToProto() to use the new method
  • Removed duplicated conversion logic from ShareProof

Files Modified:

types/row_proof.go: Added new ToProto() method
types/share_proof.go: Updated to use new RowProof.ToProto()
This is a small, focused change that improves code organization without changing any functionality. The conversion logic remains the same but is now properly encapsulated in the RowProof struct.
PR checklist

  • Tests written/updated (existing tests cover the protobuf conversion functionality)
  • Changelog entry added in .changelog:
  • Updated relevant documentation and code comments

Extract ToProto functionality from ShareProof into a dedicated method for RowProof 
to improve code modularity and maintainability. This change makes the conversion 
between domain and protobuf types more consistent across the codebase.
@VolodymyrBg VolodymyrBg requested a review from a team as a code owner February 3, 2025 19:13
@VolodymyrBg VolodymyrBg requested review from cmwaters and rach-id and removed request for a team February 3, 2025 19:13
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

thanks

we need a roundtrip test for all to and from proto methods as they are huge footguns

@VolodymyrBg
Copy link
Author

@evan-forbes added tests

@rootulp rootulp changed the title types: Add ToProto method for RowProof struct refactor(types): Add ToProto method for RowProof struct Feb 4, 2025
rootulp
rootulp previously approved these changes Feb 4, 2025
Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the unit tests 👍

Copy link
Member

@rach-id rach-id left a comment

Choose a reason for hiding this comment

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

@VolodymyrBg can you please fix the CI?

@VolodymyrBg
Copy link
Author

@VolodymyrBg can you please fix the CI?

Yes.

@VolodymyrBg
Copy link
Author

@rach-id Done

rootulp
rootulp previously approved these changes Feb 4, 2025
@rootulp rootulp dismissed evan-forbes’s stale review February 4, 2025 18:15

Unit tests were added

@rach-id
Copy link
Member

rach-id commented Feb 4, 2025

@VolodymyrBg https://github.com/celestiaorg/celestia-core/actions/runs/13141867322/job/36671638346?pr=1600

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.

4 participants