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

fix: replacing jsonProof with pb.Proof #233

Merged
merged 2 commits into from
Sep 18, 2023

Conversation

distractedm1nd
Copy link
Contributor

jsonProof is not necessary when we already have pb.Proof, which caused issues because of mismatching field names.

Related: celestiaorg/celestia-node#2631
Specifically: celestiaorg/celestia-node#2631 (comment)

@codecov
Copy link

codecov bot commented Sep 6, 2023

Codecov Report

Merging #233 (38c7b23) into master (0e219c8) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #233   +/-   ##
=======================================
  Coverage   68.30%   68.30%           
=======================================
  Files           6        6           
  Lines        1325     1325           
=======================================
  Hits          905      905           
  Misses        403      403           
  Partials       17       17           
Files Changed Coverage Δ
proof.go 90.17% <100.00%> (ø)

Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

At this point, we should consider making all the fields on the Proof public (It might be breaking).

It's feels wrong to reuse pb definition for json marshalling, even though it works.

@distractedm1nd
Copy link
Contributor Author

I still disagree with making fields public only for the point of serialization. These are not fields that should be touched outside of the given methods

@rootulp rootulp requested a review from staheri14 September 6, 2023 13:19
@rootulp rootulp added the bug Something isn't working label Sep 6, 2023
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.

[nit] I think this should be marked breaking (i.e. fix!: in PR title) because it impacts JSON encoding observed in downstream repos (i.e. celestia-node).

[question] Do we need a unit test in this PR that the field names in the proof JSON encoding match what is expected or does that test belong in celestia-node?

proof.go Show resolved Hide resolved
@Wondertan
Copy link
Member

Wondertan commented Sep 6, 2023

Actually true, encapsulation for Proofs is more important for security.

These are not fields that should be touched outside of the given methods

Using autogenerated proto for json completely solves consistency between json and proto, as it also autogenerates json tags(TIL), so it's not that bad after all.

@distractedm1nd
Copy link
Contributor Author

Yeah but the main problem is not being able to define methods on it

@Wondertan
Copy link
Member

I meant keeping native go struct

@oblique
Copy link

oblique commented Sep 12, 2023

What is the status on this?

@distractedm1nd
Copy link
Contributor Author

@rootulp Can we get this merged?

@rootulp
Copy link
Collaborator

rootulp commented Sep 18, 2023

Sorry we need one more @celestiaorg/celestia-core approval.

proof.go Show resolved Hide resolved
@rootulp rootulp merged commit 8fb4eda into celestiaorg:master Sep 18, 2023
6 checks passed
Copy link
Contributor

@ramin ramin left a comment

Choose a reason for hiding this comment

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

is there follow on work to fix rpc @distractedm1nd?

ramin pushed a commit to celestiaorg/celestia-node that referenced this pull request Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants