celestiaorg / nmt

Namespaced Merkle Tree
Apache License 2.0
107 stars 39 forks source link

fix: replacing jsonProof with pb.Proof #233

Closed distractedm1nd closed 10 months ago

distractedm1nd commented 10 months ago

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

Related: https://github.com/celestiaorg/celestia-node/issues/2631 Specifically: https://github.com/celestiaorg/celestia-node/issues/2631#issuecomment-1708048715

codecov[bot] commented 10 months ago

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%> (ø)
distractedm1nd commented 10 months ago

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

Wondertan commented 10 months ago

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 commented 10 months ago

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

Wondertan commented 10 months ago

I meant keeping native go struct

oblique commented 10 months ago

What is the status on this?

distractedm1nd commented 10 months ago

@rootulp Can we get this merged?

rootulp commented 10 months ago

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